[PATCH 2/2] pinctrl: at91-pio4: add support for slew-rate

[Claudiu.Beznea at microchip.com]

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 at 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
> 
>>   	{}
>>   };
>>   
>>
>