| Message ID | 1459436979-17275-7-git-send-email-mcoquelin.stm32@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote: > +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset) > +{ > + struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent); > + struct stm32_gpio_bank *bank = gpiochip_get_data(chip); > + unsigned int irq; > + > + regmap_field_write(pctl->irqmux[offset], bank->range.id); No. You must implement the irqchip and GPIO controllers to be orthogonal, doing things like this creates a semantic that assumes .to_irq() is always called before using the IRQ and that is not guaranteed at all. A consumer may very well use an interrupt right off the irqchip without this being called first. All this function should do is translate a number. No other semantics. This needs to be done from the irqchip (sorry). Yours, Linus Walleij
Hi Linus 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin > <mcoquelin.stm32@gmail.com> wrote: > >> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset) >> +{ >> + struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent); >> + struct stm32_gpio_bank *bank = gpiochip_get_data(chip); >> + unsigned int irq; >> + >> + regmap_field_write(pctl->irqmux[offset], bank->range.id); > > No. You must implement the irqchip and GPIO controllers to > be orthogonal, doing things like this creates a semantic that > assumes .to_irq() is always called before using the IRQ and > that is not guaranteed at all. A consumer may very well > use an interrupt right off the irqchip without this being called > first. All this function should do is translate a number. No > other semantics. > > This needs to be done from the irqchip (sorry). Actually, the register written here is not part of the irqchip. It is a system config register that is only used when using a GPIO as external interrupt. Its aim is to mux the GPIO bank on a line. For example on line 1, it can be muxed to select either gpioa1, gpiob1, gpioc1, ... How could I do it in the irqchip that has no gpio knowledge? In case the consumer uses an interrupt right off the irqchip, it should not access this register. For example the RTC wakeup lines, where we will reference directly the irqchip. That said, do you still believe the implementation is wrong? Do I miss something? Best regards, Maxime
On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote: > 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin >> <mcoquelin.stm32@gmail.com> wrote: >> >>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset) >>> +{ >>> + struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent); >>> + struct stm32_gpio_bank *bank = gpiochip_get_data(chip); >>> + unsigned int irq; >>> + >>> + regmap_field_write(pctl->irqmux[offset], bank->range.id); >> >> No. You must implement the irqchip and GPIO controllers to >> be orthogonal, doing things like this creates a semantic that >> assumes .to_irq() is always called before using the IRQ and >> that is not guaranteed at all. A consumer may very well >> use an interrupt right off the irqchip without this being called >> first. All this function should do is translate a number. No >> other semantics. >> >> This needs to be done from the irqchip (sorry). > > Actually, the register written here is not part of the irqchip. > It is a system config register that is only used when using a GPIO as > external interrupt. > Its aim is to mux the GPIO bank on a line. Then it should be done in .request() for the GPIO, not in .to_irq(). It should *also* be done in the set-up path for the irqchip side, if that line is used without any interaction with the gpio side of things. > For example on line 1, it can be muxed to select either gpioa1, > gpiob1, gpioc1, ... > How could I do it in the irqchip that has no gpio knowledge? I don't understand this. We are discussing an irqchip that is tightly coupled with a gpiochip. Usually d->hwirq is the same as the GPIO offset but that varies. The point is that each IRQ that ever get used has a 1-to-1 relationship to a certain GPIO line, and if that relationship cannot be resolved from the irqchip side, something is wrong. The irqchip needs to enable the GPIO line it is backing to recieve interrupts without any requirements that .to_irq() have been called first. If to_irq() does something else than translate to an IRQ something is wrong. Yours, Linus Walleij
On Fri, Apr 29, 2016 at 10:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin > <mcoquelin.stm32@gmail.com> wrote: >> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin >>> <mcoquelin.stm32@gmail.com> wrote: >>> >>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset) >>>> +{ >>>> + struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent); >>>> + struct stm32_gpio_bank *bank = gpiochip_get_data(chip); >>>> + unsigned int irq; >>>> + >>>> + regmap_field_write(pctl->irqmux[offset], bank->range.id); >>> >>> No. You must implement the irqchip and GPIO controllers to >>> be orthogonal, doing things like this creates a semantic that >>> assumes .to_irq() is always called before using the IRQ and >>> that is not guaranteed at all. A consumer may very well >>> use an interrupt right off the irqchip without this being called >>> first. All this function should do is translate a number. No >>> other semantics. >>> >>> This needs to be done from the irqchip (sorry). >> >> Actually, the register written here is not part of the irqchip. >> It is a system config register that is only used when using a GPIO as >> external interrupt. >> Its aim is to mux the GPIO bank on a line. > > Then it should be done in .request() for the GPIO, not in > .to_irq(). > > It should *also* be done in the set-up path for the irqchip > side, if that line is used without any interaction with the > gpio side of things. Or, hm, maybe not in the irqchip then if it is as you say that the interrupt can be used anyway, without this being set up. But it should certainly not be done in .to_irq(), rather in .request(). Yours, Linus Walleij
2016-04-29 10:53 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin > <mcoquelin.stm32@gmail.com> wrote: >> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin >>> <mcoquelin.stm32@gmail.com> wrote: >>> >>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset) >>>> +{ >>>> + struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent); >>>> + struct stm32_gpio_bank *bank = gpiochip_get_data(chip); >>>> + unsigned int irq; >>>> + >>>> + regmap_field_write(pctl->irqmux[offset], bank->range.id); >>> >>> No. You must implement the irqchip and GPIO controllers to >>> be orthogonal, doing things like this creates a semantic that >>> assumes .to_irq() is always called before using the IRQ and >>> that is not guaranteed at all. A consumer may very well >>> use an interrupt right off the irqchip without this being called >>> first. All this function should do is translate a number. No >>> other semantics. >>> >>> This needs to be done from the irqchip (sorry). >> >> Actually, the register written here is not part of the irqchip. >> It is a system config register that is only used when using a GPIO as >> external interrupt. >> Its aim is to mux the GPIO bank on a line. > > Then it should be done in .request() for the GPIO, not in > .to_irq(). Problem is that at request time, we don't know whether the GPIO is to be used as an interrupt or not. I think I may have not been clear enough in the HW architecture description. Indeed, I used the term "mux", but should instead use the term "selector". The idea is that between the GPIO controllers and the EXTI controller, there is one selector for each line number, to select the controller used as interrupt source. For example, there is a selector on line 0 to select between gpioa0, gpiob0, gpioc0,.., gpiok0, which one is connected to exti0. It means there is a HW restriction that makes impossible to use both GPIOA0 or GPIOB0 as interrupts at the same time. It looks like this: http://pastebin.com/raw/cs2WiNKZ You can directly check section 12.2.5 of the stm32f429 reference manual: http://www2.st.com/resource/en/reference_manual/dm00031020.pdf For example, we can imagine a board where gpioa0 is used SD's card detect, and gpiob0 used to control a led. If we set the mux at request time, gpiob0 request may overwrite the mux configuration set by gpioa0, whereas it is not used as interrupt. That is the reason why I did it in .to_irq(). > > It should *also* be done in the set-up path for the irqchip > side, if that line is used without any interaction with the > gpio side of things. Actually, a line is either used by a GPIO, (exclusive) or another purpose. In the case of a GPIO line, I think we should always go through the gpio. > >> For example on line 1, it can be muxed to select either gpioa1, >> gpiob1, gpioc1, ... >> How could I do it in the irqchip that has no gpio knowledge? > > I don't understand this. > > We are discussing an irqchip that is tightly coupled with > a gpiochip. Usually d->hwirq is the same as the GPIO offset > but that varies. > > The point is that each IRQ that ever get used > has a 1-to-1 relationship to a certain GPIO line, and if that > relationship cannot be resolved from the irqchip side, > something is wrong. The irqchip needs to enable the > GPIO line it is backing to recieve interrupts without any > requirements that .to_irq() have been called first. Actually, this is not a 1:1 relationship, as for example, exti0 can be connected to either gpioa0, or gpiob0,..., or gpiok0. So for the exti entries that are connected to gpios, I don't see how we can reference it from the exti controller, > > If to_irq() does something else than translate to an IRQ > something is wrong. Ok, so maybe we need an intermediate layer between gpio and exti to manage the selectors sysconf? I wiil have a look in that direction, but I think it could be over-engineered. Thanks! Maxime
On Fri, Apr 29, 2016 at 1:19 PM, Maxime Coquelin <mcoquelin.stm32@gmail.com> wrote: > 2016-04-29 10:53 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >> On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin >> <mcoquelin.stm32@gmail.com> wrote: >>> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >>>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin >>>> <mcoquelin.stm32@gmail.com> wrote: >>>> >>>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset) >>>>> +{ >>>>> + struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent); >>>>> + struct stm32_gpio_bank *bank = gpiochip_get_data(chip); >>>>> + unsigned int irq; >>>>> + >>>>> + regmap_field_write(pctl->irqmux[offset], bank->range.id); >>>> >>>> No. You must implement the irqchip and GPIO controllers to >>>> be orthogonal, doing things like this creates a semantic that >>>> assumes .to_irq() is always called before using the IRQ and >>>> that is not guaranteed at all. A consumer may very well >>>> use an interrupt right off the irqchip without this being called >>>> first. All this function should do is translate a number. No >>>> other semantics. >>>> >>>> This needs to be done from the irqchip (sorry). >>> >>> Actually, the register written here is not part of the irqchip. >>> It is a system config register that is only used when using a GPIO as >>> external interrupt. >>> Its aim is to mux the GPIO bank on a line. >> >> Then it should be done in .request() for the GPIO, not in >> .to_irq(). > > Problem is that at request time, we don't know whether the GPIO is to > be used as an interrupt or not. Well the fact that .to_irq() is called does not mean that you know it will be used as an interrupt either. Sorry. It is just a translation function, not an allocation function. > I think I may have not been clear enough in the HW architecture description. > Indeed, I used the term "mux", but should instead use the term "selector". > > The idea is that between the GPIO controllers and the EXTI controller, > there is one selector for each line number, to select the controller used as > interrupt source. > > For example, there is a selector on line 0 to select between gpioa0, gpiob0, > gpioc0,.., gpiok0, which one is connected to exti0. > > It means there is a HW restriction that makes impossible to use both GPIOA0 > or GPIOB0 as interrupts at the same time. > > It looks like this: http://pastebin.com/raw/cs2WiNKZ > You can directly check section 12.2.5 of the stm32f429 reference manual: > http://www2.st.com/resource/en/reference_manual/dm00031020.pdf Nice ASCII art, include that into the commit message :) > For example, we can imagine a board where gpioa0 is used SD's card detect, > and gpiob0 used to control a led. > > If we set the mux at request time, gpiob0 request may overwrite the mux > configuration set by gpioa0, whereas it is not used as interrupt. > > That is the reason why I did it in .to_irq(). Well now I am even *more* convinced that this should not happen in .to_irq(). .to_irq() should not do *anything*. This needs to happen as part of the irqchip setting up the actual interrupt. And it seems the problem is that this driver does *not* define its own irqchip, but it *should*. What you want to do is implement an hierarchical irqdomain in your irqchip, which is what other drivers of this type are doing, see: drivers/gpio/gpio-xgene-sb.c irq_domain_create_hierarchy() is your friend. >> It should *also* be done in the set-up path for the irqchip >> side, if that line is used without any interaction with the >> gpio side of things. > > Actually, a line is either used by a GPIO, (exclusive) or another purpose. > In the case of a GPIO line, I think we should always go through the gpio. This is a textbook example of a hierarchichal irq domain I think. >> The point is that each IRQ that ever get used >> has a 1-to-1 relationship to a certain GPIO line, and if that >> relationship cannot be resolved from the irqchip side, >> something is wrong. The irqchip needs to enable the >> GPIO line it is backing to recieve interrupts without any >> requirements that .to_irq() have been called first. > > Actually, this is not a 1:1 relationship, as for example, exti0 can be connected > to either gpioa0, or gpiob0,..., or gpiok0. That is what hierarchical irqdomain is for. You should expose an irqchip from the gpio driver, that give you unique irq translations for *all* GPIO lines. Then, at runtime, if the hierarchical irqdomain cannot map (i.e. mux) the interrupt onto one of the available lines, you will get an error. Yours, Linus Walleij
2016-04-30 13:32 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > On Fri, Apr 29, 2016 at 1:19 PM, Maxime Coquelin > <mcoquelin.stm32@gmail.com> wrote: >> It looks like this: http://pastebin.com/raw/cs2WiNKZ >> You can directly check section 12.2.5 of the stm32f429 reference manual: >> http://www2.st.com/resource/en/reference_manual/dm00031020.pdf > > Nice ASCII art, include that into the commit message :) I will! :) > >> For example, we can imagine a board where gpioa0 is used SD's card detect, >> and gpiob0 used to control a led. >> >> If we set the mux at request time, gpiob0 request may overwrite the mux >> configuration set by gpioa0, whereas it is not used as interrupt. >> >> That is the reason why I did it in .to_irq(). > > Well now I am even *more* convinced that this should not happen in > .to_irq(). .to_irq() should not do *anything*. > > This needs to happen as part of the irqchip setting up the actual > interrupt. > > And it seems the problem is that this driver does *not* define its > own irqchip, but it *should*. > > What you want to do is implement an hierarchical irqdomain in your > irqchip, which is what other drivers of this type are doing, see: > drivers/gpio/gpio-xgene-sb.c > irq_domain_create_hierarchy() is your friend. > >>> It should *also* be done in the set-up path for the irqchip >>> side, if that line is used without any interaction with the >>> gpio side of things. >> >> Actually, a line is either used by a GPIO, (exclusive) or another purpose. >> In the case of a GPIO line, I think we should always go through the gpio. > > This is a textbook example of a hierarchichal irq domain I think. > >>> The point is that each IRQ that ever get used >>> has a 1-to-1 relationship to a certain GPIO line, and if that >>> relationship cannot be resolved from the irqchip side, >>> something is wrong. The irqchip needs to enable the >>> GPIO line it is backing to recieve interrupts without any >>> requirements that .to_irq() have been called first. >> >> Actually, this is not a 1:1 relationship, as for example, exti0 can be connected >> to either gpioa0, or gpiob0,..., or gpiok0. > > That is what hierarchical irqdomain is for. > > You should expose an irqchip from the gpio driver, that give you > unique irq translations for *all* GPIO lines. > > Then, at runtime, if the hierarchical irqdomain cannot map > (i.e. mux) the interrupt onto one of the available lines, > you will get an error. Thanks a lot for this valuable feedback. I will have a look at hierachical irq domains, and hope to come back this week with an implementation. Regards, Maxime
diff --git a/drivers/pinctrl/stm32/Kconfig b/drivers/pinctrl/stm32/Kconfig index 0f28841b2332..b5cac5bfd0cd 100644 --- a/drivers/pinctrl/stm32/Kconfig +++ b/drivers/pinctrl/stm32/Kconfig @@ -6,6 +6,7 @@ config PINCTRL_STM32 select PINMUX select GENERIC_PINCONF select GPIOLIB + select MFD_SYSCON config PINCTRL_STM32F429 bool "STMicroelectronics STM32F429 pin control" if COMPILE_TEST && !MACH_STM32F429 diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c index 8deb566ed4cd..f2fa717894dc 100644 --- a/drivers/pinctrl/stm32/pinctrl-stm32.c +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c @@ -8,6 +8,8 @@ #include <linux/clk.h> #include <linux/gpio/driver.h> #include <linux/io.h> +#include <linux/irq.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> @@ -20,6 +22,7 @@ #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> @@ -77,6 +80,9 @@ struct stm32_pinctrl { struct stm32_gpio_bank *banks; unsigned nbanks; const struct stm32_pinctrl_match_data *match_data; + struct irq_domain *domain; + struct regmap *regmap; + struct regmap_field *irqmux[STM32_GPIO_PINS_PER_BANK]; }; static inline int stm32_gpio_pin(int gpio) @@ -174,6 +180,22 @@ static int stm32_gpio_direction_output(struct gpio_chip *chip, return 0; } + +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset) +{ + struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent); + struct stm32_gpio_bank *bank = gpiochip_get_data(chip); + unsigned int irq; + + regmap_field_write(pctl->irqmux[offset], bank->range.id); + + irq = irq_create_mapping(pctl->domain, offset); + if (!irq) + return -ENXIO; + + return irq; +} + static struct gpio_chip stm32_gpio_template = { .request = stm32_gpio_request, .free = stm32_gpio_free, @@ -181,6 +203,7 @@ static struct gpio_chip stm32_gpio_template = { .set = stm32_gpio_set, .direction_input = stm32_gpio_direction_input, .direction_output = stm32_gpio_direction_output, + .to_irq = stm32_gpio_to_irq, }; /* Pinctrl functions */ @@ -704,6 +727,47 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl, return 0; } +static int stm32_pctrl_dt_setup_irq(struct platform_device *pdev, + struct stm32_pinctrl *pctl) +{ + struct device_node *np = pdev->dev.of_node, *parent; + struct device *dev = &pdev->dev; + struct regmap *rm; + int offset, ret, i; + + parent = of_irq_find_parent(np); + if (!parent) + return -ENXIO; + + pctl->domain = irq_find_host(parent); + if (!pctl->domain) + return -ENXIO; + + pctl->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); + if (IS_ERR(pctl->regmap)) + return PTR_ERR(pctl->regmap); + + rm = pctl->regmap; + + ret = of_property_read_u32_index(np, "st,syscfg", 1, &offset); + if (ret) + return ret; + + for (i = 0; i < STM32_GPIO_PINS_PER_BANK; i++) { + struct reg_field mux; + + mux.reg = offset + (i / 4) * 4; + mux.lsb = (i % 4) * 4; + mux.msb = mux.lsb + 3; + + pctl->irqmux[i] = devm_regmap_field_alloc(dev, rm, mux); + if (IS_ERR(pctl->irqmux[i])) + return PTR_ERR(pctl->irqmux[i]); + } + + return 0; +} + static int stm32_pctrl_build_state(struct platform_device *pdev) { struct stm32_pinctrl *pctl = platform_get_drvdata(pdev); @@ -796,6 +860,10 @@ int stm32_pctl_probe(struct platform_device *pdev) } } + ret = stm32_pctrl_dt_setup_irq(pdev, pctl); + if (ret) + return ret; + pins = devm_kcalloc(&pdev->dev, pctl->match_data->npins, sizeof(*pins), GFP_KERNEL); if (!pins)
This patch adds IRQ support to STM32 gpios. The EXTI controller has 16 lines dedicated to GPIOs. EXTI line n can be connected to only line n of one of the GPIO ports, for example EXTI0 can be connected to either PA0, or PB0, or PC0... This port selection is done by specifying the port number into System Config registers. Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com> --- drivers/pinctrl/stm32/Kconfig | 1 + drivers/pinctrl/stm32/pinctrl-stm32.c | 68 +++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+)