2022-07-08 20:14:38

by Horatiu Vultur

Subject: [PATCH v2 2/2] pinctrl: ocelot: Fix pincfg

The blamed commit changed to use regmaps instead of __iomem. But it
didn't update the register offsets to be at word offset, so it uses byte
offset.
Another issue with the same commit is that it a limit of 32 registers
which is incorrect. The sparx5 has 64 while lan966x has 77.

Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
Signed-off-by: Horatiu Vultur <[email protected]>
---
drivers/pinctrl/pinctrl-ocelot.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 10787056c5c7..d88d6af71e46 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -1334,7 +1334,9 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
if (info->pincfg) {
u32 regcfg;

- ret = regmap_read(info->pincfg, pin, &regcfg);
+ ret = regmap_read(info->pincfg,
+ pin * regmap_get_reg_stride(info->pincfg),
+ &regcfg);
if (ret)
return ret;

@@ -1368,14 +1370,18 @@ static int ocelot_pincfg_clrsetbits(struct ocelot_pinctrl *info, u32 regaddr,
u32 val;
int ret;

- ret = regmap_read(info->pincfg, regaddr, &val);
+ ret = regmap_read(info->pincfg,
+ regaddr * regmap_get_reg_stride(info->pincfg),
+ &val);
if (ret)
return ret;

val &= ~clrbits;
val |= setbits;

- ret = regmap_write(info->pincfg, regaddr, val);
+ ret = regmap_write(info->pincfg,
+ regaddr * regmap_get_reg_stride(info->pincfg),
+ val);

return ret;
}
@@ -1944,7 +1950,6 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
- .max_register = 32,
.name = "pincfg",
};

--
2.33.0

2022-07-08 20:18:41

by Colin Foster

Subject: Re: [PATCH v2 2/2] pinctrl: ocelot: Fix pincfg

On Fri, Jul 08, 2022 at 09:55:10PM +0200, Horatiu Vultur wrote:
> The blamed commit changed to use regmaps instead of __iomem. But it
> didn't update the register offsets to be at word offset, so it uses byte
> offset.
> Another issue with the same commit is that it a limit of 32 registers
> which is incorrect. The sparx5 has 64 while lan966x has 77.
>
> Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
> Signed-off-by: Horatiu Vultur <[email protected]>
> ---
> drivers/pinctrl/pinctrl-ocelot.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> index 10787056c5c7..d88d6af71e46 100644
> --- a/drivers/pinctrl/pinctrl-ocelot.c
> +++ b/drivers/pinctrl/pinctrl-ocelot.c
> @@ -1334,7 +1334,9 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
> if (info->pincfg) {
> u32 regcfg;
>
> - ret = regmap_read(info->pincfg, pin, &regcfg);
> + ret = regmap_read(info->pincfg,
> + pin * regmap_get_reg_stride(info->pincfg),
> + &regcfg);
> if (ret)
> return ret;
>
> @@ -1368,14 +1370,18 @@ static int ocelot_pincfg_clrsetbits(struct ocelot_pinctrl *info, u32 regaddr,
> u32 val;
> int ret;
>
> - ret = regmap_read(info->pincfg, regaddr, &val);
> + ret = regmap_read(info->pincfg,
> + regaddr * regmap_get_reg_stride(info->pincfg),
> + &val);
> if (ret)
> return ret;
>
> val &= ~clrbits;
> val |= setbits;
>
> - ret = regmap_write(info->pincfg, regaddr, val);
> + ret = regmap_write(info->pincfg,
> + regaddr * regmap_get_reg_stride(info->pincfg),
> + val);
>
> return ret;
> }
> @@ -1944,7 +1950,6 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
> .reg_bits = 32,
> .val_bits = 32,
> .reg_stride = 4,
> - .max_register = 32,

What happens in /sys/kernel/debug/regmap/*-pincfg/{range,registers} when
there's no max register?


Should it be this?

struct regmap_config regmap_config = {
...
};
regmap_config.max_register = info->desc->npins * regmap_config.reg_stride;

> .name = "pincfg",
> };
>
> --
> 2.33.0
>

2022-07-08 22:05:23

by Andy Shevchenko

Subject: Re: [PATCH v2 2/2] pinctrl: ocelot: Fix pincfg

On Fri, Jul 8, 2022 at 10:17 PM Colin Foster
<[email protected]> wrote:
> On Fri, Jul 08, 2022 at 09:55:10PM +0200, Horatiu Vultur wrote:
> > The blamed commit changed to use regmaps instead of __iomem. But it
> > didn't update the register offsets to be at word offset, so it uses byte
> > offset.
> > Another issue with the same commit is that it a limit of 32 registers

it has a limit

> > which is incorrect. The sparx5 has 64 while lan966x has 77.

...

> > - .max_register = 32,
>
> What happens in /sys/kernel/debug/regmap/*-pincfg/{range,registers} when
> there's no max register?

Good question!

> Should it be this?
>
> struct regmap_config regmap_config = {
> ...
> };
> regmap_config.max_register = info->desc->npins * regmap_config.reg_stride;
>
> > .name = "pincfg",
> > };

If regmap configuration may be const, I would prefer to have a
hardcoded value and different configuration based on the chip, but if
it's not feasible, then this could suffice.

--
With Best Regards,
Andy Shevchenko

2022-07-11 10:07:19

by Andy Shevchenko

Subject: Re: [PATCH v2 2/2] pinctrl: ocelot: Fix pincfg

On Mon, Jul 11, 2022 at 8:51 AM Horatiu Vultur
<[email protected]> wrote:
> The 07/09/2022 00:02, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 10:17 PM Colin Foster
> > <[email protected]> wrote:
> > > On Fri, Jul 08, 2022 at 09:55:10PM +0200, Horatiu Vultur wrote:

Please, remove unneeded context when replying!

...

> > > > - .max_register = 32,
> > >
> > > What happens in /sys/kernel/debug/regmap/*-pincfg/{range,registers} when
> > > there's no max register?
> >
> > Good question!
>
> If .max_register is missing then I got the following:
>
> # cd /sys/kernel/debug/regmap/e2004064.pinctrl-pincfg/
> # cat range
> 0-0
> # cat registers
> 0: 00000005

This is effectively a regression.

> > > Should it be this?
> > >
> > > struct regmap_config regmap_config = {
> > > ...
> > > };
> > > regmap_config.max_register = info->desc->npins * regmap_config.reg_stride;
> > >
> > > > .name = "pincfg",
> > > > };
> >
> > If regmap configuration may be const, I would prefer to have a
> > hardcoded value and different configuration based on the chip, but if
> > it's not feasible, then this could suffice.
>
> What about if we do something like:
>
> const struct regmap_config regmap_config = {
> ...
> .max_register = info->desc->npins * 4,
> ...
> };
>
> This is based on what Colin suggested only that we keep the const.

As long as it's const, I like it.

--
With Best Regards,
Andy Shevchenko