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

[Marek Vasut]

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.

[...]

>>> +		if (cfg & PIN_CFG_IO_VMC_SD0) {
>>> +			dev_dbg(dev, "port off %u:%u set SD_CH 0 PVDD=%u\n",
>>> +				port_offset, pin, argument);
>>> +			pwr_reg = SD_CH(0);
>>> +		} else if (cfg & PIN_CFG_IO_VMC_SD1) {
>>> +			dev_dbg(dev, "port off %u:%u set SD_CH 1 PVDD=%u\n",
>>> +				port_offset, pin, argument);
>>> +			pwr_reg = SD_CH(1);
>>> +		} else if (cfg & PIN_CFG_IO_VMC_QSPI) {
>>> +			dev_dbg(dev, "port off %u:%u set QSPI PVDD=%u\n",
>>> +				port_offset, pin, argument);
>>> +			pwr_reg = QSPI;
>>> +		} else {
>>> +			dev_dbg(dev, "pin power source is not selectable\n");
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		addr = data->base + pwr_reg;
>>> +		writel((argument == 1800) ? PVDD_1800 : PVDD_3300, addr);
>>
>> Does RZG2L not do 2V5 IO ? If not, please ignore the comment .
> 
> 2V5 IO is supported on the Ethernet interfaces. Support for configuring
> those interfaces will be added along with the Ethernet driver in future
> patches.

Add a TODO comment please.