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

[Paul Barker]

On Wed, Oct 04, 2023 at 02:24:38PM +0200, Marek Vasut wrote:
> 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 ?

The hw impact is a no-op, but it wastes time unnecessarily re-reading
data from the fdt and repeating the setup, e.g. in rzg2l_cpg_clk_set()
we have to search the array of clocks each time to find the requested
entry.

I think it's worth keeping the conditional here but can drop it if
you're really against it.

Thanks,
Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231004/67d7126a/attachment.sig>