A root complex usually consist of a host bridge and multiple P2P bridges,
and someone may express that in the form of a root node with many subnodes
and list all four interrupts for each slot (child node) in the root node
like this:
pcie-controller {
...
interrupt-map-mask = <0xf800 0 0 7>;
interrupt-map = <0x0000 0 0 {INTx} &{interrupt parent} ...>
0x0800 0 0 {INTx} &{interrupt parent} ...>;
pcie@0,0 {
reg = <0x0000 0 0 0 0>;
...
};
pcie@1,0 {
reg = <0x0800 0 0 0 0>;
...
};
};
As shown above, we'd like to propagate IRQs from a root port to the devices
in the hierarchy below it in this way. However, it seems that the current
parser couldn't handle such cases and will get something unexpected below:
pcieport 0000:00:01.0: assign IRQ: got 213
igb 0000:01:00.0: assign IRQ: got 212
There is a device which is connected to 2nd slot, but the port doesn't share
the same IRQ with its downstream devices. The problem here is that, if the
loop found a P2P bridge, it wouldn't check whether the reg property exists
in ppnode or not but just pass the subordinate devfn to of_irq_parse_raw(),
thus the subsequent flow couldn't correctly resolve them.
Fix this by adding a check to fallback to standard device tree parsing.
Signed-off-by: Ryder Lee <[email protected]>
---
Please refer to the previous discussion thread: https://patchwork.ozlabs.org/patch/829108/
---
drivers/of/of_pci_irq.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index 3a05568..e445866 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -86,8 +86,18 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
out_irq->np = ppnode;
out_irq->args_count = 1;
out_irq->args[0] = pin;
- laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
- laddr[1] = laddr[2] = cpu_to_be32(0);
+
+ if (!dn && ppnode) {
+ const __be32 *addr;
+
+ addr = of_get_property(ppnode, "reg", NULL);
+ if (addr)
+ memcpy(laddr, addr, 3);
+ } else {
+ laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
+ laddr[1] = laddr[2] = cpu_to_be32(0);
+ }
+
rc = of_irq_parse_raw(laddr, out_irq);
if (rc)
goto err;
--
1.9.1
Move the interrupt-map properties from the child nodes to the root node
and update each entry accordingly.
Signed-off-by: Ryder Lee <[email protected]>
---
.../devicetree/bindings/pci/mediatek-pcie.txt | 28 ++++++++++++----------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek-pcie.txt
index 3a6ce55..b15ea2d 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie.txt
@@ -89,10 +89,21 @@ Examples for MT7623:
#address-cells = <3>;
#size-cells = <2>;
#interrupt-cells = <1>;
- interrupt-map-mask = <0xf800 0 0 0>;
- interrupt-map = <0x0000 0 0 0 &sysirq GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>,
- <0x0800 0 0 0 &sysirq GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>,
- <0x1000 0 0 0 &sysirq GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-map-mask = <0xf800 0 0 7>;
+ interrupt-map = <0x0000 0 0 1 &sysirq GIC_SPI 193 IRQ_TYPE_LEVEL_LOW
+ 0x0000 0 0 2 &sysirq GIC_SPI 193 IRQ_TYPE_LEVEL_LOW
+ 0x0000 0 0 3 &sysirq GIC_SPI 193 IRQ_TYPE_LEVEL_LOW
+ 0x0000 0 0 4 &sysirq GIC_SPI 193 IRQ_TYPE_LEVEL_LOW
+
+ 0x0800 0 0 1 &sysirq GIC_SPI 194 IRQ_TYPE_LEVEL_LOW
+ 0x0800 0 0 2 &sysirq GIC_SPI 194 IRQ_TYPE_LEVEL_LOW
+ 0x0800 0 0 3 &sysirq GIC_SPI 194 IRQ_TYPE_LEVEL_LOW
+ 0x0800 0 0 4 &sysirq GIC_SPI 194 IRQ_TYPE_LEVEL_LOW
+
+ 0x1000 0 0 1 &sysirq GIC_SPI 195 IRQ_TYPE_LEVEL_LOW
+ 0x1000 0 0 2 &sysirq GIC_SPI 195 IRQ_TYPE_LEVEL_LOW
+ 0x1000 0 0 3 &sysirq GIC_SPI 195 IRQ_TYPE_LEVEL_LOW
+ 0x1000 0 0 4 &sysirq GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>;
clocks = <&topckgen CLK_TOP_ETHIF_SEL>,
<&hifsys CLK_HIFSYS_PCIE0>,
<&hifsys CLK_HIFSYS_PCIE1>,
@@ -115,9 +126,6 @@ Examples for MT7623:
reg = <0x0000 0 0 0 0>;
#address-cells = <3>;
#size-cells = <2>;
- #interrupt-cells = <1>;
- interrupt-map-mask = <0 0 0 0>;
- interrupt-map = <0 0 0 0 &sysirq GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>;
ranges;
num-lanes = <1>;
};
@@ -127,9 +135,6 @@ Examples for MT7623:
reg = <0x0800 0 0 0 0>;
#address-cells = <3>;
#size-cells = <2>;
- #interrupt-cells = <1>;
- interrupt-map-mask = <0 0 0 0>;
- interrupt-map = <0 0 0 0 &sysirq GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>;
ranges;
num-lanes = <1>;
};
@@ -139,9 +144,6 @@ Examples for MT7623:
reg = <0x1000 0 0 0 0>;
#address-cells = <3>;
#size-cells = <2>;
- #interrupt-cells = <1>;
- interrupt-map-mask = <0 0 0 0>;
- interrupt-map = <0 0 0 0 &sysirq GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>;
ranges;
num-lanes = <1>;
};
--
1.9.1
On Wed, Jan 31, 2018 at 1:41 AM, Ryder Lee <[email protected]> wrote:
> A root complex usually consist of a host bridge and multiple P2P bridges,
> and someone may express that in the form of a root node with many subnodes
> and list all four interrupts for each slot (child node) in the root node
> like this:
>
> pcie-controller {
> ...
> interrupt-map-mask = <0xf800 0 0 7>;
> interrupt-map = <0x0000 0 0 {INTx} &{interrupt parent} ...>
> 0x0800 0 0 {INTx} &{interrupt parent} ...>;
>
> pcie@0,0 {
> reg = <0x0000 0 0 0 0>;
> ...
> };
>
> pcie@1,0 {
> reg = <0x0800 0 0 0 0>;
> ...
> };
> };
>
> As shown above, we'd like to propagate IRQs from a root port to the devices
> in the hierarchy below it in this way. However, it seems that the current
> parser couldn't handle such cases and will get something unexpected below:
>
> pcieport 0000:00:01.0: assign IRQ: got 213
> igb 0000:01:00.0: assign IRQ: got 212
>
> There is a device which is connected to 2nd slot, but the port doesn't share
> the same IRQ with its downstream devices. The problem here is that, if the
> loop found a P2P bridge, it wouldn't check whether the reg property exists
> in ppnode or not but just pass the subordinate devfn to of_irq_parse_raw(),
> thus the subsequent flow couldn't correctly resolve them.
>
> Fix this by adding a check to fallback to standard device tree parsing.
>
> Signed-off-by: Ryder Lee <[email protected]>
> ---
> Please refer to the previous discussion thread: https://patchwork.ozlabs.org/patch/829108/
> ---
> drivers/of/of_pci_irq.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index 3a05568..e445866 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -86,8 +86,18 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
> out_irq->np = ppnode;
> out_irq->args_count = 1;
> out_irq->args[0] = pin;
> - laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
> - laddr[1] = laddr[2] = cpu_to_be32(0);
> +
> + if (!dn && ppnode) {
I would think whether you have a child device in DT or not is
irrelevant. If it's the bridge address you need to look at for
resolving interrupts, that would be true regardless.
> + const __be32 *addr;
> +
> + addr = of_get_property(ppnode, "reg", NULL);
> + if (addr)
> + memcpy(laddr, addr, 3);
Can't you just adjust pdev to be ppdev in this case and then use the
existing code to set laddr?
Please copy the powerpc list on this. I worry that touching this
function will break something.
BTW, this code is moving to drivers/pci/ in 4.16.
> + } else {
> + laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
> + laddr[1] = laddr[2] = cpu_to_be32(0);
> + }
> +
> rc = of_irq_parse_raw(laddr, out_irq);
> if (rc)
> goto err;
> --
> 1.9.1
>
On Wed, 2018-01-31 at 10:02 -0600, Rob Herring wrote:
> On Wed, Jan 31, 2018 at 1:41 AM, Ryder Lee <[email protected]> wrote:
> > A root complex usually consist of a host bridge and multiple P2P bridges,
> > and someone may express that in the form of a root node with many subnodes
> > and list all four interrupts for each slot (child node) in the root node
> > like this:
> >
> > pcie-controller {
> > ...
> > interrupt-map-mask = <0xf800 0 0 7>;
> > interrupt-map = <0x0000 0 0 {INTx} &{interrupt parent} ...>
> > 0x0800 0 0 {INTx} &{interrupt parent} ...>;
> >
> > pcie@0,0 {
> > reg = <0x0000 0 0 0 0>;
> > ...
> > };
> >
> > pcie@1,0 {
> > reg = <0x0800 0 0 0 0>;
> > ...
> > };
> > };
> >
> > As shown above, we'd like to propagate IRQs from a root port to the devices
> > in the hierarchy below it in this way. However, it seems that the current
> > parser couldn't handle such cases and will get something unexpected below:
> >
> > pcieport 0000:00:01.0: assign IRQ: got 213
> > igb 0000:01:00.0: assign IRQ: got 212
> >
> > There is a device which is connected to 2nd slot, but the port doesn't share
> > the same IRQ with its downstream devices. The problem here is that, if the
> > loop found a P2P bridge, it wouldn't check whether the reg property exists
> > in ppnode or not but just pass the subordinate devfn to of_irq_parse_raw(),
> > thus the subsequent flow couldn't correctly resolve them.
> >
> > Fix this by adding a check to fallback to standard device tree parsing.
> >
> > Signed-off-by: Ryder Lee <[email protected]>
> > ---
> > Please refer to the previous discussion thread: https://patchwork.ozlabs.org/patch/829108/
> > ---
> > drivers/of/of_pci_irq.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> > index 3a05568..e445866 100644
> > --- a/drivers/of/of_pci_irq.c
> > +++ b/drivers/of/of_pci_irq.c
> > @@ -86,8 +86,18 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
> > out_irq->np = ppnode;
> > out_irq->args_count = 1;
> > out_irq->args[0] = pin;
> > - laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
> > - laddr[1] = laddr[2] = cpu_to_be32(0);
> > +
> > + if (!dn && ppnode) {
>
> I would think whether you have a child device in DT or not is
> irrelevant. If it's the bridge address you need to look at for
> resolving interrupts, that would be true regardless.
>
> > + const __be32 *addr;
> > +
> > + addr = of_get_property(ppnode, "reg", NULL);
> > + if (addr)
> > + memcpy(laddr, addr, 3);
>
> Can't you just adjust pdev to be ppdev in this case and then use the
> existing code to set laddr?
Okay, I will try it out and and see if the code gets better or worse.
> Please copy the powerpc list on this. I worry that touching this
> function will break something.
> BTW, this code is moving to drivers/pci/ in 4.16.
Sure. I will loop more people in next version.
Thanks
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Wed, Jan 31, 2018 at 03:41:50PM +0800, Ryder Lee wrote:
> Move the interrupt-map properties from the child nodes to the root node
> and update each entry accordingly.
>
> Signed-off-by: Ryder Lee <[email protected]>
> ---
> .../devicetree/bindings/pci/mediatek-pcie.txt | 28 ++++++++++++----------
> 1 file changed, 15 insertions(+), 13 deletions(-)
Reviewed-by: Rob Herring <[email protected]>
On Fri, 2018-02-02 at 17:32 +0800, Ryder Lee wrote:
> On Wed, 2018-01-31 at 10:02 -0600, Rob Herring wrote:
> > On Wed, Jan 31, 2018 at 1:41 AM, Ryder Lee <[email protected]> wrote:
> > > A root complex usually consist of a host bridge and multiple P2P bridges,
> > > and someone may express that in the form of a root node with many subnodes
> > > and list all four interrupts for each slot (child node) in the root node
> > > like this:
> > >
> > > pcie-controller {
> > > ...
> > > interrupt-map-mask = <0xf800 0 0 7>;
> > > interrupt-map = <0x0000 0 0 {INTx} &{interrupt parent} ...>
> > > 0x0800 0 0 {INTx} &{interrupt parent} ...>;
> > >
> > > pcie@0,0 {
> > > reg = <0x0000 0 0 0 0>;
> > > ...
> > > };
> > >
> > > pcie@1,0 {
> > > reg = <0x0800 0 0 0 0>;
> > > ...
> > > };
> > > };
> > >
> > > As shown above, we'd like to propagate IRQs from a root port to the devices
> > > in the hierarchy below it in this way. However, it seems that the current
> > > parser couldn't handle such cases and will get something unexpected below:
> > >
> > > pcieport 0000:00:01.0: assign IRQ: got 213
> > > igb 0000:01:00.0: assign IRQ: got 212
> > >
> > > There is a device which is connected to 2nd slot, but the port doesn't share
> > > the same IRQ with its downstream devices. The problem here is that, if the
> > > loop found a P2P bridge, it wouldn't check whether the reg property exists
> > > in ppnode or not but just pass the subordinate devfn to of_irq_parse_raw(),
> > > thus the subsequent flow couldn't correctly resolve them.
I don't really understand the problem explanation here. Something
doesn't look right as you shouldn't have to change that function, but I
just don't get what you a
Cheers,
Ben.
On Tue, 2018-02-06 at 08:36 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2018-02-02 at 17:32 +0800, Ryder Lee wrote:
> > On Wed, 2018-01-31 at 10:02 -0600, Rob Herring wrote:
> > > On Wed, Jan 31, 2018 at 1:41 AM, Ryder Lee <[email protected]> wrote:
> > > > A root complex usually consist of a host bridge and multiple P2P bridges,
> > > > and someone may express that in the form of a root node with many subnodes
> > > > and list all four interrupts for each slot (child node) in the root node
> > > > like this:
> > > >
> > > > pcie-controller {
> > > > ...
> > > > interrupt-map-mask = <0xf800 0 0 7>;
> > > > interrupt-map = <0x0000 0 0 {INTx} &{interrupt parent} ...>
> > > > 0x0800 0 0 {INTx} &{interrupt parent} ...>;
> > > >
> > > > pcie@0,0 {
> > > > reg = <0x0000 0 0 0 0>;
> > > > ...
> > > > };
> > > >
> > > > pcie@1,0 {
> > > > reg = <0x0800 0 0 0 0>;
> > > > ...
> > > > };
> > > > };
> > > >
> > > > As shown above, we'd like to propagate IRQs from a root port to the devices
> > > > in the hierarchy below it in this way. However, it seems that the current
> > > > parser couldn't handle such cases and will get something unexpected below:
> > > >
> > > > pcieport 0000:00:01.0: assign IRQ: got 213
> > > > igb 0000:01:00.0: assign IRQ: got 212
> > > >
> > > > There is a device which is connected to 2nd slot, but the port doesn't share
> > > > the same IRQ with its downstream devices. The problem here is that, if the
> > > > loop found a P2P bridge, it wouldn't check whether the reg property exists
> > > > in ppnode or not but just pass the subordinate devfn to of_irq_parse_raw(),
> > > > thus the subsequent flow couldn't correctly resolve them.
>
> I don't really understand the problem explanation here. Something
> doesn't look right as you shouldn't have to change that function, but I
> just don't get what you a
>
> Cheers,
> Ben.
>
I think the code should look at the bridge address <0x0800 ...> we list
in bindings for resolving interrupts in this case, but it seems like it
use the 'pdev->defvn << 8' which is not really we want and will lead to
mismatch.
interrupt-map-mask = <0xf800 0 0 7>;
interrupt-map = <0x0000 0 0 1 ...>,
<0x0000 0 0 2 ...>,
<0x0000 0 0 3 ...>,
<0x0000 0 0 4 ...>,
0x0800 0 0 1 ...>,
0x0800 0 0 2 ...>,
0x0800 0 0 3 ...>,
0x0800 0 0 4 ...>;
...
pcie@1,0 {
reg = <0x0800 0 0 0 0>;
...
};
Or, alternatively, we could add a interrupt-map property in both child
and root node to solve this. The below example is my original version as
I don't want to change that function either.
interrupt-map-mask = <0xf800 0 0 0>;
interrupt-map = <0x0000 0 0 0 ...>,
0x0800 0 0 0 ...>;
...
pcie@1,0 {
reg = <0x0800 0 0 0 0>;
#interrupt-cells = <1>;
interrupt-map-mask = <0 0 0 0>;
interrupt-map = <0 0 0 0 ...>;
...
};
However, I can't find any other similar case in documentation.
Thanks.
On Tue, 2018-02-06 at 10:38 +0800, Ryder Lee wrote:
>
> I think the code should look at the bridge address <0x0800 ...> we list
> in bindings for resolving interrupts in this case, but it seems like it
> use the 'pdev->defvn << 8' which is not really we want and will lead to
> mismatch.
>
> interrupt-map-mask = <0xf800 0 0 7>;
> interrupt-map = <0x0000 0 0 1 ...>,
> <0x0000 0 0 2 ...>,
> <0x0000 0 0 3 ...>,
> <0x0000 0 0 4 ...>,
>
> 0x0800 0 0 1 ...>,
> 0x0800 0 0 2 ...>,
> 0x0800 0 0 3 ...>,
> 0x0800 0 0 4 ...>;
> ...
> pcie@1,0 {
> reg = <0x0800 0 0 0 0>;
> ...
> };
>
>
> Or, alternatively, we could add a interrupt-map property in both child
> and root node to solve this. The below example is my original version as
> I don't want to change that function either.
The code looks at devfn because it's meant to work for PCI including
when the devices dont have a device node in the DT.
What I'm trying to figure out is what is it that your parent and
children are representing here. Which is/are the root complex ?
What is the actual topology as visible on the PCIe bus (is lspci output
basically) and how does that map to your representation ?
> interrupt-map-mask = <0xf800 0 0 0>;
> interrupt-map = <0x0000 0 0 0 ...>,
> 0x0800 0 0 0 ...>;
> ...
> pcie@1,0 {
> reg = <0x0800 0 0 0 0>;
> #interrupt-cells = <1>;
> interrupt-map-mask = <0 0 0 0>;
> interrupt-map = <0 0 0 0 ...>;
> ...
> };
>
> However, I can't find any other similar case in documentation.
>
> Thanks.
On Tue, 2018-02-06 at 15:05 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2018-02-06 at 10:38 +0800, Ryder Lee wrote:
> >
> > I think the code should look at the bridge address <0x0800 ...> we list
> > in bindings for resolving interrupts in this case, but it seems like it
> > use the 'pdev->defvn << 8' which is not really we want and will lead to
> > mismatch.
> >
> > interrupt-map-mask = <0xf800 0 0 7>;
> > interrupt-map = <0x0000 0 0 1 ...>,
> > <0x0000 0 0 2 ...>,
> > <0x0000 0 0 3 ...>,
> > <0x0000 0 0 4 ...>,
> >
> > 0x0800 0 0 1 ...>,
> > 0x0800 0 0 2 ...>,
> > 0x0800 0 0 3 ...>,
> > 0x0800 0 0 4 ...>;
> > ...
> > pcie@1,0 {
> > reg = <0x0800 0 0 0 0>;
> > ...
> > };
> >
> >
> > Or, alternatively, we could add a interrupt-map property in both child
> > and root node to solve this. The below example is my original version as
> > I don't want to change that function either.
>
> The code looks at devfn because it's meant to work for PCI including
> when the devices dont have a device node in the DT.
>
> What I'm trying to figure out is what is it that your parent and
> children are representing here. Which is/are the root complex ?
This is a single root complex with 2 root port (children in DT).
> What is the actual topology as visible on the PCIe bus (is lspci output
> basically) and how does that map to your representation ?
# lspci
00:00.0 Class 0604: 14c3:0801 //1st slot - pcie@0,0
00:01.0 Class 0604: 14c3:0801 //2nd slot - pcie@1,0
01:00.0 Class 0280: 14c3:7603 //A device which is connected to 1st slot
02:00.0 Class 0200: 8086:1521 //A 4 func device which is connected to
2nd slot
02:00.1 Class 0200: 8086:1521
02:00.2 Class 0200: 8086:1521
02:00.3 Class 0200: 8086:1521
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Tue, 2018-02-06 at 12:31 +0800, Ryder Lee wrote:
> On Tue, 2018-02-06 at 15:05 +1100, Benjamin Herrenschmidt wrote:
> > On Tue, 2018-02-06 at 10:38 +0800, Ryder Lee wrote:
> > >
> > > I think the code should look at the bridge address <0x0800 ...> we list
> > > in bindings for resolving interrupts in this case, but it seems like it
> > > use the 'pdev->defvn << 8' which is not really we want and will lead to
> > > mismatch.
> > >
> > > interrupt-map-mask = <0xf800 0 0 7>;
> > > interrupt-map = <0x0000 0 0 1 ...>,
> > > <0x0000 0 0 2 ...>,
> > > <0x0000 0 0 3 ...>,
> > > <0x0000 0 0 4 ...>,
> > >
> > > 0x0800 0 0 1 ...>,
> > > 0x0800 0 0 2 ...>,
> > > 0x0800 0 0 3 ...>,
> > > 0x0800 0 0 4 ...>;
> > > ...
> > > pcie@1,0 {
> > > reg = <0x0800 0 0 0 0>;
> > > ...
> > > };
> > >
> > >
> > > Or, alternatively, we could add a interrupt-map property in both child
> > > and root node to solve this. The below example is my original version as
> > > I don't want to change that function either.
> >
> > The code looks at devfn because it's meant to work for PCI including
> > when the devices dont have a device node in the DT.
> >
> > What I'm trying to figure out is what is it that your parent and
> > children are representing here. Which is/are the root complex ?
>
> This is a single root complex with 2 root port (children in DT).
>
> > What is the actual topology as visible on the PCIe bus (is lspci output
> > basically) and how does that map to your representation ?
>
> # lspci
> 00:00.0 Class 0604: 14c3:0801 //1st slot - pcie@0,0
> 00:01.0 Class 0604: 14c3:0801 //2nd slot - pcie@1,0
>
> 01:00.0 Class 0280: 14c3:7603 //A device which is connected to 1st slot
> 02:00.0 Class 0200: 8086:1521 //A 4 func device which is connected to
> 2nd slot
> 02:00.1 Class 0200: 8086:1521
> 02:00.2 Class 0200: 8086:1521
> 02:00.3 Class 0200: 8086:1521
Ok so that's a rather standard setup. The "devfn << 8" of your root
ports should be the exact same thing as their first reg property entry,
I'm not sure why you have a mismatch here.
However, that map only represents the INTA..D lines going to the root
ports, not how these get mapped to children of the root ports.
of_irq_parse_pci() will implement standard swizzling if you don't have
nodes for your devices at all. If you do, however, the code assumes
you have a correct and complete interrupt tree in the device-tree.
That means that you need in each "p2p bridge", including your root
ports, an interrupt-map that will map the children INTA...D of that
bridge to the parent INTA...D of that bridge.
Alternatively, you can make the maps in the root ports point directly
to the parent PIC. If you chose to do that, then the interrupt-map in
your root complex becomes only useful to represent the root ports own
interrutps (hotplug, AER,...) and could be replaced by just having
interrupt-parent & interrupts in those root port nodes.
Cheers,
Ben.
On Tue, 2018-02-06 at 15:50 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2018-02-06 at 12:31 +0800, Ryder Lee wrote:
> > On Tue, 2018-02-06 at 15:05 +1100, Benjamin Herrenschmidt wrote:
> > > On Tue, 2018-02-06 at 10:38 +0800, Ryder Lee wrote:
> > > >
> > > > I think the code should look at the bridge address <0x0800 ...> we list
> > > > in bindings for resolving interrupts in this case, but it seems like it
> > > > use the 'pdev->defvn << 8' which is not really we want and will lead to
> > > > mismatch.
> > > >
> > > > interrupt-map-mask = <0xf800 0 0 7>;
> > > > interrupt-map = <0x0000 0 0 1 ...>,
> > > > <0x0000 0 0 2 ...>,
> > > > <0x0000 0 0 3 ...>,
> > > > <0x0000 0 0 4 ...>,
> > > >
> > > > 0x0800 0 0 1 ...>,
> > > > 0x0800 0 0 2 ...>,
> > > > 0x0800 0 0 3 ...>,
> > > > 0x0800 0 0 4 ...>;
> > > > ...
> > > > pcie@1,0 {
> > > > reg = <0x0800 0 0 0 0>;
> > > > ...
> > > > };
> > > >
> > > >
> > > > Or, alternatively, we could add a interrupt-map property in both child
> > > > and root node to solve this. The below example is my original version as
> > > > I don't want to change that function either.
> > >
> > > The code looks at devfn because it's meant to work for PCI including
> > > when the devices dont have a device node in the DT.
> > >
> > > What I'm trying to figure out is what is it that your parent and
> > > children are representing here. Which is/are the root complex ?
> >
> > This is a single root complex with 2 root port (children in DT).
> >
> > > What is the actual topology as visible on the PCIe bus (is lspci output
> > > basically) and how does that map to your representation ?
> >
> > # lspci
> > 00:00.0 Class 0604: 14c3:0801 //1st slot - pcie@0,0
> > 00:01.0 Class 0604: 14c3:0801 //2nd slot - pcie@1,0
> >
> > 01:00.0 Class 0280: 14c3:7603 //A device which is connected to 1st slot
> > 02:00.0 Class 0200: 8086:1521 //A 4 func device which is connected to
> > 2nd slot
> > 02:00.1 Class 0200: 8086:1521
> > 02:00.2 Class 0200: 8086:1521
> > 02:00.3 Class 0200: 8086:1521
>
> Ok so that's a rather standard setup. The "devfn << 8" of your root
> ports should be the exact same thing as their first reg property entry,
> I'm not sure why you have a mismatch here.
I've added some log after 'for loop':
pr_err("busn=0x%x, devfn=0x%x, reg=0x%x\n", pdev->bus->number, pdev->devfn, of_pci_get_devfn(ppnode));
and got these:
[ 5.651836] busn=0x0, devfn=0x0, reg=0x0
[ 5.651936] pcieport 0000:00:00.0: assign IRQ: got 213
...
[ 5.652398] busn=0x0, devfn=0x8, reg=0x0
[ 5.652487] pcieport 0000:00:01.0: assign IRQ: got 214
...
[ 5.653025] busn=0x2, devfn=0x0, reg=0x8
[ 5.653058] igb 0000:02:00.0: assign IRQ: got 213
[ 5.724582] busn=0x2, devfn=0x1, reg=0x8
[ 5.724634] igb 0000:02:00.1: assign IRQ: got 213
> However, that map only represents the INTA..D lines going to the root
> ports, not how these get mapped to children of the root ports.
>
> of_irq_parse_pci() will implement standard swizzling if you don't have
> nodes for your devices at all. If you do, however, the code assumes
> you have a correct and complete interrupt tree in the device-tree.
>
> That means that you need in each "p2p bridge", including your root
> ports, an interrupt-map that will map the children INTA...D of that
> bridge to the parent INTA...D of that bridge.
>
> Alternatively, you can make the maps in the root ports point directly
> to the parent PIC. If you chose to do that, then the interrupt-map in
> your root complex becomes only useful to represent the root ports own
> interrutps (hotplug, AER,...) and could be replaced by just having
> interrupt-parent & interrupts in those root port nodes.
>
Thanks for explanation.
So I guess the better way to achieve my aim - one IRQ per slot that is
connected to all INTx and get propagated through the bridges (and for
those root ports own interrupts (PME ..)} is to add interrupt-map
properties in both parent and root port nodes.
Something like this: https://patchwork.kernel.org/patch/9970923/ ,right?
Ryder
On Tue, 2018-02-06 at 13:42 +0800, Ryder Lee wrote:
> Thanks for explanation.
>
> So I guess the better way to achieve my aim - one IRQ per slot that is
> connected to all INTx and get propagated through the bridges (and for
> those root ports own interrupts (PME ..)} is to add interrupt-map
> properties in both parent and root port nodes.
>
> Something like this: https://patchwork.kernel.org/patch/9970923// ,right?
Yup.
Cheers,
Ben.
Hi, Arnd
On Wed, 2018-02-07 at 09:31 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2018-02-06 at 13:42 +0800, Ryder Lee wrote:
> > Thanks for explanation.
> >
> > So I guess the better way to achieve my aim - one IRQ per slot that is
> > connected to all INTx and get propagated through the bridges (and for
> > those root ports own interrupts (PME ..)} is to add interrupt-map
> > properties in both parent and root port nodes.
> >
> > Something like this: https://patchwork.kernel.org/patch/9970923// ,right?
>
> Yup.
>
> Cheers,
> Ben.
>
Do you have any thoughts on the original approach? If you are okay with
it, I will resend the DT patch.
Thanks.
Hi Bjorn,
On Mon, 2018-02-05 at 00:08 -0600, Rob Herring wrote:
> On Wed, Jan 31, 2018 at 03:41:50PM +0800, Ryder Lee wrote:
> > Move the interrupt-map properties from the child nodes to the root node
> > and update each entry accordingly.
> >
> > Signed-off-by: Ryder Lee <[email protected]>
> > ---
> > .../devicetree/bindings/pci/mediatek-pcie.txt | 28 ++++++++++++----------
> > 1 file changed, 15 insertions(+), 13 deletions(-)
>
> Reviewed-by: Rob Herring <[email protected]>
Please ignore this patch as we think the original version is better than
this. Sorry for the inconvenience.
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Wed, Jan 31, 2018 at 03:41:49PM +0800, Ryder Lee wrote:
> A root complex usually consist of a host bridge and multiple P2P bridges,
> and someone may express that in the form of a root node with many subnodes
> and list all four interrupts for each slot (child node) in the root node
> like this:
>
> pcie-controller {
> ...
> interrupt-map-mask = <0xf800 0 0 7>;
> interrupt-map = <0x0000 0 0 {INTx} &{interrupt parent} ...>
> 0x0800 0 0 {INTx} &{interrupt parent} ...>;
>
> pcie@0,0 {
> reg = <0x0000 0 0 0 0>;
> ...
> };
>
> pcie@1,0 {
> reg = <0x0800 0 0 0 0>;
> ...
> };
> };
>
> As shown above, we'd like to propagate IRQs from a root port to the devices
> in the hierarchy below it in this way. However, it seems that the current
> parser couldn't handle such cases and will get something unexpected below:
>
> pcieport 0000:00:01.0: assign IRQ: got 213
> igb 0000:01:00.0: assign IRQ: got 212
>
> There is a device which is connected to 2nd slot, but the port doesn't share
> the same IRQ with its downstream devices. The problem here is that, if the
> loop found a P2P bridge, it wouldn't check whether the reg property exists
> in ppnode or not but just pass the subordinate devfn to of_irq_parse_raw(),
> thus the subsequent flow couldn't correctly resolve them.
>
> Fix this by adding a check to fallback to standard device tree parsing.
>
> Signed-off-by: Ryder Lee <[email protected]>
> ---
> Please refer to the previous discussion thread: https://patchwork.ozlabs.org/patch/829108/
> ---
> drivers/of/of_pci_irq.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
Hi Ryder,
from the thread discussion I gather I can drop this series from the PCI
queue and you will update the DT as agreed with Ben, that looks like
the most reasonable solution to the problem you are facing, please
let me know if there is anything I am missing.
Thanks,
Lorenzo
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index 3a05568..e445866 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -86,8 +86,18 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
> out_irq->np = ppnode;
> out_irq->args_count = 1;
> out_irq->args[0] = pin;
> - laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
> - laddr[1] = laddr[2] = cpu_to_be32(0);
> +
> + if (!dn && ppnode) {
> + const __be32 *addr;
> +
> + addr = of_get_property(ppnode, "reg", NULL);
> + if (addr)
> + memcpy(laddr, addr, 3);
> + } else {
> + laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
> + laddr[1] = laddr[2] = cpu_to_be32(0);
> + }
> +
> rc = of_irq_parse_raw(laddr, out_irq);
> if (rc)
> goto err;
> --
> 1.9.1
>
On Thu, 2018-03-15 at 17:43 +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 31, 2018 at 03:41:49PM +0800, Ryder Lee wrote:
> > A root complex usually consist of a host bridge and multiple P2P bridges,
> > and someone may express that in the form of a root node with many subnodes
> > and list all four interrupts for each slot (child node) in the root node
> > like this:
> >
> > pcie-controller {
> > ...
> > interrupt-map-mask = <0xf800 0 0 7>;
> > interrupt-map = <0x0000 0 0 {INTx} &{interrupt parent} ...>
> > 0x0800 0 0 {INTx} &{interrupt parent} ...>;
> >
> > pcie@0,0 {
> > reg = <0x0000 0 0 0 0>;
> > ...
> > };
> >
> > pcie@1,0 {
> > reg = <0x0800 0 0 0 0>;
> > ...
> > };
> > };
> >
> > As shown above, we'd like to propagate IRQs from a root port to the devices
> > in the hierarchy below it in this way. However, it seems that the current
> > parser couldn't handle such cases and will get something unexpected below:
> >
> > pcieport 0000:00:01.0: assign IRQ: got 213
> > igb 0000:01:00.0: assign IRQ: got 212
> >
> > There is a device which is connected to 2nd slot, but the port doesn't share
> > the same IRQ with its downstream devices. The problem here is that, if the
> > loop found a P2P bridge, it wouldn't check whether the reg property exists
> > in ppnode or not but just pass the subordinate devfn to of_irq_parse_raw(),
> > thus the subsequent flow couldn't correctly resolve them.
> >
> > Fix this by adding a check to fallback to standard device tree parsing.
> >
> > Signed-off-by: Ryder Lee <[email protected]>
> > ---
> > Please refer to the previous discussion thread: https://patchwork.ozlabs.org/patch/829108/
> > ---
> > drivers/of/of_pci_irq.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
>
> Hi Ryder,
>
> from the thread discussion I gather I can drop this series from the PCI
> queue and you will update the DT as agreed with Ben, that looks like
> the most reasonable solution to the problem you are facing, please
> let me know if there is anything I am missing.
>
> Thanks,
> Lorenzo
>
Yes, please drop the series.
Thanks