| Message ID | 20220209230452.19535-1-aidanmacdonald.0x0@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: ingenic: Fix regmap on X series SoCs | expand |
Hi Aidan, On 2022/2/10 上午7:04, Aidan MacDonald wrote: > The X series Ingenic SoCs have a shadow GPIO group which > is at a higher offset than the other groups, and is used > for all GPIO configuration. The regmap did not take this > offset into account and set max_register too low. Writes > to the shadow group registers were blocked, which made it > impossible to change any pin configuration. > > Fix this by pretending there are at least 8 chips on any > 'X' SoC for the purposes of calculating max_register. This > ensures the shadow group is accessible. > > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > drivers/pinctrl/pinctrl-ingenic.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) The M200 SoC (once planned to be called JZ4785) has a different shadow register offset address, if it needs to be supported in the future, then we need to deal with it further, but fortunately pinctrl-ingenic.c does not involve M200 support yet, so: Reviewed-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> Thanks and best regards! > > diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c > index 2712f51eb238..9d2bccda50f1 100644 > --- a/drivers/pinctrl/pinctrl-ingenic.c > +++ b/drivers/pinctrl/pinctrl-ingenic.c > @@ -4168,7 +4168,10 @@ static int __init ingenic_pinctrl_probe(struct platform_device *pdev) > return PTR_ERR(base); > > regmap_config = ingenic_pinctrl_regmap_config; > - regmap_config.max_register = chip_info->num_chips * chip_info->reg_offset; > + if (chip_info->version >= ID_X1000) > + regmap_config.max_register = MIN(8, chip_info->num_chips) * chip_info->reg_offset; > + else > + regmap_config.max_register = chip_info->num_chips * chip_info->reg_offset; > > jzpc->map = devm_regmap_init_mmio(dev, base, ®map_config); > if (IS_ERR(jzpc->map)) {
Hi Aidan, Le mer., févr. 9 2022 at 23:04:54 +0000, Aidan MacDonald <aidanmacdonald.0x0@gmail.com> a écrit : > The X series Ingenic SoCs have a shadow GPIO group which > is at a higher offset than the other groups, and is used > for all GPIO configuration. The regmap did not take this > offset into account and set max_register too low. Writes > to the shadow group registers were blocked, which made it > impossible to change any pin configuration. > > Fix this by pretending there are at least 8 chips on any > 'X' SoC for the purposes of calculating max_register. This > ensures the shadow group is accessible. I don't like your solution, it sounds very hacky. I think it would make more sense to use a dedicated x1000_pinctrl_regmap_config that would be used for the X1000 SoC. That would also allow you to express that there are no registers in the 0x400-0x700 range (through regmap_config.wr_table / .rd_table). Cheers, -Paul > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > drivers/pinctrl/pinctrl-ingenic.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-ingenic.c > b/drivers/pinctrl/pinctrl-ingenic.c > index 2712f51eb238..9d2bccda50f1 100644 > --- a/drivers/pinctrl/pinctrl-ingenic.c > +++ b/drivers/pinctrl/pinctrl-ingenic.c > @@ -4168,7 +4168,10 @@ static int __init ingenic_pinctrl_probe(struct > platform_device *pdev) > return PTR_ERR(base); > > regmap_config = ingenic_pinctrl_regmap_config; > - regmap_config.max_register = chip_info->num_chips * > chip_info->reg_offset; > + if (chip_info->version >= ID_X1000) > + regmap_config.max_register = MIN(8, chip_info->num_chips) * > chip_info->reg_offset; > + else > + regmap_config.max_register = chip_info->num_chips * > chip_info->reg_offset; > > jzpc->map = devm_regmap_init_mmio(dev, base, ®map_config); > if (IS_ERR(jzpc->map)) { > -- > 2.34.1 >
On Thu, Feb 10, 2022 at 11:03:13AM +0000, Paul Cercueil wrote: > Hi Aidan, > > Le mer., févr. 9 2022 at 23:04:54 +0000, Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> a écrit : > > The X series Ingenic SoCs have a shadow GPIO group which > > is at a higher offset than the other groups, and is used > > for all GPIO configuration. The regmap did not take this > > offset into account and set max_register too low. Writes > > to the shadow group registers were blocked, which made it > > impossible to change any pin configuration. > > > > Fix this by pretending there are at least 8 chips on any > > 'X' SoC for the purposes of calculating max_register. This > > ensures the shadow group is accessible. > > I don't like your solution, it sounds very hacky. I think it would make > more sense to use a dedicated x1000_pinctrl_regmap_config that would be > used for the X1000 SoC. That would also allow you to express that there > are no registers in the 0x400-0x700 range (through > regmap_config.wr_table / .rd_table). > > Cheers, > -Paul > That's fine, I'll put together a v2 patch using the approach you suggest. I think all the 'X' SoCs will require a similar regmap as they're using ingenic_gpio_shadow_* functions too, but I'll check their datasheets to be sure. > > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > > --- > > drivers/pinctrl/pinctrl-ingenic.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pinctrl/pinctrl-ingenic.c > > b/drivers/pinctrl/pinctrl-ingenic.c > > index 2712f51eb238..9d2bccda50f1 100644 > > --- a/drivers/pinctrl/pinctrl-ingenic.c > > +++ b/drivers/pinctrl/pinctrl-ingenic.c > > @@ -4168,7 +4168,10 @@ static int __init ingenic_pinctrl_probe(struct > > platform_device *pdev) > > return PTR_ERR(base); > > > > regmap_config = ingenic_pinctrl_regmap_config; > > - regmap_config.max_register = chip_info->num_chips * > > chip_info->reg_offset; > > + if (chip_info->version >= ID_X1000) > > + regmap_config.max_register = MIN(8, chip_info->num_chips) * > > chip_info->reg_offset; > > + else > > + regmap_config.max_register = chip_info->num_chips * > > chip_info->reg_offset; > > > > jzpc->map = devm_regmap_init_mmio(dev, base, ®map_config); > > if (IS_ERR(jzpc->map)) { > > -- > > 2.34.1 > > > >
diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c index 2712f51eb238..9d2bccda50f1 100644 --- a/drivers/pinctrl/pinctrl-ingenic.c +++ b/drivers/pinctrl/pinctrl-ingenic.c @@ -4168,7 +4168,10 @@ static int __init ingenic_pinctrl_probe(struct platform_device *pdev) return PTR_ERR(base); regmap_config = ingenic_pinctrl_regmap_config; - regmap_config.max_register = chip_info->num_chips * chip_info->reg_offset; + if (chip_info->version >= ID_X1000) + regmap_config.max_register = MIN(8, chip_info->num_chips) * chip_info->reg_offset; + else + regmap_config.max_register = chip_info->num_chips * chip_info->reg_offset; jzpc->map = devm_regmap_init_mmio(dev, base, ®map_config); if (IS_ERR(jzpc->map)) {
The X series Ingenic SoCs have a shadow GPIO group which is at a higher offset than the other groups, and is used for all GPIO configuration. The regmap did not take this offset into account and set max_register too low. Writes to the shadow group registers were blocked, which made it impossible to change any pin configuration. Fix this by pretending there are at least 8 chips on any 'X' SoC for the purposes of calculating max_register. This ensures the shadow group is accessible. Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> --- drivers/pinctrl/pinctrl-ingenic.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)