[PATCH 0/3] A minor DS414 update

[PATCH 0/3] A minor DS414 update

[Phil Sutter]

In another attempt at making the two rear USB ports usable I spent a few
free days to play around with PCI and XHCI. The USB controller is
detected and "seems" to work, but only as long as no device is plugged
in. If so, slot enable (TRB_ENABLE_SLOT) command times out and I can't
find out why.

This series collects the brighter bits: Apart from a rather unrelated
review of include/configs/ds414.h in patch one, patches two and three at
least fix for the error messages emitted by 'pci enum' trying to PNP
devices.

Phil Sutter (3):
  arm: mvebu: ds414: Config header mini-review
  pci: Make auto-config code a little more robust
  pci: pci_mvebu: Define an IO region as well

 drivers/pci/pci_auto.c  |  9 +++++----
 drivers/pci/pci_mvebu.c | 28 +++++++++++++++++++++++++++-
 include/configs/ds414.h | 21 +++------------------
 3 files changed, 35 insertions(+), 23 deletions(-)

-- 
2.30.0

[PATCH 1/3] arm: mvebu: ds414: Config header mini-review

[Phil Sutter]

A few minor changes:

* Get rid of leftover comments, other commits removed the defines they
  referred to.

* CONFIG_SYS_NETA_INTERFACE_TYPE is not used anymore since commit
  e3b9c98a23ca9 ("net: mvneta: Convert to driver model").

* Drop CONFIG_USB_MAX_CONTROLLER_COUNT: it is per-HCI type, so XHCI and
  EHCI could still both work be used.

* Unconditionally define CONFIG_EHCI_IS_TDI: it has no effect on XHCI so
  that conditional doesn't make any sense.

* Define a larger PHY_ANEG_TIMEOUT: In my test bed, the NIC is directly
  connected to some RTL8111 and the default 8s timeout was often too
  short.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/configs/ds414.h | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/include/configs/ds414.h b/include/configs/ds414.h
index a92e788a38b9e..8aa2d47bec68d 100644
--- a/include/configs/ds414.h
+++ b/include/configs/ds414.h
@@ -24,31 +24,13 @@
 #define CONFIG_SYS_I2C_SLAVE		0x0
 #define CONFIG_SYS_I2C_SPEED		100000
 
-/* Environment in SPI NOR flash */
-
-#define CONFIG_SYS_NETA_INTERFACE_TYPE	PHY_INTERFACE_MODE_RGMII
-
 /* PCIe support */
 #ifndef CONFIG_SPL_BUILD
 #define CONFIG_PCI_SCAN_SHOW
 #endif
 
 /* USB/EHCI/XHCI configuration */
-
-#define CONFIG_USB_MAX_CONTROLLER_COUNT 2
-
-/* FIXME: broken XHCI support
- * Below defines should enable support for the two rear USB3 ports. Sadly, this
- * does not work because:
- * - xhci-pci seems to not support DM_USB, so with that enabled it is not
- *   found.
- * - USB init fails, controller does not respond in time */
-
-#if !defined(CONFIG_USB_XHCI_HCD)
 #define CONFIG_EHCI_IS_TDI
-#endif
-
-/* why is this only defined in mv-common.h if CONFIG_DM is undefined? */
 
 /*
  * mv-common.h should be defined after CMD configs since it used them
@@ -95,4 +77,7 @@
 #define CONFIG_BOOTCOMMAND	"sf read ${loadaddr} 0xd0000 0x700000; bootm"
 #define CONFIG_LOADADDR		0x80000
 
+/* increase autoneg timeout, my NIC sucks */
+#define PHY_ANEG_TIMEOUT	16000
+
 #endif /* _CONFIG_SYNOLOGY_DS414_H */
-- 
2.30.0

[PATCH 1/3] arm: mvebu: ds414: Config header mini-review

[Stefan Roese]

On 03.01.21 23:06, Phil Sutter wrote:
> A few minor changes:
> 
> * Get rid of leftover comments, other commits removed the defines they
>    referred to.
> 
> * CONFIG_SYS_NETA_INTERFACE_TYPE is not used anymore since commit
>    e3b9c98a23ca9 ("net: mvneta: Convert to driver model").
> 
> * Drop CONFIG_USB_MAX_CONTROLLER_COUNT: it is per-HCI type, so XHCI and
>    EHCI could still both work be used.
> 
> * Unconditionally define CONFIG_EHCI_IS_TDI: it has no effect on XHCI so
>    that conditional doesn't make any sense.
> 
> * Define a larger PHY_ANEG_TIMEOUT: In my test bed, the NIC is directly
>    connected to some RTL8111 and the default 8s timeout was often too
>    short.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   include/configs/ds414.h | 21 +++------------------
>   1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/include/configs/ds414.h b/include/configs/ds414.h
> index a92e788a38b9e..8aa2d47bec68d 100644
> --- a/include/configs/ds414.h
> +++ b/include/configs/ds414.h
> @@ -24,31 +24,13 @@
>   #define CONFIG_SYS_I2C_SLAVE		0x0
>   #define CONFIG_SYS_I2C_SPEED		100000
>   
> -/* Environment in SPI NOR flash */
> -
> -#define CONFIG_SYS_NETA_INTERFACE_TYPE	PHY_INTERFACE_MODE_RGMII
> -
>   /* PCIe support */
>   #ifndef CONFIG_SPL_BUILD
>   #define CONFIG_PCI_SCAN_SHOW
>   #endif
>   
>   /* USB/EHCI/XHCI configuration */
> -
> -#define CONFIG_USB_MAX_CONTROLLER_COUNT 2
> -
> -/* FIXME: broken XHCI support
> - * Below defines should enable support for the two rear USB3 ports. Sadly, this
> - * does not work because:
> - * - xhci-pci seems to not support DM_USB, so with that enabled it is not
> - *   found.
> - * - USB init fails, controller does not respond in time */
> -
> -#if !defined(CONFIG_USB_XHCI_HCD)
>   #define CONFIG_EHCI_IS_TDI
> -#endif
> -
> -/* why is this only defined in mv-common.h if CONFIG_DM is undefined? */
>   
>   /*
>    * mv-common.h should be defined after CMD configs since it used them
> @@ -95,4 +77,7 @@
>   #define CONFIG_BOOTCOMMAND	"sf read ${loadaddr} 0xd0000 0x700000; bootm"
>   #define CONFIG_LOADADDR		0x80000
>   
> +/* increase autoneg timeout, my NIC sucks */
> +#define PHY_ANEG_TIMEOUT	16000
> +
>   #endif /* _CONFIG_SYNOLOGY_DS414_H */
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH 2/3] pci: Make auto-config code a little more robust

[Phil Sutter]

On my DS414, some PCI devices return odd values when probing BAR sizes.
An obvious case is all-ones response, the Linux driver
(drivers/pci/probe.c) catches those explicitly and a comment explains
that either bit 0 or bit 1 must be clear (depending on MEM or IO type).
Other BARs return e.g. 0xfff0000f or 0xfff00004 and thus manage to break
size calculation due to the "middle" zeroes. Mitigate that copying more
or less what Linux does and do a "find least bit set".

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 drivers/pci/pci_auto.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
index 3f46b7697d7ca..ea202b0e0959e 100644
--- a/drivers/pci/pci_auto.c
+++ b/drivers/pci/pci_auto.c
@@ -47,16 +47,17 @@ void dm_pciauto_setup_device(struct udevice *dev, int bars_num,
 			dm_pci_write_config32(dev, bar, 0xffffffff);
 		dm_pci_read_config32(dev, bar, &bar_response);
 
-		/* If BAR is not implemented go to the next BAR */
-		if (!bar_response)
+		/* If BAR is not implemented (or invalid) go to the next BAR */
+		if (!bar_response || bar_response == 0xffffffff)
 			continue;
 
 		found_mem64 = 0;
 
 		/* Check the BAR type and set our address mask */
 		if (bar_response & PCI_BASE_ADDRESS_SPACE) {
-			bar_size = ((~(bar_response & PCI_BASE_ADDRESS_IO_MASK))
-				   & 0xffff) + 1;
+			bar_size = bar_response & PCI_BASE_ADDRESS_IO_MASK;
+			bar_size &= ~(bar_size - 1);
+
 			if (!enum_only)
 				bar_res = io;
 
-- 
2.30.0

[PATCH 2/3] pci: Make auto-config code a little more robust

[Stefan Roese]

Adding a few more PCI experts (Simon & Bin) to Cc.

On 03.01.21 23:06, Phil Sutter wrote:
> On my DS414, some PCI devices return odd values when probing BAR sizes.
> An obvious case is all-ones response, the Linux driver
> (drivers/pci/probe.c) catches those explicitly and a comment explains
> that either bit 0 or bit 1 must be clear (depending on MEM or IO type).
> Other BARs return e.g. 0xfff0000f or 0xfff00004 and thus manage to break
> size calculation due to the "middle" zeroes. Mitigate that copying more
> or less what Linux does and do a "find least bit set".
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>   drivers/pci/pci_auto.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
> index 3f46b7697d7ca..ea202b0e0959e 100644
> --- a/drivers/pci/pci_auto.c
> +++ b/drivers/pci/pci_auto.c
> @@ -47,16 +47,17 @@ void dm_pciauto_setup_device(struct udevice *dev, int bars_num,
>   			dm_pci_write_config32(dev, bar, 0xffffffff);
>   		dm_pci_read_config32(dev, bar, &bar_response);
>   
> -		/* If BAR is not implemented go to the next BAR */
> -		if (!bar_response)
> +		/* If BAR is not implemented (or invalid) go to the next BAR */
> +		if (!bar_response || bar_response == 0xffffffff)
>   			continue;
>   
>   		found_mem64 = 0;
>   
>   		/* Check the BAR type and set our address mask */
>   		if (bar_response & PCI_BASE_ADDRESS_SPACE) {
> -			bar_size = ((~(bar_response & PCI_BASE_ADDRESS_IO_MASK))
> -				   & 0xffff) + 1;
> +			bar_size = bar_response & PCI_BASE_ADDRESS_IO_MASK;
> +			bar_size &= ~(bar_size - 1);
> +
>   			if (!enum_only)
>   				bar_res = io;
>   

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[PATCH 2/3] pci: Make auto-config code a little more robust

[Simon Glass]

On Sun, 3 Jan 2021 at 15:05, Phil Sutter <phil@nwl.cc> wrote:
>
> On my DS414, some PCI devices return odd values when probing BAR sizes.
> An obvious case is all-ones response, the Linux driver
> (drivers/pci/probe.c) catches those explicitly and a comment explains
> that either bit 0 or bit 1 must be clear (depending on MEM or IO type).
> Other BARs return e.g. 0xfff0000f or 0xfff00004 and thus manage to break
> size calculation due to the "middle" zeroes. Mitigate that copying more
> or less what Linux does and do a "find least bit set".
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  drivers/pci/pci_auto.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/3] pci: pci_mvebu: Define an IO region as well

[Phil Sutter]

Configure an IO region and window for PNP identical to how MEM region is
set up. Linux does this only if the DT defines a pcie-io-aperture
property for the SOC, but since all supported boards do this should not
be needed.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 drivers/pci/pci_mvebu.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index c9afe7515014d..9528e7c40ecfe 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -73,6 +73,7 @@ struct mvebu_pcie {
 	void __iomem *membase;
 	struct resource mem;
 	void __iomem *iobase;
+	struct resource io;
 	u32 port;
 	u32 lane;
 	int devfn;
@@ -81,6 +82,8 @@ struct mvebu_pcie {
 	char name[16];
 	unsigned int mem_target;
 	unsigned int mem_attr;
+	unsigned int io_target;
+	unsigned int io_attr;
 };
 
 /*
@@ -90,6 +93,7 @@ struct mvebu_pcie {
  */
 static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE;
 #define PCIE_MEM_SIZE	(128 << 20)
+static void __iomem *mvebu_pcie_iobase = (void __iomem *)MBUS_PCI_IO_BASE;
 
 static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
 {
@@ -306,12 +310,24 @@ static int mvebu_pcie_probe(struct udevice *dev)
 		       (u32)pcie->mem.start, PCIE_MEM_SIZE);
 	}
 
+	pcie->io.start = (u32)mvebu_pcie_iobase;
+	pcie->io.end = pcie->io.start + MBUS_PCI_IO_SIZE - 1;
+	mvebu_pcie_iobase += MBUS_PCI_IO_SIZE;
+
+	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
+					(phys_addr_t)pcie->io.start,
+					MBUS_PCI_IO_SIZE)) {
+		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
+		       (u32)pcie->io.start, MBUS_PCI_IO_SIZE);
+	}
+
 	/* Setup windows and configure host bridge */
 	mvebu_pcie_setup_wins(pcie);
 
 	/* Master + slave enable. */
 	reg = readl(pcie->base + PCIE_CMD_OFF);
 	reg |= PCI_COMMAND_MEMORY;
+	reg |= PCI_COMMAND_IO;
 	reg |= PCI_COMMAND_MASTER;
 	reg |= BIT(10);		/* disable interrupts */
 	writel(reg, pcie->base + PCIE_CMD_OFF);
@@ -323,7 +339,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
 		       0, 0,
 		       gd->ram_size,
 		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
-	hose->region_count = 2;
+	pci_set_region(hose->regions + 2, pcie->io.start,
+		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
+	hose->region_count = 3;
 
 	/* Set BAR0 to internal registers */
 	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
@@ -442,6 +460,14 @@ static int mvebu_pcie_ofdata_to_platdata(struct udevice *dev)
 		goto err;
 	}
 
+	ret = mvebu_get_tgt_attr(dev_ofnode(dev->parent), pcie->devfn,
+				 IORESOURCE_IO,
+				 &pcie->io_target, &pcie->io_attr);
+	if (ret < 0) {
+		printf("%s: cannot get tgt/attr for IO window\n", pcie->name);
+		goto err;
+	}
+
 	/* Parse PCIe controller register base from DT */
 	ret = mvebu_pcie_port_parse_dt(dev_ofnode(dev), pcie);
 	if (ret < 0)
-- 
2.30.0

[PATCH 3/3] pci: pci_mvebu: Define an IO region as well

[Stefan Roese]

On 03.01.21 23:06, Phil Sutter wrote:
> Configure an IO region and window for PNP identical to how MEM region is
> set up. Linux does this only if the DT defines a pcie-io-aperture
> property for the SOC, but since all supported boards do this should not
> be needed.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>   drivers/pci/pci_mvebu.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index c9afe7515014d..9528e7c40ecfe 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -73,6 +73,7 @@ struct mvebu_pcie {
>   	void __iomem *membase;
>   	struct resource mem;
>   	void __iomem *iobase;
> +	struct resource io;
>   	u32 port;
>   	u32 lane;
>   	int devfn;
> @@ -81,6 +82,8 @@ struct mvebu_pcie {
>   	char name[16];
>   	unsigned int mem_target;
>   	unsigned int mem_attr;
> +	unsigned int io_target;
> +	unsigned int io_attr;
>   };
>   
>   /*
> @@ -90,6 +93,7 @@ struct mvebu_pcie {
>    */
>   static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE;
>   #define PCIE_MEM_SIZE	(128 << 20)
> +static void __iomem *mvebu_pcie_iobase = (void __iomem *)MBUS_PCI_IO_BASE;
>   
>   static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
>   {
> @@ -306,12 +310,24 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   		       (u32)pcie->mem.start, PCIE_MEM_SIZE);
>   	}
>   
> +	pcie->io.start = (u32)mvebu_pcie_iobase;
> +	pcie->io.end = pcie->io.start + MBUS_PCI_IO_SIZE - 1;
> +	mvebu_pcie_iobase += MBUS_PCI_IO_SIZE;
> +
> +	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
> +					(phys_addr_t)pcie->io.start,
> +					MBUS_PCI_IO_SIZE)) {
> +		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
> +		       (u32)pcie->io.start, MBUS_PCI_IO_SIZE);
> +	}
> +
>   	/* Setup windows and configure host bridge */
>   	mvebu_pcie_setup_wins(pcie);
>   
>   	/* Master + slave enable. */
>   	reg = readl(pcie->base + PCIE_CMD_OFF);
>   	reg |= PCI_COMMAND_MEMORY;
> +	reg |= PCI_COMMAND_IO;
>   	reg |= PCI_COMMAND_MASTER;
>   	reg |= BIT(10);		/* disable interrupts */
>   	writel(reg, pcie->base + PCIE_CMD_OFF);
> @@ -323,7 +339,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   		       0, 0,
>   		       gd->ram_size,
>   		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
> -	hose->region_count = 2;
> +	pci_set_region(hose->regions + 2, pcie->io.start,
> +		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
> +	hose->region_count = 3;
>   
>   	/* Set BAR0 to internal registers */
>   	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
> @@ -442,6 +460,14 @@ static int mvebu_pcie_ofdata_to_platdata(struct udevice *dev)
>   		goto err;
>   	}
>   
> +	ret = mvebu_get_tgt_attr(dev_ofnode(dev->parent), pcie->devfn,
> +				 IORESOURCE_IO,
> +				 &pcie->io_target, &pcie->io_attr);
> +	if (ret < 0) {
> +		printf("%s: cannot get tgt/attr for IO window\n", pcie->name);
> +		goto err;
> +	}
> +
>   	/* Parse PCIe controller register base from DT */
>   	ret = mvebu_pcie_port_parse_dt(dev_ofnode(dev), pcie);
>   	if (ret < 0)
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan