[PATCH] PCI: of: Update parent unit address generation in of_pci_prop_intr_map()

[PATCH] PCI: of: Update parent unit address generation in of_pci_prop_intr_map()

[Lorenzo Pieralisi]

Some interrupt controllers require an #address-cells property in their
bindings without requiring a "reg" property to be present.

The current logic used to craft an interrupt-map property in
of_pci_prop_intr_map() is based on reading the #address-cells
property in the interrupt-parent and, if != 0, read the interrupt
parent "reg" property to determine the parent unit address to be
used to create the parent unit interrupt specifier.

First of all, it is not correct to read the "reg" property of
the interrupt-parent with an #address-cells value taken from the
interrupt-parent node, because the #address-cells value define the
number of address cells required by child nodes.

More importantly, for all modern interrupt controllers, the parent
unit address is irrelevant in HW in relation to the
device<->interrupt-controller connection and the kernel actually
ignores the parent unit address value when hierarchically parsing
the interrupt-map property (ie of_irq_parse_raw()).

For the reasons above, remove the code parsing the interrupt
parent "reg" property in of_pci_prop_intr_map() - it is not
needed and as it is it is detrimental in that it prevents
interrupt-map property generation on systems with an
interrupt-controller that has no "reg" property in its
interrupt-controller node - and leave the parent unit address
always initialized to 0 since it is simply ignored by the kernel.

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Lizhi Hou <lizhi.hou@amd.com>
Link: https://lore.kernel.org/lkml/aJms+YT8TnpzpCY8@lpieralisi/
---
 drivers/pci/of_property.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 506fcd507113..09b7bc335ec5 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -279,13 +279,20 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
 			mapp++;
 			*mapp = out_irq[i].np->phandle;
 			mapp++;
-			if (addr_sz[i]) {
-				ret = of_property_read_u32_array(out_irq[i].np,
-								 "reg", mapp,
-								 addr_sz[i]);
-				if (ret)
-					goto failed;
-			}
+
+			/*
+			 * A device address does not affect the
+			 * device<->interrupt-controller HW connection for all
+			 * modern interrupt controllers; moreover, the kernel
+			 * (ie of_irq_parse_raw()) ignores the values in the
+			 * parent unit address cells while parsing the interrupt-map
+			 * property because they are irrelevant for interrupts mapping
+			 * in modern system.
+			 *
+			 * Leave the parent unit address initialized to 0 - just
+			 * take into account the #address-cells size to build
+			 * the property properly.
+			 */
 			mapp += addr_sz[i];
 			memcpy(mapp, out_irq[i].args,
 			       out_irq[i].args_count * sizeof(u32));
-- 
2.48.0

Re: [PATCH] PCI: of: Update parent unit address generation in of_pci_prop_intr_map()

[Lizhi Hou]

On 8/18/25 02:35, Lorenzo Pieralisi wrote:
> Some interrupt controllers require an #address-cells property in their
> bindings without requiring a "reg" property to be present.
>
> The current logic used to craft an interrupt-map property in
> of_pci_prop_intr_map() is based on reading the #address-cells
> property in the interrupt-parent and, if != 0, read the interrupt
> parent "reg" property to determine the parent unit address to be
> used to create the parent unit interrupt specifier.
>
> First of all, it is not correct to read the "reg" property of
> the interrupt-parent with an #address-cells value taken from the
> interrupt-parent node, because the #address-cells value define the
> number of address cells required by child nodes.
>
> More importantly, for all modern interrupt controllers, the parent
> unit address is irrelevant in HW in relation to the
> device<->interrupt-controller connection and the kernel actually
> ignores the parent unit address value when hierarchically parsing
> the interrupt-map property (ie of_irq_parse_raw()).
>
> For the reasons above, remove the code parsing the interrupt
> parent "reg" property in of_pci_prop_intr_map() - it is not
> needed and as it is it is detrimental in that it prevents
> interrupt-map property generation on systems with an
> interrupt-controller that has no "reg" property in its
> interrupt-controller node - and leave the parent unit address
> always initialized to 0 since it is simply ignored by the kernel.
>
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Lizhi Hou <lizhi.hou@amd.com>
> Link: https://lore.kernel.org/lkml/aJms+YT8TnpzpCY8@lpieralisi/
> ---
>   drivers/pci/of_property.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 506fcd507113..09b7bc335ec5 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
> @@ -279,13 +279,20 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
>   			mapp++;
>   			*mapp = out_irq[i].np->phandle;
>   			mapp++;
> -			if (addr_sz[i]) {
> -				ret = of_property_read_u32_array(out_irq[i].np,
> -								 "reg", mapp,
> -								 addr_sz[i]);
> -				if (ret)
> -					goto failed;
> -			}
> +
> +			/*
> +			 * A device address does not affect the
> +			 * device<->interrupt-controller HW connection for all
> +			 * modern interrupt controllers; moreover, the kernel
> +			 * (ie of_irq_parse_raw()) ignores the values in the
> +			 * parent unit address cells while parsing the interrupt-map
> +			 * property because they are irrelevant for interrupts mapping
> +			 * in modern system.
> +			 *
> +			 * Leave the parent unit address initialized to 0 - just
> +			 * take into account the #address-cells size to build
> +			 * the property properly.
> +			 */
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>   			mapp += addr_sz[i];
>   			memcpy(mapp, out_irq[i].args,
>   			       out_irq[i].args_count * sizeof(u32));

Re: [PATCH] PCI: of: Update parent unit address generation in of_pci_prop_intr_map()

[Lorenzo Pieralisi]

On Mon, Aug 18, 2025 at 11:35:04AM +0200, Lorenzo Pieralisi wrote:

[...]

>  drivers/pci/of_property.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 506fcd507113..09b7bc335ec5 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
> @@ -279,13 +279,20 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
>  			mapp++;
>  			*mapp = out_irq[i].np->phandle;
>  			mapp++;
> -			if (addr_sz[i]) {
> -				ret = of_property_read_u32_array(out_irq[i].np,
> -								 "reg", mapp,
> -								 addr_sz[i]);
> -				if (ret)
> -					goto failed;
> -			}
> +
> +			/*
> +			 * A device address does not affect the
> +			 * device<->interrupt-controller HW connection for all
> +			 * modern interrupt controllers; moreover, the kernel
> +			 * (ie of_irq_parse_raw()) ignores the values in the
> +			 * parent unit address cells while parsing the interrupt-map
> +			 * property because they are irrelevant for interrupts mapping
> +			 * in modern system.

Rob,

if you apply directly the line above should be "in modern systems" please.

Thanks,
Lorenzo

> +			 *
> +			 * Leave the parent unit address initialized to 0 - just
> +			 * take into account the #address-cells size to build
> +			 * the property properly.
> +			 */
>  			mapp += addr_sz[i];
>  			memcpy(mapp, out_irq[i].args,
>  			       out_irq[i].args_count * sizeof(u32));
> -- 
> 2.48.0
> 

Re: [PATCH] PCI: of: Update parent unit address generation in of_pci_prop_intr_map()

[Rob Herring]

On Tue, Aug 19, 2025 at 8:19 AM Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
>
> On Mon, Aug 18, 2025 at 11:35:04AM +0200, Lorenzo Pieralisi wrote:
>
> [...]
>
> >  drivers/pci/of_property.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> > index 506fcd507113..09b7bc335ec5 100644
> > --- a/drivers/pci/of_property.c
> > +++ b/drivers/pci/of_property.c
> > @@ -279,13 +279,20 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
> >                       mapp++;
> >                       *mapp = out_irq[i].np->phandle;
> >                       mapp++;
> > -                     if (addr_sz[i]) {
> > -                             ret = of_property_read_u32_array(out_irq[i].np,
> > -                                                              "reg", mapp,
> > -                                                              addr_sz[i]);
> > -                             if (ret)
> > -                                     goto failed;
> > -                     }
> > +
> > +                     /*
> > +                      * A device address does not affect the
> > +                      * device<->interrupt-controller HW connection for all
> > +                      * modern interrupt controllers; moreover, the kernel
> > +                      * (ie of_irq_parse_raw()) ignores the values in the
> > +                      * parent unit address cells while parsing the interrupt-map
> > +                      * property because they are irrelevant for interrupts mapping
> > +                      * in modern system.
>
> Rob,
>
> if you apply directly the line above should be "in modern systems" please.

But this should go via PCI tree.

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH] PCI: of: Update parent unit address generation in of_pci_prop_intr_map()

[Bjorn Helgaas]

On Mon, Aug 18, 2025 at 11:35:04AM +0200, Lorenzo Pieralisi wrote:
> Some interrupt controllers require an #address-cells property in their
> bindings without requiring a "reg" property to be present.
> 
> The current logic used to craft an interrupt-map property in
> of_pci_prop_intr_map() is based on reading the #address-cells
> property in the interrupt-parent and, if != 0, read the interrupt
> parent "reg" property to determine the parent unit address to be
> used to create the parent unit interrupt specifier.
> 
> First of all, it is not correct to read the "reg" property of
> the interrupt-parent with an #address-cells value taken from the
> interrupt-parent node, because the #address-cells value define the
> number of address cells required by child nodes.
> 
> More importantly, for all modern interrupt controllers, the parent
> unit address is irrelevant in HW in relation to the
> device<->interrupt-controller connection and the kernel actually
> ignores the parent unit address value when hierarchically parsing
> the interrupt-map property (ie of_irq_parse_raw()).
> 
> For the reasons above, remove the code parsing the interrupt
> parent "reg" property in of_pci_prop_intr_map() - it is not
> needed and as it is it is detrimental in that it prevents
> interrupt-map property generation on systems with an
> interrupt-controller that has no "reg" property in its
> interrupt-controller node - and leave the parent unit address
> always initialized to 0 since it is simply ignored by the kernel.
> 
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Lizhi Hou <lizhi.hou@amd.com>
> Link: https://lore.kernel.org/lkml/aJms+YT8TnpzpCY8@lpieralisi/

Applied to pci/of for v6.18, thanks, Lorenzo!

> ---
>  drivers/pci/of_property.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 506fcd507113..09b7bc335ec5 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
> @@ -279,13 +279,20 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
>  			mapp++;
>  			*mapp = out_irq[i].np->phandle;
>  			mapp++;
> -			if (addr_sz[i]) {
> -				ret = of_property_read_u32_array(out_irq[i].np,
> -								 "reg", mapp,
> -								 addr_sz[i]);
> -				if (ret)
> -					goto failed;
> -			}
> +
> +			/*
> +			 * A device address does not affect the
> +			 * device<->interrupt-controller HW connection for all
> +			 * modern interrupt controllers; moreover, the kernel
> +			 * (ie of_irq_parse_raw()) ignores the values in the
> +			 * parent unit address cells while parsing the interrupt-map
> +			 * property because they are irrelevant for interrupts mapping
> +			 * in modern system.
> +			 *
> +			 * Leave the parent unit address initialized to 0 - just
> +			 * take into account the #address-cells size to build
> +			 * the property properly.
> +			 */
>  			mapp += addr_sz[i];
>  			memcpy(mapp, out_irq[i].args,
>  			       out_irq[i].args_count * sizeof(u32));
> -- 
> 2.48.0
>