mbox series

[0/5] Add GPIO support for MStar/SigmaStar ARMv7

Message ID 20201011024831.3868571-1-daniel@0x0f.com
Headers show
Series Add GPIO support for MStar/SigmaStar ARMv7 | expand

Message

Daniel Palmer Oct. 11, 2020, 2:48 a.m. UTC
At the moment the MStar/SigmaStar support is only really
capable of shell from an initramfs and not much else.

Most of the interesting drivers are blocked on clock and pinctrl
drivers and those are going to take me a little while to get cleaned
up.

Clock and pinctrl aren't needed for basic GPIO to work (all pins
start off as GPIOs..) and it makes it possible to actually do something
so this series adds everything that is needed for the main GPIO
block in these chips.

Daniel Palmer (5):
  dt-bindings: gpio: Binding for MStar MSC313 GPIO controller
  dt-bindings: gpio: Add a binding header for the MSC313 GPIO driver
  gpio: msc313: MStar MSC313 GPIO driver
  ARM: mstar: Add gpio controller to MStar base dtsi
  ARM: mstar: Fill in GPIO controller properties for infinity

 .../bindings/gpio/mstar,msc313-gpio.yaml      |  69 ++++
 MAINTAINERS                                   |   3 +
 arch/arm/boot/dts/mstar-infinity.dtsi         |  16 +
 arch/arm/boot/dts/mstar-v7.dtsi               |   7 +
 drivers/gpio/Kconfig                          |   9 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-msc313.c                    | 341 ++++++++++++++++++
 include/dt-bindings/gpio/msc313-gpio.h        |  95 +++++
 8 files changed, 541 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 create mode 100644 drivers/gpio/gpio-msc313.c
 create mode 100644 include/dt-bindings/gpio/msc313-gpio.h

Comments

Linus Walleij Oct. 16, 2020, 4:36 p.m. UTC | #1
On Sun, Oct 11, 2020 at 4:48 AM Daniel Palmer <daniel@0x0f.com> wrote:

> Add a binding description for the MStar/SigmaStar GPIO controller
> found in the MSC313 and later ARMv7 SoCs.
>
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>

I think Krzysztof is working on some generic bindings that
will make it easier to write YAML GPIO controller bindings,
but I don't know the status of them. I would be happy to merge
them early for v5.11 though.

Yours,
Linus Walleij
Linus Walleij Oct. 16, 2020, 4:56 p.m. UTC | #2
Hi Daniel,

thanks for your patch!

Some comments below, we need some work but keep at it.

On Sun, Oct 11, 2020 at 4:48 AM Daniel Palmer <daniel@0x0f.com> wrote:

> This adds a driver that supports the GPIO block found in
> MStar/SigmaStar ARMv7 SoCs.
>
> The controller seems to support 128 lines but where they
> are wired up differs between chips and no currently known
> chip uses anywhere near 128 lines so there needs to be some
> per-chip data to collect together what lines actually have
> physical pins attached and map the right names to them.
>
> The core peripherals seem to use the same lines on the
> currently known chips but the lines used for the sensor
> interface, lcd controller etc pins seem to be totally
> different between the infinity and mercury chips
>
> The code tries to collect all of the re-usable names,
> offsets etc together so that it's easy to build the extra
> per-chip data for other chips in the future.
>
> So far this only supports the MSC313 and MSC313E chips.
>
> Support for the SSC8336N (mercury5) is trivial to add once
> all of the lines have been mapped out.
>
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>

(...)

> +config GPIO_MSC313
> +       bool "MStar MSC313 GPIO support"
> +       default y if ARCH_MSTARV7
> +       depends on ARCH_MSTARV7
> +       select GPIO_GENERIC

Selecting GPIO_GENERIC, that is good.
But you're not using it, because you can't.
This chip does not have the bits lined up nicely
in one register, instead there seems to be something
like one register per line, right?
So skip GPIO_GENERIC.

> +#define MSC313_GPIO_IN  BIT(0)
> +#define MSC313_GPIO_OUT BIT(4)
> +#define MSC313_GPIO_OEN BIT(5)
> +
> +#define MSC313_GPIO_BITSTOSAVE (MSC313_GPIO_OUT | MSC313_GPIO_OEN)

Some comment here telling us why these need saving and
not others.

> +#define FUART_NAMES                    \
> +       MSC313_PINNAME_FUART_RX,        \
> +       MSC313_PINNAME_FUART_TX,        \
> +       MSC313_PINNAME_FUART_CTS,       \
> +       MSC313_PINNAME_FUART_RTS
> +
> +#define OFF_FUART_RX   0x50
> +#define OFF_FUART_TX   0x54
> +#define OFF_FUART_CTS  0x58
> +#define OFF_FUART_RTS  0x5c
> +
> +#define FUART_OFFSETS  \
> +       OFF_FUART_RX,   \
> +       OFF_FUART_TX,   \
> +       OFF_FUART_CTS,  \
> +       OFF_FUART_RTS

This looks a bit strange. The GPIO driver should not really
have to know about any other use cases for pins than
GPIO. But I guess it is intuitive for the driver.

> +#define SD_NAMES               \
> +       MSC313_PINNAME_SD_CLK,  \
> +       MSC313_PINNAME_SD_CMD,  \
> +       MSC313_PINNAME_SD_D0,   \
> +       MSC313_PINNAME_SD_D1,   \
> +       MSC313_PINNAME_SD_D2,   \
> +       MSC313_PINNAME_SD_D3
> +
> +#define OFF_SD_CLK     0x140
> +#define OFF_SD_CMD     0x144
> +#define OFF_SD_D0      0x148
> +#define OFF_SD_D1      0x14cchild_to_parent_hwirq
> +#define OFF_SD_D2      0x150
> +#define OFF_SD_D3      0x154
> +
> +#define SD_OFFSETS     \
> +       OFF_SD_CLK,     \
> +       OFF_SD_CMD,     \
> +       OFF_SD_D0,      \
> +       OFF_SD_D1,      \
> +       OFF_SD_D2,      \
> +       OFF_SD_D3
> +
> +#define I2C1_NAMES                     \
> +       MSC313_PINNAME_I2C1_SCL,        \
> +       MSC313_PINNAME_I2C1_SCA
> +
> +#define OFF_I2C1_SCL   0x188
> +#define OFF_I2C1_SCA   0x18c
> +
> +#define I2C1_OFFSETS   \
> +       OFF_I2C1_SCL,   \
> +       OFF_I2C1_SCA
> +
> +#define SPI0_NAMES             \
> +       MSC313_PINNAME_SPI0_CZ, \
> +       MSC313_PINNAME_SPI0_CK, \
> +       MSC313_PINNAME_SPI0_DI, \
> +       MSC313_PINNAME_SPI0_DO
> +
> +#define OFF_SPI0_CZ    0x1c0
> +#define OFF_SPI0_CK    0x1c4
> +#define OFF_SPI0_DI    0x1c8
> +#define OFF_SPI0_DO    0x1cc
> +
> +#define SPI0_OFFSETS   \
> +       OFF_SPI0_CZ,    \
> +       OFF_SPI0_CK,    \
> +       OFF_SPI0_DI,    \
> +       OFF_SPI0_DO

Same with all these. I suppose it is the offsets of stuff
that would be there unless we were using it for GPIO.

> +static int msc313_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct msc313_gpio *gpio = gpiochip_get_data(chip);
> +> +

> +       return gpio->irqs[offset];
> +}

Please do not use custom IRQ handling like this.
As there seems to be one IRQ per line, look into using

        select GPIOLIB_IRQCHIP
        select IRQ_DOMAIN_HIERARCHY

See for example in gpio-ixp4xx.c how we deal with
hiearchical GPIO IRQs.

> +       gpiochip->to_irq = msc313_gpio_to_irq;
> +       gpiochip->base = -1;
> +       gpiochip->ngpio = gpio->gpio_data->num;
> +       gpiochip->names = gpio->gpio_data->names;
> +
> +       for (i = 0; i < gpiochip->ngpio; i++)
> +               gpio->irqs[i] = of_irq_get_byname(pdev->dev.of_node, gpio->gpio_data->names[i]);

Use hierarchical generic GPIO IRQs for these.

Assign ->fwnode, ->parent_domain, ->child_to_parent_hwirq,
and probably also ->handler on the struct gpio_irq_chip *.

Skip assigning gpiochip->to_irq, the generic code will
handle that.

Again see gpio-ixp4xx.c for an example.

Yours,
Linus Walleij
Krzysztof Kozlowski Oct. 19, 2020, 4:13 p.m. UTC | #3
On Fri, 16 Oct 2020 at 18:36, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sun, Oct 11, 2020 at 4:48 AM Daniel Palmer <daniel@0x0f.com> wrote:
>
> > Add a binding description for the MStar/SigmaStar GPIO controller
> > found in the MSC313 and later ARMv7 SoCs.
> >
> > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
>
> I think Krzysztof is working on some generic bindings that
> will make it easier to write YAML GPIO controller bindings,
> but I don't know the status of them. I would be happy to merge
> them early for v5.11 though.

Hi,

The generic GPIO controller dtschema got dropped because Rob wants it
to be part of dtschema (outside of kernel) and then
relicensing/rewriting property descriptions plays a role. Only the
GPIO hogs went to common dtschema package.

Therefore as of now, one should include all generic properties
directly in the GPIO controller bindings.

Best regards,
Krzysztof
Daniel Palmer Oct. 21, 2020, 11:07 a.m. UTC | #4
Hi Linus,

Sorry to pester you again...

On Sat, 17 Oct 2020 at 01:56, Linus Walleij <linus.walleij@linaro.org> wrote:

> > +       gpiochip->to_irq = msc313_gpio_to_irq;
> > +       gpiochip->base = -1;
> > +       gpiochip->ngpio = gpio->gpio_data->num;
> > +       gpiochip->names = gpio->gpio_data->names;
> > +
> > +       for (i = 0; i < gpiochip->ngpio; i++)
> > +               gpio->irqs[i] = of_irq_get_byname(pdev->dev.of_node, gpio->gpio_data->names[i]);
>
> Use hierarchical generic GPIO IRQs for these.
>
> Assign ->fwnode, ->parent_domain, ->child_to_parent_hwirq,
> and probably also ->handler on the struct gpio_irq_chip *.
>
> Skip assigning gpiochip->to_irq, the generic code will
> handle that.
>
> Again see gpio-ixp4xx.c for an example.

I sent a v2 with this conversion already and it looks a lot better.
Based on Andy Shevchenko's comments[0] I'll be sending a v3 that fixes
up all style and other issues he found.
Before I do that I have a question that maybe you could help me with:
Andy noted a few times that I have this driver as a built in driver
and not a module.
The gpio-ixp4xx.c driver is also a built in driver. Is there a reason
why it's ok there but not this driver?
I've actually changed it to allow building as a module already but I
don't want to push a v3 if something like the interrupt handling means
it should actually be a built in and I'm just missing something.

Thanks,

Daniel

0 - https://lore.kernel.org/linux-gpio/CAHp75Vf5iUzKp32CqBbv_5MRo8q8CyBPsBcgzKsww6BFtGJwUA@mail.gmail.com/
Linus Walleij Nov. 5, 2020, 9:13 a.m. UTC | #5
On Mon, Oct 19, 2020 at 6:13 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:

> The generic GPIO controller dtschema got dropped because Rob wants it
> to be part of dtschema (outside of kernel) and then
> relicensing/rewriting property descriptions plays a role. Only the
> GPIO hogs went to common dtschema package.
>
> Therefore as of now, one should include all generic properties
> directly in the GPIO controller bindings.

Oh now I am confused.

Rob, what is the plan here?

Am I *not* to create say gpio-controller.yaml for $ref:in into
other controllers?

Yours,
Linus Walleij
Linus Walleij Nov. 5, 2020, 9:21 a.m. UTC | #6
On Wed, Oct 21, 2020 at 1:07 PM Daniel Palmer <daniel@0x0f.com> wrote:

> Sorry to pester you again...

Don't worry. I'm more worried that my replies are slow.

> Before I do that I have a question that maybe you could help me with:
> Andy noted a few times that I have this driver as a built in driver
> and not a module.
> The gpio-ixp4xx.c driver is also a built in driver. Is there a reason
> why it's ok there but not this driver?

Not that I know of. There is a lot of push for modularization right
now because Android (and other distributions) likes it, so if your
SoC could be used by Android or Fedora or Debian etc it is
generally a good idea to modularize.

These distributions use the generic ARM (etc) kernel and try
to load as many drivers as possible as modules.

It is not always possible because some GPIOs might be needed
very early, such as on-chip GPIO. So you better make sure
that the platform can get to userspace also without this driver
compiled in, otherwise it *MUST* be bool so people don't get
ammunition to shoot themselves in the foot and configure a
non-bootable kernel just because they could modularize this
driver.

If your SoC is only used by OpenWrt (like ixp4xx) then it is fine
to just use bool because that distribution is always built with an
image for a specific hardware, whereas distributions are generic.

So it actually depends a bit on the usecase of the SoC.

Yours,
Linus Walleij
Willy Tarreau Nov. 5, 2020, 9:31 a.m. UTC | #7
On Thu, Nov 05, 2020 at 10:21:27AM +0100, Linus Walleij wrote:
> If your SoC is only used by OpenWrt (like ixp4xx) then it is fine
> to just use bool because that distribution is always built with an
> image for a specific hardware, whereas distributions are generic.

Speaking for myself (since I have a few now), I'm not running OpenWRT
on mine but my own distro, and I guess most users will run either
Buildroot or their own distro. It's unlikely that we'll see very
generic distros there given the limited storage you'd typically have
in an SPI NOR (16-32 MB) and the small RAM (64MB) which tends to
discourage anyone from booting a regular distro over other storage
anyway.

Thus my guess is that most users will keep building their own kernels.

But this just emphasizes your points :-)

Just my two cents,
Willy
Linus Walleij Nov. 5, 2020, 9:42 a.m. UTC | #8
On Thu, Nov 5, 2020 at 10:31 AM Willy Tarreau <w@1wt.eu> wrote:
> On Thu, Nov 05, 2020 at 10:21:27AM +0100, Linus Walleij wrote:

> > If your SoC is only used by OpenWrt (like ixp4xx) then it is fine
> > to just use bool because that distribution is always built with an
> > image for a specific hardware, whereas distributions are generic.
>
> Speaking for myself (since I have a few now), I'm not running OpenWRT
> on mine but my own distro, and I guess most users will run either
> Buildroot or their own distro. It's unlikely that we'll see very
> generic distros there given the limited storage you'd typically have
> in an SPI NOR (16-32 MB) and the small RAM (64MB) which tends to
> discourage anyone from booting a regular distro over other storage
> anyway.
>
> Thus my guess is that most users will keep building their own kernels.
>
> But this just emphasizes your points :-)

I think that is a good argument to keep this as bool.

Yours,
Linus Walleij
Daniel Palmer Nov. 5, 2020, 3:39 p.m. UTC | #9
Hi Linus,

Thanks for all of the comments.

On Thu, 5 Nov 2020 at 18:42, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Nov 5, 2020 at 10:31 AM Willy Tarreau <w@1wt.eu> wrote:
> > On Thu, Nov 05, 2020 at 10:21:27AM +0100, Linus Walleij wrote:
>
> > > If your SoC is only used by OpenWrt (like ixp4xx) then it is fine
> > > to just use bool because that distribution is always built with an
> > > image for a specific hardware, whereas distributions are generic.
> >
.. snip ..
>> It's unlikely that we'll see very
> > generic distros there given the limited storage you'd typically have
> > in an SPI NOR (16-32 MB) and the small RAM (64MB) which tends to
> > discourage anyone from booting a regular distro over other storage
> > anyway.
> >
> > Thus my guess is that most users will keep building their own kernels.
> >
> > But this just emphasizes your points :-)
>
> I think that is a good argument to keep this as bool.

Thanks. I did change it to a tristate for v3 but I'll change it back.

Just a heads up:
There is another GPIO driver for this chip (same functionality,
totally different register layout for no reason) that'll look pretty
similar to this that'll follow soon.
It might be similar enough that people confuse the two series as the same thing.

Thanks,

Daniel