| Message ID | 20240214-mbly-gpio-v1-18-f88c0ccf372b@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rework Nomadik GPIO to add Mobileye EyeQ5 support | expand |
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
Hello, On Wed Feb 21, 2024 at 3:31 PM CET, Linus Walleij wrote: > 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... Can I help in the debugging process? Reading the code once again I'd guess of_device_get_match_data(&gpio_pdev->dev) could be the root cause. We are accessing match data for the GPIO device while probing the pinctrl device. Maybe something isn't initialised properly yet? The rest looks rather harmless, I've checked all conditional expressions. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Feb 21, 2024 at 2:45 PM CET, Linus Walleij wrote: > 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> Makes complete sense. This is old heritage from my initial prototype that should have long gone disappeared. Thanks for your feedback & reviews! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Wed, Feb 21, 2024 at 5:16 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Trying to figure it out... > > Can I help in the debugging process? Nah, I found it :) > Reading the code once again I'd guess > of_device_get_match_data(&gpio_pdev->dev) could be the root cause. We > are accessing match data for the GPIO device while probing the pinctrl > device. Maybe something isn't initialised properly yet? The rest looks > rather harmless, I've checked all conditional expressions. Yep spot on. The nmk_gpio_populate_chip() is sometimes called from the pinctrl driver before the gpio probe() has been called, so the match data is NULL and we crash. This looks like it does for historical reasons and there could be better ways to fix it now that Saravana Kannan has fixed up the probe ordering code. The following is one way to fix it for now using device_is_compatible() (illustrating some other changes I did as well): diff --git a/drivers/gpio/gpio-nomadik.c b/drivers/gpio/gpio-nomadik.c index 21bb6d6363fc..11071a982ebb 100644 --- a/drivers/gpio/gpio-nomadik.c +++ b/drivers/gpio/gpio-nomadik.c @@ -27,6 +27,7 @@ #include <linux/of_platform.h> #include <linux/pinctrl/pinctrl.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/reset.h> #include <linux/seq_file.h> #include <linux/types.h> @@ -37,15 +38,13 @@ static DEFINE_SPINLOCK(nmk_gpio_slpm_lock); #endif -#define NMK_GPIO_FLAG_QUIRK_MBLY BIT(0) - void __nmk_gpio_set_slpm(struct nmk_gpio_chip *nmk_chip, unsigned int offset, enum nmk_gpio_slpm mode) { u32 slpm; /* We should NOT have been called. */ - if (WARN_ON(nmk_chip->quirk_mbly)) + if (WARN_ON(nmk_chip->is_mobileye_soc)) return; slpm = readl(nmk_chip->addr + NMK_GPIO_SLPC); @@ -105,7 +104,7 @@ static void __nmk_gpio_irq_modify(struct nmk_gpio_chip *nmk_chip, fimscval = &nmk_chip->fimsc; } else { /* We should NOT have been called. */ - if (WARN_ON(nmk_chip->quirk_mbly)) + if (WARN_ON(nmk_chip->is_mobileye_soc)) return; rimscreg = NMK_GPIO_RWIMSC; fimscreg = NMK_GPIO_FWIMSC; @@ -134,7 +133,7 @@ static void __nmk_gpio_set_wake(struct nmk_gpio_chip *nmk_chip, int offset, bool on) { /* We should NOT have been called. */ - if (WARN_ON(nmk_chip->quirk_mbly)) + if (WARN_ON(nmk_chip->is_mobileye_soc)) return; /* @@ -161,7 +160,7 @@ static void nmk_gpio_irq_maskunmask(struct nmk_gpio_chip *nmk_chip, __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, enable); - if (!nmk_chip->quirk_mbly && !(nmk_chip->real_wake & BIT(d->hwirq))) + if (!nmk_chip->is_mobileye_soc && !(nmk_chip->real_wake & BIT(d->hwirq))) __nmk_gpio_set_wake(nmk_chip, d->hwirq, enable); spin_unlock(&nmk_chip->lock); @@ -194,7 +193,7 @@ static int nmk_gpio_irq_set_wake(struct irq_data *d, unsigned int on) unsigned long flags; /* Handler is registered in all cases. */ - if (nmk_chip->quirk_mbly) + if (nmk_chip->is_mobileye_soc) return -ENXIO; clk_enable(nmk_chip->clk); @@ -235,7 +234,7 @@ static int nmk_gpio_irq_set_type(struct irq_data *d, unsigned int type) if (enabled) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, false); - if (!nmk_chip->quirk_mbly && (enabled || wake)) + if (!nmk_chip->is_mobileye_soc && (enabled || wake)) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, WAKE, false); nmk_chip->edge_rising &= ~BIT(d->hwirq); @@ -249,7 +248,7 @@ static int nmk_gpio_irq_set_type(struct irq_data *d, unsigned int type) if (enabled) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, true); - if (!nmk_chip->quirk_mbly && (enabled || wake)) + if (!nmk_chip->is_mobileye_soc && (enabled || wake)) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, WAKE, true); spin_unlock_irqrestore(&nmk_chip->lock, flags); @@ -383,7 +382,7 @@ static int nmk_gpio_get_mode(struct nmk_gpio_chip *nmk_chip, int offset) u32 afunc, bfunc; /* We don't support modes. */ - if (nmk_chip->quirk_mbly) + if (nmk_chip->is_mobileye_soc) return NMK_GPIO_ALT_GPIO; clk_enable(nmk_chip->clk); @@ -517,7 +516,6 @@ struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np, struct resource *res; struct clk *clk; void __iomem *base; - uintptr_t flags; u32 id, ngpio; gpio_pdev = of_find_device_by_node(np); @@ -551,8 +549,7 @@ struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np, dev_dbg(&pdev->dev, "populate: using default ngpio (%d)\n", ngpio); } - flags = (uintptr_t)of_device_get_match_data(&gpio_pdev->dev); - nmk_chip->quirk_mbly = !!(flags & NMK_GPIO_FLAG_QUIRK_MBLY); + nmk_chip->is_mobileye_soc = device_is_compatible(&gpio_pdev->dev, "mobileye,eyeq5-gpio"); nmk_chip->bank = id; chip = &nmk_chip->chip; chip->base = -1; @@ -667,7 +664,7 @@ static int nmk_gpio_probe(struct platform_device *pdev) return ret; } - if (!nmk_chip->quirk_mbly) { + if (!nmk_chip->is_mobileye_soc) { clk_enable(nmk_chip->clk); nmk_chip->lowemi = readl_relaxed(nmk_chip->addr + NMK_GPIO_LOWEMI); clk_disable(nmk_chip->clk); @@ -690,7 +687,6 @@ static const struct of_device_id nmk_gpio_match[] = { }, { .compatible = "mobileye,eyeq5-gpio", - .data = (void*)NMK_GPIO_FLAG_QUIRK_MBLY, }, {} }; diff --git a/include/linux/gpio/gpio-nomadik.h b/include/linux/gpio/gpio-nomadik.h index 8d0134dd3771..ede16cdaa920 100644 --- a/include/linux/gpio/gpio-nomadik.h +++ b/include/linux/gpio/gpio-nomadik.h @@ -51,6 +51,7 @@ enum nmk_gpio_slpm { struct nmk_gpio_chip { struct gpio_chip chip; + bool is_mobileye_soc; void __iomem *addr; struct clk *clk; unsigned int bank; Yours, Linus Walleij
Hello, On Wed Feb 21, 2024 at 8:36 PM CET, Linus Walleij wrote: > On Wed, Feb 21, 2024 at 5:16 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Trying to figure it out... > > > > Can I help in the debugging process? > > Nah, I found it :) > > > Reading the code once again I'd guess > > of_device_get_match_data(&gpio_pdev->dev) could be the root cause. We > > are accessing match data for the GPIO device while probing the pinctrl > > device. Maybe something isn't initialised properly yet? The rest looks > > rather harmless, I've checked all conditional expressions. > > Yep spot on. The nmk_gpio_populate_chip() is sometimes called from > the pinctrl driver before the gpio probe() has been called, so the match > data is NULL and we crash. > > This looks like it does for historical reasons and there could be better > ways to fix it now that Saravana Kannan has fixed up the probe ordering > code. > > The following is one way to fix it for now using device_is_compatible() > (illustrating some other changes I did as well): Thanks for the debugging and the proposed fix. Indeed matching on compatible to avoid match data for a bool makes sense. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index ff83371251c1..fe6112abb73a 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -480,11 +480,12 @@ config GPIO_MXS config GPIO_NOMADIK bool "Nomadik GPIO driver" - depends on ARCH_U8500 || ARCH_NOMADIK || COMPILE_TEST + depends on ARCH_U8500 || ARCH_NOMADIK || MACH_EYEQ5 || COMPILE_TEST select OF_GPIO select GPIOLIB_IRQCHIP help - Say yes here to support the Nomadik SoC GPIO block. + Say yes here to support the Nomadik SoC GPIO block. This block is also + used by the Mobileye EyeQ5 SoC. It handles up to 32 GPIOs per bank, that can all be interrupt sources. It is deeply interconnected with the associated pinctrl driver as GPIO diff --git a/drivers/gpio/gpio-nomadik.c b/drivers/gpio/gpio-nomadik.c index 02b53c58adf7..21bb6d6363fc 100644 --- a/drivers/gpio/gpio-nomadik.c +++ b/drivers/gpio/gpio-nomadik.c @@ -7,6 +7,12 @@ * The GPIO chips are shared with pinctrl-nomadik if used; it needs access for * pinmuxing functionality and others. * + * This driver also handles the mobileye,eyeq5-gpio compatible. It is an STA2X11 + * but with only data, direction and interrupts register active. We want to + * avoid touching SLPM, RWIMSC, FWIMSC, AFSLA and AFSLB registers; that is, + * wake and alternate function registers. It is NOT compatible with + * pinctrl-nomadik. + * * Copyright (C) 2008,2009 STMicroelectronics * Copyright (C) 2009 Alessandro Rubini <rubini@unipv.it> * Rewritten based on work by Prafulla WADASKAR <prafulla.wadaskar@st.com> @@ -31,11 +37,17 @@ static DEFINE_SPINLOCK(nmk_gpio_slpm_lock); #endif +#define NMK_GPIO_FLAG_QUIRK_MBLY BIT(0) + void __nmk_gpio_set_slpm(struct nmk_gpio_chip *nmk_chip, unsigned int offset, enum nmk_gpio_slpm mode) { u32 slpm; + /* We should NOT have been called. */ + if (WARN_ON(nmk_chip->quirk_mbly)) + return; + slpm = readl(nmk_chip->addr + NMK_GPIO_SLPC); if (mode == NMK_GPIO_SLPM_NOCHANGE) slpm |= BIT(offset); @@ -92,6 +104,9 @@ static void __nmk_gpio_irq_modify(struct nmk_gpio_chip *nmk_chip, rimscval = &nmk_chip->rimsc; fimscval = &nmk_chip->fimsc; } else { + /* We should NOT have been called. */ + if (WARN_ON(nmk_chip->quirk_mbly)) + return; rimscreg = NMK_GPIO_RWIMSC; fimscreg = NMK_GPIO_FWIMSC; rimscval = &nmk_chip->rwimsc; @@ -118,6 +133,10 @@ static void __nmk_gpio_irq_modify(struct nmk_gpio_chip *nmk_chip, static void __nmk_gpio_set_wake(struct nmk_gpio_chip *nmk_chip, int offset, bool on) { + /* We should NOT have been called. */ + if (WARN_ON(nmk_chip->quirk_mbly)) + return; + /* * Ensure WAKEUP_ENABLE is on. No need to disable it if wakeup is * disabled, since setting SLPM to 1 increases power consumption, and @@ -142,7 +161,7 @@ static void nmk_gpio_irq_maskunmask(struct nmk_gpio_chip *nmk_chip, __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, enable); - if (!(nmk_chip->real_wake & BIT(d->hwirq))) + if (!nmk_chip->quirk_mbly && !(nmk_chip->real_wake & BIT(d->hwirq))) __nmk_gpio_set_wake(nmk_chip, d->hwirq, enable); spin_unlock(&nmk_chip->lock); @@ -174,6 +193,10 @@ static int nmk_gpio_irq_set_wake(struct irq_data *d, unsigned int on) struct nmk_gpio_chip *nmk_chip = gpiochip_get_data(gc); unsigned long flags; + /* Handler is registered in all cases. */ + if (nmk_chip->quirk_mbly) + return -ENXIO; + clk_enable(nmk_chip->clk); spin_lock_irqsave(&nmk_gpio_slpm_lock, flags); spin_lock(&nmk_chip->lock); @@ -212,7 +235,7 @@ static int nmk_gpio_irq_set_type(struct irq_data *d, unsigned int type) if (enabled) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, false); - if (enabled || wake) + if (!nmk_chip->quirk_mbly && (enabled || wake)) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, WAKE, false); nmk_chip->edge_rising &= ~BIT(d->hwirq); @@ -226,7 +249,7 @@ static int nmk_gpio_irq_set_type(struct irq_data *d, unsigned int type) if (enabled) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, NORMAL, true); - if (enabled || wake) + if (!nmk_chip->quirk_mbly && (enabled || wake)) __nmk_gpio_irq_modify(nmk_chip, d->hwirq, WAKE, true); spin_unlock_irqrestore(&nmk_chip->lock, flags); @@ -359,6 +382,10 @@ static int nmk_gpio_get_mode(struct nmk_gpio_chip *nmk_chip, int offset) { u32 afunc, bfunc; + /* We don't support modes. */ + if (nmk_chip->quirk_mbly) + return NMK_GPIO_ALT_GPIO; + clk_enable(nmk_chip->clk); afunc = readl(nmk_chip->addr + NMK_GPIO_AFSLA) & BIT(offset); @@ -490,6 +517,7 @@ struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np, struct resource *res; struct clk *clk; void __iomem *base; + uintptr_t flags; u32 id, ngpio; gpio_pdev = of_find_device_by_node(np); @@ -523,6 +551,8 @@ struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np, dev_dbg(&pdev->dev, "populate: using default ngpio (%d)\n", ngpio); } + flags = (uintptr_t)of_device_get_match_data(&gpio_pdev->dev); + nmk_chip->quirk_mbly = !!(flags & NMK_GPIO_FLAG_QUIRK_MBLY); nmk_chip->bank = id; chip = &nmk_chip->chip; chip->base = -1; @@ -637,9 +667,11 @@ static int nmk_gpio_probe(struct platform_device *pdev) return ret; } - clk_enable(nmk_chip->clk); - nmk_chip->lowemi = readl_relaxed(nmk_chip->addr + NMK_GPIO_LOWEMI); - clk_disable(nmk_chip->clk); + if (!nmk_chip->quirk_mbly) { + clk_enable(nmk_chip->clk); + nmk_chip->lowemi = readl_relaxed(nmk_chip->addr + NMK_GPIO_LOWEMI); + clk_disable(nmk_chip->clk); + } ret = gpiochip_add_data(chip, nmk_chip); if (ret) @@ -653,7 +685,13 @@ static int nmk_gpio_probe(struct platform_device *pdev) } static const struct of_device_id nmk_gpio_match[] = { - { .compatible = "st,nomadik-gpio", }, + { + .compatible = "st,nomadik-gpio", + }, + { + .compatible = "mobileye,eyeq5-gpio", + .data = (void*)NMK_GPIO_FLAG_QUIRK_MBLY, + }, {} }; diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c index 99bdb25f358d..d1c5f353c6a9 100644 --- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c +++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c @@ -1229,6 +1229,8 @@ static int nmk_pinctrl_probe(struct platform_device *pdev) dev_err(&pdev->dev, "could not populate nmk chip struct - continue anyway\n"); of_node_put(gpio_np); + /* We are NOT compatible with mobileye,eyeq5-gpio. */ + BUG_ON(nmk_chip->quirk_mbly); } prcm_np = of_parse_phandle(np, "prcm", 0); diff --git a/include/linux/gpio/gpio-nomadik.h b/include/linux/gpio/gpio-nomadik.h index 8f7bb756ad35..8d0134dd3771 100644 --- a/include/linux/gpio/gpio-nomadik.h +++ b/include/linux/gpio/gpio-nomadik.h @@ -58,6 +58,7 @@ struct nmk_gpio_chip { spinlock_t lock; raw_spinlock_t fake_lock; bool sleepmode; + bool quirk_mbly; /* Keep track of configured edges */ u32 edge_rising; u32 edge_falling;
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> --- drivers/gpio/Kconfig | 5 +-- drivers/gpio/gpio-nomadik.c | 52 ++++++++++++++++++++++++++----- drivers/pinctrl/nomadik/pinctrl-nomadik.c | 2 ++ include/linux/gpio/gpio-nomadik.h | 1 + 4 files changed, 51 insertions(+), 9 deletions(-)