Hi Paul Barker,
> Subject: Re: [PATCH 09/16] pinctrl: renesas: Add RZ/G2L PFC driver
>
> On Wed, Oct 04, 2023 at 12:56:14PM +0000, Biju Das wrote:
> > 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??
>
> We already have this broken down into two drivers within the u-boot driver
> module - rzg2l_pfc_bind() calls device_bind_with_driver_data() to bind the
> "rzg2l-pfc-gpio" and "rzg2l-pfc-pinctrl" drivers to the device.
> It should be no trouble to separate the code into two .c files in the right
> places.
Thanks for the explanation.
Cheers,
Biju
