Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
2017-08-08 0:37 GMT+09:00 Linus Walleij: > On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamada > wrote: > >> Adding "interrupts" property in DT causes >> of_pupulate_default_populate() to assign virtual IRQ numbers >> before driver probing. So it does not work well with IRQ domain hierarchy. > > I think I heard some noise about this the week before. > >> For pinctrl/stm32/pinctrl-stm32.c, >> I do not see "interrupts", so it just straight maps the irq numbers. > > I think OMAP and DaVinci does someting similar too. This is from a recent > DaVinci patch from Keerthy: > > +Example for 66AK2G: > + > +gpio0: gpio@2603000 { > + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; > + reg = <0x02603000 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupts = , > + , > + , > + , > + , > + , > + , > + , > + ; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <144>; > + ti,davinci-gpio-unbanked = <0>; > + clocks = <_clks 0x001b 0x0>; > + clock-names = "gpio"; > +}; > > > That looks fairly similar. > > Yours, > Linus Walleij > -- I will send v2 for this driver. Thanks. -- Best Regards Masahiro Yamada
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
2017-08-08 0:37 GMT+09:00 Linus Walleij : > On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamada > wrote: > >> Adding "interrupts" property in DT causes >> of_pupulate_default_populate() to assign virtual IRQ numbers >> before driver probing. So it does not work well with IRQ domain hierarchy. > > I think I heard some noise about this the week before. > >> For pinctrl/stm32/pinctrl-stm32.c, >> I do not see "interrupts", so it just straight maps the irq numbers. > > I think OMAP and DaVinci does someting similar too. This is from a recent > DaVinci patch from Keerthy: > > +Example for 66AK2G: > + > +gpio0: gpio@2603000 { > + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; > + reg = <0x02603000 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupts = , > + , > + , > + , > + , > + , > + , > + , > + ; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <144>; > + ti,davinci-gpio-unbanked = <0>; > + clocks = <_clks 0x001b 0x0>; > + clock-names = "gpio"; > +}; > > > That looks fairly similar. > > Yours, > Linus Walleij > -- I will send v2 for this driver. Thanks. -- Best Regards Masahiro Yamada
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
On Tuesday 08 August 2017 06:36 AM, Masahiro Yamada wrote: > Hi Linus, > > 2017-08-08 0:37 GMT+09:00 Linus Walleij: >> On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamada >> wrote: >> >>> Adding "interrupts" property in DT causes >>> of_pupulate_default_populate() to assign virtual IRQ numbers >>> before driver probing. So it does not work well with IRQ domain hierarchy. >> >> I think I heard some noise about this the week before. >> >>> For pinctrl/stm32/pinctrl-stm32.c, >>> I do not see "interrupts", so it just straight maps the irq numbers. >> >> I think OMAP and DaVinci does someting similar too. This is from a recent >> DaVinci patch from Keerthy: >> >> +Example for 66AK2G: >> + >> +gpio0: gpio@2603000 { >> + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; >> + reg = <0x02603000 0x100>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + interrupts = , >> + , >> + , >> + , >> + , >> + , >> + , >> + , >> + ; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + ti,ngpio = <144>; >> + ti,davinci-gpio-unbanked = <0>; >> + clocks = <_clks 0x001b 0x0>; >> + clock-names = "gpio"; >> +}; >> >> >> That looks fairly similar. >> > > I do not think so. > > > I do not see .alloc hook in drivers/gpio/gpio-davinci.c > so this driver is unrelated to IRQ domain hierarchy. Hi Masahiro, Yes CONFIG_IRQ_DOMAIN_HIERARCHY is not enabled in keystone_defconfig or davinci_all_defconfig. Regards, Keerthy > > > > > >
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
On Tuesday 08 August 2017 06:36 AM, Masahiro Yamada wrote: > Hi Linus, > > 2017-08-08 0:37 GMT+09:00 Linus Walleij : >> On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamada >> wrote: >> >>> Adding "interrupts" property in DT causes >>> of_pupulate_default_populate() to assign virtual IRQ numbers >>> before driver probing. So it does not work well with IRQ domain hierarchy. >> >> I think I heard some noise about this the week before. >> >>> For pinctrl/stm32/pinctrl-stm32.c, >>> I do not see "interrupts", so it just straight maps the irq numbers. >> >> I think OMAP and DaVinci does someting similar too. This is from a recent >> DaVinci patch from Keerthy: >> >> +Example for 66AK2G: >> + >> +gpio0: gpio@2603000 { >> + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; >> + reg = <0x02603000 0x100>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + interrupts = , >> + , >> + , >> + , >> + , >> + , >> + , >> + , >> + ; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + ti,ngpio = <144>; >> + ti,davinci-gpio-unbanked = <0>; >> + clocks = <_clks 0x001b 0x0>; >> + clock-names = "gpio"; >> +}; >> >> >> That looks fairly similar. >> > > I do not think so. > > > I do not see .alloc hook in drivers/gpio/gpio-davinci.c > so this driver is unrelated to IRQ domain hierarchy. Hi Masahiro, Yes CONFIG_IRQ_DOMAIN_HIERARCHY is not enabled in keystone_defconfig or davinci_all_defconfig. Regards, Keerthy > > > > > >
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
Hi Linus, 2017-08-08 0:37 GMT+09:00 Linus Walleij: > On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamada > wrote: > >> Adding "interrupts" property in DT causes >> of_pupulate_default_populate() to assign virtual IRQ numbers >> before driver probing. So it does not work well with IRQ domain hierarchy. > > I think I heard some noise about this the week before. > >> For pinctrl/stm32/pinctrl-stm32.c, >> I do not see "interrupts", so it just straight maps the irq numbers. > > I think OMAP and DaVinci does someting similar too. This is from a recent > DaVinci patch from Keerthy: > > +Example for 66AK2G: > + > +gpio0: gpio@2603000 { > + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; > + reg = <0x02603000 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupts = , > + , > + , > + , > + , > + , > + , > + , > + ; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <144>; > + ti,davinci-gpio-unbanked = <0>; > + clocks = <_clks 0x001b 0x0>; > + clock-names = "gpio"; > +}; > > > That looks fairly similar. > I do not think so. I do not see .alloc hook in drivers/gpio/gpio-davinci.c so this driver is unrelated to IRQ domain hierarchy. -- Best Regards Masahiro Yamada
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
Hi Linus, 2017-08-08 0:37 GMT+09:00 Linus Walleij : > On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamada > wrote: > >> Adding "interrupts" property in DT causes >> of_pupulate_default_populate() to assign virtual IRQ numbers >> before driver probing. So it does not work well with IRQ domain hierarchy. > > I think I heard some noise about this the week before. > >> For pinctrl/stm32/pinctrl-stm32.c, >> I do not see "interrupts", so it just straight maps the irq numbers. > > I think OMAP and DaVinci does someting similar too. This is from a recent > DaVinci patch from Keerthy: > > +Example for 66AK2G: > + > +gpio0: gpio@2603000 { > + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; > + reg = <0x02603000 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupts = , > + , > + , > + , > + , > + , > + , > + , > + ; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <144>; > + ti,davinci-gpio-unbanked = <0>; > + clocks = <_clks 0x001b 0x0>; > + clock-names = "gpio"; > +}; > > > That looks fairly similar. > I do not think so. I do not see .alloc hook in drivers/gpio/gpio-davinci.c so this driver is unrelated to IRQ domain hierarchy. -- Best Regards Masahiro Yamada
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamadawrote: > Adding "interrupts" property in DT causes > of_pupulate_default_populate() to assign virtual IRQ numbers > before driver probing. So it does not work well with IRQ domain hierarchy. I think I heard some noise about this the week before. > For pinctrl/stm32/pinctrl-stm32.c, > I do not see "interrupts", so it just straight maps the irq numbers. I think OMAP and DaVinci does someting similar too. This is from a recent DaVinci patch from Keerthy: +Example for 66AK2G: + +gpio0: gpio@2603000 { + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; + reg = <0x02603000 0x100>; + gpio-controller; + #gpio-cells = <2>; + interrupts = , + , + , + , + , + , + , + , + ; + interrupt-controller; + #interrupt-cells = <2>; + ti,ngpio = <144>; + ti,davinci-gpio-unbanked = <0>; + clocks = <_clks 0x001b 0x0>; + clock-names = "gpio"; +}; That looks fairly similar. Yours, Linus Walleij
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
On Mon, Aug 7, 2017 at 3:50 PM, Masahiro Yamada wrote: > Adding "interrupts" property in DT causes > of_pupulate_default_populate() to assign virtual IRQ numbers > before driver probing. So it does not work well with IRQ domain hierarchy. I think I heard some noise about this the week before. > For pinctrl/stm32/pinctrl-stm32.c, > I do not see "interrupts", so it just straight maps the irq numbers. I think OMAP and DaVinci does someting similar too. This is from a recent DaVinci patch from Keerthy: +Example for 66AK2G: + +gpio0: gpio@2603000 { + compatible = "ti,k2g-gpio", "ti,keystone-gpio"; + reg = <0x02603000 0x100>; + gpio-controller; + #gpio-cells = <2>; + interrupts = , + , + , + , + , + , + , + , + ; + interrupt-controller; + #interrupt-cells = <2>; + ti,ngpio = <144>; + ti,davinci-gpio-unbanked = <0>; + clocks = <_clks 0x001b 0x0>; + clock-names = "gpio"; +}; That looks fairly similar. Yours, Linus Walleij
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
Hi Linus, 2017-08-07 21:42 GMT+09:00 Linus Walleij: > On Thu, Aug 3, 2017 at 1:06 PM, Masahiro Yamada > wrote: > >> This GPIO controller device is used on UniPhier SoCs. >> >> Signed-off-by: Masahiro Yamada > (...) >> +static const u32 uniphier_gpio_irq_parent_hwirqs[] = { >> + 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, >> + 186, 187, 188, 189, 190, 249, 250, 251, >> +}; > (..) >> + /* parent is UniPhier AIDET */ >> + parent_fwspec.fwnode = domain->parent->fwnode; >> + parent_fwspec.param_count = 2; >> + parent_fwspec.param[0] = >> uniphier_gpio_irq_parent_hwirqs[hwirq]; >> + parent_fwspec.param[1] = type; >> + >> + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, >> + _fwspec); > > This is not right. > > That parent IRQ needs to come from the device tree, not from a static array > like this, and it is definately to use hierarchical irqdomain. > > Please look at similar drivers merged recently for inspiration: > gpio/gpio-xgene-sb.c > pinctrl/stm32/pinctrl-stm32.c > I was waiting for such a comment! I was actually tackling to specify IRQ-mapping with "interrupts" property. Adding "interrupts" property in DT causes of_pupulate_default_populate() to assign virtual IRQ numbers before driver probing. So it does not work well with IRQ domain hierarchy. I send a question: https://lkml.org/lkml/2017/7/6/758 But, I did not get any answer. So, I thought the only solution is to specify the IRQ-remapping somehow in the driver. For gpio/gpio-xgene-sb.c, the driver code seems suspicious. As you see xgene_gpio_sb_probe(), it uses the first entry of "interrupts" for just getting the ->parent_irq_base. The assigned virtual IRQs will not be used. ret = platform_get_irq(pdev, 0); if (ret > 0) { priv->parent_irq_base = irq_get_irq_data(ret)->hwirq; parent_domain = irq_get_irq_data(ret)->domain; } Then, xgene_gpio_sb_domain_alloc() allocates different virtual IRQs expecting a particular binding between the parent. parent_fwspec.fwnode = domain->parent->fwnode; if (is_of_node(parent_fwspec.fwnode)) { parent_fwspec.param_count = 3; parent_fwspec.param[0] = 0;/* SPI */ /* Skip SGIs and PPIs*/ parent_fwspec.param[1] = hwirq + priv->parent_irq_base - 32; parent_fwspec.param[2] = fwspec->param[1]; } else if (is_fwnode_irqchip(parent_fwspec.fwnode)) { parent_fwspec.param_count = 2; parent_fwspec.param[0] = hwirq + priv->parent_irq_base; parent_fwspec.param[1] = fwspec->param[1]; } else return -EINVAL; It uses ->parent_irq_base derived from DT, but other parts are really hard-coded. For pinctrl/stm32/pinctrl-stm32.c, I do not see "interrupts", so it just straight maps the irq numbers. -- Best Regards Masahiro Yamada
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
Hi Linus, 2017-08-07 21:42 GMT+09:00 Linus Walleij : > On Thu, Aug 3, 2017 at 1:06 PM, Masahiro Yamada > wrote: > >> This GPIO controller device is used on UniPhier SoCs. >> >> Signed-off-by: Masahiro Yamada > (...) >> +static const u32 uniphier_gpio_irq_parent_hwirqs[] = { >> + 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, >> + 186, 187, 188, 189, 190, 249, 250, 251, >> +}; > (..) >> + /* parent is UniPhier AIDET */ >> + parent_fwspec.fwnode = domain->parent->fwnode; >> + parent_fwspec.param_count = 2; >> + parent_fwspec.param[0] = >> uniphier_gpio_irq_parent_hwirqs[hwirq]; >> + parent_fwspec.param[1] = type; >> + >> + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, >> + _fwspec); > > This is not right. > > That parent IRQ needs to come from the device tree, not from a static array > like this, and it is definately to use hierarchical irqdomain. > > Please look at similar drivers merged recently for inspiration: > gpio/gpio-xgene-sb.c > pinctrl/stm32/pinctrl-stm32.c > I was waiting for such a comment! I was actually tackling to specify IRQ-mapping with "interrupts" property. Adding "interrupts" property in DT causes of_pupulate_default_populate() to assign virtual IRQ numbers before driver probing. So it does not work well with IRQ domain hierarchy. I send a question: https://lkml.org/lkml/2017/7/6/758 But, I did not get any answer. So, I thought the only solution is to specify the IRQ-remapping somehow in the driver. For gpio/gpio-xgene-sb.c, the driver code seems suspicious. As you see xgene_gpio_sb_probe(), it uses the first entry of "interrupts" for just getting the ->parent_irq_base. The assigned virtual IRQs will not be used. ret = platform_get_irq(pdev, 0); if (ret > 0) { priv->parent_irq_base = irq_get_irq_data(ret)->hwirq; parent_domain = irq_get_irq_data(ret)->domain; } Then, xgene_gpio_sb_domain_alloc() allocates different virtual IRQs expecting a particular binding between the parent. parent_fwspec.fwnode = domain->parent->fwnode; if (is_of_node(parent_fwspec.fwnode)) { parent_fwspec.param_count = 3; parent_fwspec.param[0] = 0;/* SPI */ /* Skip SGIs and PPIs*/ parent_fwspec.param[1] = hwirq + priv->parent_irq_base - 32; parent_fwspec.param[2] = fwspec->param[1]; } else if (is_fwnode_irqchip(parent_fwspec.fwnode)) { parent_fwspec.param_count = 2; parent_fwspec.param[0] = hwirq + priv->parent_irq_base; parent_fwspec.param[1] = fwspec->param[1]; } else return -EINVAL; It uses ->parent_irq_base derived from DT, but other parts are really hard-coded. For pinctrl/stm32/pinctrl-stm32.c, I do not see "interrupts", so it just straight maps the irq numbers. -- Best Regards Masahiro Yamada
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
On Thu, Aug 3, 2017 at 1:06 PM, Masahiro Yamadawrote: > This GPIO controller device is used on UniPhier SoCs. > > Signed-off-by: Masahiro Yamada (...) > +static const u32 uniphier_gpio_irq_parent_hwirqs[] = { > + 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, > + 186, 187, 188, 189, 190, 249, 250, 251, > +}; (..) > + /* parent is UniPhier AIDET */ > + parent_fwspec.fwnode = domain->parent->fwnode; > + parent_fwspec.param_count = 2; > + parent_fwspec.param[0] = > uniphier_gpio_irq_parent_hwirqs[hwirq]; > + parent_fwspec.param[1] = type; > + > + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, > + _fwspec); This is not right. That parent IRQ needs to come from the device tree, not from a static array like this, and it is definately to use hierarchical irqdomain. Please look at similar drivers merged recently for inspiration: gpio/gpio-xgene-sb.c pinctrl/stm32/pinctrl-stm32.c Yours, Linus Walleij
Re: [PATCH] gpio: uniphier: add UniPhier GPIO controller driver
On Thu, Aug 3, 2017 at 1:06 PM, Masahiro Yamada wrote: > This GPIO controller device is used on UniPhier SoCs. > > Signed-off-by: Masahiro Yamada (...) > +static const u32 uniphier_gpio_irq_parent_hwirqs[] = { > + 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, > + 186, 187, 188, 189, 190, 249, 250, 251, > +}; (..) > + /* parent is UniPhier AIDET */ > + parent_fwspec.fwnode = domain->parent->fwnode; > + parent_fwspec.param_count = 2; > + parent_fwspec.param[0] = > uniphier_gpio_irq_parent_hwirqs[hwirq]; > + parent_fwspec.param[1] = type; > + > + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, > + _fwspec); This is not right. That parent IRQ needs to come from the device tree, not from a static array like this, and it is definately to use hierarchical irqdomain. Please look at similar drivers merged recently for inspiration: gpio/gpio-xgene-sb.c pinctrl/stm32/pinctrl-stm32.c Yours, Linus Walleij