[PATCH v2 0/2] of: property: Add fw_devlink support for more props

[PATCH v2 0/2] of: property: Add fw_devlink support for more props

[Saravana Kannan]

Sending again because I messed up the To/Cc for the coverletter.

This series combines two patches [1] [2] that'd conflict.

Greg,

Can you please pull this into driver-core-next?

-Saravana

[1] - https://lore.kernel.org/lkml/20210115210159.3090203-1-saravanak@google.com/
[2] - https://lore.kernel.org/lkml/20201218210750.3455872-1-saravanak@google.com/

Individual -> Series:
Patch 1/2: Addressed Geert's gpio-hog problem with gpio[s] property
Patch 2/2: Switched to using of_irq_find_parent()

v1 -> v2:
Patch 1/2: Added the Reviewed-by tags
Patch 2/2: Updated commit text and comments. Added Reviewed-by tags.

Saravana Kannan (2):
  of: property: Add fw_devlink support for "gpio" and "gpios" binding
  of: property: Add fw_devlink support for interrupts

 drivers/of/property.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

-- 
2.30.0.296.g2bfb1c46d8-goog

[PATCH v2 1/2] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Saravana Kannan]

To provide backward compatibility for boards that use deprecated DT
bindings, we need to add fw_devlink support for "gpio" and "gpios".

We also need to ignore these properties on nodes with "gpio-hog"
property because their gpio[s] are all supplied by the parent node.

Cc: linux-tegra <linux-tegra@vger.kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Thierry Reding <treding@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 5f9eed79a8aa..b2ea1951d937 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1271,6 +1271,28 @@ static struct device_node *parse_iommu_maps(struct device_node *np,
 	return of_parse_phandle(np, prop_name, (index * 4) + 1);
 }
 
+static struct device_node *parse_gpio_compat(struct device_node *np,
+					     const char *prop_name, int index)
+{
+	struct of_phandle_args sup_args;
+
+	if (strcmp(prop_name, "gpio") && strcmp(prop_name, "gpios"))
+		return NULL;
+
+	/*
+	 * Ignore node with gpio-hog property since its gpios are all provided
+	 * by its parent.
+	 */
+	if (of_find_property(np, "gpio-hog", NULL))
+		return NULL;
+
+	if (of_parse_phandle_with_args(np, prop_name, "#gpio-cells", index,
+				       &sup_args))
+		return NULL;
+
+	return sup_args.np;
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
@@ -1296,6 +1318,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_pinctrl6, },
 	{ .parse_prop = parse_pinctrl7, },
 	{ .parse_prop = parse_pinctrl8, },
+	{ .parse_prop = parse_gpio_compat, },
 	{ .parse_prop = parse_regulators, },
 	{ .parse_prop = parse_gpio, },
 	{ .parse_prop = parse_gpios, },
-- 
2.30.0.296.g2bfb1c46d8-goog

Re: [PATCH v2 1/2] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Linus Walleij]

On Thu, Jan 21, 2021 at 11:57 PM Saravana Kannan <saravanak@google.com> wrote:

> To provide backward compatibility for boards that use deprecated DT
> bindings, we need to add fw_devlink support for "gpio" and "gpios".
>
> We also need to ignore these properties on nodes with "gpio-hog"
> property because their gpio[s] are all supplied by the parent node.
>
> Cc: linux-tegra <linux-tegra@vger.kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Saravana Kannan <saravanak@google.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

This allows fw_devlink to create device links between consumers of an
interrupt and the supplier of the interrupt.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Thierry Reding <treding@nvidia.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index b2ea1951d937..6287c6d60bb7 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -24,6 +24,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_graph.h>
+#include <linux/of_irq.h>
 #include <linux/string.h>
 #include <linux/moduleparam.h>
 
@@ -1293,6 +1294,15 @@ static struct device_node *parse_gpio_compat(struct device_node *np,
 	return sup_args.np;
 }
 
+static struct device_node *parse_interrupts(struct device_node *np,
+					    const char *prop_name, int index)
+{
+	if (strcmp(prop_name, "interrupts") || index)
+		return NULL;
+
+	return of_irq_find_parent(np);
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
@@ -1319,6 +1329,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_pinctrl7, },
 	{ .parse_prop = parse_pinctrl8, },
 	{ .parse_prop = parse_gpio_compat, },
+	{ .parse_prop = parse_interrupts, },
 	{ .parse_prop = parse_regulators, },
 	{ .parse_prop = parse_gpio, },
 	{ .parse_prop = parse_gpios, },
-- 
2.30.0.296.g2bfb1c46d8-goog

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Marek Szyprowski]

Hi Saravana,

On 21.01.2021 23:57, Saravana Kannan wrote:
> This allows fw_devlink to create device links between consumers of an
> interrupt and the supplier of the interrupt.
>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Saravana Kannan <saravanak@google.com>

This patch landed some time ago in linux-next as commit 4104ca776ba3 
("of: property: Add fw_devlink support for interrupts"). It breaks MMC 
host controller operation on ARM Juno R1 board (the mmci@50000 device 
defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't 
check further what's wrong there as without MMC mounting rootfs fails in 
my test system.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Saravana,
>
> On 21.01.2021 23:57, Saravana Kannan wrote:
> > This allows fw_devlink to create device links between consumers of an
> > interrupt and the supplier of the interrupt.
> >
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Kevin Hilman <khilman@baylibre.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Thierry Reding <treding@nvidia.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> This patch landed some time ago in linux-next as commit 4104ca776ba3
> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> host controller operation on ARM Juno R1 board (the mmci@50000 device
> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't

I grepped around and it looks like the final board file is this or
whatever includes it?
arch/arm64/boot/dts/arm/juno-base.dtsi

This patch just finds the interrupt-parent and then tries to use that
as a supplier if "interrupts" property is listed. But the only
interrupt parent I can see is:
        gic: interrupt-controller@2c010000 {
                compatible = "arm,gic-400", "arm,cortex-a15-gic";

And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
a NOP since those suppliers are never devices and are ignored.
$ git grep "arm,gic-400" -- drivers/
drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);

This doesn't make any sense. Am I looking at the right files? Am I
missing something?

-Saravana

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Marek Szyprowski]

Hi Saravana,

On 04.02.2021 22:31, Saravana Kannan wrote:
> On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 21.01.2021 23:57, Saravana Kannan wrote:
>>> This allows fw_devlink to create device links between consumers of an
>>> interrupt and the supplier of the interrupt.
>>>
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: Kevin Hilman <khilman@baylibre.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> Reviewed-by: Thierry Reding <treding@nvidia.com>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> This patch landed some time ago in linux-next as commit 4104ca776ba3
>> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
>> host controller operation on ARM Juno R1 board (the mmci@50000 device
>> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> I grepped around and it looks like the final board file is this or
> whatever includes it?
> arch/arm64/boot/dts/arm/juno-base.dtsi
The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> This patch just finds the interrupt-parent and then tries to use that
> as a supplier if "interrupts" property is listed. But the only
> interrupt parent I can see is:
>          gic: interrupt-controller@2c010000 {
>                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
>
> And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> a NOP since those suppliers are never devices and are ignored.
> $ git grep "arm,gic-400" -- drivers/
> drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
>
> This doesn't make any sense. Am I looking at the right files? Am I
> missing something?

Okay, I've added displaying a list of deferred devices when mounting 
rootfs fails and got following items:

Deferred devices:
18000000.ethernet        platform: probe deferral - supplier 
bus@8000000:motherboard-bus not ready
1c050000.mmci    amba: probe deferral - supplier 
bus@8000000:motherboard-bus not ready
1c1d0000.gpio    amba: probe deferral - supplier 
bus@8000000:motherboard-bus not ready
2b600000.iommu   platform: probe deferral - wait for supplier 
scpi-power-domains
7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
1c060000.kmi     amba: probe deferral - supplier 
bus@8000000:motherboard-bus not ready
1c070000.kmi     amba: probe deferral - supplier 
bus@8000000:motherboard-bus not ready
1c170000.rtc     amba: probe deferral - supplier 
bus@8000000:motherboard-bus not ready
1c0f0000.wdt     amba: probe deferral - supplier 
bus@8000000:motherboard-bus not ready
gpio-keys
Kernel panic - not syncing: VFS: Unable to mount root fs on 
unknown-block(0,0)

I don't see the 'bus@8000000:motherboard-bus' on the deferred devices 
list, so it looks that device core added a link to something that is not 
a platform device...

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Geert Uytterhoeven]

Hi Marek,

On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 04.02.2021 22:31, Saravana Kannan wrote:
> > On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 21.01.2021 23:57, Saravana Kannan wrote:
> >>> This allows fw_devlink to create device links between consumers of an
> >>> interrupt and the supplier of the interrupt.
> >>>
> >>> Cc: Marc Zyngier <maz@kernel.org>
> >>> Cc: Kevin Hilman <khilman@baylibre.com>
> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> This patch landed some time ago in linux-next as commit 4104ca776ba3
> >> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> >> host controller operation on ARM Juno R1 board (the mmci@50000 device
> >> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> > I grepped around and it looks like the final board file is this or
> > whatever includes it?
> > arch/arm64/boot/dts/arm/juno-base.dtsi
> The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> > This patch just finds the interrupt-parent and then tries to use that
> > as a supplier if "interrupts" property is listed. But the only
> > interrupt parent I can see is:
> >          gic: interrupt-controller@2c010000 {
> >                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
> >
> > And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> > a NOP since those suppliers are never devices and are ignored.
> > $ git grep "arm,gic-400" -- drivers/
> > drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> >
> > This doesn't make any sense. Am I looking at the right files? Am I
> > missing something?
>
> Okay, I've added displaying a list of deferred devices when mounting
> rootfs fails and got following items:
>
> Deferred devices:
> 18000000.ethernet        platform: probe deferral - supplier
> bus@8000000:motherboard-bus not ready
> 1c050000.mmci    amba: probe deferral - supplier
> bus@8000000:motherboard-bus not ready
> 1c1d0000.gpio    amba: probe deferral - supplier
> bus@8000000:motherboard-bus not ready
> 2b600000.iommu   platform: probe deferral - wait for supplier
> scpi-power-domains
> 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> 1c060000.kmi     amba: probe deferral - supplier
> bus@8000000:motherboard-bus not ready
> 1c070000.kmi     amba: probe deferral - supplier
> bus@8000000:motherboard-bus not ready
> 1c170000.rtc     amba: probe deferral - supplier
> bus@8000000:motherboard-bus not ready
> 1c0f0000.wdt     amba: probe deferral - supplier
> bus@8000000:motherboard-bus not ready
> gpio-keys
> Kernel panic - not syncing: VFS: Unable to mount root fs on
> unknown-block(0,0)
>
> I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> list, so it looks that device core added a link to something that is not
> a platform device...

Lemme guess: bus@8000000 is a simple bus, but it has an
interrupt-map, and the devlink code doesn't follow the mapping?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Marek,
>
> On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> > On 04.02.2021 22:31, Saravana Kannan wrote:
> > > On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> > > <m.szyprowski@samsung.com> wrote:
> > >> On 21.01.2021 23:57, Saravana Kannan wrote:
> > >>> This allows fw_devlink to create device links between consumers of an
> > >>> interrupt and the supplier of the interrupt.
> > >>>
> > >>> Cc: Marc Zyngier <maz@kernel.org>
> > >>> Cc: Kevin Hilman <khilman@baylibre.com>
> > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >>> Reviewed-by: Rob Herring <robh@kernel.org>
> > >>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> > >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >> This patch landed some time ago in linux-next as commit 4104ca776ba3
> > >> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> > >> host controller operation on ARM Juno R1 board (the mmci@50000 device
> > >> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> > > I grepped around and it looks like the final board file is this or
> > > whatever includes it?
> > > arch/arm64/boot/dts/arm/juno-base.dtsi
> > The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> > > This patch just finds the interrupt-parent and then tries to use that
> > > as a supplier if "interrupts" property is listed. But the only
> > > interrupt parent I can see is:
> > >          gic: interrupt-controller@2c010000 {
> > >                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
> > >
> > > And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> > > a NOP since those suppliers are never devices and are ignored.
> > > $ git grep "arm,gic-400" -- drivers/
> > > drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> > >
> > > This doesn't make any sense. Am I looking at the right files? Am I
> > > missing something?
> >
> > Okay, I've added displaying a list of deferred devices when mounting
> > rootfs fails and got following items:
> >
> > Deferred devices:
> > 18000000.ethernet        platform: probe deferral - supplier
> > bus@8000000:motherboard-bus not ready
> > 1c050000.mmci    amba: probe deferral - supplier
> > bus@8000000:motherboard-bus not ready
> > 1c1d0000.gpio    amba: probe deferral - supplier
> > bus@8000000:motherboard-bus not ready
> > 2b600000.iommu   platform: probe deferral - wait for supplier
> > scpi-power-domains
> > 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > 1c060000.kmi     amba: probe deferral - supplier
> > bus@8000000:motherboard-bus not ready
> > 1c070000.kmi     amba: probe deferral - supplier
> > bus@8000000:motherboard-bus not ready
> > 1c170000.rtc     amba: probe deferral - supplier
> > bus@8000000:motherboard-bus not ready
> > 1c0f0000.wdt     amba: probe deferral - supplier
> > bus@8000000:motherboard-bus not ready
> > gpio-keys
> > Kernel panic - not syncing: VFS: Unable to mount root fs on
> > unknown-block(0,0)
> >
> > I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> > list, so it looks that device core added a link to something that is not
> > a platform device...

Probe deferred devices (even platform devices) not showing up in that
list is not unusual. That's because devices end up on that list only
after a driver for them is matched and then it fails.

>
> Lemme guess: bus@8000000 is a simple bus, but it has an
> interrupt-map, and the devlink code doesn't follow the mapping?
>

No, what's happening is that (and this is something I just learned)
that if a parent has an "#interrupt-cells" property, it becomes your
interrupt parent. In this case, the motherboard-bus (still a platform
device) is the parent, but it never probes (because it's simple-bus
and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
this mmci device is marked as a consumer of this bus (while still a
grand-child). Yeah, I'm working on patches (multiple rewrites) to take
care of cases like this.

-Saravana

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Geert Uytterhoeven]

Hi Saravana,

On Fri, Feb 5, 2021 at 11:06 AM Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> > > On 04.02.2021 22:31, Saravana Kannan wrote:
> > > > On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> > > > <m.szyprowski@samsung.com> wrote:
> > > >> On 21.01.2021 23:57, Saravana Kannan wrote:
> > > >>> This allows fw_devlink to create device links between consumers of an
> > > >>> interrupt and the supplier of the interrupt.
> > > >>>
> > > >>> Cc: Marc Zyngier <maz@kernel.org>
> > > >>> Cc: Kevin Hilman <khilman@baylibre.com>
> > > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > >>> Reviewed-by: Rob Herring <robh@kernel.org>
> > > >>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> > > >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > >> This patch landed some time ago in linux-next as commit 4104ca776ba3
> > > >> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> > > >> host controller operation on ARM Juno R1 board (the mmci@50000 device
> > > >> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> > > > I grepped around and it looks like the final board file is this or
> > > > whatever includes it?
> > > > arch/arm64/boot/dts/arm/juno-base.dtsi
> > > The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> > > > This patch just finds the interrupt-parent and then tries to use that
> > > > as a supplier if "interrupts" property is listed. But the only
> > > > interrupt parent I can see is:
> > > >          gic: interrupt-controller@2c010000 {
> > > >                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
> > > >
> > > > And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> > > > a NOP since those suppliers are never devices and are ignored.
> > > > $ git grep "arm,gic-400" -- drivers/
> > > > drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> > > >
> > > > This doesn't make any sense. Am I looking at the right files? Am I
> > > > missing something?
> > >
> > > Okay, I've added displaying a list of deferred devices when mounting
> > > rootfs fails and got following items:
> > >
> > > Deferred devices:
> > > 18000000.ethernet        platform: probe deferral - supplier
> > > bus@8000000:motherboard-bus not ready
> > > 1c050000.mmci    amba: probe deferral - supplier
> > > bus@8000000:motherboard-bus not ready
> > > 1c1d0000.gpio    amba: probe deferral - supplier
> > > bus@8000000:motherboard-bus not ready
> > > 2b600000.iommu   platform: probe deferral - wait for supplier
> > > scpi-power-domains
> > > 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > 1c060000.kmi     amba: probe deferral - supplier
> > > bus@8000000:motherboard-bus not ready
> > > 1c070000.kmi     amba: probe deferral - supplier
> > > bus@8000000:motherboard-bus not ready
> > > 1c170000.rtc     amba: probe deferral - supplier
> > > bus@8000000:motherboard-bus not ready
> > > 1c0f0000.wdt     amba: probe deferral - supplier
> > > bus@8000000:motherboard-bus not ready
> > > gpio-keys
> > > Kernel panic - not syncing: VFS: Unable to mount root fs on
> > > unknown-block(0,0)
> > >
> > > I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> > > list, so it looks that device core added a link to something that is not
> > > a platform device...
>
> Probe deferred devices (even platform devices) not showing up in that
> list is not unusual. That's because devices end up on that list only
> after a driver for them is matched and then it fails.
>
> > Lemme guess: bus@8000000 is a simple bus, but it has an
> > interrupt-map, and the devlink code doesn't follow the mapping?
> >
>
> No, what's happening is that (and this is something I just learned)
> that if a parent has an "#interrupt-cells" property, it becomes your
> interrupt parent. In this case, the motherboard-bus (still a platform
> device) is the parent, but it never probes (because it's simple-bus
> and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
> this mmci device is marked as a consumer of this bus (while still a
> grand-child). Yeah, I'm working on patches (multiple rewrites) to take
> care of cases like this.

One more reason to scrap the different handling of "simple-bus" and
"simple-pm-bus", and use drivers/bus/simple-pm-bus.c, which is a
platform device driver, for both? (like I originally intended ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Fri, Feb 5, 2021 at 2:20 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Fri, Feb 5, 2021 at 11:06 AM Saravana Kannan <saravanak@google.com> wrote:
> > On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
> > > <m.szyprowski@samsung.com> wrote:
> > > > On 04.02.2021 22:31, Saravana Kannan wrote:
> > > > > On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> > > > > <m.szyprowski@samsung.com> wrote:
> > > > >> On 21.01.2021 23:57, Saravana Kannan wrote:
> > > > >>> This allows fw_devlink to create device links between consumers of an
> > > > >>> interrupt and the supplier of the interrupt.
> > > > >>>
> > > > >>> Cc: Marc Zyngier <maz@kernel.org>
> > > > >>> Cc: Kevin Hilman <khilman@baylibre.com>
> > > > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > >>> Reviewed-by: Rob Herring <robh@kernel.org>
> > > > >>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> > > > >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > >> This patch landed some time ago in linux-next as commit 4104ca776ba3
> > > > >> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> > > > >> host controller operation on ARM Juno R1 board (the mmci@50000 device
> > > > >> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> > > > > I grepped around and it looks like the final board file is this or
> > > > > whatever includes it?
> > > > > arch/arm64/boot/dts/arm/juno-base.dtsi
> > > > The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> > > > > This patch just finds the interrupt-parent and then tries to use that
> > > > > as a supplier if "interrupts" property is listed. But the only
> > > > > interrupt parent I can see is:
> > > > >          gic: interrupt-controller@2c010000 {
> > > > >                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
> > > > >
> > > > > And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> > > > > a NOP since those suppliers are never devices and are ignored.
> > > > > $ git grep "arm,gic-400" -- drivers/
> > > > > drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> > > > >
> > > > > This doesn't make any sense. Am I looking at the right files? Am I
> > > > > missing something?
> > > >
> > > > Okay, I've added displaying a list of deferred devices when mounting
> > > > rootfs fails and got following items:
> > > >
> > > > Deferred devices:
> > > > 18000000.ethernet        platform: probe deferral - supplier
> > > > bus@8000000:motherboard-bus not ready
> > > > 1c050000.mmci    amba: probe deferral - supplier
> > > > bus@8000000:motherboard-bus not ready
> > > > 1c1d0000.gpio    amba: probe deferral - supplier
> > > > bus@8000000:motherboard-bus not ready
> > > > 2b600000.iommu   platform: probe deferral - wait for supplier
> > > > scpi-power-domains
> > > > 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > 1c060000.kmi     amba: probe deferral - supplier
> > > > bus@8000000:motherboard-bus not ready
> > > > 1c070000.kmi     amba: probe deferral - supplier
> > > > bus@8000000:motherboard-bus not ready
> > > > 1c170000.rtc     amba: probe deferral - supplier
> > > > bus@8000000:motherboard-bus not ready
> > > > 1c0f0000.wdt     amba: probe deferral - supplier
> > > > bus@8000000:motherboard-bus not ready
> > > > gpio-keys
> > > > Kernel panic - not syncing: VFS: Unable to mount root fs on
> > > > unknown-block(0,0)
> > > >
> > > > I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> > > > list, so it looks that device core added a link to something that is not
> > > > a platform device...
> >
> > Probe deferred devices (even platform devices) not showing up in that
> > list is not unusual. That's because devices end up on that list only
> > after a driver for them is matched and then it fails.
> >
> > > Lemme guess: bus@8000000 is a simple bus, but it has an
> > > interrupt-map, and the devlink code doesn't follow the mapping?
> > >
> >
> > No, what's happening is that (and this is something I just learned)
> > that if a parent has an "#interrupt-cells" property, it becomes your
> > interrupt parent. In this case, the motherboard-bus (still a platform
> > device) is the parent, but it never probes (because it's simple-bus
> > and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
> > this mmci device is marked as a consumer of this bus (while still a
> > grand-child). Yeah, I'm working on patches (multiple rewrites) to take
> > care of cases like this.
>
> One more reason to scrap the different handling of "simple-bus" and
> "simple-pm-bus", and use drivers/bus/simple-pm-bus.c, which is a
> platform device driver, for both? (like I originally intended ;-)

I'm not sure if this will cause more issues since people are used to
simple-bus not needing a driver. I'm afraid to open that pandora's
box. Maybe last resort if I don't have any other options.

But keeping that aside, I'm confused how interrupts are even working
if the parent is a DT node with no driver (let alone a device). Any
ideas on what's going on or what I'm misunderstanding?

-Saravana

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Geert Uytterhoeven]

Hi Saravana,

On Fri, Feb 5, 2021 at 6:20 PM Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Feb 5, 2021 at 2:20 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Feb 5, 2021 at 11:06 AM Saravana Kannan <saravanak@google.com> wrote:
> > > On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
> > > > <m.szyprowski@samsung.com> wrote:
> > > > > On 04.02.2021 22:31, Saravana Kannan wrote:
> > > > > > On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> > > > > > <m.szyprowski@samsung.com> wrote:
> > > > > >> On 21.01.2021 23:57, Saravana Kannan wrote:
> > > > > >>> This allows fw_devlink to create device links between consumers of an
> > > > > >>> interrupt and the supplier of the interrupt.
> > > > > >>>
> > > > > >>> Cc: Marc Zyngier <maz@kernel.org>
> > > > > >>> Cc: Kevin Hilman <khilman@baylibre.com>
> > > > > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > >>> Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > >>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> > > > > >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > > >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > >> This patch landed some time ago in linux-next as commit 4104ca776ba3
> > > > > >> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> > > > > >> host controller operation on ARM Juno R1 board (the mmci@50000 device
> > > > > >> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> > > > > > I grepped around and it looks like the final board file is this or
> > > > > > whatever includes it?
> > > > > > arch/arm64/boot/dts/arm/juno-base.dtsi
> > > > > The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> > > > > > This patch just finds the interrupt-parent and then tries to use that
> > > > > > as a supplier if "interrupts" property is listed. But the only
> > > > > > interrupt parent I can see is:
> > > > > >          gic: interrupt-controller@2c010000 {
> > > > > >                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
> > > > > >
> > > > > > And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> > > > > > a NOP since those suppliers are never devices and are ignored.
> > > > > > $ git grep "arm,gic-400" -- drivers/
> > > > > > drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> > > > > >
> > > > > > This doesn't make any sense. Am I looking at the right files? Am I
> > > > > > missing something?
> > > > >
> > > > > Okay, I've added displaying a list of deferred devices when mounting
> > > > > rootfs fails and got following items:
> > > > >
> > > > > Deferred devices:
> > > > > 18000000.ethernet        platform: probe deferral - supplier
> > > > > bus@8000000:motherboard-bus not ready
> > > > > 1c050000.mmci    amba: probe deferral - supplier
> > > > > bus@8000000:motherboard-bus not ready
> > > > > 1c1d0000.gpio    amba: probe deferral - supplier
> > > > > bus@8000000:motherboard-bus not ready
> > > > > 2b600000.iommu   platform: probe deferral - wait for supplier
> > > > > scpi-power-domains
> > > > > 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > > 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > > 1c060000.kmi     amba: probe deferral - supplier
> > > > > bus@8000000:motherboard-bus not ready
> > > > > 1c070000.kmi     amba: probe deferral - supplier
> > > > > bus@8000000:motherboard-bus not ready
> > > > > 1c170000.rtc     amba: probe deferral - supplier
> > > > > bus@8000000:motherboard-bus not ready
> > > > > 1c0f0000.wdt     amba: probe deferral - supplier
> > > > > bus@8000000:motherboard-bus not ready
> > > > > gpio-keys
> > > > > Kernel panic - not syncing: VFS: Unable to mount root fs on
> > > > > unknown-block(0,0)
> > > > >
> > > > > I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> > > > > list, so it looks that device core added a link to something that is not
> > > > > a platform device...
> > >
> > > Probe deferred devices (even platform devices) not showing up in that
> > > list is not unusual. That's because devices end up on that list only
> > > after a driver for them is matched and then it fails.
> > >
> > > > Lemme guess: bus@8000000 is a simple bus, but it has an
> > > > interrupt-map, and the devlink code doesn't follow the mapping?
> > > >
> > >
> > > No, what's happening is that (and this is something I just learned)
> > > that if a parent has an "#interrupt-cells" property, it becomes your
> > > interrupt parent. In this case, the motherboard-bus (still a platform
> > > device) is the parent, but it never probes (because it's simple-bus
> > > and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
> > > this mmci device is marked as a consumer of this bus (while still a
> > > grand-child). Yeah, I'm working on patches (multiple rewrites) to take
> > > care of cases like this.
> >
> > One more reason to scrap the different handling of "simple-bus" and
> > "simple-pm-bus", and use drivers/bus/simple-pm-bus.c, which is a
> > platform device driver, for both? (like I originally intended ;-)
>
> I'm not sure if this will cause more issues since people are used to
> simple-bus not needing a driver. I'm afraid to open that pandora's
> box. Maybe last resort if I don't have any other options.
>
> But keeping that aside, I'm confused how interrupts are even working
> if the parent is a DT node with no driver (let alone a device). Any
> ideas on what's going on or what I'm misunderstanding?

No driver is needed, as the interrupts are just translated by the map,
and passed to another interrupt controller, which does have a driver.

Cfr. Section 2.4.3 "Interrupt Nexus Properties" in the DeviceTree
Specification (https://www.devicetree.org/).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Fri, Feb 5, 2021 at 9:52 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Fri, Feb 5, 2021 at 6:20 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Fri, Feb 5, 2021 at 2:20 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Feb 5, 2021 at 11:06 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
> > > > > <m.szyprowski@samsung.com> wrote:
> > > > > > On 04.02.2021 22:31, Saravana Kannan wrote:
> > > > > > > On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> > > > > > > <m.szyprowski@samsung.com> wrote:
> > > > > > >> On 21.01.2021 23:57, Saravana Kannan wrote:
> > > > > > >>> This allows fw_devlink to create device links between consumers of an
> > > > > > >>> interrupt and the supplier of the interrupt.
> > > > > > >>>
> > > > > > >>> Cc: Marc Zyngier <maz@kernel.org>
> > > > > > >>> Cc: Kevin Hilman <khilman@baylibre.com>
> > > > > > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > >>> Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > > >>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> > > > > > >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > > > >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > >> This patch landed some time ago in linux-next as commit 4104ca776ba3
> > > > > > >> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> > > > > > >> host controller operation on ARM Juno R1 board (the mmci@50000 device
> > > > > > >> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> > > > > > > I grepped around and it looks like the final board file is this or
> > > > > > > whatever includes it?
> > > > > > > arch/arm64/boot/dts/arm/juno-base.dtsi
> > > > > > The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> > > > > > > This patch just finds the interrupt-parent and then tries to use that
> > > > > > > as a supplier if "interrupts" property is listed. But the only
> > > > > > > interrupt parent I can see is:
> > > > > > >          gic: interrupt-controller@2c010000 {
> > > > > > >                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
> > > > > > >
> > > > > > > And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> > > > > > > a NOP since those suppliers are never devices and are ignored.
> > > > > > > $ git grep "arm,gic-400" -- drivers/
> > > > > > > drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> > > > > > >
> > > > > > > This doesn't make any sense. Am I looking at the right files? Am I
> > > > > > > missing something?
> > > > > >
> > > > > > Okay, I've added displaying a list of deferred devices when mounting
> > > > > > rootfs fails and got following items:
> > > > > >
> > > > > > Deferred devices:
> > > > > > 18000000.ethernet        platform: probe deferral - supplier
> > > > > > bus@8000000:motherboard-bus not ready
> > > > > > 1c050000.mmci    amba: probe deferral - supplier
> > > > > > bus@8000000:motherboard-bus not ready
> > > > > > 1c1d0000.gpio    amba: probe deferral - supplier
> > > > > > bus@8000000:motherboard-bus not ready
> > > > > > 2b600000.iommu   platform: probe deferral - wait for supplier
> > > > > > scpi-power-domains
> > > > > > 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > > > 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > > > 1c060000.kmi     amba: probe deferral - supplier
> > > > > > bus@8000000:motherboard-bus not ready
> > > > > > 1c070000.kmi     amba: probe deferral - supplier
> > > > > > bus@8000000:motherboard-bus not ready
> > > > > > 1c170000.rtc     amba: probe deferral - supplier
> > > > > > bus@8000000:motherboard-bus not ready
> > > > > > 1c0f0000.wdt     amba: probe deferral - supplier
> > > > > > bus@8000000:motherboard-bus not ready
> > > > > > gpio-keys
> > > > > > Kernel panic - not syncing: VFS: Unable to mount root fs on
> > > > > > unknown-block(0,0)
> > > > > >
> > > > > > I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> > > > > > list, so it looks that device core added a link to something that is not
> > > > > > a platform device...
> > > >
> > > > Probe deferred devices (even platform devices) not showing up in that
> > > > list is not unusual. That's because devices end up on that list only
> > > > after a driver for them is matched and then it fails.
> > > >
> > > > > Lemme guess: bus@8000000 is a simple bus, but it has an
> > > > > interrupt-map, and the devlink code doesn't follow the mapping?
> > > > >
> > > >
> > > > No, what's happening is that (and this is something I just learned)
> > > > that if a parent has an "#interrupt-cells" property, it becomes your
> > > > interrupt parent. In this case, the motherboard-bus (still a platform
> > > > device) is the parent, but it never probes (because it's simple-bus
> > > > and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
> > > > this mmci device is marked as a consumer of this bus (while still a
> > > > grand-child). Yeah, I'm working on patches (multiple rewrites) to take
> > > > care of cases like this.
> > >
> > > One more reason to scrap the different handling of "simple-bus" and
> > > "simple-pm-bus", and use drivers/bus/simple-pm-bus.c, which is a
> > > platform device driver, for both? (like I originally intended ;-)
> >
> > I'm not sure if this will cause more issues since people are used to
> > simple-bus not needing a driver. I'm afraid to open that pandora's
> > box. Maybe last resort if I don't have any other options.
> >
> > But keeping that aside, I'm confused how interrupts are even working
> > if the parent is a DT node with no driver (let alone a device). Any
> > ideas on what's going on or what I'm misunderstanding?
>
> No driver is needed, as the interrupts are just translated by the map,
> and passed to another interrupt controller, which does have a driver.
>
> Cfr. Section 2.4.3 "Interrupt Nexus Properties" in the DeviceTree
> Specification (https://www.devicetree.org/).
>

Yeah, I need to add interrupt-map support. Sigh. Only so many things I
can fix at a time. Let me know if you want to help.

-Saravana

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Fri, Feb 5, 2021 at 9:55 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, Feb 5, 2021 at 9:52 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Saravana,
> >
> > On Fri, Feb 5, 2021 at 6:20 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Fri, Feb 5, 2021 at 2:20 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Fri, Feb 5, 2021 at 11:06 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > > On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
> > > > > > <m.szyprowski@samsung.com> wrote:
> > > > > > > On 04.02.2021 22:31, Saravana Kannan wrote:
> > > > > > > > On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> > > > > > > > <m.szyprowski@samsung.com> wrote:
> > > > > > > >> On 21.01.2021 23:57, Saravana Kannan wrote:
> > > > > > > >>> This allows fw_devlink to create device links between consumers of an
> > > > > > > >>> interrupt and the supplier of the interrupt.
> > > > > > > >>>
> > > > > > > >>> Cc: Marc Zyngier <maz@kernel.org>
> > > > > > > >>> Cc: Kevin Hilman <khilman@baylibre.com>
> > > > > > > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > >>> Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > > > >>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> > > > > > > >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > > > > >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > >> This patch landed some time ago in linux-next as commit 4104ca776ba3
> > > > > > > >> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> > > > > > > >> host controller operation on ARM Juno R1 board (the mmci@50000 device
> > > > > > > >> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> > > > > > > > I grepped around and it looks like the final board file is this or
> > > > > > > > whatever includes it?
> > > > > > > > arch/arm64/boot/dts/arm/juno-base.dtsi
> > > > > > > The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> > > > > > > > This patch just finds the interrupt-parent and then tries to use that
> > > > > > > > as a supplier if "interrupts" property is listed. But the only
> > > > > > > > interrupt parent I can see is:
> > > > > > > >          gic: interrupt-controller@2c010000 {
> > > > > > > >                  compatible = "arm,gic-400", "arm,cortex-a15-gic";
> > > > > > > >
> > > > > > > > And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> > > > > > > > a NOP since those suppliers are never devices and are ignored.
> > > > > > > > $ git grep "arm,gic-400" -- drivers/
> > > > > > > > drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> > > > > > > >
> > > > > > > > This doesn't make any sense. Am I looking at the right files? Am I
> > > > > > > > missing something?
> > > > > > >
> > > > > > > Okay, I've added displaying a list of deferred devices when mounting
> > > > > > > rootfs fails and got following items:
> > > > > > >
> > > > > > > Deferred devices:
> > > > > > > 18000000.ethernet        platform: probe deferral - supplier
> > > > > > > bus@8000000:motherboard-bus not ready
> > > > > > > 1c050000.mmci    amba: probe deferral - supplier
> > > > > > > bus@8000000:motherboard-bus not ready
> > > > > > > 1c1d0000.gpio    amba: probe deferral - supplier
> > > > > > > bus@8000000:motherboard-bus not ready
> > > > > > > 2b600000.iommu   platform: probe deferral - wait for supplier
> > > > > > > scpi-power-domains
> > > > > > > 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > > > > 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> > > > > > > 1c060000.kmi     amba: probe deferral - supplier
> > > > > > > bus@8000000:motherboard-bus not ready
> > > > > > > 1c070000.kmi     amba: probe deferral - supplier
> > > > > > > bus@8000000:motherboard-bus not ready
> > > > > > > 1c170000.rtc     amba: probe deferral - supplier
> > > > > > > bus@8000000:motherboard-bus not ready
> > > > > > > 1c0f0000.wdt     amba: probe deferral - supplier
> > > > > > > bus@8000000:motherboard-bus not ready
> > > > > > > gpio-keys
> > > > > > > Kernel panic - not syncing: VFS: Unable to mount root fs on
> > > > > > > unknown-block(0,0)
> > > > > > >
> > > > > > > I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> > > > > > > list, so it looks that device core added a link to something that is not
> > > > > > > a platform device...
> > > > >
> > > > > Probe deferred devices (even platform devices) not showing up in that
> > > > > list is not unusual. That's because devices end up on that list only
> > > > > after a driver for them is matched and then it fails.
> > > > >
> > > > > > Lemme guess: bus@8000000 is a simple bus, but it has an
> > > > > > interrupt-map, and the devlink code doesn't follow the mapping?
> > > > > >
> > > > >
> > > > > No, what's happening is that (and this is something I just learned)
> > > > > that if a parent has an "#interrupt-cells" property, it becomes your
> > > > > interrupt parent. In this case, the motherboard-bus (still a platform
> > > > > device) is the parent, but it never probes (because it's simple-bus
> > > > > and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
> > > > > this mmci device is marked as a consumer of this bus (while still a
> > > > > grand-child). Yeah, I'm working on patches (multiple rewrites) to take
> > > > > care of cases like this.
> > > >
> > > > One more reason to scrap the different handling of "simple-bus" and
> > > > "simple-pm-bus", and use drivers/bus/simple-pm-bus.c, which is a
> > > > platform device driver, for both? (like I originally intended ;-)
> > >
> > > I'm not sure if this will cause more issues since people are used to
> > > simple-bus not needing a driver. I'm afraid to open that pandora's
> > > box. Maybe last resort if I don't have any other options.
> > >
> > > But keeping that aside, I'm confused how interrupts are even working
> > > if the parent is a DT node with no driver (let alone a device). Any
> > > ideas on what's going on or what I'm misunderstanding?
> >
> > No driver is needed, as the interrupts are just translated by the map,
> > and passed to another interrupt controller, which does have a driver.
> >
> > Cfr. Section 2.4.3 "Interrupt Nexus Properties" in the DeviceTree
> > Specification (https://www.devicetree.org/).
> >
>
> Yeah, I need to add interrupt-map support. Sigh. Only so many things I
> can fix at a time. Let me know if you want to help.
>

Marek,

After reading the DT spec and poking at the code, I THINK this code is
correct. Can you give it a shot? If it works, then I can clean it up,
roll in interrupts-extended and send a patch.

-Saravana

--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1300,10 +1300,12 @@ static struct device_node
*parse_gpio_compat(struct device_node *np,
 static struct device_node *parse_interrupts(struct device_node *np,
                                            const char *prop_name, int index)
 {
-       if (strcmp(prop_name, "interrupts") || index)
+       struct of_phandle_args sup_args;
+       if (strcmp(prop_name, "interrupts"))
                return NULL;

-       return of_irq_find_parent(np);
+       return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Marek Szyprowski]

Hi Saravana,

On 06.02.2021 05:32, Saravana Kannan wrote:
> On Fri, Feb 5, 2021 at 9:55 AM Saravana Kannan <saravanak@google.com> wrote:
>> On Fri, Feb 5, 2021 at 9:52 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Fri, Feb 5, 2021 at 6:20 PM Saravana Kannan <saravanak@google.com> wrote:
>>>> On Fri, Feb 5, 2021 at 2:20 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Fri, Feb 5, 2021 at 11:06 AM Saravana Kannan <saravanak@google.com> wrote:
>>>>>> On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>>> On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
>>>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>>>> On 04.02.2021 22:31, Saravana Kannan wrote:
>>>>>>>>> On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
>>>>>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>>>>>> On 21.01.2021 23:57, Saravana Kannan wrote:
>>>>>>>>>>> This allows fw_devlink to create device links between consumers of an
>>>>>>>>>>> interrupt and the supplier of the interrupt.
>>>>>>>>>>>
>>>>>>>>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>>>>>>>>> Cc: Kevin Hilman <khilman@baylibre.com>
>>>>>>>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>>>>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>>>>>>>>> Reviewed-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>>>>>>> This patch landed some time ago in linux-next as commit 4104ca776ba3
>>>>>>>>>> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
>>>>>>>>>> host controller operation on ARM Juno R1 board (the mmci@50000 device
>>>>>>>>>> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
>>>>>>>>> I grepped around and it looks like the final board file is this or
>>>>>>>>> whatever includes it?
>>>>>>>>> arch/arm64/boot/dts/arm/juno-base.dtsi
>>>>>>>> The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
>>>>>>>>> This patch just finds the interrupt-parent and then tries to use that
>>>>>>>>> as a supplier if "interrupts" property is listed. But the only
>>>>>>>>> interrupt parent I can see is:
>>>>>>>>>           gic: interrupt-controller@2c010000 {
>>>>>>>>>                   compatible = "arm,gic-400", "arm,cortex-a15-gic";
>>>>>>>>>
>>>>>>>>> And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
>>>>>>>>> a NOP since those suppliers are never devices and are ignored.
>>>>>>>>> $ git grep "arm,gic-400" -- drivers/
>>>>>>>>> drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
>>>>>>>>>
>>>>>>>>> This doesn't make any sense. Am I looking at the right files? Am I
>>>>>>>>> missing something?
>>>>>>>> Okay, I've added displaying a list of deferred devices when mounting
>>>>>>>> rootfs fails and got following items:
>>>>>>>>
>>>>>>>> Deferred devices:
>>>>>>>> 18000000.ethernet        platform: probe deferral - supplier
>>>>>>>> bus@8000000:motherboard-bus not ready
>>>>>>>> 1c050000.mmci    amba: probe deferral - supplier
>>>>>>>> bus@8000000:motherboard-bus not ready
>>>>>>>> 1c1d0000.gpio    amba: probe deferral - supplier
>>>>>>>> bus@8000000:motherboard-bus not ready
>>>>>>>> 2b600000.iommu   platform: probe deferral - wait for supplier
>>>>>>>> scpi-power-domains
>>>>>>>> 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
>>>>>>>> 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
>>>>>>>> 1c060000.kmi     amba: probe deferral - supplier
>>>>>>>> bus@8000000:motherboard-bus not ready
>>>>>>>> 1c070000.kmi     amba: probe deferral - supplier
>>>>>>>> bus@8000000:motherboard-bus not ready
>>>>>>>> 1c170000.rtc     amba: probe deferral - supplier
>>>>>>>> bus@8000000:motherboard-bus not ready
>>>>>>>> 1c0f0000.wdt     amba: probe deferral - supplier
>>>>>>>> bus@8000000:motherboard-bus not ready
>>>>>>>> gpio-keys
>>>>>>>> Kernel panic - not syncing: VFS: Unable to mount root fs on
>>>>>>>> unknown-block(0,0)
>>>>>>>>
>>>>>>>> I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
>>>>>>>> list, so it looks that device core added a link to something that is not
>>>>>>>> a platform device...
>>>>>> Probe deferred devices (even platform devices) not showing up in that
>>>>>> list is not unusual. That's because devices end up on that list only
>>>>>> after a driver for them is matched and then it fails.
>>>>>>
>>>>>>> Lemme guess: bus@8000000 is a simple bus, but it has an
>>>>>>> interrupt-map, and the devlink code doesn't follow the mapping?
>>>>>>>
>>>>>> No, what's happening is that (and this is something I just learned)
>>>>>> that if a parent has an "#interrupt-cells" property, it becomes your
>>>>>> interrupt parent. In this case, the motherboard-bus (still a platform
>>>>>> device) is the parent, but it never probes (because it's simple-bus
>>>>>> and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
>>>>>> this mmci device is marked as a consumer of this bus (while still a
>>>>>> grand-child). Yeah, I'm working on patches (multiple rewrites) to take
>>>>>> care of cases like this.
>>>>> One more reason to scrap the different handling of "simple-bus" and
>>>>> "simple-pm-bus", and use drivers/bus/simple-pm-bus.c, which is a
>>>>> platform device driver, for both? (like I originally intended ;-)
>>>> I'm not sure if this will cause more issues since people are used to
>>>> simple-bus not needing a driver. I'm afraid to open that pandora's
>>>> box. Maybe last resort if I don't have any other options.
>>>>
>>>> But keeping that aside, I'm confused how interrupts are even working
>>>> if the parent is a DT node with no driver (let alone a device). Any
>>>> ideas on what's going on or what I'm misunderstanding?
>>> No driver is needed, as the interrupts are just translated by the map,
>>> and passed to another interrupt controller, which does have a driver.
>>>
>>> Cfr. Section 2.4.3 "Interrupt Nexus Properties" in the DeviceTree
>>> Specification (https://protect2.fireeye.com/v1/url?k=72fff987-2d64c09f-72fe72c8-0cc47a314e9a-fd7dac11a78508f3&q=1&e=c0dbf5ca-130b-4aac-a011-447e82ca1914&u=https%3A%2F%2Fwww.devicetree.org%2F).
>>>
>> Yeah, I need to add interrupt-map support. Sigh. Only so many things I
>> can fix at a time. Let me know if you want to help.
>>
> Marek,
>
> After reading the DT spec and poking at the code, I THINK this code is
> correct. Can you give it a shot? If it works, then I can clean it up,
> roll in interrupts-extended and send a patch.

Yep, this fixes this issue. Fell free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> -Saravana
>
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1300,10 +1300,12 @@ static struct device_node
> *parse_gpio_compat(struct device_node *np,
>   static struct device_node *parse_interrupts(struct device_node *np,
>                                              const char *prop_name, int index)
>   {
> -       if (strcmp(prop_name, "interrupts") || index)
> +       struct of_phandle_args sup_args;
> +       if (strcmp(prop_name, "interrupts"))
>                  return NULL;
>
> -       return of_irq_find_parent(np);
> +       return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
>   }
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Mon, Feb 8, 2021 at 12:14 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Saravana,
>
> On 06.02.2021 05:32, Saravana Kannan wrote:
> > On Fri, Feb 5, 2021 at 9:55 AM Saravana Kannan <saravanak@google.com> wrote:
> >> On Fri, Feb 5, 2021 at 9:52 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Fri, Feb 5, 2021 at 6:20 PM Saravana Kannan <saravanak@google.com> wrote:
> >>>> On Fri, Feb 5, 2021 at 2:20 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>>> On Fri, Feb 5, 2021 at 11:06 AM Saravana Kannan <saravanak@google.com> wrote:
> >>>>>> On Fri, Feb 5, 2021 at 12:06 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>>>>> On Fri, Feb 5, 2021 at 8:38 AM Marek Szyprowski
> >>>>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>>>> On 04.02.2021 22:31, Saravana Kannan wrote:
> >>>>>>>>> On Thu, Feb 4, 2021 at 3:52 AM Marek Szyprowski
> >>>>>>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>>>>>> On 21.01.2021 23:57, Saravana Kannan wrote:
> >>>>>>>>>>> This allows fw_devlink to create device links between consumers of an
> >>>>>>>>>>> interrupt and the supplier of the interrupt.
> >>>>>>>>>>>
> >>>>>>>>>>> Cc: Marc Zyngier <maz@kernel.org>
> >>>>>>>>>>> Cc: Kevin Hilman <khilman@baylibre.com>
> >>>>>>>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>>>>>>>>>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>>>>>>>>>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> >>>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>>>>>>>> This patch landed some time ago in linux-next as commit 4104ca776ba3
> >>>>>>>>>> ("of: property: Add fw_devlink support for interrupts"). It breaks MMC
> >>>>>>>>>> host controller operation on ARM Juno R1 board (the mmci@50000 device
> >>>>>>>>>> defined in arch/arm64/boot/dts/arm/juno-motherboard.dtsi). I didn't
> >>>>>>>>> I grepped around and it looks like the final board file is this or
> >>>>>>>>> whatever includes it?
> >>>>>>>>> arch/arm64/boot/dts/arm/juno-base.dtsi
> >>>>>>>> The final board file is arch/arm64/boot/dts/arm/juno-r1.dts
> >>>>>>>>> This patch just finds the interrupt-parent and then tries to use that
> >>>>>>>>> as a supplier if "interrupts" property is listed. But the only
> >>>>>>>>> interrupt parent I can see is:
> >>>>>>>>>           gic: interrupt-controller@2c010000 {
> >>>>>>>>>                   compatible = "arm,gic-400", "arm,cortex-a15-gic";
> >>>>>>>>>
> >>>>>>>>> And the driver uses IRQCHIP_DECLARE() and hence should be pretty much
> >>>>>>>>> a NOP since those suppliers are never devices and are ignored.
> >>>>>>>>> $ git grep "arm,gic-400" -- drivers/
> >>>>>>>>> drivers/irqchip/irq-gic.c:IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> >>>>>>>>>
> >>>>>>>>> This doesn't make any sense. Am I looking at the right files? Am I
> >>>>>>>>> missing something?
> >>>>>>>> Okay, I've added displaying a list of deferred devices when mounting
> >>>>>>>> rootfs fails and got following items:
> >>>>>>>>
> >>>>>>>> Deferred devices:
> >>>>>>>> 18000000.ethernet        platform: probe deferral - supplier
> >>>>>>>> bus@8000000:motherboard-bus not ready
> >>>>>>>> 1c050000.mmci    amba: probe deferral - supplier
> >>>>>>>> bus@8000000:motherboard-bus not ready
> >>>>>>>> 1c1d0000.gpio    amba: probe deferral - supplier
> >>>>>>>> bus@8000000:motherboard-bus not ready
> >>>>>>>> 2b600000.iommu   platform: probe deferral - wait for supplier
> >>>>>>>> scpi-power-domains
> >>>>>>>> 7ff50000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> >>>>>>>> 7ff60000.hdlcd   platform: probe deferral - wait for supplier scpi-clk
> >>>>>>>> 1c060000.kmi     amba: probe deferral - supplier
> >>>>>>>> bus@8000000:motherboard-bus not ready
> >>>>>>>> 1c070000.kmi     amba: probe deferral - supplier
> >>>>>>>> bus@8000000:motherboard-bus not ready
> >>>>>>>> 1c170000.rtc     amba: probe deferral - supplier
> >>>>>>>> bus@8000000:motherboard-bus not ready
> >>>>>>>> 1c0f0000.wdt     amba: probe deferral - supplier
> >>>>>>>> bus@8000000:motherboard-bus not ready
> >>>>>>>> gpio-keys
> >>>>>>>> Kernel panic - not syncing: VFS: Unable to mount root fs on
> >>>>>>>> unknown-block(0,0)
> >>>>>>>>
> >>>>>>>> I don't see the 'bus@8000000:motherboard-bus' on the deferred devices
> >>>>>>>> list, so it looks that device core added a link to something that is not
> >>>>>>>> a platform device...
> >>>>>> Probe deferred devices (even platform devices) not showing up in that
> >>>>>> list is not unusual. That's because devices end up on that list only
> >>>>>> after a driver for them is matched and then it fails.
> >>>>>>
> >>>>>>> Lemme guess: bus@8000000 is a simple bus, but it has an
> >>>>>>> interrupt-map, and the devlink code doesn't follow the mapping?
> >>>>>>>
> >>>>>> No, what's happening is that (and this is something I just learned)
> >>>>>> that if a parent has an "#interrupt-cells" property, it becomes your
> >>>>>> interrupt parent. In this case, the motherboard-bus (still a platform
> >>>>>> device) is the parent, but it never probes (because it's simple-bus
> >>>>>> and "arm,vexpress,v2p-p1"). But it becomes the interrupt parent. And
> >>>>>> this mmci device is marked as a consumer of this bus (while still a
> >>>>>> grand-child). Yeah, I'm working on patches (multiple rewrites) to take
> >>>>>> care of cases like this.
> >>>>> One more reason to scrap the different handling of "simple-bus" and
> >>>>> "simple-pm-bus", and use drivers/bus/simple-pm-bus.c, which is a
> >>>>> platform device driver, for both? (like I originally intended ;-)
> >>>> I'm not sure if this will cause more issues since people are used to
> >>>> simple-bus not needing a driver. I'm afraid to open that pandora's
> >>>> box. Maybe last resort if I don't have any other options.
> >>>>
> >>>> But keeping that aside, I'm confused how interrupts are even working
> >>>> if the parent is a DT node with no driver (let alone a device). Any
> >>>> ideas on what's going on or what I'm misunderstanding?
> >>> No driver is needed, as the interrupts are just translated by the map,
> >>> and passed to another interrupt controller, which does have a driver.
> >>>
> >>> Cfr. Section 2.4.3 "Interrupt Nexus Properties" in the DeviceTree
> >>> Specification (https://protect2.fireeye.com/v1/url?k=72fff987-2d64c09f-72fe72c8-0cc47a314e9a-fd7dac11a78508f3&q=1&e=c0dbf5ca-130b-4aac-a011-447e82ca1914&u=https%3A%2F%2Fwww.devicetree.org%2F).
> >>>
> >> Yeah, I need to add interrupt-map support. Sigh. Only so many things I
> >> can fix at a time. Let me know if you want to help.
> >>
> > Marek,
> >
> > After reading the DT spec and poking at the code, I THINK this code is
> > correct. Can you give it a shot? If it works, then I can clean it up,
> > roll in interrupts-extended and send a patch.
>
> Yep, this fixes this issue. Fell free to add:
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>

Thanks! I'll send out the proper patch.

-Saravana

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Guenter Roeck]

Hi,

On Thu, Jan 21, 2021 at 02:57:12PM -0800, Saravana Kannan wrote:
> This allows fw_devlink to create device links between consumers of an
> interrupt and the supplier of the interrupt.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Saravana Kannan <saravanak@google.com>

This patch causes all ppc64:powernv qemu emulations to fail.
The problem is always the same: The root file system can not be mounted.

Example:

[   14.245672][    T1] VFS: Cannot open root device "sda" or unknown-block(0,0): error -6
[   14.246063][    T1] Please append a correct "root=" boot option; here are the available partitions:
[   14.246609][    T1] 1f00          131072 mtdblock0
[   14.246648][    T1]  (driver?)
[   14.247137][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
[   14.247631][    T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc7-next-20210212 #1
[   14.248166][    T1] Call Trace:
[   14.248344][    T1] [c000000002c07a70] [c0000000008f052c] dump_stack+0x100/0x174 (unreliable)
[   14.248780][    T1] [c000000002c07ab0] [c00000000010d0e0] panic+0x190/0x450
[   14.249097][    T1] [c000000002c07b50] [c0000000014d1af8] mount_block_root+0x320/0x430
[   14.249442][    T1] [c000000002c07c50] [c0000000014d1e64] prepare_namespace+0x1b0/0x204
[   14.249798][    T1] [c000000002c07cc0] [c0000000014d1544] kernel_init_freeable+0x3dc/0x438
[   14.250145][    T1] [c000000002c07da0] [c000000000012b7c] kernel_init+0x2c/0x170
[   14.250466][    T1] [c000000002c07e10] [c00000000000d56c] ret_from_kernel_thread+0x5c/0x70
[   28.068945385,5] OPAL: Reboot request...

Another:

[   14.273398][    T1] md: Autodetecting RAID arrays.
[   14.273665][    T1] md: autorun ...
[   14.273860][    T1] md: ... autorun DONE.
[   14.275078][    T1] Waiting for root device /dev/mmcblk0...

[ waits until terminated ]

Key difference seems to be that PCI devices are no longer instantiated
with this patch applied. Specifically, I see

[    1.153780][    T1] pci 0005:01     : [PE# fd] Setting up window#0 0..7fffffff pg=10000^M
[    1.154475][    T1] pci 0005:01     : [PE# fd] Enabling 64-bit DMA bypass^M
[    1.155749][    T1] pci 0005:01:00.0: Adding to iommu group 0^M
[    1.160543][    T1] pci 0005:00:00.0: enabling device (0105 -> 0107)^M

in both cases, but (exmple nvme) I don't see

[   13.520561][   T11] nvme nvme0: pci function 0005:01:00.0^M
[   13.521747][   T45] nvme 0005:01:00.0: enabling device (0100 -> 0102)^M

after this patch has been applied.

Reverting th patch plus its fix resolves the problem.

Bisect log attached.

Guenter

---
# bad: [07f7e57c63aaa2afb4ea31edef05e08699a63a00] Add linux-next specific files for 20210212
# good: [92bf22614b21a2706f4993b278017e437f7785b3] Linux 5.11-rc7
git bisect start 'HEAD' 'v5.11-rc7'
# good: [987d576a592082b8e0e499990236f49ad655f5dc] Merge remote-tracking branch 'crypto/master'
git bisect good 987d576a592082b8e0e499990236f49ad655f5dc
# good: [e254726a51e0440c05eb4606215772c34b77a53f] Merge remote-tracking branch 'spi/for-next'
git bisect good e254726a51e0440c05eb4606215772c34b77a53f
# bad: [309b7dce497b1ce820b2afb13a19b0ad39b23a4a] Merge remote-tracking branch 'char-misc/char-misc-next'
git bisect bad 309b7dce497b1ce820b2afb13a19b0ad39b23a4a
# good: [5c45f9ce2ad5c4abf79baf114e42620da77c84fe] Merge remote-tracking branch 'chrome-platform/for-next'
git bisect good 5c45f9ce2ad5c4abf79baf114e42620da77c84fe
# bad: [b45d849cd57bf11d1824f68d92404ceea1293886] Merge remote-tracking branch 'usb/usb-next'
git bisect bad b45d849cd57bf11d1824f68d92404ceea1293886
# good: [89451aabea5f91a6c1b6dc4c52cac4caffecbc8a] Merge tag 'tag-ib-usb-typec-chrome-platform-cros-ec-typec-clear-pd-discovery-events-for-5.12' of git://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux into usb-next
git bisect good 89451aabea5f91a6c1b6dc4c52cac4caffecbc8a
# good: [43861d29c0810a70792bf69d37482efb7bb6677d] USB: quirks: sort quirk entries
git bisect good 43861d29c0810a70792bf69d37482efb7bb6677d
# good: [08f4a6b903369ee0147b557931b7075c17e015f6] dt-bindings: usb: dwc3: add description for rk3328
git bisect good 08f4a6b903369ee0147b557931b7075c17e015f6
# bad: [1753c4d1edbcf1b35e585ff76777d09434344c8f] of: property: Don't add links to absent suppliers
git bisect bad 1753c4d1edbcf1b35e585ff76777d09434344c8f
# good: [072a51be8ecfb84e15b27b7f80a601560f386788] Merge 5.11-rc5 into driver-core-next
git bisect good 072a51be8ecfb84e15b27b7f80a601560f386788
# bad: [4731210c09f5977300f439b6c56ba220c65b2348] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default
git bisect bad 4731210c09f5977300f439b6c56ba220c65b2348
# bad: [4044b2fcfb2048a256529ecbd869b43713982006] drivers: base: change 'driver_create_groups' to 'driver_add_groups' in printk
git bisect bad 4044b2fcfb2048a256529ecbd869b43713982006
# bad: [4104ca776ba38d81bd6610256d3b0d7e6a058067] of: property: Add fw_devlink support for interrupts
git bisect bad 4104ca776ba38d81bd6610256d3b0d7e6a058067
# good: [e13f5b7a130f7b6d4d34be27a87393890b5ee2ba] of: property: Add fw_devlink support for "gpio" and "gpios" binding
git bisect good e13f5b7a130f7b6d4d34be27a87393890b5ee2ba
# first bad commit: [4104ca776ba38d81bd6610256d3b0d7e6a058067] of: property: Add fw_devlink support for interrupts

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Sat, Feb 13, 2021 at 10:54 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Hi,
>
> On Thu, Jan 21, 2021 at 02:57:12PM -0800, Saravana Kannan wrote:
> > This allows fw_devlink to create device links between consumers of an
> > interrupt and the supplier of the interrupt.
> >
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Kevin Hilman <khilman@baylibre.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Thierry Reding <treding@nvidia.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> This patch causes all ppc64:powernv qemu emulations to fail.
> The problem is always the same: The root file system can not be mounted.
>
> Example:
>
> [   14.245672][    T1] VFS: Cannot open root device "sda" or unknown-block(0,0): error -6
> [   14.246063][    T1] Please append a correct "root=" boot option; here are the available partitions:
> [   14.246609][    T1] 1f00          131072 mtdblock0
> [   14.246648][    T1]  (driver?)
> [   14.247137][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
> [   14.247631][    T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc7-next-20210212 #1
> [   14.248166][    T1] Call Trace:
> [   14.248344][    T1] [c000000002c07a70] [c0000000008f052c] dump_stack+0x100/0x174 (unreliable)
> [   14.248780][    T1] [c000000002c07ab0] [c00000000010d0e0] panic+0x190/0x450
> [   14.249097][    T1] [c000000002c07b50] [c0000000014d1af8] mount_block_root+0x320/0x430
> [   14.249442][    T1] [c000000002c07c50] [c0000000014d1e64] prepare_namespace+0x1b0/0x204
> [   14.249798][    T1] [c000000002c07cc0] [c0000000014d1544] kernel_init_freeable+0x3dc/0x438
> [   14.250145][    T1] [c000000002c07da0] [c000000000012b7c] kernel_init+0x2c/0x170
> [   14.250466][    T1] [c000000002c07e10] [c00000000000d56c] ret_from_kernel_thread+0x5c/0x70
> [   28.068945385,5] OPAL: Reboot request...
>
> Another:
>
> [   14.273398][    T1] md: Autodetecting RAID arrays.
> [   14.273665][    T1] md: autorun ...
> [   14.273860][    T1] md: ... autorun DONE.
> [   14.275078][    T1] Waiting for root device /dev/mmcblk0...
>
> [ waits until terminated ]
>
> Key difference seems to be that PCI devices are no longer instantiated
> with this patch applied. Specifically, I see
>
> [    1.153780][    T1] pci 0005:01     : [PE# fd] Setting up window#0 0..7fffffff pg=10000^M
> [    1.154475][    T1] pci 0005:01     : [PE# fd] Enabling 64-bit DMA bypass^M
> [    1.155749][    T1] pci 0005:01:00.0: Adding to iommu group 0^M
> [    1.160543][    T1] pci 0005:00:00.0: enabling device (0105 -> 0107)^M
>
> in both cases, but (exmple nvme) I don't see
>
> [   13.520561][   T11] nvme nvme0: pci function 0005:01:00.0^M
> [   13.521747][   T45] nvme 0005:01:00.0: enabling device (0100 -> 0102)^M
>
> after this patch has been applied.
>
> Reverting th patch plus its fix resolves the problem.
>
> Bisect log attached.

Hi Guenter,

Thanks for the report.

Can you please give me the following details:
* The DTS file for the board (not the SoC).
* A boot log with the logs enabled in device_links_check_suppliers()
and device_link_add()

That should help me debug this.

Rob,

Looks like Guenter has this patch[1] too. What PPC specific IRQ hack
am I missing? Any ideas?

[1] - https://lore.kernel.org/lkml/20210209010439.3529036-1-saravanak@google.com/

Thanks,
Saravana

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Guenter Roeck]

On 2/14/21 1:12 PM, Saravana Kannan wrote:
[ ... ]
> Can you please give me the following details:
> * The DTS file for the board (not the SoC).

I don't have it, sorry. The devicetree file is generated by qemu.
Is there a way to extract it from a running system ?

> * A boot log with the logs enabled in device_links_check_suppliers()
> and device_link_add()
> 
There is not much to be seen, except this:

[   14.084606][   T11] pci 0005:01:00.0: probe deferral - wait for supplier interrupt-controller@0

repeated three times.

Guenter

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Guenter Roeck]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]

On 2/14/21 1:12 PM, Saravana Kannan wrote:
[ ... ]
> 
> Can you please give me the following details:
> * The DTS file for the board (not the SoC).

The devicetree file extracted from the running system is attached.
Hope it helps.

Guenter

[-- Attachment #2: powernv.dts --]
[-- Type: audio/vnd.dts, Size: 697342 bytes --]

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Sun, Feb 14, 2021 at 7:58 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 2/14/21 1:12 PM, Saravana Kannan wrote:
> [ ... ]
> >
> > Can you please give me the following details:
> > * The DTS file for the board (not the SoC).
>
> The devicetree file extracted from the running system is attached.
> Hope it helps.

Hi Guenter,

Thanks for the DTS file and logs. That helps a lot.

Looking at the attachment and this line from the earlier email:
[   14.084606][   T11] pci 0005:01:00.0: probe deferral - wait for
supplier interrupt-controller@0

It's clear the PCI node is waiting on:
        interrupt-controller@0 {
                #address-cells = <0x00>;
                device_type = "PowerPC-Interrupt-Source-Controller";
                compatible = "ibm,opal-xive-vc\0IBM,opal-xics";
                #interrupt-cells = <0x02>;
                reg = <0x00 0x00 0x00 0x00>;
                phandle = <0x804b>;
                interrupt-controller;
        };

If I grep for "ibm,opal-xive-vc", I see only one instance of it in the
code. And that eventually ends up getting called like this:
irq_find_matching_fwspec() -> xive_irq_domain_match() -> xive_native_match()

static bool xive_native_match(struct device_node *node)
{
        return of_device_is_compatible(node, "ibm,opal-xive-vc");
}

However, when the IRQ domain are first registered, in xive_init_host()
the "np" passed in is NOT the same node that xive_native_match() would
match.
static void __init xive_init_host(struct device_node *np)
{
        xive_irq_domain = irq_domain_add_nomap(np, XIVE_MAX_IRQ,
                                               &xive_irq_domain_ops, NULL);
        if (WARN_ON(xive_irq_domain == NULL))
                return;
        irq_set_default_host(xive_irq_domain);
}

Instead, the "np" here is:
        interrupt-controller@6030203180000 {
                ibm,xive-provision-page-size = <0x10000>;
                ibm,xive-eq-sizes = <0x0c 0x10 0x15 0x18>;
                single-escalation-support;
                ibm,xive-provision-chips = <0x00>;
                ibm,xive-#priorities = <0x08>;
                compatible = "ibm,opal-xive-pe\0ibm,opal-intc";
                reg = <0x60302 0x3180000 0x00 0x10000 0x60302
0x3190000 0x00 0x10000 0x60302 0x31a0000 0x00 0x10000 0x60302
0x31b0000 0x00 0x10000>;
                phandle = <0x8051>;
        };

There are many ways to fix this, but I first want to make sure this is
a valid way to register irqdomains before trying to fix it. I just
find it weird that the node that's registered is unrelated (not a
parent/child) of the node that matches.

Marc,

Is this a valid way to register irqdomains? Just registering
interrupt-controller@6030203180000 DT node where there are multiple
interrupt controllers?

Thanks,
Saravana

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Marc Zyngier]

Hi Saravana,

On Mon, 15 Feb 2021 08:29:53 +0000,
Saravana Kannan <saravanak@google.com> wrote:
> 
> On Sun, Feb 14, 2021 at 7:58 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 2/14/21 1:12 PM, Saravana Kannan wrote:
> > [ ... ]
> > >
> > > Can you please give me the following details:
> > > * The DTS file for the board (not the SoC).
> >
> > The devicetree file extracted from the running system is attached.
> > Hope it helps.
> 
> Hi Guenter,
> 
> Thanks for the DTS file and logs. That helps a lot.
> 
> Looking at the attachment and this line from the earlier email:
> [   14.084606][   T11] pci 0005:01:00.0: probe deferral - wait for
> supplier interrupt-controller@0
> 
> It's clear the PCI node is waiting on:
>         interrupt-controller@0 {
>                 #address-cells = <0x00>;
>                 device_type = "PowerPC-Interrupt-Source-Controller";
>                 compatible = "ibm,opal-xive-vc\0IBM,opal-xics";
>                 #interrupt-cells = <0x02>;
>                 reg = <0x00 0x00 0x00 0x00>;
>                 phandle = <0x804b>;
>                 interrupt-controller;
>         };
> 
> If I grep for "ibm,opal-xive-vc", I see only one instance of it in the
> code. And that eventually ends up getting called like this:
> irq_find_matching_fwspec() -> xive_irq_domain_match() -> xive_native_match()
> 
> static bool xive_native_match(struct device_node *node)
> {
>         return of_device_is_compatible(node, "ibm,opal-xive-vc");
> }
> 
> However, when the IRQ domain are first registered, in xive_init_host()
> the "np" passed in is NOT the same node that xive_native_match() would
> match.
> static void __init xive_init_host(struct device_node *np)
> {
>         xive_irq_domain = irq_domain_add_nomap(np, XIVE_MAX_IRQ,
>                                                &xive_irq_domain_ops, NULL);
>         if (WARN_ON(xive_irq_domain == NULL))
>                 return;
>         irq_set_default_host(xive_irq_domain);
> }
> 
> Instead, the "np" here is:
>         interrupt-controller@6030203180000 {
>                 ibm,xive-provision-page-size = <0x10000>;
>                 ibm,xive-eq-sizes = <0x0c 0x10 0x15 0x18>;
>                 single-escalation-support;
>                 ibm,xive-provision-chips = <0x00>;
>                 ibm,xive-#priorities = <0x08>;
>                 compatible = "ibm,opal-xive-pe\0ibm,opal-intc";
>                 reg = <0x60302 0x3180000 0x00 0x10000 0x60302
> 0x3190000 0x00 0x10000 0x60302 0x31a0000 0x00 0x10000 0x60302
> 0x31b0000 0x00 0x10000>;
>                 phandle = <0x8051>;
>         };
> 
> There are many ways to fix this, but I first want to make sure this is
> a valid way to register irqdomains before trying to fix it. I just
> find it weird that the node that's registered is unrelated (not a
> parent/child) of the node that matches.
> 
> Marc,
> 
> Is this a valid way to register irqdomains? Just registering
> interrupt-controller@6030203180000 DT node where there are multiple
> interrupt controllers?

Absolutely.

The node is only one of the many possible ways to retrieve a
domain. In general, what you pass as the of_node/fwnode_handle can be
anything you want. It doesn't have to represent anything in the system
(we even create then ex-nihilo in some cases), and the match/select
callbacks are authoritative when they exist.

There is also the use of a default domain, which is used as a fallback
when no domain is found via the normal matching procedure.

PPC has established a way of dealing with domains long before ARM did,
closer to the board files of old than what we would do today (code
driven rather than data structure driven).

Strictly mapping domains onto HW blocks is a desirable property, but
that is all it is. That doesn't affect the very purpose of the IRQ
domains, which is to translate numbers from one context into another.

I'd be all for rationalising this, but it is pretty hard to introduce
semantic where there is none.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

On Mon, Feb 15, 2021 at 1:09 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Saravana,
>
> On Mon, 15 Feb 2021 08:29:53 +0000,
> Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Sun, Feb 14, 2021 at 7:58 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On 2/14/21 1:12 PM, Saravana Kannan wrote:
> > > [ ... ]
> > > >
> > > > Can you please give me the following details:
> > > > * The DTS file for the board (not the SoC).
> > >
> > > The devicetree file extracted from the running system is attached.
> > > Hope it helps.
> >
> > Hi Guenter,
> >
> > Thanks for the DTS file and logs. That helps a lot.
> >
> > Looking at the attachment and this line from the earlier email:
> > [   14.084606][   T11] pci 0005:01:00.0: probe deferral - wait for
> > supplier interrupt-controller@0
> >
> > It's clear the PCI node is waiting on:
> >         interrupt-controller@0 {
> >                 #address-cells = <0x00>;
> >                 device_type = "PowerPC-Interrupt-Source-Controller";
> >                 compatible = "ibm,opal-xive-vc\0IBM,opal-xics";
> >                 #interrupt-cells = <0x02>;
> >                 reg = <0x00 0x00 0x00 0x00>;
> >                 phandle = <0x804b>;
> >                 interrupt-controller;
> >         };
> >
> > If I grep for "ibm,opal-xive-vc", I see only one instance of it in the
> > code. And that eventually ends up getting called like this:
> > irq_find_matching_fwspec() -> xive_irq_domain_match() -> xive_native_match()
> >
> > static bool xive_native_match(struct device_node *node)
> > {
> >         return of_device_is_compatible(node, "ibm,opal-xive-vc");
> > }
> >
> > However, when the IRQ domain are first registered, in xive_init_host()
> > the "np" passed in is NOT the same node that xive_native_match() would
> > match.
> > static void __init xive_init_host(struct device_node *np)
> > {
> >         xive_irq_domain = irq_domain_add_nomap(np, XIVE_MAX_IRQ,
> >                                                &xive_irq_domain_ops, NULL);
> >         if (WARN_ON(xive_irq_domain == NULL))
> >                 return;
> >         irq_set_default_host(xive_irq_domain);
> > }
> >
> > Instead, the "np" here is:
> >         interrupt-controller@6030203180000 {
> >                 ibm,xive-provision-page-size = <0x10000>;
> >                 ibm,xive-eq-sizes = <0x0c 0x10 0x15 0x18>;
> >                 single-escalation-support;
> >                 ibm,xive-provision-chips = <0x00>;
> >                 ibm,xive-#priorities = <0x08>;
> >                 compatible = "ibm,opal-xive-pe\0ibm,opal-intc";
> >                 reg = <0x60302 0x3180000 0x00 0x10000 0x60302
> > 0x3190000 0x00 0x10000 0x60302 0x31a0000 0x00 0x10000 0x60302
> > 0x31b0000 0x00 0x10000>;
> >                 phandle = <0x8051>;
> >         };
> >
> > There are many ways to fix this, but I first want to make sure this is
> > a valid way to register irqdomains before trying to fix it. I just
> > find it weird that the node that's registered is unrelated (not a
> > parent/child) of the node that matches.
> >
> > Marc,
> >
> > Is this a valid way to register irqdomains? Just registering
> > interrupt-controller@6030203180000 DT node where there are multiple
> > interrupt controllers?
>
> Absolutely.
>
> The node is only one of the many possible ways to retrieve a
> domain. In general, what you pass as the of_node/fwnode_handle can be
> anything you want. It doesn't have to represent anything in the system
> (we even create then ex-nihilo in some cases), and the match/select
> callbacks are authoritative when they exist.
>
> There is also the use of a default domain, which is used as a fallback
> when no domain is found via the normal matching procedure.
>
> PPC has established a way of dealing with domains long before ARM did,
> closer to the board files of old than what we would do today (code
> driven rather than data structure driven).
>
> Strictly mapping domains onto HW blocks is a desirable property, but
> that is all it is. That doesn't affect the very purpose of the IRQ
> domains, which is to translate numbers from one context into another.
>
> I'd be all for rationalising this, but it is pretty hard to introduce
> semantic where there is none.

Ok, I'm going to disable parsing "interrupts" for PPC. It doesn't look
like any of the irq drivers are even remotely ready to be converted to
a proper device driver anyway.

And if this continues for other properties, I'll just disable
fw_devlink for PPC entirely.

-Saravana

Re: [PATCH v2 0/2] of: property: Add fw_devlink support for more props

[Greg Kroah-Hartman]

On Thu, Jan 21, 2021 at 02:57:10PM -0800, Saravana Kannan wrote:
> Sending again because I messed up the To/Cc for the coverletter.
> 
> This series combines two patches [1] [2] that'd conflict.
> 
> Greg,
> 
> Can you please pull this into driver-core-next?

Now queued up, thanks.

greg k-h

Re: [PATCH v2 0/2] of: property: Add fw_devlink support for more props

[Saravana Kannan]

On Sun, Jan 31, 2021 at 8:38 AM Martin Kaiser <martin@kaiser.cx> wrote:
>
> Dear all,
>
> Thus wrote Saravana Kannan (saravanak@google.com):
>
> > Sending again because I messed up the To/Cc for the coverletter.
>
> > This series combines two patches [1] [2] that'd conflict.
>
> > Greg,
>
> > Can you please pull this into driver-core-next?
>
> > -Saravana
>
> > [1] - https://lore.kernel.org/lkml/20210115210159.3090203-1-saravanak@google.com/
> > [2] - https://lore.kernel.org/lkml/20201218210750.3455872-1-saravanak@google.com/
>
> I'm running linux-next on my hardware which is based on the imx258
> chipset by Freescale/NXP.
>
> When those two patches appeared in linux-next, my system would not boot
> any more. It was stuck right after
>
> Uncompressing Linux... done, booting the kernel.
>
> Reverting the irq-patch made the system boot again. Still, a number of
> devices like usb or nand flash controller are not found any more.
> If I revert the gpio patch as well, all devices are available again.
>
> My system's device tree is based on arch/arm/boot/dts/imx25.dtsi with
> very few adaptations for my board.
>
> I tried to play around with the new parse_interrupts() function to
> figure out which device causes the boot failure. If I skip the following
> device, I can boot again:
>
> -       return of_irq_find_parent(np);
> +       np_ret = of_irq_find_parent(np);
> +       if (!strcmp(np->full_name, "serial@50008000")) {
> +           printk(KERN_ERR "skip serial@50008000\n");
> +           return NULL;
> +       }
> +      return np_ret;
>
> This is uart4 of the imx258 chip, which I use as my serial console. The
> imx25.dtsi device tree seems ok, we find an interrupt parent. The
> problem must be in the code that processes the result of
> parse_interrupts().
>
> I tried to boot the unmodified code with qemu, simulating the imx25-pdk
> device. This wouldn't boot either.
>
> Does this ring any bells with anyone?

This series [1] has a high chance of fixing it for you if
CONFIG_MODULES is disabled in your set up. Can you give it a shot?

The real problem is that arch/arm/mach-imx/avic.c doesn't set the
OF_POPULATED flag for the "fsl,avic" node. fw_devlink uses this
information to know that this device node will never have a struct
device created for it. The proper way to do this for root IRQCHIP
nodes is to use IRQCHIP_DECLARE(). I Cc'ed you on a clean up patch for
IMX [2], can you please give [2] a shot *without* [1] and with
CONFIG_MODULES enabled? Things should boot properly with this
combination too.

Btw, for future reference, you can try enabling the logs in
device_links_check_suppliers() to see what devices are being blocked
on what supplier nodes.

[1] - https://lore.kernel.org/lkml/20210130040344.2807439-1-saravanak@google.com/
[2] - https://lore.kernel.org/lkml/20210131205654.3379661-1-saravanak@google.com/T/#u

Thanks,
Saravana

Re: [PATCH v2 0/2] of: property: Add fw_devlink support for more props

[Martin Kaiser]

Hi Saravana,

Thus wrote Saravana Kannan (saravanak@google.com):

> This series [1] has a high chance of fixing it for you if
> CONFIG_MODULES is disabled in your set up. Can you give it a shot?

sure. This fixes things for me if CONFIG_MODULES is disabled. Booting is
still stuck if modules are enabled.

> The real problem is that arch/arm/mach-imx/avic.c doesn't set the
> OF_POPULATED flag for the "fsl,avic" node. fw_devlink uses this
> information to know that this device node will never have a struct
> device created for it. The proper way to do this for root IRQCHIP
> nodes is to use IRQCHIP_DECLARE(). I Cc'ed you on a clean up patch for
> IMX [2], can you please give [2] a shot *without* [1] and with
> CONFIG_MODULES enabled? Things should boot properly with this
> combination too.

This works as well.

Thanks,
Martin

Re: [PATCH v2 0/2] of: property: Add fw_devlink support for more props

[Saravana Kannan]

On Mon, Feb 1, 2021 at 2:52 AM Martin Kaiser <martin@kaiser.cx> wrote:
>
> Hi Saravana,
>
> Thus wrote Saravana Kannan (saravanak@google.com):
>
> > This series [1] has a high chance of fixing it for you if
> > CONFIG_MODULES is disabled in your set up. Can you give it a shot?
>
> sure. This fixes things for me if CONFIG_MODULES is disabled. Booting is
> still stuck if modules are enabled.
>
> > The real problem is that arch/arm/mach-imx/avic.c doesn't set the
> > OF_POPULATED flag for the "fsl,avic" node. fw_devlink uses this
> > information to know that this device node will never have a struct
> > device created for it. The proper way to do this for root IRQCHIP
> > nodes is to use IRQCHIP_DECLARE(). I Cc'ed you on a clean up patch for
> > IMX [2], can you please give [2] a shot *without* [1] and with
> > CONFIG_MODULES enabled? Things should boot properly with this
> > combination too.
>
> This works as well.

Thanks for testing both. Mind giving Tested-by for [1] too?

-Saravana

[PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Saravana Kannan]

This allows fw_devlink to create device links between consumers of an
interrupt and the supplier of the interrupt.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Thierry Reding <treding@nvidia.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index b2ea1951d937..6287c6d60bb7 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -24,6 +24,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_graph.h>
+#include <linux/of_irq.h>
 #include <linux/string.h>
 #include <linux/moduleparam.h>
 
@@ -1293,6 +1294,15 @@ static struct device_node *parse_gpio_compat(struct device_node *np,
 	return sup_args.np;
 }
 
+static struct device_node *parse_interrupts(struct device_node *np,
+					    const char *prop_name, int index)
+{
+	if (strcmp(prop_name, "interrupts") || index)
+		return NULL;
+
+	return of_irq_find_parent(np);
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
@@ -1319,6 +1329,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_pinctrl7, },
 	{ .parse_prop = parse_pinctrl8, },
 	{ .parse_prop = parse_gpio_compat, },
+	{ .parse_prop = parse_interrupts, },
 	{ .parse_prop = parse_regulators, },
 	{ .parse_prop = parse_gpio, },
 	{ .parse_prop = parse_gpios, },
-- 
2.30.0.296.g2bfb1c46d8-goog