diff mbox series

[v2,03/13] pinctrl: renesas: pinctrl-rzg2l: Allow more bits for pin configuration

Message ID 20240423175900.702640-4-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series Add PFC support for Renesas RZ/V2H(P) SoC | expand

Commit Message

Lad, Prabhakar April 23, 2024, 5:58 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The pin configuration bits have been growing for every new SoCs being
added for the pinctrl-rzg2l driver which would mean updating the macros
every time for each new configuration. To avoid this allocate additional
bits for pin configuration by relocating the known fixed bits to the very
end of the configuration.

Also update the size of 'cfg' to 'u64' to allow more configuration bits in
the 'struct rzg2l_variable_pin_cfg'.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC->v2
- Merged the macros and rzg2l_variable_pin_cfg changes into single patch
- Updated types for the config changes
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 30 ++++++++++++++-----------
 1 file changed, 17 insertions(+), 13 deletions(-)

Comments

Geert Uytterhoeven May 22, 2024, 10:19 a.m. UTC | #1 
Hi Prabhakar,

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> The pin configuration bits have been growing for every new SoCs being
> added for the pinctrl-rzg2l driver which would mean updating the macros
> every time for each new configuration. To avoid this allocate additional
> bits for pin configuration by relocating the known fixed bits to the very
> end of the configuration.
>
> Also update the size of 'cfg' to 'u64' to allow more configuration bits in
> the 'struct rzg2l_variable_pin_cfg'.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - Merged the macros and rzg2l_variable_pin_cfg changes into single patch
> - Updated types for the config changes

Thanks for the update!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -78,9 +78,9 @@
>                                          PIN_CFG_FILNUM | \
>                                          PIN_CFG_FILCLKSEL)
>
> -#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(35, 28)
> -#define PIN_CFG_PIN_REG_MASK           GENMASK(27, 20)
> -#define PIN_CFG_MASK                   GENMASK(19, 0)
> +#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(62, 55)
> +#define PIN_CFG_PIN_REG_MASK           GENMASK_ULL(54, 47)
> +#define PIN_CFG_MASK                   GENMASK_ULL(46, 0)
>
>  /*
>   * m indicates the bitmap of supported pins, a is the register index

> @@ -241,9 +241,9 @@ struct rzg2l_dedicated_configs {
>   * @pin: port pin
>   */
>  struct rzg2l_variable_pin_cfg {
> -       u32 cfg:20;
> -       u32 port:5;
> -       u32 pin:3;
> +       u64 cfg:46;

47, to match PIN_CFG_MASK()?

> +       u64 port:5;
> +       u64 pin:3;
>  };

To avoid such mistakes, and to increase uniformity, I think it would
be good to get rid of this structure, and replace it by masks, to be
used with FIELD_GET() and FIELD_PREP_CONST().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Lad, Prabhakar May 28, 2024, 6:47 p.m. UTC | #2 
Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 11:19 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The pin configuration bits have been growing for every new SoCs being
> > added for the pinctrl-rzg2l driver which would mean updating the macros
> > every time for each new configuration. To avoid this allocate additional
> > bits for pin configuration by relocating the known fixed bits to the very
> > end of the configuration.
> >
> > Also update the size of 'cfg' to 'u64' to allow more configuration bits in
> > the 'struct rzg2l_variable_pin_cfg'.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - Merged the macros and rzg2l_variable_pin_cfg changes into single patch
> > - Updated types for the config changes
>
> Thanks for the update!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -78,9 +78,9 @@
> >                                          PIN_CFG_FILNUM | \
> >                                          PIN_CFG_FILCLKSEL)
> >
> > -#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(35, 28)
> > -#define PIN_CFG_PIN_REG_MASK           GENMASK(27, 20)
> > -#define PIN_CFG_MASK                   GENMASK(19, 0)
> > +#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(62, 55)
> > +#define PIN_CFG_PIN_REG_MASK           GENMASK_ULL(54, 47)
> > +#define PIN_CFG_MASK                   GENMASK_ULL(46, 0)
> >
> >  /*
> >   * m indicates the bitmap of supported pins, a is the register index
>
> > @@ -241,9 +241,9 @@ struct rzg2l_dedicated_configs {
> >   * @pin: port pin
> >   */
> >  struct rzg2l_variable_pin_cfg {
> > -       u32 cfg:20;
> > -       u32 port:5;
> > -       u32 pin:3;
> > +       u64 cfg:46;
>
> 47, to match PIN_CFG_MASK()?
>
Oops, I missed that.

> > +       u64 port:5;
> > +       u64 pin:3;
> >  };
>
> To avoid such mistakes, and to increase uniformity, I think it would
> be good to get rid of this structure, and replace it by masks, to be
> used with FIELD_GET() and FIELD_PREP_CONST().
>
Agreed, I will make a patch on top of this patch (so that its easier
for review).

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index dbcf009837ef..9bb9cc63f9df 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -78,9 +78,9 @@ 
 					 PIN_CFG_FILNUM | \
 					 PIN_CFG_FILCLKSEL)
 
-#define PIN_CFG_PIN_MAP_MASK		GENMASK_ULL(35, 28)
-#define PIN_CFG_PIN_REG_MASK		GENMASK(27, 20)
-#define PIN_CFG_MASK			GENMASK(19, 0)
+#define PIN_CFG_PIN_MAP_MASK		GENMASK_ULL(62, 55)
+#define PIN_CFG_PIN_REG_MASK		GENMASK_ULL(54, 47)
+#define PIN_CFG_MASK			GENMASK_ULL(46, 0)
 
 /*
  * m indicates the bitmap of supported pins, a is the register index
@@ -102,8 +102,8 @@ 
  * (b * 8) and f is the pin configuration capabilities supported.
  */
 #define RZG2L_SINGLE_PIN		BIT_ULL(63)
-#define RZG2L_SINGLE_PIN_INDEX_MASK	GENMASK(30, 24)
-#define RZG2L_SINGLE_PIN_BITS_MASK	GENMASK(22, 20)
+#define RZG2L_SINGLE_PIN_INDEX_MASK	GENMASK_ULL(62, 56)
+#define RZG2L_SINGLE_PIN_BITS_MASK	GENMASK_ULL(55, 53)
 
 #define RZG2L_SINGLE_PIN_PACK(p, b, f)	(RZG2L_SINGLE_PIN | \
 					 FIELD_PREP_CONST(RZG2L_SINGLE_PIN_INDEX_MASK, (p)) | \
@@ -241,9 +241,9 @@  struct rzg2l_dedicated_configs {
  * @pin: port pin
  */
 struct rzg2l_variable_pin_cfg {
-	u32 cfg:20;
-	u32 port:5;
-	u32 pin:3;
+	u64 cfg:46;
+	u64 port:5;
+	u64 pin:3;
 };
 
 struct rzg2l_pinctrl_data {
@@ -1082,7 +1082,8 @@  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
 	const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin];
 	u64 *pin_data = pin->drv_data;
 	unsigned int arg = 0;
-	u32 off, cfg;
+	u32 off;
+	u64 cfg;
 	int ret;
 	u8 bit;
 
@@ -1186,7 +1187,8 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 	u64 *pin_data = pin->drv_data;
 	enum pin_config_param param;
 	unsigned int i, arg, index;
-	u32 cfg, off;
+	u32 off;
+	u64 cfg;
 	int ret;
 	u8 bit;
 
@@ -2414,9 +2416,9 @@  static void rzg2l_pinctrl_pm_setup_regs(struct rzg2l_pinctrl *pctrl, bool suspen
 
 	for (u32 port = 0; port < nports; port++) {
 		bool has_iolh, has_ien;
-		u32 off, caps;
+		u64 cfg, caps;
 		u8 pincnt;
-		u64 cfg;
+		u32 off;
 
 		cfg = pctrl->data->port_pin_configs[port];
 		off = RZG2L_PIN_CFG_TO_PORT_OFFSET(cfg);
@@ -2460,12 +2462,14 @@  static void rzg2l_pinctrl_pm_setup_regs(struct rzg2l_pinctrl *pctrl, bool suspen
 static void rzg2l_pinctrl_pm_setup_dedicated_regs(struct rzg2l_pinctrl *pctrl, bool suspend)
 {
 	struct rzg2l_pinctrl_reg_cache *cache = pctrl->dedicated_cache;
+	u64 caps;
+	u32 i;
 
 	/*
 	 * Make sure entries in pctrl->data->n_dedicated_pins[] having the same
 	 * port offset are close together.
 	 */
-	for (u32 i = 0, caps = 0; i < pctrl->data->n_dedicated_pins; i++) {
+	for (i = 0, caps = 0; i < pctrl->data->n_dedicated_pins; i++) {
 		bool has_iolh, has_ien;
 		u32 off, next_off = 0;
 		u64 cfg, next_cfg;