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...@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 ?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
signature.asc
Description: PGP signature
