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