[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[Marek Behún]

The local device (host "bridge"?) on pci_mvebu controller reports
PCI_CLASS_MEMORY_OTHER in the class register.

This does not work in U-Boot, because U-Boot then tries to autoconfigure
this device in pci_auto.c. This causes the real connected PCIe device
not to work (in U-Boot nor in Linux).

Linux solves this by emulating PCI bridge device (see
drivers/pci/pci-bridge-emul.c in Linux). The Marvell vendor/device IDs
are used for this pci-bridge-emul device in Linux, but the actual
register accesses are emulated and the value of class register is
PCI_CLASS_BRIDGE_PCI.

The simplest solution here in my opinion is to just ignore the local
device in this driver's read/write methods. This fixes PCIe issues for
me.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Stefan Roese <sr@denx.de>
Cc: Anton Schubert <anton.schubert@gmx.de>
Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
Cc: Mario Six <mario.six@gdsys.cc>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Phil Sutter <phil@nwl.cc>
Cc: VlaoMao <vlaomao@gmail.com>
---
 drivers/pci/pci_mvebu.c | 59 +++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index e21dc10c2f..653f445a0f 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -143,31 +143,31 @@ static int mvebu_pcie_read_config(struct udevice *bus, pci_dev_t bdf,
 	struct mvebu_pcie *pcie = dev_get_platdata(bus);
 	int local_bus = PCI_BUS(pcie->dev);
 	int local_dev = PCI_DEV(pcie->dev);
+	int other_dev;
 	u32 reg;
 	u32 data;
 
 	debug("PCIE CFG read:  (b,d,f)=(%2d,%2d,%2d) ",
 	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
 
-	/* Only allow one other device besides the local one on the local bus */
-	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != local_dev) {
-		if (local_dev == 0 && PCI_DEV(bdf) != 1) {
-			debug("- out of range\n");
-			/*
-			 * If local dev is 0, the first other dev can
-			 * only be 1
-			 */
-			*valuep = pci_get_ff(size);
-			return 0;
-		} else if (local_dev != 0 && PCI_DEV(bdf) != 0) {
-			debug("- out of range\n");
-			/*
-			 * If local dev is not 0, the first other dev can
-			 * only be 0
-			 */
-			*valuep = pci_get_ff(size);
-			return 0;
-		}
+	/*
+	 * The local device has PCI_CLASS_MEMORY_OTHER in the class register.
+	 * This does not work for U-Boot because pci_auto.c tries to configure
+	 * this as a device. This results in U-Boot/Linux being unable to access
+	 * PCIe devices.
+	 *
+	 * Linux solves this by emulating a bridge (see
+	 * drivers/pci/pci-bridge-emul.c in Linux).
+	 *
+	 * Let's ignore the local device in U-Boot.
+	 *
+	 * Also only allow one other device besides the (ignored) local one.
+	 */
+
+	other_dev = local_dev ? 0 : 1;
+	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != other_dev) {
+		*valuep = pci_get_ff(size);
+		return 0;
 	}
 
 	/* write address */
@@ -187,28 +187,17 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
 	struct mvebu_pcie *pcie = dev_get_platdata(bus);
 	int local_bus = PCI_BUS(pcie->dev);
 	int local_dev = PCI_DEV(pcie->dev);
+	int other_dev;
 	u32 data;
 
 	debug("PCIE CFG write: (b,d,f)=(%2d,%2d,%2d) ",
 	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
 	debug("(addr,val)=(0x%04x, 0x%08lx)\n", offset, value);
 
-	/* Only allow one other device besides the local one on the local bus */
-	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != local_dev) {
-		if (local_dev == 0 && PCI_DEV(bdf) != 1) {
-			/*
-			 * If local dev is 0, the first other dev can
-			 * only be 1
-			 */
-			return 0;
-		} else if (local_dev != 0 && PCI_DEV(bdf) != 0) {
-			/*
-			 * If local dev is not 0, the first other dev can
-			 * only be 0
-			 */
-			return 0;
-		}
-	}
+	/* See the comment in mvebu_pcie_read_config */
+	other_dev = local_dev ? 0 : 1;
+	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != other_dev)
+		return 0;
 
 	writel(PCIE_CONF_ADDR(bdf, offset), pcie->base + PCIE_CONF_ADDR_OFF);
 	data = pci_conv_size_to_32(0, value, offset, size);
-- 
2.21.0

[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[Chris Packham]

Hi Marek,

On Fri, May 10, 2019 at 2:56 PM Marek Behún <marek.behun@nic.cz> wrote:
>
> The local device (host "bridge"?) on pci_mvebu controller reports
> PCI_CLASS_MEMORY_OTHER in the class register.
>
> This does not work in U-Boot, because U-Boot then tries to autoconfigure
> this device in pci_auto.c. This causes the real connected PCIe device
> not to work (in U-Boot nor in Linux).
>
> Linux solves this by emulating PCI bridge device (see
> drivers/pci/pci-bridge-emul.c in Linux). The Marvell vendor/device IDs
> are used for this pci-bridge-emul device in Linux, but the actual
> register accesses are emulated and the value of class register is
> PCI_CLASS_BRIDGE_PCI.
>
> The simplest solution here in my opinion is to just ignore the local
> device in this driver's read/write methods. This fixes PCIe issues for
> me.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Anton Schubert <anton.schubert@gmx.de>
> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
> Cc: Mario Six <mario.six@gdsys.cc>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Phil Sutter <phil@nwl.cc>
> Cc: VlaoMao <vlaomao@gmail.com>

On the Allied Telesis x530

before:

=> pci enum
=> pci
Scanning PCI devices on bus 0
BusDevFun  VendorId   DeviceId   Device Class       Sub-Class
_____________________________________________________________
00.00.00   0x11ab     0x6820     Memory controller       0x80
00.01.00   0x11ab     0xc807     Network controller      0x00

after:
=> pci enum
=> pci
Scanning PCI devices on bus 0
BusDevFun  VendorId   DeviceId   Device Class       Sub-Class
_____________________________________________________________
00.01.00   0x11ab     0xc807     Network controller      0x00

Which appears to be the desired behaviour. The system booting to Linux
still seems OK.

Tested-by: Chris Packham <judge.packham@gmail.com>

> ---
>  drivers/pci/pci_mvebu.c | 59 +++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index e21dc10c2f..653f445a0f 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -143,31 +143,31 @@ static int mvebu_pcie_read_config(struct udevice *bus, pci_dev_t bdf,
>         struct mvebu_pcie *pcie = dev_get_platdata(bus);
>         int local_bus = PCI_BUS(pcie->dev);
>         int local_dev = PCI_DEV(pcie->dev);
> +       int other_dev;
>         u32 reg;
>         u32 data;
>
>         debug("PCIE CFG read:  (b,d,f)=(%2d,%2d,%2d) ",
>               PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
>
> -       /* Only allow one other device besides the local one on the local bus */
> -       if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != local_dev) {
> -               if (local_dev == 0 && PCI_DEV(bdf) != 1) {
> -                       debug("- out of range\n");
> -                       /*
> -                        * If local dev is 0, the first other dev can
> -                        * only be 1
> -                        */
> -                       *valuep = pci_get_ff(size);
> -                       return 0;
> -               } else if (local_dev != 0 && PCI_DEV(bdf) != 0) {
> -                       debug("- out of range\n");
> -                       /*
> -                        * If local dev is not 0, the first other dev can
> -                        * only be 0
> -                        */
> -                       *valuep = pci_get_ff(size);
> -                       return 0;
> -               }
> +       /*
> +        * The local device has PCI_CLASS_MEMORY_OTHER in the class register.
> +        * This does not work for U-Boot because pci_auto.c tries to configure
> +        * this as a device. This results in U-Boot/Linux being unable to access
> +        * PCIe devices.
> +        *
> +        * Linux solves this by emulating a bridge (see
> +        * drivers/pci/pci-bridge-emul.c in Linux).
> +        *
> +        * Let's ignore the local device in U-Boot.
> +        *
> +        * Also only allow one other device besides the (ignored) local one.
> +        */
> +
> +       other_dev = local_dev ? 0 : 1;
> +       if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != other_dev) {
> +               *valuep = pci_get_ff(size);
> +               return 0;
>         }
>
>         /* write address */
> @@ -187,28 +187,17 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>         struct mvebu_pcie *pcie = dev_get_platdata(bus);
>         int local_bus = PCI_BUS(pcie->dev);
>         int local_dev = PCI_DEV(pcie->dev);
> +       int other_dev;
>         u32 data;
>
>         debug("PCIE CFG write: (b,d,f)=(%2d,%2d,%2d) ",
>               PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
>         debug("(addr,val)=(0x%04x, 0x%08lx)\n", offset, value);
>
> -       /* Only allow one other device besides the local one on the local bus */
> -       if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != local_dev) {
> -               if (local_dev == 0 && PCI_DEV(bdf) != 1) {
> -                       /*
> -                        * If local dev is 0, the first other dev can
> -                        * only be 1
> -                        */
> -                       return 0;
> -               } else if (local_dev != 0 && PCI_DEV(bdf) != 0) {
> -                       /*
> -                        * If local dev is not 0, the first other dev can
> -                        * only be 0
> -                        */
> -                       return 0;
> -               }
> -       }
> +       /* See the comment in mvebu_pcie_read_config */
> +       other_dev = local_dev ? 0 : 1;
> +       if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != other_dev)
> +               return 0;
>
>         writel(PCIE_CONF_ADDR(bdf, offset), pcie->base + PCIE_CONF_ADDR_OFF);
>         data = pci_conv_size_to_32(0, value, offset, size);
> --
> 2.21.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[Stefan Roese]

On 10.05.19 04:56, Marek Behún wrote:
> The local device (host "bridge"?) on pci_mvebu controller reports
> PCI_CLASS_MEMORY_OTHER in the class register.
> 
> This does not work in U-Boot, because U-Boot then tries to autoconfigure
> this device in pci_auto.c. This causes the real connected PCIe device
> not to work (in U-Boot nor in Linux).

What exactly does not work or is the issue here with the current
implementation in Linux? I'm asking, since we have not noticed any
issues here?

Thanks,
Stefan
  
> Linux solves this by emulating PCI bridge device (see
> drivers/pci/pci-bridge-emul.c in Linux). The Marvell vendor/device IDs
> are used for this pci-bridge-emul device in Linux, but the actual
> register accesses are emulated and the value of class register is
> PCI_CLASS_BRIDGE_PCI.
> 
> The simplest solution here in my opinion is to just ignore the local
> device in this driver's read/write methods. This fixes PCIe issues for
> me.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Anton Schubert <anton.schubert@gmx.de>
> Cc: Dirk Eibach <dirk.eibach@gdsys.cc>
> Cc: Mario Six <mario.six@gdsys.cc>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Phil Sutter <phil@nwl.cc>
> Cc: VlaoMao <vlaomao@gmail.com>
> ---
>   drivers/pci/pci_mvebu.c | 59 +++++++++++++++++------------------------
>   1 file changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index e21dc10c2f..653f445a0f 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -143,31 +143,31 @@ static int mvebu_pcie_read_config(struct udevice *bus, pci_dev_t bdf,
>   	struct mvebu_pcie *pcie = dev_get_platdata(bus);
>   	int local_bus = PCI_BUS(pcie->dev);
>   	int local_dev = PCI_DEV(pcie->dev);
> +	int other_dev;
>   	u32 reg;
>   	u32 data;
>   
>   	debug("PCIE CFG read:  (b,d,f)=(%2d,%2d,%2d) ",
>   	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
>   
> -	/* Only allow one other device besides the local one on the local bus */
> -	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != local_dev) {
> -		if (local_dev == 0 && PCI_DEV(bdf) != 1) {
> -			debug("- out of range\n");
> -			/*
> -			 * If local dev is 0, the first other dev can
> -			 * only be 1
> -			 */
> -			*valuep = pci_get_ff(size);
> -			return 0;
> -		} else if (local_dev != 0 && PCI_DEV(bdf) != 0) {
> -			debug("- out of range\n");
> -			/*
> -			 * If local dev is not 0, the first other dev can
> -			 * only be 0
> -			 */
> -			*valuep = pci_get_ff(size);
> -			return 0;
> -		}
> +	/*
> +	 * The local device has PCI_CLASS_MEMORY_OTHER in the class register.
> +	 * This does not work for U-Boot because pci_auto.c tries to configure
> +	 * this as a device. This results in U-Boot/Linux being unable to access
> +	 * PCIe devices.
> +	 *
> +	 * Linux solves this by emulating a bridge (see
> +	 * drivers/pci/pci-bridge-emul.c in Linux).
> +	 *
> +	 * Let's ignore the local device in U-Boot.
> +	 *
> +	 * Also only allow one other device besides the (ignored) local one.
> +	 */
> +
> +	other_dev = local_dev ? 0 : 1;
> +	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != other_dev) {
> +		*valuep = pci_get_ff(size);
> +		return 0;
>   	}
>   
>   	/* write address */
> @@ -187,28 +187,17 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>   	struct mvebu_pcie *pcie = dev_get_platdata(bus);
>   	int local_bus = PCI_BUS(pcie->dev);
>   	int local_dev = PCI_DEV(pcie->dev);
> +	int other_dev;
>   	u32 data;
>   
>   	debug("PCIE CFG write: (b,d,f)=(%2d,%2d,%2d) ",
>   	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
>   	debug("(addr,val)=(0x%04x, 0x%08lx)\n", offset, value);
>   
> -	/* Only allow one other device besides the local one on the local bus */
> -	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != local_dev) {
> -		if (local_dev == 0 && PCI_DEV(bdf) != 1) {
> -			/*
> -			 * If local dev is 0, the first other dev can
> -			 * only be 1
> -			 */
> -			return 0;
> -		} else if (local_dev != 0 && PCI_DEV(bdf) != 0) {
> -			/*
> -			 * If local dev is not 0, the first other dev can
> -			 * only be 0
> -			 */
> -			return 0;
> -		}
> -	}
> +	/* See the comment in mvebu_pcie_read_config */
> +	other_dev = local_dev ? 0 : 1;
> +	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) != other_dev)
> +		return 0;
>   
>   	writel(PCIE_CONF_ADDR(bdf, offset), pcie->base + PCIE_CONF_ADDR_OFF);
>   	data = pci_conv_size_to_32(0, value, offset, size);
> 

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

[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[Marek Behun]

On Fri, 10 May 2019 10:15:25 +0200
Stefan Roese <sr@denx.de> wrote:

> On 10.05.19 04:56, Marek Behún wrote:
> > The local device (host "bridge"?) on pci_mvebu controller reports
> > PCI_CLASS_MEMORY_OTHER in the class register.
> > 
> > This does not work in U-Boot, because U-Boot then tries to autoconfigure
> > this device in pci_auto.c. This causes the real connected PCIe device
> > not to work (in U-Boot nor in Linux).  
> 
> What exactly does not work or is the issue here with the current
> implementation in Linux? I'm asking, since we have not noticed any
> issues here?
> 
ath10k firmware fails to load for network device.
SATA device timeouts on a PCIe SATA expander.
As if some PCIe I/O did not work. The devices are still enumerated and
recognized by Linux, but simply won't work. Nor in U-Boot.

[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[Stefan Roese]

On 10.05.19 13:44, Marek Behun wrote:
> On Fri, 10 May 2019 10:15:25 +0200
> Stefan Roese <sr@denx.de> wrote:
> 
>> On 10.05.19 04:56, Marek Behún wrote:
>>> The local device (host "bridge"?) on pci_mvebu controller reports
>>> PCI_CLASS_MEMORY_OTHER in the class register.
>>>
>>> This does not work in U-Boot, because U-Boot then tries to autoconfigure
>>> this device in pci_auto.c. This causes the real connected PCIe device
>>> not to work (in U-Boot nor in Linux).
>>
>> What exactly does not work or is the issue here with the current
>> implementation in Linux? I'm asking, since we have not noticed any
>> issues here?
>>
> ath10k firmware fails to load for network device.
> SATA device timeouts on a PCIe SATA expander.
> As if some PCIe I/O did not work. The devices are still enumerated and
> recognized by Linux, but simply won't work. Nor in U-Boot.

I see. Strange that we did not see any issues on our board (or
other users). I'll try to pull this patch into this upcoming
release though.

Thanks,
Stefan

[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[Chris Packham]

Hi Marek,

On Fri, May 10, 2019 at 11:44 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Fri, 10 May 2019 10:15:25 +0200
> Stefan Roese <sr@denx.de> wrote:
>
> > On 10.05.19 04:56, Marek Behún wrote:
> > > The local device (host "bridge"?) on pci_mvebu controller reports
> > > PCI_CLASS_MEMORY_OTHER in the class register.
> > >
> > > This does not work in U-Boot, because U-Boot then tries to autoconfigure
> > > this device in pci_auto.c. This causes the real connected PCIe device
> > > not to work (in U-Boot nor in Linux).
> >
> > What exactly does not work or is the issue here with the current
> > implementation in Linux? I'm asking, since we have not noticed any
> > issues here?
> >
> ath10k firmware fails to load for network device.
> SATA device timeouts on a PCIe SATA expander.
> As if some PCIe I/O did not work. The devices are still enumerated and
> recognized by Linux, but simply won't work. Nor in U-Boot.

Is there a PCIe bridge involved in either of these? In the pre-history
of Kirkwood (which I believe has the same or similar IP block for
PCIe) I've hit platforms where the PCIe I/O fails because something
about the host bridge interferes with a real bridge on the bus where a
directly connected device worked fine. Marvell were shipping a patch
that adds a fake PCIe bridge in their LSP and I think that is what was
eventually ported to upstream Linux.

[U-Boot] [PATCH 1/1] pci: pci_mvebu: ignore the local device

[Marek Behun]

> Is there a PCIe bridge involved in either of these? In the pre-history
> of Kirkwood (which I believe has the same or similar IP block for
> PCIe) I've hit platforms where the PCIe I/O fails because something
> about the host bridge interferes with a real bridge on the bus where a
> directly connected device worked fine. Marvell were shipping a patch
> that adds a fake PCIe bridge in their LSP and I think that is what was
> eventually ported to upstream Linux.

Hi Chris,
there is no real bridge on Turris Omnia, the devices are connected
directly to CPU.
Marek