Hi Marek and Paul, > Subject: Re: [PATCH 09/16] pinctrl: renesas: Add RZ/G2L PFC driver > > On 10/4/23 10:40, Paul Barker wrote: > > On 03/10/2023 14:20, Marek Vasut wrote: > >> On 9/20/23 14:42, Paul Barker wrote: > >>> This driver provides pinctrl and gpio control for the Renesas RZ/G2L > >>> (R9A07G044) SoC. > >>> > >>> This patch is based on the corresponding Linux v6.5 driver. > >>> > >>> Signed-off-by: Paul Barker <paul.barker...@bp.renesas.com> > >>> Reviewed-by: Biju Das <biju.das...@bp.renesas.com> > >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com> > >>> --- > >>> arch/arm/mach-rmobile/Kconfig | 1 + > >>> drivers/pinctrl/renesas/Kconfig | 9 + > >>> drivers/pinctrl/renesas/Makefile | 1 + > >>> drivers/pinctrl/renesas/rzg2l-pfc.c | 906 > ++++++++++++++++++++++++++++ > >>> 4 files changed, 917 insertions(+) > >>> create mode 100644 drivers/pinctrl/renesas/rzg2l-pfc.c > >>> > >>> diff --git a/arch/arm/mach-rmobile/Kconfig > >>> b/arch/arm/mach-rmobile/Kconfig index 91f544c78337..973e84fcf7ba > >>> 100644 > >>> --- a/arch/arm/mach-rmobile/Kconfig > >>> +++ b/arch/arm/mach-rmobile/Kconfig > >>> @@ -76,6 +76,7 @@ config RZG2L > >>> imply SYS_MALLOC_F > >>> imply RENESAS_SDHI > >>> imply CLK_RZG2L > >>> + imply PINCTRL_RZG2L > >> > >> Keep the list sorted > >> > >> [...] > >> > >> Drop the parenthesis around values please, fix globally and in other > >> patches too. > >> > >>> +#define PWPR (0x3014) > >>> +#define SD_CH(n) (0x3000 + (n) * 4) > >>> +#define QSPI (0x3008) > >>> + > >>> +#define PVDD_1800 1 /* I/O domain voltage <= 1.8V */ > >>> +#define PVDD_3300 0 /* I/O domain voltage >= 3.3V */ > >>> + > >>> +#define PWPR_B0WI BIT(7) /* Bit Write Disable */ > >>> +#define PWPR_PFCWE BIT(6) /* PFC Register Write Enable */ > >>> + > >>> +#define PM_MASK 0x03 > >>> +#define PVDD_MASK 0x01 > >>> +#define PFC_MASK 0x07 > >>> +#define IEN_MASK 0x01 > >>> +#define IOLH_MASK 0x03 > >>> + > >>> +#define PM_HIGH_Z 0x0 > >>> +#define PM_INPUT 0x1 > >>> +#define PM_OUTPUT 0x2 > >>> +#define PM_OUTPUT_IEN 0x3 > >>> + > >>> +struct rzg2l_pfc_driver_data { > >>> + uint num_dedicated_pins; > >>> + uint num_ports; > >>> + const u32 *gpio_configs; > >>> +}; > >>> + > >>> +struct rzg2l_pfc_data { > >>> + void __iomem *base; > >>> + uint num_dedicated_pins; > >>> + uint num_ports; > >>> + uint num_pins; > >>> + const u32 *gpio_configs; > >>> + bool pfc_enabled; > >>> +}; > >>> + > >>> +struct rzg2l_dedicated_configs { > >>> + const char *name; > >>> + u32 config; > >>> +}; > >>> + > >>> +/* > >>> + * We need to ensure that the module clock is enabled and all > >>> +resets are > >>> + * de-asserted before using either the gpio or pinctrl > >>> +functionality. Error > >>> + * handling can be quite simple here as if the PFC cannot be > >>> +enabled then we > >>> + * will not be able to progress with the boot anyway. > >>> + */ > >>> +static int rzg2l_pfc_enable(struct udevice *dev) { > >>> + struct rzg2l_pfc_data *data = > >>> + (struct rzg2l_pfc_data *)dev_get_driver_data(dev); > >>> + struct reset_ctl_bulk rsts; > >>> + struct clk clk; > >>> + int ret; > >>> + > >>> + if (data->pfc_enabled) > >> > >> When does this get triggered ? > > > > This is initialised to false in rzg2l_pfc_bind(), then this function > > rzg2l_pfc_enable() sets it to true before a successful return. The > > effect is that the PFC is enabled just once, regardless of whether the > > pinctrl or gpio driver is probed first. > > Why would be call to rzg2l_pfc_enable() a problem in the first place ? > It just grabs and enables clock and ungates reset, the second time this is > called the impact on harware should be no-op , right ? > > >>> + return 0; > >> > >> [...] > >> > >>> +static int rzg2l_gpio_set_value(struct udevice *dev, unsigned int > offset, > >>> + int value) > >>> +{ > >>> + struct rzg2l_pfc_data *data = > >>> + (struct rzg2l_pfc_data *)dev_get_driver_data(dev); > >>> + u32 port = RZG2L_PINMUX_TO_PORT(offset); > >>> + u8 pin = RZG2L_PINMUX_TO_PIN(offset); > >> > >> A lot of this can also be const > > > > This is aligned with the Linux driver to make it easier to port any > > future fixes across. > > Maybe send patches to Geert and see what happens ? > > [...] > > >>> + return -EINVAL; > >>> + } > >>> + > >>> + uc_priv->gpio_count = args.args[2]; > >>> + return rzg2l_pfc_enable(dev); > >>> +} > >>> + > >>> +U_BOOT_DRIVER(rzg2l_pfc_gpio) = { > >>> + .name = "rzg2l-pfc-gpio", > >>> + .id = UCLASS_GPIO, > >>> + .ops = &rzg2l_gpio_ops, > >>> + .probe = rzg2l_gpio_probe, > >>> +}; > >> > >> Can you please split the GPIO and PFC drivers into separate files ? > > > > I assume you mean gpio and pinctrl here - the PFC handles both. I can > > move the gpio driver out to drivers/gpio. > > Thanks. R-Car already does it that way.
RCar has separate GPIO block and Pin control block So we have separate drivers. On RZ/G2L we have only pinctrl block, ie, the reason it is integrated driver in linux. If you make separate driver, how do you plan to share resources in u-boot. For eg: register/clock/reset?? Cheers, Biju