From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claudiu.Beznea at microchip.com Date: Tue, 23 Feb 2021 11:09:07 +0000 Subject: [PATCH 2/2] pinctrl: at91-pio4: add support for slew-rate In-Reply-To: References: <1611752430-12304-1-git-send-email-claudiu.beznea@microchip.com> <1611752430-12304-3-git-send-email-claudiu.beznea@microchip.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> --- >> 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 > >> {} >> }; >> >> >