| Message ID | 20230912045157.177966-26-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new Renesas RZ/G3S SoC and RZ/G3S SMARC EVK | expand |
Hi Claudiu, Thanks for your patch! On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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 <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> 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 -- 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
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 <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> 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 <claudiu.beznea.uj@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > 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 -- 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
Hi Claudiu, On Tue, Sep 26, 2023 at 11:55 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 21.09.2023 15:51, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> 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 <claudiu.beznea.uj@bp.renesas.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > 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
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;