pinctrl: starfive: switch to regmap for gpio registers

[pdp7]

switch from readl() and writel() to regmap_read() and regmap_write() for the GPIO registers. Leave the padctl registers with using readl and writel for now as that is a different io memory range.

[esmil]

I don't think just wrapping readl() and writel() in regmap_read() and regmap_write() like this adds any value. What would add value is reusing some gpio wrappers like drivers/gpio/gpio-mmap.c or drivers/gpio/gpio-regmap.c.

For gpio-mmap I don't think we can use it as is because it assumes every GPIO is represented as 1 bit in each type of register, and the number GPIOs matches the register width. We have 64 GPIOs in 32bit register pairs, and the output and output enable functionality has a full 32bit register each. Maybe it could be modelled with 2 gpio_chips with 32 GPIOs each, but that just seems awkward and then userspace would need to know that GPIO42 is actually GPIO 10 on the 2nd gpiochip.

I think gpio-regmap could be brought to work, but I don't think it'll help making the code any more understandable. In particular the fact that the DOUT and DOEN registers are used for both pinmuxing and output/output enable respectively will be hidden inside the reg_mask_xlate callback. Also gpio-regmap doesn't implement the add_pin_ranges and set_config callbacks. The first is used to set up the gpio to pin mapping which depends on the chosen signal group (IO_PADSHARE_SEL value) and the second makes gpiomon -B pull-up work.

[pdp7]

I do think regmap_write() and regmap_read() are useful as they decouple void pointer to io memory from the address offset. This allows the offsets to be calculated as regular unsigned values.

Regarding the higher levels of abstraction, I agree I'm not sure how bgpio_init() could help given gpio-mmio seems to not fit this starfive soc. Linus W did not respond yet but I think it would take a lot of gpio-mmio changes for it to support an soc like this. In that case, is it still really "generic"?

It seems like the xlate functionality in regmap-gpio could be used, but I am not quite sure. I'm interested to see Michael Walle's response and how much he thinks it would be good to improve regmap-gpio for additional use cases. The StarFive SoC is much more complicated than the hardware that Michael's sl28cpld driver targets.

[esmil]

I do think regmap_write() and regmap_read() are useful as they decouple void pointer to io memory from the address offset. This allows the offsets to be calculated as regular unsigned values.

Hmm.. I'm not exactly sure why unsigned arithmetic is any better that pointer arithmetic. But if it is, couldn't you accomplish exactly the same with

static void starfive_reg_set(struct starfive_pinctrl *sfp, unsigned int offset, u32 value)
{
    writel_relaxed(value, sfp->base + offset);
}

static u32 starfive_reg_get(struct starfive_pinctrl *sfp, unsigned int offset)
{
    return readl_relaxed(sfp->base + offset);
}

..which regmaps on mmio eventually amounts to, except this version will be inlined by the compiler and hence cost nothing unlike regmaps?

[pdp7]

Yes, after the discussion with Michael Walle on IRC, I don't think it really makes sense to try to leverage gpio-regmap. Too much would need to be added to it for it to work with this SoC. At which point the value in being generic goes away.

I think the structure of this SoC really does mean it is needs to handle all the gpio and pinctrl in a single driver that does not leverage gpio-regmap or gpio-mmio. I guess it could be interesting to find a common abstraction if there is any other SoC that has similar register structure but I am not aware of any.

[esmil]

Thank you for following up on this.