Hi Eugen,

On 23.02.2021 11:41, Eugen Hristev - M18282 wrote:

On 27.01.2021 15:00, Claudiu Beznea wrote:

...

SAMA7G5 supports slew rate configuration. Adapt the driver for this. For switching frequencies lower than 50MHz the slew rate needs to be enabled. Since most of the pins on SAMA7G5 fall into this category enabled the slew rate by default.

Signed-off-by: Claudiu Beznea claudiu.beznea@microchip.com

arch/arm/mach-at91/include/mach/atmel_pio4.h | 1 + drivers/pinctrl/pinctrl-at91-pio4.c | 26 +++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-at91/include/mach/atmel_pio4.h b/arch/arm/mach-at91/include/mach/atmel_pio4.h index 35ac7b2d40e1..c3bd9140dfef 100644 --- a/arch/arm/mach-at91/include/mach/atmel_pio4.h +++ b/arch/arm/mach-at91/include/mach/atmel_pio4.h @@ -44,6 +44,7 @@ struct atmel_pio4_port { #define ATMEL_PIO_DIR_MASK BIT(8) #define ATMEL_PIO_PUEN_MASK BIT(9) #define ATMEL_PIO_PDEN_MASK BIT(10) +#define ATMEL_PIO_SR BIT(11) #define ATMEL_PIO_IFEN_MASK BIT(12) #define ATMEL_PIO_IFSCEN_MASK BIT(13) #define ATMEL_PIO_OPD_MASK BIT(14) diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c index 3a5143adc381..5c6ece745ab0 100644 --- a/drivers/pinctrl/pinctrl-at91-pio4.c +++ b/drivers/pinctrl/pinctrl-at91-pio4.c @@ -24,6 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;

struct atmel_pio4_plat { struct atmel_pio4_port *reg_base;

• unsigned int slew_rate_support; };

  static const struct pinconf_param conf_params[] = {

@@ -35,9 +36,11 @@ static const struct pinconf_param conf_params[] = { { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, { "atmel,drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },

• { "slew-rate", PIN_CONFIG_SLEW_RATE, 0},

Hi Claudiu,

If slew rate is enabled by default, we should have here 0 or 1 ?

It doesn't matter the value here since the driver is used with versions of this IP that either support or not this feature. What matters is the value of priv->slew_rate_support that is set based on the IP version. At the moment this feature is available only on SAMA7G5. If we would rely on the value on conf_params we could end-up setting the SR bit for versions of this IP that doesn't support the SR feature. Please share if you see it implemented otherwise.

...

};

-static u32 atmel_pinctrl_get_pinconf(struct udevice *config) +static u32 atmel_pinctrl_get_pinconf(struct udevice *config,

• 		     struct atmel_pio4_plat *plat)
  

  { const struct pinconf_param *params; u32 param, arg, conf = 0;

@@ -52,6 +55,10 @@ static u32 atmel_pinctrl_get_pinconf(struct udevice *config) param = params->param; arg = params->default_value;

• /* Keep slew rate enabled by default. */
  

• if (plat->slew_rate_support)
  

• 	conf |= ATMEL_PIO_SR;
  

• switch (param) { case PIN_CONFIG_BIAS_DISABLE: conf &= (~ATMEL_PIO_PUEN_MASK);

@@ -90,6 +97,15 @@ static u32 atmel_pinctrl_get_pinconf(struct udevice *config) conf |= (val << ATMEL_PIO_DRVSTR_OFFSET) & ATMEL_PIO_DRVSTR_MASK; break;

• case PIN_CONFIG_SLEW_RATE:
  

• 	if (!plat->slew_rate_support)
  

• 		break;
  

• 	dev_read_u32(config, params->property, &val);
  

• 	/* And disable it if requested. */
  

• 	if (val == 0)
  

• 		conf &= ~ATMEL_PIO_SR;
  

• 	break;
  

  default: printf("%s: Unsupported configuration parameter: %u\n", __func__, param);

@@ -115,6 +131,7 @@ static inline struct atmel_pio4_port *atmel_pio4_bank_base(struct udevice *dev,

static int atmel_pinctrl_set_state(struct udevice *dev, struct udevice *config) {

• struct atmel_pio4_plat *plat = dev_get_plat(dev); struct atmel_pio4_port *bank_base; const void *blob = gd->fdt_blob; int node = dev_of_offset(config);

@@ -123,7 +140,7 @@ static int atmel_pinctrl_set_state(struct udevice *dev, struct udevice *config) u32 i, conf; int count;

• conf = atmel_pinctrl_get_pinconf(config);

• conf = atmel_pinctrl_get_pinconf(config, plat);

  count = fdtdec_get_int_array_count(blob, node, "pinmux", cells, ARRAY_SIZE(cells));

@@ -163,6 +180,7 @@ const struct pinctrl_ops atmel_pinctrl_ops = { static int atmel_pinctrl_probe(struct udevice *dev) { struct atmel_pio4_plat *plat = dev_get_plat(dev);

• ulong priv = dev_get_driver_data(dev); fdt_addr_t addr_base;

  dev = dev_get_parent(dev);

@@ -171,13 +189,15 @@ static int atmel_pinctrl_probe(struct udevice *dev) return -EINVAL;

plat->reg_base = (struct atmel_pio4_port *)addr_base;

• plat->slew_rate_support = priv;

  return 0; }

  static const struct udevice_id atmel_pinctrl_match[] = { { .compatible = "atmel,sama5d2-pinctrl" },

• { .compatible = "microchip,sama7g5-pinctrl" },

• { .compatible = "microchip,sama7g5-pinctrl",

•  .data = (ulong)1, },
  

Is this fine to have the data converted to an ulong,

The type of data is ulong.

a hardcoded '1', and then in the plat struct we have an unsigned int that takes this value ?

Why not? At the moment it could only be 0 or 1.

Maybe it would be better to have a config struct as we have in the macb driver for example ? And this struct would have a field that makes more sense, like 'has_slew_rate' or something.

I avoided to to this since it was only about a structure with a simple member. Please let me know if you still want to go with a new strucure.

Thank you, Claudiu Beznea

Eugen

...

{} };