[25/37] pinctrl: renesas: rzg2l: adapt function number for RZ/G3S

Commit Message

Claudiu Beznea Sept. 12, 2023, 4:51 a.m. UTC

From: Claudiu Beznea <[e-mail]>

On RZ/G3S PFC register allow setting 8 functions for individual ports
(function1 to function8). For function1 register need to be configured
with 0, for function8 register need to be configured with 7.
We cannot use zero based addressing when requesting functions from
different code places as documentation (RZG3S_pinfunction_List_r1.0.xlsx)
states explicitly that function0 has different meaning.

For this add a new member to struct rzg2l_hwcfg that will keep the
offset that need to be substracted before applying a value to PFC register.

Signed-off-by: Claudiu Beznea <[e-mail]>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven Sept. 21, 2023, 12:51 p.m. UTC | #1

Hi Claudiu,

Thanks for your patch!

On Tue, Sep 12, 2023 at 6:53 AM Claudiu <[e-mail]> wrote:
> From: Claudiu Beznea <[e-mail]>
>
> On RZ/G3S PFC register allow setting 8 functions for individual ports
> (function1 to function8). For function1 register need to be configured
> with 0, for function8 register need to be configured with 7.
> We cannot use zero based addressing when requesting functions from
> different code places as documentation (RZG3S_pinfunction_List_r1.0.xlsx)
> states explicitly that function0 has different meaning.

According to that table, function0 is GPIO.

> For this add a new member to struct rzg2l_hwcfg that will keep the
> offset that need to be substracted before applying a value to PFC register.
>
> Signed-off-by: Claudiu Beznea <[e-mail]>

Reviewed-by: Geert Uytterhoeven <[e-mail]>

But one question below...

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -136,9 +136,11 @@ struct rzg2l_register_offsets {
>  /**
>   * struct rzg2l_hwcfg - hardware configuration data structure
>   * @regs: hardware specific register offsets
> + * @func_base: base number for port function (see register PFC)
>   */
>  struct rzg2l_hwcfg {
>         const struct rzg2l_register_offsets regs;
> +       u8 func_base;
>  };
>
>  struct rzg2l_dedicated_configs {
> @@ -221,6 +223,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
>                                  unsigned int group_selector)
>  {
>         struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
>         const struct pinctrl_pin_desc *pin_desc;
>         unsigned int i, *psel_val, *pin_data;
>         struct function_desc *func;
> @@ -247,9 +250,9 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
>                 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data);
>
>                 dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port,
> -                       pin, off, psel_val[i]);
> +                       pin, off, psel_val[i] - hwcfg->func_base);
>
> -               rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]);
> +               rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i] - hwcfg->func_base);
>         }
>
>         return 0;

Perhaps the adjustment should be done in rzg2l_dt_subnode_to_map()
instead, when obtaining MUX_FUNC() from DT? That would allow you to do
some basic validation on it too, which is currently completely missing
(reject out-of-range values overflowing into adjacent PFC fields,
reject zero on RZ/G3S).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Claudiu Beznea Sept. 26, 2023, 9:55 a.m. UTC | #2

Hi, Geert,

On 21.09.2023 15:51, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> Thanks for your patch!
> 
> On Tue, Sep 12, 2023 at 6:53 AM Claudiu <[e-mail]> wrote:
>> From: Claudiu Beznea <[e-mail]>
>>
>> On RZ/G3S PFC register allow setting 8 functions for individual ports
>> (function1 to function8). For function1 register need to be configured
>> with 0, for function8 register need to be configured with 7.
>> We cannot use zero based addressing when requesting functions from
>> different code places as documentation (RZG3S_pinfunction_List_r1.0.xlsx)
>> states explicitly that function0 has different meaning.
> 
> According to that table, function0 is GPIO.

Yes, I'll mention it like this in the next version.

> 
>> For this add a new member to struct rzg2l_hwcfg that will keep the
>> offset that need to be substracted before applying a value to PFC register.
>>
>> Signed-off-by: Claudiu Beznea <[e-mail]>
> 
> Reviewed-by: Geert Uytterhoeven <[e-mail]>
> 
> But one question below...
> 
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -136,9 +136,11 @@ struct rzg2l_register_offsets {
>>  /**
>>   * struct rzg2l_hwcfg - hardware configuration data structure
>>   * @regs: hardware specific register offsets
>> + * @func_base: base number for port function (see register PFC)
>>   */
>>  struct rzg2l_hwcfg {
>>         const struct rzg2l_register_offsets regs;
>> +       u8 func_base;
>>  };
>>
>>  struct rzg2l_dedicated_configs {
>> @@ -221,6 +223,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
>>                                  unsigned int group_selector)
>>  {
>>         struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>> +       const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
>>         const struct pinctrl_pin_desc *pin_desc;
>>         unsigned int i, *psel_val, *pin_data;
>>         struct function_desc *func;
>> @@ -247,9 +250,9 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
>>                 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data);
>>
>>                 dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port,
>> -                       pin, off, psel_val[i]);
>> +                       pin, off, psel_val[i] - hwcfg->func_base);
>>
>> -               rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]);
>> +               rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i] - hwcfg->func_base);
>>         }
>>
>>         return 0;
> 
> Perhaps the adjustment should be done in rzg2l_dt_subnode_to_map()
> instead, when obtaining MUX_FUNC() from DT? That would allow you to do
> some basic validation on it too, which is currently completely missing
> (reject out-of-range values overflowing into adjacent PFC fields,
> reject zero on RZ/G3S).

I'll have a look on this. I see .set_mux() can also be called from sysfs
though pinmux-select exported file thus, I don't know at the moment if
validating it on rzg2l_dt_subnode_to_map() will be enough.

Would it be OK to have this outside of this series or you would prefer it now?

Thank you,
Claudiu Beznea

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
> 
> 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

Geert Uytterhoeven Sept. 26, 2023, 2:23 p.m. UTC | #3

Hi Claudiu,

On Tue, Sep 26, 2023 at 11:55 AM claudiu beznea
<[e-mail]> wrote:
> On 21.09.2023 15:51, Geert Uytterhoeven wrote:
> > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <[e-mail]> wrote:
> >> From: Claudiu Beznea <[e-mail]>
> >>
> >> On RZ/G3S PFC register allow setting 8 functions for individual ports
> >> (function1 to function8). For function1 register need to be configured
> >> with 0, for function8 register need to be configured with 7.
> >> We cannot use zero based addressing when requesting functions from
> >> different code places as documentation (RZG3S_pinfunction_List_r1.0.xlsx)
> >> states explicitly that function0 has different meaning.
> >
> > According to that table, function0 is GPIO.
>
> Yes, I'll mention it like this in the next version.
>
> >> For this add a new member to struct rzg2l_hwcfg that will keep the
> >> offset that need to be substracted before applying a value to PFC register.
> >>
> >> Signed-off-by: Claudiu Beznea <[e-mail]>
> >
> > Reviewed-by: Geert Uytterhoeven <[e-mail]>
> >
> > But one question below...
> >
> >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> @@ -136,9 +136,11 @@ struct rzg2l_register_offsets {
> >>  /**
> >>   * struct rzg2l_hwcfg - hardware configuration data structure
> >>   * @regs: hardware specific register offsets
> >> + * @func_base: base number for port function (see register PFC)
> >>   */
> >>  struct rzg2l_hwcfg {
> >>         const struct rzg2l_register_offsets regs;
> >> +       u8 func_base;
> >>  };
> >>
> >>  struct rzg2l_dedicated_configs {
> >> @@ -221,6 +223,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
> >>                                  unsigned int group_selector)
> >>  {
> >>         struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> >> +       const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> >>         const struct pinctrl_pin_desc *pin_desc;
> >>         unsigned int i, *psel_val, *pin_data;
> >>         struct function_desc *func;
> >> @@ -247,9 +250,9 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
> >>                 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data);
> >>
> >>                 dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port,
> >> -                       pin, off, psel_val[i]);
> >> +                       pin, off, psel_val[i] - hwcfg->func_base);
> >>
> >> -               rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]);
> >> +               rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i] - hwcfg->func_base);
> >>         }
> >>
> >>         return 0;
> >
> > Perhaps the adjustment should be done in rzg2l_dt_subnode_to_map()
> > instead, when obtaining MUX_FUNC() from DT? That would allow you to do
> > some basic validation on it too, which is currently completely missing
> > (reject out-of-range values overflowing into adjacent PFC fields,
> > reject zero on RZ/G3S).
>
> I'll have a look on this. I see .set_mux() can also be called from sysfs
> though pinmux-select exported file thus, I don't know at the moment if
> validating it on rzg2l_dt_subnode_to_map() will be enough.

OK, that's a good reason to keep it as-is.

> Would it be OK to have this outside of this series or you would prefer it now?

That can be done later. I believe currently there is no validation against
the register field size limit anyway.
Thanks!

Gr{oetje,eeting}s,

                        Geert

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 8bdf065aa85b..80cacac7ec95 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -136,9 +136,11 @@  struct rzg2l_register_offsets {
 /**
  * struct rzg2l_hwcfg - hardware configuration data structure
  * @regs: hardware specific register offsets
+ * @func_base: base number for port function (see register PFC)
  */
 struct rzg2l_hwcfg {
 	const struct rzg2l_register_offsets regs;
+	u8 func_base;
 };
 
 struct rzg2l_dedicated_configs {
@@ -221,6 +223,7 @@  static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
 				 unsigned int group_selector)
 {
 	struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
 	const struct pinctrl_pin_desc *pin_desc;
 	unsigned int i, *psel_val, *pin_data;
 	struct function_desc *func;
@@ -247,9 +250,9 @@  static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
 		off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data);
 
 		dev_dbg(pctrl->dev, "port:%u pin: %u off:%x PSEL:%u\n", port,
-			pin, off, psel_val[i]);
+			pin, off, psel_val[i] - hwcfg->func_base);
 
-		rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i]);
+		rzg2l_pinctrl_set_pfc_mode(pctrl, pin, off, psel_val[i] - hwcfg->func_base);
 	}
 
 	return 0;