[PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
merged in the 5.5 time frame, PCIe on the venerable XGene platform has
been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
IB window setup") fixed XGene-2, but left the rest of the zoo
unusable.

It is understood that this systems come with "creative" DTs that don't
match the expectations of modern kernels. However, there is little to
be gained by forcing these changes on users -- the firmware is not
upgradable, and the current owner of the IP will deny that these
machines have ever existed.

Given that, revert both changes and let people enjoy their XGene boxes
once again.

* From v1 [1]:
  - Also revert c7a75d07827a ("PCI: xgene: Fix IB window setup")

[1] https://lore.kernel.org/r/20220314144429.1947610-1-maz@kernel.org

Marc Zyngier (2):
  PCI: xgene: Revert "PCI: xgene: Use inbound resources for setup"
  PCI: xgene: Revert "PCI: xgene: Fix IB window setup"

 drivers/pci/controller/pci-xgene.c | 35 ++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
merged in the 5.5 time frame, PCIe on the venerable XGene platform has
been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
IB window setup") fixed XGene-2, but left the rest of the zoo
unusable.

It is understood that this systems come with "creative" DTs that don't
match the expectations of modern kernels. However, there is little to
be gained by forcing these changes on users -- the firmware is not
upgradable, and the current owner of the IP will deny that these
machines have ever existed.

Given that, revert both changes and let people enjoy their XGene boxes
once again.

* From v1 [1]:
  - Also revert c7a75d07827a ("PCI: xgene: Fix IB window setup")

[1] https://lore.kernel.org/r/20220314144429.1947610-1-maz@kernel.org

Marc Zyngier (2):
  PCI: xgene: Revert "PCI: xgene: Use inbound resources for setup"
  PCI: xgene: Revert "PCI: xgene: Fix IB window setup"

 drivers/pci/controller/pci-xgene.c | 35 ++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 12 deletions(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] PCI: xgene: Revert "PCI: xgene: Use inbound resources for setup"

[Marc Zyngier]

Commit 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup")
killed PCIe on my XGene-1 box (a Mustang board). The machine itself
is still alive, but half of its storage (over NVMe) is gone, and the
NVMe driver just times out.

Note that this machine boots with a device tree provided by the
UEFI firmware (2016 vintage), which could well be non conformant
with the spec, hence the breakage.

With the patch reverted, the box boots 5.17-rc8 with flying colors.

Cc: Rob Herring <robh@kernel.org>
Cc: Toan Le <toan@os.amperecomputing.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Krzysztof Wilczyński <kw@linux.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stéphane Graber <stgraber@ubuntu.com>
Cc: dann frazier <dann.frazier@canonical.com>
Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
Fixes: 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup")
Link: https://lore.kernel.org/all/Yf2wTLjmcRj+AbDv@xps13.dannf
---
 drivers/pci/controller/pci-xgene.c | 33 ++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index 0d5acbfc7143..aa41ceaf031f 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -479,28 +479,27 @@ static int xgene_pcie_select_ib_reg(u8 *ib_reg_mask, u64 size)
 }
 
 static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
-				    struct resource_entry *entry,
-				    u8 *ib_reg_mask)
+				    struct of_pci_range *range, u8 *ib_reg_mask)
 {
 	void __iomem *cfg_base = port->cfg_base;
 	struct device *dev = port->dev;
 	void __iomem *bar_addr;
 	u32 pim_reg;
-	u64 cpu_addr = entry->res->start;
-	u64 pci_addr = cpu_addr - entry->offset;
-	u64 size = resource_size(entry->res);
+	u64 cpu_addr = range->cpu_addr;
+	u64 pci_addr = range->pci_addr;
+	u64 size = range->size;
 	u64 mask = ~(size - 1) | EN_REG;
 	u32 flags = PCI_BASE_ADDRESS_MEM_TYPE_64;
 	u32 bar_low;
 	int region;
 
-	region = xgene_pcie_select_ib_reg(ib_reg_mask, size);
+	region = xgene_pcie_select_ib_reg(ib_reg_mask, range->size);
 	if (region < 0) {
 		dev_warn(dev, "invalid pcie dma-range config\n");
 		return;
 	}
 
-	if (entry->res->flags & IORESOURCE_PREFETCH)
+	if (range->flags & IORESOURCE_PREFETCH)
 		flags |= PCI_BASE_ADDRESS_MEM_PREFETCH;
 
 	bar_low = pcie_bar_low_val((u32)cpu_addr, flags);
@@ -531,13 +530,25 @@ static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
 
 static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie *port)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(port);
-	struct resource_entry *entry;
+	struct device_node *np = port->node;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	struct device *dev = port->dev;
 	u8 ib_reg_mask = 0;
 
-	resource_list_for_each_entry(entry, &bridge->dma_ranges)
-		xgene_pcie_setup_ib_reg(port, entry, &ib_reg_mask);
+	if (of_pci_dma_range_parser_init(&parser, np)) {
+		dev_err(dev, "missing dma-ranges property\n");
+		return -EINVAL;
+	}
+
+	/* Get the dma-ranges from DT */
+	for_each_of_pci_range(&parser, &range) {
+		u64 end = range.cpu_addr + range.size - 1;
 
+		dev_dbg(dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
+			range.flags, range.cpu_addr, end, range.pci_addr);
+		xgene_pcie_setup_ib_reg(port, &range, &ib_reg_mask);
+	}
 	return 0;
 }
 
-- 
2.34.1

[PATCH v2 1/2] PCI: xgene: Revert "PCI: xgene: Use inbound resources for setup"

[Marc Zyngier]

Commit 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup")
killed PCIe on my XGene-1 box (a Mustang board). The machine itself
is still alive, but half of its storage (over NVMe) is gone, and the
NVMe driver just times out.

Note that this machine boots with a device tree provided by the
UEFI firmware (2016 vintage), which could well be non conformant
with the spec, hence the breakage.

With the patch reverted, the box boots 5.17-rc8 with flying colors.

Cc: Rob Herring <robh@kernel.org>
Cc: Toan Le <toan@os.amperecomputing.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Krzysztof Wilczyński <kw@linux.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stéphane Graber <stgraber@ubuntu.com>
Cc: dann frazier <dann.frazier@canonical.com>
Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
Fixes: 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup")
Link: https://lore.kernel.org/all/Yf2wTLjmcRj+AbDv@xps13.dannf
---
 drivers/pci/controller/pci-xgene.c | 33 ++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index 0d5acbfc7143..aa41ceaf031f 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -479,28 +479,27 @@ static int xgene_pcie_select_ib_reg(u8 *ib_reg_mask, u64 size)
 }
 
 static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
-				    struct resource_entry *entry,
-				    u8 *ib_reg_mask)
+				    struct of_pci_range *range, u8 *ib_reg_mask)
 {
 	void __iomem *cfg_base = port->cfg_base;
 	struct device *dev = port->dev;
 	void __iomem *bar_addr;
 	u32 pim_reg;
-	u64 cpu_addr = entry->res->start;
-	u64 pci_addr = cpu_addr - entry->offset;
-	u64 size = resource_size(entry->res);
+	u64 cpu_addr = range->cpu_addr;
+	u64 pci_addr = range->pci_addr;
+	u64 size = range->size;
 	u64 mask = ~(size - 1) | EN_REG;
 	u32 flags = PCI_BASE_ADDRESS_MEM_TYPE_64;
 	u32 bar_low;
 	int region;
 
-	region = xgene_pcie_select_ib_reg(ib_reg_mask, size);
+	region = xgene_pcie_select_ib_reg(ib_reg_mask, range->size);
 	if (region < 0) {
 		dev_warn(dev, "invalid pcie dma-range config\n");
 		return;
 	}
 
-	if (entry->res->flags & IORESOURCE_PREFETCH)
+	if (range->flags & IORESOURCE_PREFETCH)
 		flags |= PCI_BASE_ADDRESS_MEM_PREFETCH;
 
 	bar_low = pcie_bar_low_val((u32)cpu_addr, flags);
@@ -531,13 +530,25 @@ static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
 
 static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie *port)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(port);
-	struct resource_entry *entry;
+	struct device_node *np = port->node;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	struct device *dev = port->dev;
 	u8 ib_reg_mask = 0;
 
-	resource_list_for_each_entry(entry, &bridge->dma_ranges)
-		xgene_pcie_setup_ib_reg(port, entry, &ib_reg_mask);
+	if (of_pci_dma_range_parser_init(&parser, np)) {
+		dev_err(dev, "missing dma-ranges property\n");
+		return -EINVAL;
+	}
+
+	/* Get the dma-ranges from DT */
+	for_each_of_pci_range(&parser, &range) {
+		u64 end = range.cpu_addr + range.size - 1;
 
+		dev_dbg(dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
+			range.flags, range.cpu_addr, end, range.pci_addr);
+		xgene_pcie_setup_ib_reg(port, &range, &ib_reg_mask);
+	}
 	return 0;
 }
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] PCI: xgene: Revert "PCI: xgene: Fix IB window setup"

[Marc Zyngier]

Commit c7a75d07827a ("PCI: xgene: Fix IB window setup") tried to
fix the damages that 6dce5aa59e0b ("PCI: xgene: Use inbound resources
for setup") caused, but actually didn't improve anything for some
plarforms (at least Mustang and m400 are still broken).

Given that 6dce5aa59e0b has been reverted, revert this patch as well,
restoring the PCIe support on XGene to its pre-5.5, working state.

Cc: Rob Herring <robh@kernel.org>
Cc: Toan Le <toan@os.amperecomputing.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Krzysztof Wilczyński <kw@linux.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stéphane Graber <stgraber@ubuntu.com>
Cc: dann frazier <dann.frazier@canonical.com>
Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
Fixes: c7a75d07827a ("PCI: xgene: Fix IB window setup")
Link: https://lore.kernel.org/r/YjN8pT5e6/8cRohQ@xps13.dannf
---
 drivers/pci/controller/pci-xgene.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index aa41ceaf031f..7c763d820c52 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -465,7 +465,7 @@ static int xgene_pcie_select_ib_reg(u8 *ib_reg_mask, u64 size)
 		return 1;
 	}
 
-	if ((size > SZ_1K) && (size < SZ_4G) && !(*ib_reg_mask & (1 << 0))) {
+	if ((size > SZ_1K) && (size < SZ_1T) && !(*ib_reg_mask & (1 << 0))) {
 		*ib_reg_mask |= (1 << 0);
 		return 0;
 	}
-- 
2.34.1

[PATCH v2 2/2] PCI: xgene: Revert "PCI: xgene: Fix IB window setup"

[Marc Zyngier]

Commit c7a75d07827a ("PCI: xgene: Fix IB window setup") tried to
fix the damages that 6dce5aa59e0b ("PCI: xgene: Use inbound resources
for setup") caused, but actually didn't improve anything for some
plarforms (at least Mustang and m400 are still broken).

Given that 6dce5aa59e0b has been reverted, revert this patch as well,
restoring the PCIe support on XGene to its pre-5.5, working state.

Cc: Rob Herring <robh@kernel.org>
Cc: Toan Le <toan@os.amperecomputing.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Krzysztof Wilczyński <kw@linux.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stéphane Graber <stgraber@ubuntu.com>
Cc: dann frazier <dann.frazier@canonical.com>
Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
Fixes: c7a75d07827a ("PCI: xgene: Fix IB window setup")
Link: https://lore.kernel.org/r/YjN8pT5e6/8cRohQ@xps13.dannf
---
 drivers/pci/controller/pci-xgene.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index aa41ceaf031f..7c763d820c52 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -465,7 +465,7 @@ static int xgene_pcie_select_ib_reg(u8 *ib_reg_mask, u64 size)
 		return 1;
 	}
 
-	if ((size > SZ_1K) && (size < SZ_4G) && !(*ib_reg_mask & (1 << 0))) {
+	if ((size > SZ_1K) && (size < SZ_1T) && !(*ib_reg_mask & (1 << 0))) {
 		*ib_reg_mask |= (1 << 0);
 		return 0;
 	}
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Lorenzo Pieralisi]

On Mon, 21 Mar 2022 10:48:41 +0000, Marc Zyngier wrote:
> Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> IB window setup") fixed XGene-2, but left the rest of the zoo
> unusable.
> 
> [...]

Applied to pci/xgene, thanks!

[1/2] PCI: xgene: Revert "PCI: xgene: Use inbound resources for setup"
      https://git.kernel.org/lpieralisi/pci/c/1874b6d7ab
[2/2] PCI: xgene: Revert "PCI: xgene: Fix IB window setup"
      https://git.kernel.org/lpieralisi/pci/c/825da4e9ce

Thanks,
Lorenzo

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Lorenzo Pieralisi]

On Mon, 21 Mar 2022 10:48:41 +0000, Marc Zyngier wrote:
> Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> IB window setup") fixed XGene-2, but left the rest of the zoo
> unusable.
> 
> [...]

Applied to pci/xgene, thanks!

[1/2] PCI: xgene: Revert "PCI: xgene: Use inbound resources for setup"
      https://git.kernel.org/lpieralisi/pci/c/1874b6d7ab
[2/2] PCI: xgene: Revert "PCI: xgene: Fix IB window setup"
      https://git.kernel.org/lpieralisi/pci/c/825da4e9ce

Thanks,
Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> IB window setup") fixed XGene-2, but left the rest of the zoo
> unusable.
>
> It is understood that this systems come with "creative" DTs that don't
> match the expectations of modern kernels. However, there is little to
> be gained by forcing these changes on users -- the firmware is not
> upgradable, and the current owner of the IP will deny that these
> machines have ever existed.

The gain for fixing this properly is not having drivers do their own
dma-ranges parsing. We've seen what happens when drivers do their own
parsing of standard properties (e.g. interrupt-map). Currently, we
don't have any drivers doing their own parsing:

$ git grep of_pci_dma_range_parser_init
drivers/of/address.c:int of_pci_dma_range_parser_init(struct
of_pci_range_parser *parser,
drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
drivers/of/address.c:#define of_dma_range_parser_init
of_pci_dma_range_parser_init
drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
include/linux/of_address.h:extern int
of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
include/linux/of_address.h:static inline int
of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,

And we can probably further refactor this to be private to drivers/pci/of.c.

For XGene-2 the issue is simply that the driver depends on the order
of dma-ranges entries.

For XGene-1, I'd still like to understand what the issue is. Reverting
the first fix and fixing 'dma-ranges' should have fixed it. I need a
dump of how the IB registers are initialized in both cases. I'm not
saying changing 'dma-ranges' in the firmware is going to be required
here. There's a couple of other ways we could fix that without a
firmware change, but first I need to understand why it broke.

Rob

P.S. We're carrying ACPI and DT support for these platforms. It seems
the few users are using DT, so can we drop the ACPI support? Or do I
need to break it first and wait a year? ;)

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> IB window setup") fixed XGene-2, but left the rest of the zoo
> unusable.
>
> It is understood that this systems come with "creative" DTs that don't
> match the expectations of modern kernels. However, there is little to
> be gained by forcing these changes on users -- the firmware is not
> upgradable, and the current owner of the IP will deny that these
> machines have ever existed.

The gain for fixing this properly is not having drivers do their own
dma-ranges parsing. We've seen what happens when drivers do their own
parsing of standard properties (e.g. interrupt-map). Currently, we
don't have any drivers doing their own parsing:

$ git grep of_pci_dma_range_parser_init
drivers/of/address.c:int of_pci_dma_range_parser_init(struct
of_pci_range_parser *parser,
drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
drivers/of/address.c:#define of_dma_range_parser_init
of_pci_dma_range_parser_init
drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
include/linux/of_address.h:extern int
of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
include/linux/of_address.h:static inline int
of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,

And we can probably further refactor this to be private to drivers/pci/of.c.

For XGene-2 the issue is simply that the driver depends on the order
of dma-ranges entries.

For XGene-1, I'd still like to understand what the issue is. Reverting
the first fix and fixing 'dma-ranges' should have fixed it. I need a
dump of how the IB registers are initialized in both cases. I'm not
saying changing 'dma-ranges' in the firmware is going to be required
here. There's a couple of other ways we could fix that without a
firmware change, but first I need to understand why it broke.

Rob

P.S. We're carrying ACPI and DT support for these platforms. It seems
the few users are using DT, so can we drop the ACPI support? Or do I
need to break it first and wait a year? ;)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > IB window setup") fixed XGene-2, but left the rest of the zoo
> > unusable.
> >
> > It is understood that this systems come with "creative" DTs that don't
> > match the expectations of modern kernels. However, there is little to
> > be gained by forcing these changes on users -- the firmware is not
> > upgradable, and the current owner of the IP will deny that these
> > machines have ever existed.
> 
> The gain for fixing this properly is not having drivers do their own
> dma-ranges parsing. We've seen what happens when drivers do their own
> parsing of standard properties (e.g. interrupt-map). Currently, we
> don't have any drivers doing their own parsing:
> 
> $ git grep of_pci_dma_range_parser_init
> drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> of_pci_range_parser *parser,
> drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> drivers/of/address.c:#define of_dma_range_parser_init
> of_pci_dma_range_parser_init
> drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> include/linux/of_address.h:extern int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> include/linux/of_address.h:static inline int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> 
> And we can probably further refactor this to be private to drivers/pci/of.c.
> 
> For XGene-2 the issue is simply that the driver depends on the order
> of dma-ranges entries.
> 
> For XGene-1, I'd still like to understand what the issue is. Reverting
> the first fix and fixing 'dma-ranges' should have fixed it. I need a
> dump of how the IB registers are initialized in both cases.

Happy to provide that for the m400 if told how :)

  -dann

> I'm not
> saying changing 'dma-ranges' in the firmware is going to be required
> here. There's a couple of other ways we could fix that without a
> firmware change, but first I need to understand why it broke.

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > IB window setup") fixed XGene-2, but left the rest of the zoo
> > unusable.
> >
> > It is understood that this systems come with "creative" DTs that don't
> > match the expectations of modern kernels. However, there is little to
> > be gained by forcing these changes on users -- the firmware is not
> > upgradable, and the current owner of the IP will deny that these
> > machines have ever existed.
> 
> The gain for fixing this properly is not having drivers do their own
> dma-ranges parsing. We've seen what happens when drivers do their own
> parsing of standard properties (e.g. interrupt-map). Currently, we
> don't have any drivers doing their own parsing:
> 
> $ git grep of_pci_dma_range_parser_init
> drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> of_pci_range_parser *parser,
> drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> drivers/of/address.c:#define of_dma_range_parser_init
> of_pci_dma_range_parser_init
> drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> include/linux/of_address.h:extern int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> include/linux/of_address.h:static inline int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> 
> And we can probably further refactor this to be private to drivers/pci/of.c.
> 
> For XGene-2 the issue is simply that the driver depends on the order
> of dma-ranges entries.
> 
> For XGene-1, I'd still like to understand what the issue is. Reverting
> the first fix and fixing 'dma-ranges' should have fixed it. I need a
> dump of how the IB registers are initialized in both cases.

Happy to provide that for the m400 if told how :)

  -dann

> I'm not
> saying changing 'dma-ranges' in the firmware is going to be required
> here. There's a couple of other ways we could fix that without a
> firmware change, but first I need to understand why it broke.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > unusable.
> > >
> > > It is understood that this systems come with "creative" DTs that don't
> > > match the expectations of modern kernels. However, there is little to
> > > be gained by forcing these changes on users -- the firmware is not
> > > upgradable, and the current owner of the IP will deny that these
> > > machines have ever existed.
> > 
> > The gain for fixing this properly is not having drivers do their own
> > dma-ranges parsing. We've seen what happens when drivers do their own
> > parsing of standard properties (e.g. interrupt-map). Currently, we
> > don't have any drivers doing their own parsing:
> > 
> > $ git grep of_pci_dma_range_parser_init
> > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > of_pci_range_parser *parser,
> > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > drivers/of/address.c:#define of_dma_range_parser_init
> > of_pci_dma_range_parser_init
> > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > include/linux/of_address.h:extern int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > include/linux/of_address.h:static inline int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > 
> > And we can probably further refactor this to be private to drivers/pci/of.c.
> > 
> > For XGene-2 the issue is simply that the driver depends on the order
> > of dma-ranges entries.
> > 
> > For XGene-1, I'd still like to understand what the issue is. Reverting
> > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > dump of how the IB registers are initialized in both cases.
> 
> Happy to provide that for the m400 if told how :)

Something like the below patch. This should be with the 'dma-ranges' 
DT change and only c7a75d07827a reverted.

8<-------------------------------------------------------------------
diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index 0d5acbfc7143..6a435c31f45e 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -78,6 +78,7 @@ static u32 xgene_pcie_readl(struct xgene_pcie *port, u32 reg)
 
 static void xgene_pcie_writel(struct xgene_pcie *port, u32 reg, u32 val)
 {
+	dev_info(port->dev, "0x%04x <- 0x%08x\n", reg, val);
 	writel(val, port->csr_base + reg);
 }
 
@@ -508,7 +509,9 @@ static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
 	case 0:
 		xgene_pcie_set_ib_mask(port, BRIDGE_CFG_4, flags, size);
 		bar_addr = cfg_base + PCI_BASE_ADDRESS_0;
+		dev_info(port->dev, "BAR0L <- 0x%08x\n", bar_low);
 		writel(bar_low, bar_addr);
+		dev_info(port->dev, "BAR0H <- 0x%08x\n", upper_32_bits(cpu_addr));
 		writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
 		pim_reg = PIM1_1L;
 		break;

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > unusable.
> > >
> > > It is understood that this systems come with "creative" DTs that don't
> > > match the expectations of modern kernels. However, there is little to
> > > be gained by forcing these changes on users -- the firmware is not
> > > upgradable, and the current owner of the IP will deny that these
> > > machines have ever existed.
> > 
> > The gain for fixing this properly is not having drivers do their own
> > dma-ranges parsing. We've seen what happens when drivers do their own
> > parsing of standard properties (e.g. interrupt-map). Currently, we
> > don't have any drivers doing their own parsing:
> > 
> > $ git grep of_pci_dma_range_parser_init
> > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > of_pci_range_parser *parser,
> > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > drivers/of/address.c:#define of_dma_range_parser_init
> > of_pci_dma_range_parser_init
> > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > include/linux/of_address.h:extern int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > include/linux/of_address.h:static inline int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > 
> > And we can probably further refactor this to be private to drivers/pci/of.c.
> > 
> > For XGene-2 the issue is simply that the driver depends on the order
> > of dma-ranges entries.
> > 
> > For XGene-1, I'd still like to understand what the issue is. Reverting
> > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > dump of how the IB registers are initialized in both cases.
> 
> Happy to provide that for the m400 if told how :)

Something like the below patch. This should be with the 'dma-ranges' 
DT change and only c7a75d07827a reverted.

8<-------------------------------------------------------------------
diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index 0d5acbfc7143..6a435c31f45e 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -78,6 +78,7 @@ static u32 xgene_pcie_readl(struct xgene_pcie *port, u32 reg)
 
 static void xgene_pcie_writel(struct xgene_pcie *port, u32 reg, u32 val)
 {
+	dev_info(port->dev, "0x%04x <- 0x%08x\n", reg, val);
 	writel(val, port->csr_base + reg);
 }
 
@@ -508,7 +509,9 @@ static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
 	case 0:
 		xgene_pcie_set_ib_mask(port, BRIDGE_CFG_4, flags, size);
 		bar_addr = cfg_base + PCI_BASE_ADDRESS_0;
+		dev_info(port->dev, "BAR0L <- 0x%08x\n", bar_low);
 		writel(bar_low, bar_addr);
+		dev_info(port->dev, "BAR0H <- 0x%08x\n", upper_32_bits(cpu_addr));
 		writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
 		pim_reg = PIM1_1L;
 		break;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

On Mon, 21 Mar 2022 15:17:34 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > IB window setup") fixed XGene-2, but left the rest of the zoo
> > unusable.
> >
> > It is understood that this systems come with "creative" DTs that don't
> > match the expectations of modern kernels. However, there is little to
> > be gained by forcing these changes on users -- the firmware is not
> > upgradable, and the current owner of the IP will deny that these
> > machines have ever existed.
> 
> The gain for fixing this properly is not having drivers do their own
> dma-ranges parsing. We've seen what happens when drivers do their own
> parsing of standard properties (e.g. interrupt-map).

We have, and we added the required exceptions for the legacy platforms
that the code base supported until then. We didn't leave things broken
just because we didn't like the way things were done a long time ago.

> Currently, we don't have any drivers doing their own parsing:
> 
> $ git grep of_pci_dma_range_parser_init
> drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> of_pci_range_parser *parser,
> drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> drivers/of/address.c:#define of_dma_range_parser_init
> of_pci_dma_range_parser_init
> drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> include/linux/of_address.h:extern int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> include/linux/of_address.h:static inline int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> 
> And we can probably further refactor this to be private to drivers/pci/of.c.
> 
> For XGene-2 the issue is simply that the driver depends on the order
> of dma-ranges entries.
> 
> For XGene-1, I'd still like to understand what the issue is. Reverting
> the first fix and fixing 'dma-ranges' should have fixed it. I need a
> dump of how the IB registers are initialized in both cases. I'm not
> saying changing 'dma-ranges' in the firmware is going to be required
> here. There's a couple of other ways we could fix that without a
> firmware change, but first I need to understand why it broke.

Reverting 6dce5aa59e0b was enough for me, without changing anything
else. m400 probably uses an even older firmware (AFAIR, it was stuck
with an ancient version of u-boot that HP never updated, while Mustang
had a few updates). In any case, that DT cannot be changed.

> 
> Rob
> 
> P.S. We're carrying ACPI and DT support for these platforms. It seems
> the few users are using DT, so can we drop the ACPI support? Or do I
> need to break it first and wait a year? ;)

I'm not sure people on the list are representative of all the users,
and I didn't realise the plan was "let's break everything we don't
like and see if someone wakes up" either. That definitely puts things
in a different perspective.

	M.

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

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

On Mon, 21 Mar 2022 15:17:34 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > IB window setup") fixed XGene-2, but left the rest of the zoo
> > unusable.
> >
> > It is understood that this systems come with "creative" DTs that don't
> > match the expectations of modern kernels. However, there is little to
> > be gained by forcing these changes on users -- the firmware is not
> > upgradable, and the current owner of the IP will deny that these
> > machines have ever existed.
> 
> The gain for fixing this properly is not having drivers do their own
> dma-ranges parsing. We've seen what happens when drivers do their own
> parsing of standard properties (e.g. interrupt-map).

We have, and we added the required exceptions for the legacy platforms
that the code base supported until then. We didn't leave things broken
just because we didn't like the way things were done a long time ago.

> Currently, we don't have any drivers doing their own parsing:
> 
> $ git grep of_pci_dma_range_parser_init
> drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> of_pci_range_parser *parser,
> drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> drivers/of/address.c:#define of_dma_range_parser_init
> of_pci_dma_range_parser_init
> drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> include/linux/of_address.h:extern int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> include/linux/of_address.h:static inline int
> of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> 
> And we can probably further refactor this to be private to drivers/pci/of.c.
> 
> For XGene-2 the issue is simply that the driver depends on the order
> of dma-ranges entries.
> 
> For XGene-1, I'd still like to understand what the issue is. Reverting
> the first fix and fixing 'dma-ranges' should have fixed it. I need a
> dump of how the IB registers are initialized in both cases. I'm not
> saying changing 'dma-ranges' in the firmware is going to be required
> here. There's a couple of other ways we could fix that without a
> firmware change, but first I need to understand why it broke.

Reverting 6dce5aa59e0b was enough for me, without changing anything
else. m400 probably uses an even older firmware (AFAIR, it was stuck
with an ancient version of u-boot that HP never updated, while Mustang
had a few updates). In any case, that DT cannot be changed.

> 
> Rob
> 
> P.S. We're carrying ACPI and DT support for these platforms. It seems
> the few users are using DT, so can we drop the ACPI support? Or do I
> need to break it first and wait a year? ;)

I'm not sure people on the list are representative of all the users,
and I didn't realise the plan was "let's break everything we don't
like and see if someone wakes up" either. That definitely puts things
in a different perspective.

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 21 Mar 2022 15:17:34 +0000,
> Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > unusable.
> > >
> > > It is understood that this systems come with "creative" DTs that don't
> > > match the expectations of modern kernels. However, there is little to
> > > be gained by forcing these changes on users -- the firmware is not
> > > upgradable, and the current owner of the IP will deny that these
> > > machines have ever existed.
> >
> > The gain for fixing this properly is not having drivers do their own
> > dma-ranges parsing. We've seen what happens when drivers do their own
> > parsing of standard properties (e.g. interrupt-map).
>
> We have, and we added the required exceptions for the legacy platforms
> that the code base supported until then. We didn't leave things broken
> just because we didn't like the way things were done a long time ago.
>
> > Currently, we don't have any drivers doing their own parsing:
> >
> > $ git grep of_pci_dma_range_parser_init
> > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > of_pci_range_parser *parser,
> > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > drivers/of/address.c:#define of_dma_range_parser_init
> > of_pci_dma_range_parser_init
> > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > include/linux/of_address.h:extern int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > include/linux/of_address.h:static inline int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> >
> > And we can probably further refactor this to be private to drivers/pci/of.c.
> >
> > For XGene-2 the issue is simply that the driver depends on the order
> > of dma-ranges entries.
> >
> > For XGene-1, I'd still like to understand what the issue is. Reverting
> > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > dump of how the IB registers are initialized in both cases. I'm not
> > saying changing 'dma-ranges' in the firmware is going to be required
> > here. There's a couple of other ways we could fix that without a
> > firmware change, but first I need to understand why it broke.
>
> Reverting 6dce5aa59e0b was enough for me, without changing anything
> else.

Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.

Can you tell me what 'dma-ranges' contains on your system?

> m400 probably uses an even older firmware (AFAIR, it was stuck
> with an ancient version of u-boot that HP never updated, while Mustang
> had a few updates). In any case, that DT cannot be changed.

How is Dann changing it then? I assume he's not changing the firmware,
but overriding it. That could be a possible solution.

Do the DT's in the kernel tree correspond to anything anyone is using?
I ask because at some point someone will need to address all the
warnings they have or we should drop the dts files if they aren't
close to reality. The same thing applies to Seattle BTW.

> > Rob
> >
> > P.S. We're carrying ACPI and DT support for these platforms. It seems
> > the few users are using DT, so can we drop the ACPI support? Or do I
> > need to break it first and wait a year? ;)
>
> I'm not sure people on the list are representative of all the users,
> and I didn't realise the plan was "let's break everything we don't
> like and see if someone wakes up" either. That definitely puts things
> in a different perspective.

I wasn't really suggesting breaking things on purpose. However, there
is a cost to keeping code and it would be nice to know what's being
used or not. The cost isn't *that* big, but it is not zero for what's
not many users.

At least for Stéphane, using ACPI didn't even work. I'm assuming there
is some version of h/w and f/w out there that did work with the ACPI
support in the kernel? That may have never been seen by anyone but APM
and Jon Masters (his Tested-by is on the patch from APM adding ACPI
support). It's not hard to imagine there was a version of firmware
just to shut Jon up.

Rob

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 21 Mar 2022 15:17:34 +0000,
> Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > unusable.
> > >
> > > It is understood that this systems come with "creative" DTs that don't
> > > match the expectations of modern kernels. However, there is little to
> > > be gained by forcing these changes on users -- the firmware is not
> > > upgradable, and the current owner of the IP will deny that these
> > > machines have ever existed.
> >
> > The gain for fixing this properly is not having drivers do their own
> > dma-ranges parsing. We've seen what happens when drivers do their own
> > parsing of standard properties (e.g. interrupt-map).
>
> We have, and we added the required exceptions for the legacy platforms
> that the code base supported until then. We didn't leave things broken
> just because we didn't like the way things were done a long time ago.
>
> > Currently, we don't have any drivers doing their own parsing:
> >
> > $ git grep of_pci_dma_range_parser_init
> > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > of_pci_range_parser *parser,
> > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > drivers/of/address.c:#define of_dma_range_parser_init
> > of_pci_dma_range_parser_init
> > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > include/linux/of_address.h:extern int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > include/linux/of_address.h:static inline int
> > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> >
> > And we can probably further refactor this to be private to drivers/pci/of.c.
> >
> > For XGene-2 the issue is simply that the driver depends on the order
> > of dma-ranges entries.
> >
> > For XGene-1, I'd still like to understand what the issue is. Reverting
> > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > dump of how the IB registers are initialized in both cases. I'm not
> > saying changing 'dma-ranges' in the firmware is going to be required
> > here. There's a couple of other ways we could fix that without a
> > firmware change, but first I need to understand why it broke.
>
> Reverting 6dce5aa59e0b was enough for me, without changing anything
> else.

Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.

Can you tell me what 'dma-ranges' contains on your system?

> m400 probably uses an even older firmware (AFAIR, it was stuck
> with an ancient version of u-boot that HP never updated, while Mustang
> had a few updates). In any case, that DT cannot be changed.

How is Dann changing it then? I assume he's not changing the firmware,
but overriding it. That could be a possible solution.

Do the DT's in the kernel tree correspond to anything anyone is using?
I ask because at some point someone will need to address all the
warnings they have or we should drop the dts files if they aren't
close to reality. The same thing applies to Seattle BTW.

> > Rob
> >
> > P.S. We're carrying ACPI and DT support for these platforms. It seems
> > the few users are using DT, so can we drop the ACPI support? Or do I
> > need to break it first and wait a year? ;)
>
> I'm not sure people on the list are representative of all the users,
> and I didn't realise the plan was "let's break everything we don't
> like and see if someone wakes up" either. That definitely puts things
> in a different perspective.

I wasn't really suggesting breaking things on purpose. However, there
is a cost to keeping code and it would be nice to know what's being
used or not. The cost isn't *that* big, but it is not zero for what's
not many users.

At least for Stéphane, using ACPI didn't even work. I'm assuming there
is some version of h/w and f/w out there that did work with the ACPI
support in the kernel? That may have never been seen by anyone but APM
and Jon Masters (his Tested-by is on the patch from APM adding ACPI
support). It's not hard to imagine there was a version of firmware
just to shut Jon up.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

On Mon, 21 Mar 2022 18:03:27 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Mon, 21 Mar 2022 15:17:34 +0000,
> > Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > dump of how the IB registers are initialized in both cases. I'm not
> > > saying changing 'dma-ranges' in the firmware is going to be required
> > > here. There's a couple of other ways we could fix that without a
> > > firmware change, but first I need to understand why it broke.
> >
> > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > else.
> 
> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> 
> Can you tell me what 'dma-ranges' contains on your system?

Each pcie node (all 5 of them) has:

dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
              0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;

> 
> > m400 probably uses an even older firmware (AFAIR, it was stuck
> > with an ancient version of u-boot that HP never updated, while Mustang
> > had a few updates). In any case, that DT cannot be changed.
> 
> How is Dann changing it then? I assume he's not changing the firmware,
> but overriding it. That could be a possible solution.

I don't know about you, but changing DT is not an acceptable solution
for me. If I'm bisecting something and have to pick the right DT based
on the kernel revision I'm using, that's a huge regression. And I'm
not even mentioning the poor sod who simply updates their distro, only
to find that the box doesn't boot anymore thanks to a kernel upgrade.

We're not talking about a closed embedded device here, but a fully
functional desktop/server box that still run rings around your average
RPi.

> Do the DT's in the kernel tree correspond to anything anyone is using?
> I ask because at some point someone will need to address all the
> warnings they have or we should drop the dts files if they aren't
> close to reality. The same thing applies to Seattle BTW.

I'd be perfectly happy to see both of these go. The last time I used
the kernel DT on my Seattle was some time in 2015, at which point I
got a firmware correctly describing the SMMUs. Oh, and ACPI works just
fine on Seattle.

> > > P.S. We're carrying ACPI and DT support for these platforms. It seems
> > > the few users are using DT, so can we drop the ACPI support? Or do I
> > > need to break it first and wait a year? ;)
> >
> > I'm not sure people on the list are representative of all the users,
> > and I didn't realise the plan was "let's break everything we don't
> > like and see if someone wakes up" either. That definitely puts things
> > in a different perspective.
> 
> I wasn't really suggesting breaking things on purpose. However, there
> is a cost to keeping code and it would be nice to know what's being
> used or not. The cost isn't *that* big, but it is not zero for what's
> not many users.
> 
> At least for Stéphane, using ACPI didn't even work. I'm assuming there
> is some version of h/w and f/w out there that did work with the ACPI
> support in the kernel? That may have never been seen by anyone but APM
> and Jon Masters (his Tested-by is on the patch from APM adding ACPI
> support). It's not hard to imagine there was a version of firmware
> just to shut Jon up.

Well, you can ask Jon. ACPI doesn't work on my box as it doesn't
(properly) describe a console, but my FW is rather old, as explained
in the release notes:

    Version 3.06.25 (10/17/2016)
    Mustang Tianocore (UEFI BIOS) Firmware Features:
     - TianoCore tag linaro-edk2-2014.07 with custom changes

Yes, 2014. Good vintage. But is that the latest and greatest? I have
no idea, and the APM website is long gone.

	M.

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

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

On Mon, 21 Mar 2022 18:03:27 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Mon, 21 Mar 2022 15:17:34 +0000,
> > Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > dump of how the IB registers are initialized in both cases. I'm not
> > > saying changing 'dma-ranges' in the firmware is going to be required
> > > here. There's a couple of other ways we could fix that without a
> > > firmware change, but first I need to understand why it broke.
> >
> > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > else.
> 
> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> 
> Can you tell me what 'dma-ranges' contains on your system?

Each pcie node (all 5 of them) has:

dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
              0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;

> 
> > m400 probably uses an even older firmware (AFAIR, it was stuck
> > with an ancient version of u-boot that HP never updated, while Mustang
> > had a few updates). In any case, that DT cannot be changed.
> 
> How is Dann changing it then? I assume he's not changing the firmware,
> but overriding it. That could be a possible solution.

I don't know about you, but changing DT is not an acceptable solution
for me. If I'm bisecting something and have to pick the right DT based
on the kernel revision I'm using, that's a huge regression. And I'm
not even mentioning the poor sod who simply updates their distro, only
to find that the box doesn't boot anymore thanks to a kernel upgrade.

We're not talking about a closed embedded device here, but a fully
functional desktop/server box that still run rings around your average
RPi.

> Do the DT's in the kernel tree correspond to anything anyone is using?
> I ask because at some point someone will need to address all the
> warnings they have or we should drop the dts files if they aren't
> close to reality. The same thing applies to Seattle BTW.

I'd be perfectly happy to see both of these go. The last time I used
the kernel DT on my Seattle was some time in 2015, at which point I
got a firmware correctly describing the SMMUs. Oh, and ACPI works just
fine on Seattle.

> > > P.S. We're carrying ACPI and DT support for these platforms. It seems
> > > the few users are using DT, so can we drop the ACPI support? Or do I
> > > need to break it first and wait a year? ;)
> >
> > I'm not sure people on the list are representative of all the users,
> > and I didn't realise the plan was "let's break everything we don't
> > like and see if someone wakes up" either. That definitely puts things
> > in a different perspective.
> 
> I wasn't really suggesting breaking things on purpose. However, there
> is a cost to keeping code and it would be nice to know what's being
> used or not. The cost isn't *that* big, but it is not zero for what's
> not many users.
> 
> At least for Stéphane, using ACPI didn't even work. I'm assuming there
> is some version of h/w and f/w out there that did work with the ACPI
> support in the kernel? That may have never been seen by anyone but APM
> and Jon Masters (his Tested-by is on the patch from APM adding ACPI
> support). It's not hard to imagine there was a version of firmware
> just to shut Jon up.

Well, you can ask Jon. ACPI doesn't work on my box as it doesn't
(properly) describe a console, but my FW is rather old, as explained
in the release notes:

    Version 3.06.25 (10/17/2016)
    Mustang Tianocore (UEFI BIOS) Firmware Features:
     - TianoCore tag linaro-edk2-2014.07 with custom changes

Yes, 2014. Good vintage. But is that the latest and greatest? I have
no idea, and the APM website is long gone.

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Robin Murphy]

On 2022-03-21 19:21, Marc Zyngier wrote:
> On Mon, 21 Mar 2022 18:03:27 +0000,
> Rob Herring <robh@kernel.org> wrote:
>>
>> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>>>
>>> On Mon, 21 Mar 2022 15:17:34 +0000,
>>> Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>
>>>> For XGene-1, I'd still like to understand what the issue is. Reverting
>>>> the first fix and fixing 'dma-ranges' should have fixed it. I need a
>>>> dump of how the IB registers are initialized in both cases. I'm not
>>>> saying changing 'dma-ranges' in the firmware is going to be required
>>>> here. There's a couple of other ways we could fix that without a
>>>> firmware change, but first I need to understand why it broke.
>>>
>>> Reverting 6dce5aa59e0b was enough for me, without changing anything
>>> else.
>>
>> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
>>
>> Can you tell me what 'dma-ranges' contains on your system?
> 
> Each pcie node (all 5 of them) has:
> 
> dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
>                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;

Hmm, is there anyone other than iommu-dma who actually depends on the 
resource list being sorted in ascending order of bus address? I recall 
at the time I pushed for creating the list in sorted order as it was the 
simplest and most efficient option, but there's no technical reason we 
couldn't create it in as-found order and defer the sorting until 
iova_reserve_pci_windows() (at worst that could even operate on a 
temporary copy if need be). It's just more code, which didn't need to 
exist without a good reason, but if this is one then exist it certainly may.

Cheers,
Robin.

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Robin Murphy]

On 2022-03-21 19:21, Marc Zyngier wrote:
> On Mon, 21 Mar 2022 18:03:27 +0000,
> Rob Herring <robh@kernel.org> wrote:
>>
>> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>>>
>>> On Mon, 21 Mar 2022 15:17:34 +0000,
>>> Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>
>>>> For XGene-1, I'd still like to understand what the issue is. Reverting
>>>> the first fix and fixing 'dma-ranges' should have fixed it. I need a
>>>> dump of how the IB registers are initialized in both cases. I'm not
>>>> saying changing 'dma-ranges' in the firmware is going to be required
>>>> here. There's a couple of other ways we could fix that without a
>>>> firmware change, but first I need to understand why it broke.
>>>
>>> Reverting 6dce5aa59e0b was enough for me, without changing anything
>>> else.
>>
>> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
>>
>> Can you tell me what 'dma-ranges' contains on your system?
> 
> Each pcie node (all 5 of them) has:
> 
> dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
>                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;

Hmm, is there anyone other than iommu-dma who actually depends on the 
resource list being sorted in ascending order of bus address? I recall 
at the time I pushed for creating the list in sorted order as it was the 
simplest and most efficient option, but there's no technical reason we 
couldn't create it in as-found order and defer the sorting until 
iova_reserve_pci_windows() (at worst that could even operate on a 
temporary copy if need be). It's just more code, which didn't need to 
exist without a good reason, but if this is one then exist it certainly may.

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Mon, Mar 21, 2022 at 01:03:27PM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Mon, 21 Mar 2022 15:17:34 +0000,
> > Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > unusable.
> > > >
> > > > It is understood that this systems come with "creative" DTs that don't
> > > > match the expectations of modern kernels. However, there is little to
> > > > be gained by forcing these changes on users -- the firmware is not
> > > > upgradable, and the current owner of the IP will deny that these
> > > > machines have ever existed.
> > >
> > > The gain for fixing this properly is not having drivers do their own
> > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > parsing of standard properties (e.g. interrupt-map).
> >
> > We have, and we added the required exceptions for the legacy platforms
> > that the code base supported until then. We didn't leave things broken
> > just because we didn't like the way things were done a long time ago.
> >
> > > Currently, we don't have any drivers doing their own parsing:
> > >
> > > $ git grep of_pci_dma_range_parser_init
> > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > of_pci_range_parser *parser,
> > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > drivers/of/address.c:#define of_dma_range_parser_init
> > > of_pci_dma_range_parser_init
> > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > include/linux/of_address.h:extern int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > include/linux/of_address.h:static inline int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > >
> > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > >
> > > For XGene-2 the issue is simply that the driver depends on the order
> > > of dma-ranges entries.
> > >
> > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > dump of how the IB registers are initialized in both cases. I'm not
> > > saying changing 'dma-ranges' in the firmware is going to be required
> > > here. There's a couple of other ways we could fix that without a
> > > firmware change, but first I need to understand why it broke.
> >
> > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > else.
> 
> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> 
> Can you tell me what 'dma-ranges' contains on your system?
> 
> > m400 probably uses an even older firmware (AFAIR, it was stuck
> > with an ancient version of u-boot that HP never updated, while Mustang
> > had a few updates). In any case, that DT cannot be changed.
> 
> How is Dann changing it then? I assume he's not changing the firmware,
> but overriding it. That could be a possible solution.

Correct, I'm just overriding it for testing. I'm using the pxelinux
emulation provided by the m400's u-boot, which supports an FDT field:

---------
$ cat /srv/tftp/pxelinux.cfg/default
DEFAULT default

LABEL default
  KERNEL uImage
  APPEND initrd=uInitrd console=ttyS0,9600n8r ro root=LABEL=cloudimg-rootfs
  FDT m400.dtb
---------

This loads the specified file into ${fdt_addr_r}, overriding the blob
that the firmware had already loaded there.

> Do the DT's in the kernel tree correspond to anything anyone is
> using?

Upstream apm-mustang.dtb is what Ubuntu uses for Mustang boards w/
u-boot firmware. That used to work fine, but I haven't tried lately.

  -dann

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Mon, Mar 21, 2022 at 01:03:27PM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Mon, 21 Mar 2022 15:17:34 +0000,
> > Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > unusable.
> > > >
> > > > It is understood that this systems come with "creative" DTs that don't
> > > > match the expectations of modern kernels. However, there is little to
> > > > be gained by forcing these changes on users -- the firmware is not
> > > > upgradable, and the current owner of the IP will deny that these
> > > > machines have ever existed.
> > >
> > > The gain for fixing this properly is not having drivers do their own
> > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > parsing of standard properties (e.g. interrupt-map).
> >
> > We have, and we added the required exceptions for the legacy platforms
> > that the code base supported until then. We didn't leave things broken
> > just because we didn't like the way things were done a long time ago.
> >
> > > Currently, we don't have any drivers doing their own parsing:
> > >
> > > $ git grep of_pci_dma_range_parser_init
> > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > of_pci_range_parser *parser,
> > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > drivers/of/address.c:#define of_dma_range_parser_init
> > > of_pci_dma_range_parser_init
> > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > include/linux/of_address.h:extern int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > include/linux/of_address.h:static inline int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > >
> > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > >
> > > For XGene-2 the issue is simply that the driver depends on the order
> > > of dma-ranges entries.
> > >
> > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > dump of how the IB registers are initialized in both cases. I'm not
> > > saying changing 'dma-ranges' in the firmware is going to be required
> > > here. There's a couple of other ways we could fix that without a
> > > firmware change, but first I need to understand why it broke.
> >
> > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > else.
> 
> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> 
> Can you tell me what 'dma-ranges' contains on your system?
> 
> > m400 probably uses an even older firmware (AFAIR, it was stuck
> > with an ancient version of u-boot that HP never updated, while Mustang
> > had a few updates). In any case, that DT cannot be changed.
> 
> How is Dann changing it then? I assume he's not changing the firmware,
> but overriding it. That could be a possible solution.

Correct, I'm just overriding it for testing. I'm using the pxelinux
emulation provided by the m400's u-boot, which supports an FDT field:

---------
$ cat /srv/tftp/pxelinux.cfg/default
DEFAULT default

LABEL default
  KERNEL uImage
  APPEND initrd=uInitrd console=ttyS0,9600n8r ro root=LABEL=cloudimg-rootfs
  FDT m400.dtb
---------

This loads the specified file into ${fdt_addr_r}, overriding the blob
that the firmware had already loaded there.

> Do the DT's in the kernel tree correspond to anything anyone is
> using?

Upstream apm-mustang.dtb is what Ubuntu uses for Mustang boards w/
u-boot firmware. That used to work fine, but I haven't tried lately.

  -dann

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Mon, Mar 21, 2022 at 11:08:20AM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> > On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > unusable.
> > > >
> > > > It is understood that this systems come with "creative" DTs that don't
> > > > match the expectations of modern kernels. However, there is little to
> > > > be gained by forcing these changes on users -- the firmware is not
> > > > upgradable, and the current owner of the IP will deny that these
> > > > machines have ever existed.
> > > 
> > > The gain for fixing this properly is not having drivers do their own
> > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > parsing of standard properties (e.g. interrupt-map). Currently, we
> > > don't have any drivers doing their own parsing:
> > > 
> > > $ git grep of_pci_dma_range_parser_init
> > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > of_pci_range_parser *parser,
> > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > drivers/of/address.c:#define of_dma_range_parser_init
> > > of_pci_dma_range_parser_init
> > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > include/linux/of_address.h:extern int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > include/linux/of_address.h:static inline int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > 
> > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > > 
> > > For XGene-2 the issue is simply that the driver depends on the order
> > > of dma-ranges entries.
> > > 
> > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > dump of how the IB registers are initialized in both cases.
> > 
> > Happy to provide that for the m400 if told how :)
> 
> Something like the below patch. This should be with the 'dma-ranges' 
> DT change and only c7a75d07827a reverted.

https://paste.ubuntu.com/p/RHzBd5jT6v/

Note that networking does come up with this setup. That surprised me
because I thought I'd tested this combo before, but apparently what
I'd tested before was 6dce5aa59e0b reverted + the dtb change:
  https://lore.kernel.org/linux-pci/YgXG838iMrS1l8SC@xps13.dannf/

  -dann


> 8<-------------------------------------------------------------------
> diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
> index 0d5acbfc7143..6a435c31f45e 100644
> --- a/drivers/pci/controller/pci-xgene.c
> +++ b/drivers/pci/controller/pci-xgene.c
> @@ -78,6 +78,7 @@ static u32 xgene_pcie_readl(struct xgene_pcie *port, u32 reg)
>  
>  static void xgene_pcie_writel(struct xgene_pcie *port, u32 reg, u32 val)
>  {
> +	dev_info(port->dev, "0x%04x <- 0x%08x\n", reg, val);
>  	writel(val, port->csr_base + reg);
>  }
>  
> @@ -508,7 +509,9 @@ static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
>  	case 0:
>  		xgene_pcie_set_ib_mask(port, BRIDGE_CFG_4, flags, size);
>  		bar_addr = cfg_base + PCI_BASE_ADDRESS_0;
> +		dev_info(port->dev, "BAR0L <- 0x%08x\n", bar_low);
>  		writel(bar_low, bar_addr);
> +		dev_info(port->dev, "BAR0H <- 0x%08x\n", upper_32_bits(cpu_addr));
>  		writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
>  		pim_reg = PIM1_1L;
>  		break;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Mon, Mar 21, 2022 at 11:08:20AM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> > On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > unusable.
> > > >
> > > > It is understood that this systems come with "creative" DTs that don't
> > > > match the expectations of modern kernels. However, there is little to
> > > > be gained by forcing these changes on users -- the firmware is not
> > > > upgradable, and the current owner of the IP will deny that these
> > > > machines have ever existed.
> > > 
> > > The gain for fixing this properly is not having drivers do their own
> > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > parsing of standard properties (e.g. interrupt-map). Currently, we
> > > don't have any drivers doing their own parsing:
> > > 
> > > $ git grep of_pci_dma_range_parser_init
> > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > of_pci_range_parser *parser,
> > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > drivers/of/address.c:#define of_dma_range_parser_init
> > > of_pci_dma_range_parser_init
> > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > include/linux/of_address.h:extern int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > include/linux/of_address.h:static inline int
> > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > 
> > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > > 
> > > For XGene-2 the issue is simply that the driver depends on the order
> > > of dma-ranges entries.
> > > 
> > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > dump of how the IB registers are initialized in both cases.
> > 
> > Happy to provide that for the m400 if told how :)
> 
> Something like the below patch. This should be with the 'dma-ranges' 
> DT change and only c7a75d07827a reverted.

https://paste.ubuntu.com/p/RHzBd5jT6v/

Note that networking does come up with this setup. That surprised me
because I thought I'd tested this combo before, but apparently what
I'd tested before was 6dce5aa59e0b reverted + the dtb change:
  https://lore.kernel.org/linux-pci/YgXG838iMrS1l8SC@xps13.dannf/

  -dann


> 8<-------------------------------------------------------------------
> diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
> index 0d5acbfc7143..6a435c31f45e 100644
> --- a/drivers/pci/controller/pci-xgene.c
> +++ b/drivers/pci/controller/pci-xgene.c
> @@ -78,6 +78,7 @@ static u32 xgene_pcie_readl(struct xgene_pcie *port, u32 reg)
>  
>  static void xgene_pcie_writel(struct xgene_pcie *port, u32 reg, u32 val)
>  {
> +	dev_info(port->dev, "0x%04x <- 0x%08x\n", reg, val);
>  	writel(val, port->csr_base + reg);
>  }
>  
> @@ -508,7 +509,9 @@ static void xgene_pcie_setup_ib_reg(struct xgene_pcie *port,
>  	case 0:
>  		xgene_pcie_set_ib_mask(port, BRIDGE_CFG_4, flags, size);
>  		bar_addr = cfg_base + PCI_BASE_ADDRESS_0;
> +		dev_info(port->dev, "BAR0L <- 0x%08x\n", bar_low);
>  		writel(bar_low, bar_addr);
> +		dev_info(port->dev, "BAR0H <- 0x%08x\n", upper_32_bits(cpu_addr));
>  		writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
>  		pim_reg = PIM1_1L;
>  		break;

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Robin Murphy]

On 2022-03-21 20:06, Robin Murphy wrote:
> On 2022-03-21 19:21, Marc Zyngier wrote:
>> On Mon, 21 Mar 2022 18:03:27 +0000,
>> Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>
>>>> On Mon, 21 Mar 2022 15:17:34 +0000,
>>>> Rob Herring <robh@kernel.org> wrote:
>>>>>
>>>>> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>> For XGene-1, I'd still like to understand what the issue is. Reverting
>>>>> the first fix and fixing 'dma-ranges' should have fixed it. I need a
>>>>> dump of how the IB registers are initialized in both cases. I'm not
>>>>> saying changing 'dma-ranges' in the firmware is going to be required
>>>>> here. There's a couple of other ways we could fix that without a
>>>>> firmware change, but first I need to understand why it broke.
>>>>
>>>> Reverting 6dce5aa59e0b was enough for me, without changing anything
>>>> else.
>>>
>>> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
>>>
>>> Can you tell me what 'dma-ranges' contains on your system?
>>
>> Each pcie node (all 5 of them) has:
>>
>> dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
>>                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;
> 
> Hmm, is there anyone other than iommu-dma who actually depends on the 
> resource list being sorted in ascending order of bus address? I recall 
> at the time I pushed for creating the list in sorted order as it was the 
> simplest and most efficient option, but there's no technical reason we 
> couldn't create it in as-found order and defer the sorting until 
> iova_reserve_pci_windows() (at worst that could even operate on a 
> temporary copy if need be). It's just more code, which didn't need to 
> exist without a good reason, but if this is one then exist it certainly 
> may.

Taking a closer look, the Cadence driver is already re-sorting the list
for its own setup, so iommu-dma can't assume the initial sort is
preserved and needs to do its own anyway. Does the (untested) diff below
end up helping X-Gene also?

Robin.

----->8-----
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index b22034975301..8ef603c9ca3e 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -20,6 +20,7 @@
  #include <linux/iommu.h>
  #include <linux/iova.h>
  #include <linux/irq.h>
+#include <linux/list_sort.h>
  #include <linux/mm.h>
  #include <linux/mutex.h>
  #include <linux/pci.h>
@@ -414,6 +415,14 @@ static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
  	return 0;
  }
  
+static int iommu_dma_ranges_sort(void *priv, const struct list_head *a, const struct list_head *b)
+{
+	struct resource_entry *res_a = list_entry(a, typeof(*res_a), node);
+	struct resource_entry *res_b = list_entry(b, typeof(*res_b), node);
+
+	return res_a->res->start > res_b->res->start;
+}
+
  static int iova_reserve_pci_windows(struct pci_dev *dev,
  		struct iova_domain *iovad)
  {
@@ -432,6 +441,7 @@ static int iova_reserve_pci_windows(struct pci_dev *dev,
  	}
  
  	/* Get reserved DMA windows from host bridge */
+	list_sort(NULL, &bridge->dma_ranges, iommu_dma_ranges_sort);
  	resource_list_for_each_entry(window, &bridge->dma_ranges) {
  		end = window->res->start - window->offset;
  resv_iova:
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index cb2e8351c2cc..d176b4bc6193 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -393,12 +393,7 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
  			goto failed;
  		}
  
-		/* Keep the resource list sorted */
-		resource_list_for_each_entry(entry, ib_resources)
-			if (entry->res->start > res->start)
-				break;
-
-		pci_add_resource_offset(&entry->node, res,
+		pci_add_resource_offset(ib_resources, res,
  					res->start - range.pci_addr);
  	}
  

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Robin Murphy]

On 2022-03-21 20:06, Robin Murphy wrote:
> On 2022-03-21 19:21, Marc Zyngier wrote:
>> On Mon, 21 Mar 2022 18:03:27 +0000,
>> Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>
>>>> On Mon, 21 Mar 2022 15:17:34 +0000,
>>>> Rob Herring <robh@kernel.org> wrote:
>>>>>
>>>>> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>> For XGene-1, I'd still like to understand what the issue is. Reverting
>>>>> the first fix and fixing 'dma-ranges' should have fixed it. I need a
>>>>> dump of how the IB registers are initialized in both cases. I'm not
>>>>> saying changing 'dma-ranges' in the firmware is going to be required
>>>>> here. There's a couple of other ways we could fix that without a
>>>>> firmware change, but first I need to understand why it broke.
>>>>
>>>> Reverting 6dce5aa59e0b was enough for me, without changing anything
>>>> else.
>>>
>>> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
>>>
>>> Can you tell me what 'dma-ranges' contains on your system?
>>
>> Each pcie node (all 5 of them) has:
>>
>> dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
>>                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;
> 
> Hmm, is there anyone other than iommu-dma who actually depends on the 
> resource list being sorted in ascending order of bus address? I recall 
> at the time I pushed for creating the list in sorted order as it was the 
> simplest and most efficient option, but there's no technical reason we 
> couldn't create it in as-found order and defer the sorting until 
> iova_reserve_pci_windows() (at worst that could even operate on a 
> temporary copy if need be). It's just more code, which didn't need to 
> exist without a good reason, but if this is one then exist it certainly 
> may.

Taking a closer look, the Cadence driver is already re-sorting the list
for its own setup, so iommu-dma can't assume the initial sort is
preserved and needs to do its own anyway. Does the (untested) diff below
end up helping X-Gene also?

Robin.

----->8-----
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index b22034975301..8ef603c9ca3e 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -20,6 +20,7 @@
  #include <linux/iommu.h>
  #include <linux/iova.h>
  #include <linux/irq.h>
+#include <linux/list_sort.h>
  #include <linux/mm.h>
  #include <linux/mutex.h>
  #include <linux/pci.h>
@@ -414,6 +415,14 @@ static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
  	return 0;
  }
  
+static int iommu_dma_ranges_sort(void *priv, const struct list_head *a, const struct list_head *b)
+{
+	struct resource_entry *res_a = list_entry(a, typeof(*res_a), node);
+	struct resource_entry *res_b = list_entry(b, typeof(*res_b), node);
+
+	return res_a->res->start > res_b->res->start;
+}
+
  static int iova_reserve_pci_windows(struct pci_dev *dev,
  		struct iova_domain *iovad)
  {
@@ -432,6 +441,7 @@ static int iova_reserve_pci_windows(struct pci_dev *dev,
  	}
  
  	/* Get reserved DMA windows from host bridge */
+	list_sort(NULL, &bridge->dma_ranges, iommu_dma_ranges_sort);
  	resource_list_for_each_entry(window, &bridge->dma_ranges) {
  		end = window->res->start - window->offset;
  resv_iova:
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index cb2e8351c2cc..d176b4bc6193 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -393,12 +393,7 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
  			goto failed;
  		}
  
-		/* Keep the resource list sorted */
-		resource_list_for_each_entry(entry, ib_resources)
-			if (entry->res->start > res->start)
-				break;
-
-		pci_add_resource_offset(&entry->node, res,
+		pci_add_resource_offset(ib_resources, res,
  					res->start - range.pci_addr);
  	}
  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Tue, Mar 22, 2022 at 01:16:35PM +0000, Robin Murphy wrote:
> On 2022-03-21 20:06, Robin Murphy wrote:
> > On 2022-03-21 19:21, Marc Zyngier wrote:
> > > On Mon, 21 Mar 2022 18:03:27 +0000,
> > > Rob Herring <robh@kernel.org> wrote:
> > > > 
> > > > On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > 
> > > > > On Mon, 21 Mar 2022 15:17:34 +0000,
> > > > > Rob Herring <robh@kernel.org> wrote:
> > > > > > 
> > > > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > > 
> > > > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > > > dump of how the IB registers are initialized in both cases. I'm not
> > > > > > saying changing 'dma-ranges' in the firmware is going to be required
> > > > > > here. There's a couple of other ways we could fix that without a
> > > > > > firmware change, but first I need to understand why it broke.
> > > > > 
> > > > > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > > > > else.
> > > > 
> > > > Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> > > > 
> > > > Can you tell me what 'dma-ranges' contains on your system?
> > > 
> > > Each pcie node (all 5 of them) has:
> > > 
> > > dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
> > >                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;

This is the same as what Stéphane has for Merlin. So c7a75d07827a ("PCI: 
xgene: Fix IB window setup") should have fixed Mustang.

> > 
> > Hmm, is there anyone other than iommu-dma who actually depends on the
> > resource list being sorted in ascending order of bus address? I recall
> > at the time I pushed for creating the list in sorted order as it was the
> > simplest and most efficient option, but there's no technical reason we
> > couldn't create it in as-found order and defer the sorting until
> > iova_reserve_pci_windows() (at worst that could even operate on a
> > temporary copy if need be). It's just more code, which didn't need to
> > exist without a good reason, but if this is one then exist it certainly
> > may.
> 
> Taking a closer look, the Cadence driver is already re-sorting the list
> for its own setup, so iommu-dma can't assume the initial sort is
> preserved and needs to do its own anyway. Does the (untested) diff below
> end up helping X-Gene also?

There's no IOMMU on X-Gene 1 or 2 based on the upstream dts files, so 
how would this matter?

Rob

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Tue, Mar 22, 2022 at 01:16:35PM +0000, Robin Murphy wrote:
> On 2022-03-21 20:06, Robin Murphy wrote:
> > On 2022-03-21 19:21, Marc Zyngier wrote:
> > > On Mon, 21 Mar 2022 18:03:27 +0000,
> > > Rob Herring <robh@kernel.org> wrote:
> > > > 
> > > > On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > 
> > > > > On Mon, 21 Mar 2022 15:17:34 +0000,
> > > > > Rob Herring <robh@kernel.org> wrote:
> > > > > > 
> > > > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > > 
> > > > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > > > dump of how the IB registers are initialized in both cases. I'm not
> > > > > > saying changing 'dma-ranges' in the firmware is going to be required
> > > > > > here. There's a couple of other ways we could fix that without a
> > > > > > firmware change, but first I need to understand why it broke.
> > > > > 
> > > > > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > > > > else.
> > > > 
> > > > Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> > > > 
> > > > Can you tell me what 'dma-ranges' contains on your system?
> > > 
> > > Each pcie node (all 5 of them) has:
> > > 
> > > dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
> > >                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;

This is the same as what Stéphane has for Merlin. So c7a75d07827a ("PCI: 
xgene: Fix IB window setup") should have fixed Mustang.

> > 
> > Hmm, is there anyone other than iommu-dma who actually depends on the
> > resource list being sorted in ascending order of bus address? I recall
> > at the time I pushed for creating the list in sorted order as it was the
> > simplest and most efficient option, but there's no technical reason we
> > couldn't create it in as-found order and defer the sorting until
> > iova_reserve_pci_windows() (at worst that could even operate on a
> > temporary copy if need be). It's just more code, which didn't need to
> > exist without a good reason, but if this is one then exist it certainly
> > may.
> 
> Taking a closer look, the Cadence driver is already re-sorting the list
> for its own setup, so iommu-dma can't assume the initial sort is
> preserved and needs to do its own anyway. Does the (untested) diff below
> end up helping X-Gene also?

There's no IOMMU on X-Gene 1 or 2 based on the upstream dts files, so 
how would this matter?

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Robin Murphy]

On 2022-03-22 14:39, Rob Herring wrote:
> On Tue, Mar 22, 2022 at 01:16:35PM +0000, Robin Murphy wrote:
>> On 2022-03-21 20:06, Robin Murphy wrote:
>>> On 2022-03-21 19:21, Marc Zyngier wrote:
>>>> On Mon, 21 Mar 2022 18:03:27 +0000,
>>>> Rob Herring <robh@kernel.org> wrote:
>>>>>
>>>>> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>>> On Mon, 21 Mar 2022 15:17:34 +0000,
>>>>>> Rob Herring <robh@kernel.org> wrote:
>>>>>>>
>>>>>>> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>>>
>>>>>>> For XGene-1, I'd still like to understand what the issue is. Reverting
>>>>>>> the first fix and fixing 'dma-ranges' should have fixed it. I need a
>>>>>>> dump of how the IB registers are initialized in both cases. I'm not
>>>>>>> saying changing 'dma-ranges' in the firmware is going to be required
>>>>>>> here. There's a couple of other ways we could fix that without a
>>>>>>> firmware change, but first I need to understand why it broke.
>>>>>>
>>>>>> Reverting 6dce5aa59e0b was enough for me, without changing anything
>>>>>> else.
>>>>>
>>>>> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
>>>>>
>>>>> Can you tell me what 'dma-ranges' contains on your system?
>>>>
>>>> Each pcie node (all 5 of them) has:
>>>>
>>>> dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
>>>> �������������� 0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;
> 
> This is the same as what St�phane has for Merlin. So c7a75d07827a ("PCI:
> xgene: Fix IB window setup") should have fixed Mustang.

Unless XGene 1 has some weird implicit requirement on the order in which 
the registers are programmed, that XGene 2 doesn't. And from looking at 
the code, I don't see any obvious less-mad possibility to explain the 
breakage.

>>> Hmm, is there anyone other than iommu-dma who actually depends on the
>>> resource list being sorted in ascending order of bus address? I recall
>>> at the time I pushed for creating the list in sorted order as it was the
>>> simplest and most efficient option, but there's no technical reason we
>>> couldn't create it in as-found order and defer the sorting until
>>> iova_reserve_pci_windows() (at worst that could even operate on a
>>> temporary copy if need be). It's just more code, which didn't need to
>>> exist without a good reason, but if this is one then exist it certainly
>>> may.
>>
>> Taking a closer look, the Cadence driver is already re-sorting the list
>> for its own setup, so iommu-dma can't assume the initial sort is
>> preserved and needs to do its own anyway. Does the (untested) diff below
>> end up helping X-Gene also?
> 
> There's no IOMMU on X-Gene 1 or 2 based on the upstream dts files, so
> how would this matter?

Because devm_of_pci_get_host_bridge_resources() is forcing the 
dma_ranges list to be in a different order from the original DT for 
iommu-dma's benefit, but whether iommu-dma actually consumes it or not 
later is immaterial.

Robin.

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Robin Murphy]

On 2022-03-22 14:39, Rob Herring wrote:
> On Tue, Mar 22, 2022 at 01:16:35PM +0000, Robin Murphy wrote:
>> On 2022-03-21 20:06, Robin Murphy wrote:
>>> On 2022-03-21 19:21, Marc Zyngier wrote:
>>>> On Mon, 21 Mar 2022 18:03:27 +0000,
>>>> Rob Herring <robh@kernel.org> wrote:
>>>>>
>>>>> On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>>> On Mon, 21 Mar 2022 15:17:34 +0000,
>>>>>> Rob Herring <robh@kernel.org> wrote:
>>>>>>>
>>>>>>> On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>>>
>>>>>>> For XGene-1, I'd still like to understand what the issue is. Reverting
>>>>>>> the first fix and fixing 'dma-ranges' should have fixed it. I need a
>>>>>>> dump of how the IB registers are initialized in both cases. I'm not
>>>>>>> saying changing 'dma-ranges' in the firmware is going to be required
>>>>>>> here. There's a couple of other ways we could fix that without a
>>>>>>> firmware change, but first I need to understand why it broke.
>>>>>>
>>>>>> Reverting 6dce5aa59e0b was enough for me, without changing anything
>>>>>> else.
>>>>>
>>>>> Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
>>>>>
>>>>> Can you tell me what 'dma-ranges' contains on your system?
>>>>
>>>> Each pcie node (all 5 of them) has:
>>>>
>>>> dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
>>>> �������������� 0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;
> 
> This is the same as what St�phane has for Merlin. So c7a75d07827a ("PCI:
> xgene: Fix IB window setup") should have fixed Mustang.

Unless XGene 1 has some weird implicit requirement on the order in which 
the registers are programmed, that XGene 2 doesn't. And from looking at 
the code, I don't see any obvious less-mad possibility to explain the 
breakage.

>>> Hmm, is there anyone other than iommu-dma who actually depends on the
>>> resource list being sorted in ascending order of bus address? I recall
>>> at the time I pushed for creating the list in sorted order as it was the
>>> simplest and most efficient option, but there's no technical reason we
>>> couldn't create it in as-found order and defer the sorting until
>>> iova_reserve_pci_windows() (at worst that could even operate on a
>>> temporary copy if need be). It's just more code, which didn't need to
>>> exist without a good reason, but if this is one then exist it certainly
>>> may.
>>
>> Taking a closer look, the Cadence driver is already re-sorting the list
>> for its own setup, so iommu-dma can't assume the initial sort is
>> preserved and needs to do its own anyway. Does the (untested) diff below
>> end up helping X-Gene also?
> 
> There's no IOMMU on X-Gene 1 or 2 based on the upstream dts files, so
> how would this matter?

Because devm_of_pci_get_host_bridge_resources() is forcing the 
dma_ranges list to be in a different order from the original DT for 
iommu-dma's benefit, but whether iommu-dma actually consumes it or not 
later is immaterial.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

On Tue, 22 Mar 2022 14:39:43 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Tue, Mar 22, 2022 at 01:16:35PM +0000, Robin Murphy wrote:
> > On 2022-03-21 20:06, Robin Murphy wrote:
> > > On 2022-03-21 19:21, Marc Zyngier wrote:
> > > > On Mon, 21 Mar 2022 18:03:27 +0000,
> > > > Rob Herring <robh@kernel.org> wrote:
> > > > > 
> > > > > On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > 
> > > > > > On Mon, 21 Mar 2022 15:17:34 +0000,
> > > > > > Rob Herring <robh@kernel.org> wrote:
> > > > > > > 
> > > > > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > > > 
> > > > > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > > > > dump of how the IB registers are initialized in both cases. I'm not
> > > > > > > saying changing 'dma-ranges' in the firmware is going to be required
> > > > > > > here. There's a couple of other ways we could fix that without a
> > > > > > > firmware change, but first I need to understand why it broke.
> > > > > > 
> > > > > > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > > > > > else.
> > > > > 
> > > > > Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> > > > > 
> > > > > Can you tell me what 'dma-ranges' contains on your system?
> > > > 
> > > > Each pcie node (all 5 of them) has:
> > > > 
> > > > dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
> > > >                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;
> 
> This is the same as what Stéphane has for Merlin. So c7a75d07827a ("PCI: 
> xgene: Fix IB window setup") should have fixed Mustang.

Should, but didn't. The DT also carries additional properties:

ib-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00
             0x00 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;

which the driver ignores, but that could be relevant. FWIW, I've
stashed the DT at [1].

	M.

[1] http://www.loen.fr/tmp/mustang.dts

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

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Marc Zyngier]

On Tue, 22 Mar 2022 14:39:43 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Tue, Mar 22, 2022 at 01:16:35PM +0000, Robin Murphy wrote:
> > On 2022-03-21 20:06, Robin Murphy wrote:
> > > On 2022-03-21 19:21, Marc Zyngier wrote:
> > > > On Mon, 21 Mar 2022 18:03:27 +0000,
> > > > Rob Herring <robh@kernel.org> wrote:
> > > > > 
> > > > > On Mon, Mar 21, 2022 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > 
> > > > > > On Mon, 21 Mar 2022 15:17:34 +0000,
> > > > > > Rob Herring <robh@kernel.org> wrote:
> > > > > > > 
> > > > > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > > > 
> > > > > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > > > > dump of how the IB registers are initialized in both cases. I'm not
> > > > > > > saying changing 'dma-ranges' in the firmware is going to be required
> > > > > > > here. There's a couple of other ways we could fix that without a
> > > > > > > firmware change, but first I need to understand why it broke.
> > > > > > 
> > > > > > Reverting 6dce5aa59e0b was enough for me, without changing anything
> > > > > > else.
> > > > > 
> > > > > Meaning c7a75d07827a didn't matter for you. I'm not sure that it would.
> > > > > 
> > > > > Can you tell me what 'dma-ranges' contains on your system?
> > > > 
> > > > Each pcie node (all 5 of them) has:
> > > > 
> > > > dma-ranges = <0x42000000 0x80 0x00 0x80 0x00 0x00 0x80000000
> > > >                0x42000000 0x00 0x00 0x00 0x00 0x80 0x00>;
> 
> This is the same as what Stéphane has for Merlin. So c7a75d07827a ("PCI: 
> xgene: Fix IB window setup") should have fixed Mustang.

Should, but didn't. The DT also carries additional properties:

ib-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00
             0x00 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;

which the driver ignores, but that could be relevant. FWIW, I've
stashed the DT at [1].

	M.

[1] http://www.loen.fr/tmp/mustang.dts

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 04:32:40PM -0600, dann frazier wrote:
> On Mon, Mar 21, 2022 at 11:08:20AM -0500, Rob Herring wrote:
> > On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> > > On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > > unusable.
> > > > >
> > > > > It is understood that this systems come with "creative" DTs that don't
> > > > > match the expectations of modern kernels. However, there is little to
> > > > > be gained by forcing these changes on users -- the firmware is not
> > > > > upgradable, and the current owner of the IP will deny that these
> > > > > machines have ever existed.
> > > > 
> > > > The gain for fixing this properly is not having drivers do their own
> > > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > > parsing of standard properties (e.g. interrupt-map). Currently, we
> > > > don't have any drivers doing their own parsing:
> > > > 
> > > > $ git grep of_pci_dma_range_parser_init
> > > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > > of_pci_range_parser *parser,
> > > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > > drivers/of/address.c:#define of_dma_range_parser_init
> > > > of_pci_dma_range_parser_init
> > > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > > include/linux/of_address.h:extern int
> > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > include/linux/of_address.h:static inline int
> > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > 
> > > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > > > 
> > > > For XGene-2 the issue is simply that the driver depends on the order
> > > > of dma-ranges entries.
> > > > 
> > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > dump of how the IB registers are initialized in both cases.
> > > 
> > > Happy to provide that for the m400 if told how :)
> > 
> > Something like the below patch. This should be with the 'dma-ranges' 
> > DT change and only c7a75d07827a reverted.
> 
> https://paste.ubuntu.com/p/RHzBd5jT6v/
> 
> Note that networking does come up with this setup. That surprised me
> because I thought I'd tested this combo before, but apparently what
> I'd tested before was 6dce5aa59e0b reverted + the dtb change:
>   https://lore.kernel.org/linux-pci/YgXG838iMrS1l8SC@xps13.dannf/

That doesn't make sense. I just noticed there's an error in what I 
told you to do for dma-ranges. I fixed the wrong cell as it should be:

- dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x00 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;
+ dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x42000000 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;

Rob

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Rob Herring]

On Mon, Mar 21, 2022 at 04:32:40PM -0600, dann frazier wrote:
> On Mon, Mar 21, 2022 at 11:08:20AM -0500, Rob Herring wrote:
> > On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> > > On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > > unusable.
> > > > >
> > > > > It is understood that this systems come with "creative" DTs that don't
> > > > > match the expectations of modern kernels. However, there is little to
> > > > > be gained by forcing these changes on users -- the firmware is not
> > > > > upgradable, and the current owner of the IP will deny that these
> > > > > machines have ever existed.
> > > > 
> > > > The gain for fixing this properly is not having drivers do their own
> > > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > > parsing of standard properties (e.g. interrupt-map). Currently, we
> > > > don't have any drivers doing their own parsing:
> > > > 
> > > > $ git grep of_pci_dma_range_parser_init
> > > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > > of_pci_range_parser *parser,
> > > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > > drivers/of/address.c:#define of_dma_range_parser_init
> > > > of_pci_dma_range_parser_init
> > > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > > include/linux/of_address.h:extern int
> > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > include/linux/of_address.h:static inline int
> > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > 
> > > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > > > 
> > > > For XGene-2 the issue is simply that the driver depends on the order
> > > > of dma-ranges entries.
> > > > 
> > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > dump of how the IB registers are initialized in both cases.
> > > 
> > > Happy to provide that for the m400 if told how :)
> > 
> > Something like the below patch. This should be with the 'dma-ranges' 
> > DT change and only c7a75d07827a reverted.
> 
> https://paste.ubuntu.com/p/RHzBd5jT6v/
> 
> Note that networking does come up with this setup. That surprised me
> because I thought I'd tested this combo before, but apparently what
> I'd tested before was 6dce5aa59e0b reverted + the dtb change:
>   https://lore.kernel.org/linux-pci/YgXG838iMrS1l8SC@xps13.dannf/

That doesn't make sense. I just noticed there's an error in what I 
told you to do for dma-ranges. I fixed the wrong cell as it should be:

- dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x00 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;
+ dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x42000000 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Tue, Mar 22, 2022 at 04:00:22PM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 04:32:40PM -0600, dann frazier wrote:
> > On Mon, Mar 21, 2022 at 11:08:20AM -0500, Rob Herring wrote:
> > > On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> > > > On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > >
> > > > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > > > unusable.
> > > > > >
> > > > > > It is understood that this systems come with "creative" DTs that don't
> > > > > > match the expectations of modern kernels. However, there is little to
> > > > > > be gained by forcing these changes on users -- the firmware is not
> > > > > > upgradable, and the current owner of the IP will deny that these
> > > > > > machines have ever existed.
> > > > > 
> > > > > The gain for fixing this properly is not having drivers do their own
> > > > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > > > parsing of standard properties (e.g. interrupt-map). Currently, we
> > > > > don't have any drivers doing their own parsing:
> > > > > 
> > > > > $ git grep of_pci_dma_range_parser_init
> > > > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > > > of_pci_range_parser *parser,
> > > > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > > > drivers/of/address.c:#define of_dma_range_parser_init
> > > > > of_pci_dma_range_parser_init
> > > > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > > > include/linux/of_address.h:extern int
> > > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > > include/linux/of_address.h:static inline int
> > > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > > 
> > > > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > > > > 
> > > > > For XGene-2 the issue is simply that the driver depends on the order
> > > > > of dma-ranges entries.
> > > > > 
> > > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > > dump of how the IB registers are initialized in both cases.
> > > > 
> > > > Happy to provide that for the m400 if told how :)
> > > 
> > > Something like the below patch. This should be with the 'dma-ranges' 
> > > DT change and only c7a75d07827a reverted.
> > 
> > https://paste.ubuntu.com/p/RHzBd5jT6v/
> > 
> > Note that networking does come up with this setup. That surprised me
> > because I thought I'd tested this combo before, but apparently what
> > I'd tested before was 6dce5aa59e0b reverted + the dtb change:
> >   https://lore.kernel.org/linux-pci/YgXG838iMrS1l8SC@xps13.dannf/
> 
> That doesn't make sense. I just noticed there's an error in what I 
> told you to do for dma-ranges. I fixed the wrong cell as it should be:
> 
> - dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x00 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;
> + dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x42000000 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;

OK, thanks Rob. Here's an updated log:

  https://paste.ubuntu.com/p/FmSTbM6Zq3/

  -dann

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[dann frazier]

On Tue, Mar 22, 2022 at 04:00:22PM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 04:32:40PM -0600, dann frazier wrote:
> > On Mon, Mar 21, 2022 at 11:08:20AM -0500, Rob Herring wrote:
> > > On Mon, Mar 21, 2022 at 09:50:24AM -0600, dann frazier wrote:
> > > > On Mon, Mar 21, 2022 at 10:17:34AM -0500, Rob Herring wrote:
> > > > > On Mon, Mar 21, 2022 at 5:49 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > >
> > > > > > Since 6dce5aa59e0b ("PCI: xgene: Use inbound resources for setup") was
> > > > > > merged in the 5.5 time frame, PCIe on the venerable XGene platform has
> > > > > > been unusable: 6dce5aa59e0b broke both XGene-1 (Mustang and m400) and
> > > > > > XGene-2 (Merlin), while the addition of c7a75d07827a ("PCI: xgene: Fix
> > > > > > IB window setup") fixed XGene-2, but left the rest of the zoo
> > > > > > unusable.
> > > > > >
> > > > > > It is understood that this systems come with "creative" DTs that don't
> > > > > > match the expectations of modern kernels. However, there is little to
> > > > > > be gained by forcing these changes on users -- the firmware is not
> > > > > > upgradable, and the current owner of the IP will deny that these
> > > > > > machines have ever existed.
> > > > > 
> > > > > The gain for fixing this properly is not having drivers do their own
> > > > > dma-ranges parsing. We've seen what happens when drivers do their own
> > > > > parsing of standard properties (e.g. interrupt-map). Currently, we
> > > > > don't have any drivers doing their own parsing:
> > > > > 
> > > > > $ git grep of_pci_dma_range_parser_init
> > > > > drivers/of/address.c:int of_pci_dma_range_parser_init(struct
> > > > > of_pci_range_parser *parser,
> > > > > drivers/of/address.c:EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> > > > > drivers/of/address.c:#define of_dma_range_parser_init
> > > > > of_pci_dma_range_parser_init
> > > > > drivers/of/unittest.c:  if (of_pci_dma_range_parser_init(&parser, np)) {
> > > > > drivers/pci/of.c:       err = of_pci_dma_range_parser_init(&parser, dev_node);
> > > > > include/linux/of_address.h:extern int
> > > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > > include/linux/of_address.h:static inline int
> > > > > of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> > > > > 
> > > > > And we can probably further refactor this to be private to drivers/pci/of.c.
> > > > > 
> > > > > For XGene-2 the issue is simply that the driver depends on the order
> > > > > of dma-ranges entries.
> > > > > 
> > > > > For XGene-1, I'd still like to understand what the issue is. Reverting
> > > > > the first fix and fixing 'dma-ranges' should have fixed it. I need a
> > > > > dump of how the IB registers are initialized in both cases.
> > > > 
> > > > Happy to provide that for the m400 if told how :)
> > > 
> > > Something like the below patch. This should be with the 'dma-ranges' 
> > > DT change and only c7a75d07827a reverted.
> > 
> > https://paste.ubuntu.com/p/RHzBd5jT6v/
> > 
> > Note that networking does come up with this setup. That surprised me
> > because I thought I'd tested this combo before, but apparently what
> > I'd tested before was 6dce5aa59e0b reverted + the dtb change:
> >   https://lore.kernel.org/linux-pci/YgXG838iMrS1l8SC@xps13.dannf/
> 
> That doesn't make sense. I just noticed there's an error in what I 
> told you to do for dma-ranges. I fixed the wrong cell as it should be:
> 
> - dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x00 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;
> + dma-ranges = <0x42000000 0x40 0x00 0x40 0x00 0x40 0x00 0x42000000 0x00 0x79000000 0x00 0x79000000 0x00 0x800000>;

OK, thanks Rob. Here's an updated log:

  https://paste.ubuntu.com/p/FmSTbM6Zq3/

  -dann

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Ard Biesheuvel]

On Mon, 21 Mar 2022 at 19:04, Rob Herring <robh@kernel.org> wrote:
>
...
>
> Do the DT's in the kernel tree correspond to anything anyone is using?
> I ask because at some point someone will need to address all the
> warnings they have or we should drop the dts files if they aren't
> close to reality. The same thing applies to Seattle BTW.
>

I sent these a while ago to sync the Seattle kernel DT with the
version that is in the Tianocore tree, and is built into the open
version of the Seattle firmware

https://lore.kernel.org/all/20191203152306.7839-1-ardb@kernel.org/

I wouldn't mind dropping them entirely, btw.

Re: [PATCH v2 0/2] PCI: xgene: Restore working PCIe functionnality

[Ard Biesheuvel]

On Mon, 21 Mar 2022 at 19:04, Rob Herring <robh@kernel.org> wrote:
>
...
>
> Do the DT's in the kernel tree correspond to anything anyone is using?
> I ask because at some point someone will need to address all the
> warnings they have or we should drop the dts files if they aren't
> close to reality. The same thing applies to Seattle BTW.
>

I sent these a while ago to sync the Seattle kernel DT with the
version that is in the Tianocore tree, and is built into the open
version of the Seattle firmware

https://lore.kernel.org/all/20191203152306.7839-1-ardb@kernel.org/

I wouldn't mind dropping them entirely, btw.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel