| Message ID | 20240214-mbly-gpio-v1-0-f88c0ccf372b@bootlin.com |
|---|---|
| Headers | show |
| Series | Rework Nomadik GPIO to add Mobileye EyeQ5 support | expand |
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Create gpio/st,nomadik-gpio.yaml json-schema dt-bindings file as a > direct translation from gpio-nmk.txt. Remove the txt file. > > Add clocks and gpio-ranges properties which were missing and are being > used in Nomadik devicetrees. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Thanks for doing this. With the fix pointed out by Krzysztof folded in: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > This GPIO controller can support a lesser number of GPIOs than 32. > Express that in devicetree using an optional, generic property. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml b/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml > index a999908173d2..bbd23daed229 100644 > --- a/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml > +++ b/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml > @@ -50,6 +50,10 @@ properties: > gpio-ranges: > maxItems: 1 > > + ngpios: > + minimum: 0 > + maximum: 32 I can't help but wonder what good is ngpios = <0>; but OK. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hello, On Mon Feb 19, 2024 at 3:50 PM CET, Linus Walleij wrote: > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > This GPIO controller can support a lesser number of GPIOs than 32. > > Express that in devicetree using an optional, generic property. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml b/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml > > index a999908173d2..bbd23daed229 100644 > > --- a/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml > > +++ b/Documentation/devicetree/bindings/gpio/st,nomadik-gpio.yaml > > @@ -50,6 +50,10 @@ properties: > > gpio-ranges: > > maxItems: 1 > > > > + ngpios: > > + minimum: 0 > > + maximum: 32 > > I can't help but wonder what good is ngpios = <0>; but OK. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> A min value is required, else it is equal to the max. There was no reason to pick anything bigger than zero. I'll admit it is not a setup I have tested so the driver might have issues with the ngpios==0 edge-case. Of course I do not expect anyone to care. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Add optional reset device-tree property to the Nomadik GPIO controller. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Hi, > > This patch series reworks the Nomadik GPIO driver to bring it up to date > to current kernel standards. We then add Mobileye EyeQ5 support that > uses the same IP block but with limited functionality. We also add > features required by our newly supported platform: > > - Dynamic GPIO ID allocation; > - Make clock optional; > - Shared IRQ (usecase: EyeQ5 has two banks using the same IRQ); > - Handle variadic GPIO counts (usecase: EyeQ5 has <32 GPIOs per bank); > - Grab optional reset at probe (usecase: EyeQ5 has a reset available). > > This GPIO platform driver was previously declared & registered inside > drivers/pinctrl/nomadik/pinctrl-nomadik.c, side-by-side with the > pinctrl driver. Both are tightly integrated, mostly for muxing reasons. > Now that gpio-nomadik is used for another platform, we loosen the > relationship. The behavior should not change on already supported > hardware but I do not have Nomadik hardware to test for that. > I hope Linus can leave his Tested-by under this series then. > We have some dependencies, kept neatly to the end. Those are: > - The base platform support series from Grégory [1]. This relates to the > last four patches (20 thru 23), ie defconfig and devicetree. > - The OLB syscon support series [0]. It provides reset and pinctrl nodes > inside the devicetree. This relates to the last two patches (22 and > 23), ie resets and gpio-ranges DT props. GPIO works fine without it > if patches 22 and 23 are dropped. > > This has been tested on the EyeQ5 hardware, with the two parent series > applied. It also works fine without the OLB syscon series when our last > two patches are removed. It has been built on both Arm defconfigs that > rely on pinctrl-nomadik: nhk8815_defconfig and u8500_defconfig. I don't > have any Nomadik hardware to test though. > > Have a nice day, > Théo > Are you targeting the GPIO branch with this or pinctrl? I guess GPIO so I'll need Linus' Acks under the pinctrl patches. > [0]: https://lore.kernel.org/lkml/20240212-mbly-clk-v6-0-c46fa1f93839@bootlin.com/ > [1]: https://lore.kernel.org/lkml/20240205153503.574468-1-gregory.clement@bootlin.com/ Please advise the relevant maintainers that they should provide an immutable branch for these series once they're queued in their respective trees. Bart [snip]
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > handler type from a chained handler (as used by gpiolib > for ->parent_handler) to a threaded IRQ. > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > CASCADED GPIO IRQCHIPS" for additional information. > Any reason for not using preempt_disable()? Bart [snip]
Hello, On Mon Feb 19, 2024 at 4:48 PM CET, Bartosz Golaszewski wrote: > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > > handler type from a chained handler (as used by gpiolib > > for ->parent_handler) to a threaded IRQ. > > > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > > CASCADED GPIO IRQCHIPS" for additional information. > > > > Any reason for not using preempt_disable()? I did what the doc recommended: > The generic_handle_irq() is expected to be called with IRQ disabled, > so the IRQ core will complain if it is called from an IRQ handler which is > forced to a thread. The "fake?" raw lock can be used to work around this > problem:: > > raw_spinlock_t wa_lock; > static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank) > unsigned long wa_lock_flags; > raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags); > generic_handle_irq(irq_find_mapping(bank->chip.irq.domain, bit)); > raw_spin_unlock_irqrestore(&bank->wa_lock, wa_lock_flags); If you confirm I should be using preempt_disable() that's what I'll do in the next revision. I could even throw in a documentation patch if the advice is outdated. Thanks Bartosz, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, Feb 19, 2024 at 4:54 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Hello, > > On Mon Feb 19, 2024 at 4:48 PM CET, Bartosz Golaszewski wrote: > > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > > > handler type from a chained handler (as used by gpiolib > > > for ->parent_handler) to a threaded IRQ. > > > > > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > > > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > > > CASCADED GPIO IRQCHIPS" for additional information. > > > > > > > Any reason for not using preempt_disable()? > > I did what the doc recommended: > > > The generic_handle_irq() is expected to be called with IRQ disabled, > > so the IRQ core will complain if it is called from an IRQ handler which is > > forced to a thread. The "fake?" raw lock can be used to work around this > > problem:: > > > > raw_spinlock_t wa_lock; > > static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank) > > unsigned long wa_lock_flags; > > raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags); > > generic_handle_irq(irq_find_mapping(bank->chip.irq.domain, bit)); > > raw_spin_unlock_irqrestore(&bank->wa_lock, wa_lock_flags); > > If you confirm I should be using preempt_disable() that's what I'll do > in the next revision. I could even throw in a documentation patch if > the advice is outdated. > > Thanks Bartosz, This was added 9 years ago: commit c307b002548590c5d8c32b964831de671ad4affe Author: Grygorii Strashko <grygorii.strashko@ti.com> Date: Tue Oct 20 17:22:15 2015 +0300 gpio: add a real time compliance notes Put in a compliance checklist. Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> I'm Cc'ing Grygorii - maybe he can remember if there was any reason for using a spinlock over preempt_disable(). But for now I'd go with the latter. Bart > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, Feb 19, 2024 at 4:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > > handler type from a chained handler (as used by gpiolib > > for ->parent_handler) to a threaded IRQ. > > > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > > CASCADED GPIO IRQCHIPS" for additional information. > > > > Any reason for not using preempt_disable()? I think this needs to be discussed with tglx if Grygorii is not available. Yours, Linus Walleij
Hi Theo, thanks for your patch! On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > We create a custom compatible for the STA2X11 IP block as integrated > into the Mobileye EyeQ5 platform. Its wake and alternate functions have > been disabled, we want to avoid touching those registers. > > We both do: (1) early return in functions that do not support the > platform, but with warnings, and (2) avoid calling those functions in > the first place. > > We ensure that pinctrl-nomadik is not used with this STA2X11 variant. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> (...) >+ bool quirk_mbly; Compulsive abbreviation? I would just rename it: bool is_mobileye_soc; Nevermind the long name, it makes it crystal clear for readers what is going on. (Rusty Russell's API naming guidelines.) With that changed: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > We create a custom compatible for the STA2X11 IP block as integrated > into the Mobileye EyeQ5 platform. Its wake and alternate functions have > been disabled, we want to avoid touching those registers. > > We both do: (1) early return in functions that do not support the > platform, but with warnings, and (2) avoid calling those functions in > the first place. > > We ensure that pinctrl-nomadik is not used with this STA2X11 variant. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> When testing I noticed that this patch breaks Ux500 (up until patch 17 all works fine!). But I don't know why. Trying to figure it out... Yours, Linus Walleij
Hi, This patch series reworks the Nomadik GPIO driver to bring it up to date to current kernel standards. We then add Mobileye EyeQ5 support that uses the same IP block but with limited functionality. We also add features required by our newly supported platform: - Dynamic GPIO ID allocation; - Make clock optional; - Shared IRQ (usecase: EyeQ5 has two banks using the same IRQ); - Handle variadic GPIO counts (usecase: EyeQ5 has <32 GPIOs per bank); - Grab optional reset at probe (usecase: EyeQ5 has a reset available). This GPIO platform driver was previously declared & registered inside drivers/pinctrl/nomadik/pinctrl-nomadik.c, side-by-side with the pinctrl driver. Both are tightly integrated, mostly for muxing reasons. Now that gpio-nomadik is used for another platform, we loosen the relationship. The behavior should not change on already supported hardware but I do not have Nomadik hardware to test for that. We have some dependencies, kept neatly to the end. Those are: - The base platform support series from Grégory [1]. This relates to the last four patches (20 thru 23), ie defconfig and devicetree. - The OLB syscon support series [0]. It provides reset and pinctrl nodes inside the devicetree. This relates to the last two patches (22 and 23), ie resets and gpio-ranges DT props. GPIO works fine without it if patches 22 and 23 are dropped. This has been tested on the EyeQ5 hardware, with the two parent series applied. It also works fine without the OLB syscon series when our last two patches are removed. It has been built on both Arm defconfigs that rely on pinctrl-nomadik: nhk8815_defconfig and u8500_defconfig. I don't have any Nomadik hardware to test though. Have a nice day, Théo [0]: https://lore.kernel.org/lkml/20240212-mbly-clk-v6-0-c46fa1f93839@bootlin.com/ [1]: https://lore.kernel.org/lkml/20240205153503.574468-1-gregory.clement@bootlin.com/ Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Théo Lebrun (23): dt-bindings: gpio: nomadik: convert into yaml format dt-bindings: gpio: nomadik: add optional ngpios property dt-bindings: gpio: nomadik: add mobileye,eyeq5-gpio compatible dt-bindings: gpio: nomadik: add optional reset property gpio: nomadik: extract GPIO platform driver from drivers/pinctrl/nomadik/ pinctrl: nomadik: fix build warning (-Wformat) pinctrl: nomadik: fix build warning (-Wpointer-to-int-cast) pinctrl: nomadik: minimise indentation in probe pinctrl: nomadik: follow type-system kernel coding conventions pinctrl: nomadik: follow whitespace kernel coding conventions pinctrl: nomadik: follow conditional kernel coding conventions gpio: nomadik: request dynamic ID allocation gpio: nomadik: fix offset bug in nmk_pmx_set() gpio: nomadik: make clock optional gpio: nomadik: change driver name from gpio to gpio-nomadik gpio: nomadik: support shared GPIO IRQs gpio: nomadik: handle variadic GPIO count gpio: nomadik: support mobileye,eyeq5-gpio gpio: nomadik: grab optional reset control and deassert it at probe MIPS: eyeq5_defconfig: enable GPIO by default MIPS: mobileye: eyeq5: add two GPIO bank nodes MIPS: mobileye: eyeq5: add resets to GPIO banks MIPS: mobileye: eyeq5: map GPIOs to pins using gpio-ranges .../devicetree/bindings/gpio/gpio-nmk.txt | 31 - .../devicetree/bindings/gpio/st,nomadik-gpio.yaml | 96 +++ MAINTAINERS | 2 + arch/mips/boot/dts/mobileye/eyeq5.dtsi | 30 + arch/mips/configs/eyeq5_defconfig | 2 + drivers/gpio/Kconfig | 13 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-nomadik.c | 725 ++++++++++++++++ drivers/pinctrl/nomadik/Kconfig | 5 +- drivers/pinctrl/nomadik/pinctrl-nomadik-db8500.c | 3 +- drivers/pinctrl/nomadik/pinctrl-nomadik-stn8815.c | 3 +- drivers/pinctrl/nomadik/pinctrl-nomadik.c | 938 +++------------------ .../linux/gpio/gpio-nomadik.h | 124 ++- 13 files changed, 1118 insertions(+), 855 deletions(-) --- base-commit: d55aa725e32849f709b61eab3b7a50b810a71a84 change-id: 20231023-mbly-gpio-a30571ec3283 Best regards,