[PATCH 09/16] pinctrl: renesas: Add RZ/G2L PFC driver

[Biju Das]

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.ct at bp.renesas.com>
> >>> Reviewed-by: Biju Das <biju.das.jz at bp.renesas.com>
> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj at 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