[PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports

[PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports

[Stefan Roese]

This patch changes the PCI config routines in the Armada XP / 38x driver
to not allow access to the PCIe root ports.

While updating the Armada XP based theadorable to the latest mainline
and testing it with the DM PCI driver I noticed, that the PCI root
bridge was being configured incorrectly. Resulting in the PCIe Intel
WiFi was not working correctly in Linux. With this patch applied, all
PCIe devices work without any issues in Linux again.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Marek Beh?n <marek.behun@nic.cz>
Cc: Phil Sutter <phil@nwl.cc>
Cc: Mario Six <mario.six@gdsys.cc>
---
 drivers/pci/pci_mvebu.c | 66 +++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index 9528e7c40ecf..46ffa8df4403 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -153,28 +153,21 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,
 	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;
-		}
+	debug("PCIE CFG read:  loc_bus=%d loc_dev=%d (b,d,f)=(%2d,%2d,%2d) ",
+	      local_bus, local_dev, PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+
+	/* Don't access the local host controller via this API */
+	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) == local_dev) {
+		debug("- skipping host controller\n");
+		*valuep = pci_get_ff(size);
+		return 0;
+	}
+
+	/* If local dev is 0, the first other dev can only be 1 */
+	if (PCI_BUS(bdf) == local_bus && local_dev == 0 && PCI_DEV(bdf) != 1) {
+		debug("- out of range\n");
+		*valuep = pci_get_ff(size);
+		return 0;
 	}
 
 	/* write address */
@@ -196,25 +189,20 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
 	int local_dev = PCI_DEV(pcie->dev);
 	u32 data;
 
-	debug("PCIE CFG write: (b,d,f)=(%2d,%2d,%2d) ",
-	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+	debug("PCIE CFG write: loc_bus=%d loc_dev=%d (b,d,f)=(%2d,%2d,%2d) ",
+	      local_bus, local_dev, 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;
-		}
+	/* Don't access the local host controller via this API */
+	if (PCI_BUS(bdf) == local_bus && PCI_DEV(bdf) == local_dev) {
+		debug("- skipping host controller\n");
+		return 0;
+	}
+
+	/* If local dev is 0, the first other dev can only be 1 */
+	if (PCI_BUS(bdf) == local_bus && local_dev == 0 && PCI_DEV(bdf) != 1) {
+		debug("- out of range\n");
+		return 0;
 	}
 
 	writel(PCIE_CONF_ADDR(bdf, offset), pcie->base + PCIE_CONF_ADDR_OFF);
-- 
2.30.0

[PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports

[Stefan Roese]

On 25.01.21 15:25, Stefan Roese wrote:
> This patch changes the PCI config routines in the Armada XP / 38x driver
> to not allow access to the PCIe root ports.
> 
> While updating the Armada XP based theadorable to the latest mainline
> and testing it with the DM PCI driver I noticed, that the PCI root
> bridge was being configured incorrectly. Resulting in the PCIe Intel
> WiFi was not working correctly in Linux. With this patch applied, all
> PCIe devices work without any issues in Linux again.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Marek Beh?n <marek.behun@nic.cz>
> Cc: Phil Sutter <phil@nwl.cc>
> Cc: Mario Six <mario.six@gdsys.cc>

Applied to u-boot-marvell/master

Thanks,
Stefan

[PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports

[Marek Behun]

On Mon, 25 Jan 2021 15:25:31 +0100
Stefan Roese <sr@denx.de> wrote:

> This patch changes the PCI config routines in the Armada XP / 38x driver
> to not allow access to the PCIe root ports.
> 
> While updating the Armada XP based theadorable to the latest mainline
> and testing it with the DM PCI driver I noticed, that the PCI root
> bridge was being configured incorrectly. Resulting in the PCIe Intel
> WiFi was not working correctly in Linux. With this patch applied, all
> PCIe devices work without any issues in Linux again.
> 

Hi Stefan,

I overlooked this patch and coincidentally also discovered this bug
last week. Your patches solves this issue, but

- previously, when pci-mvebu did not use DM, it was solved differently:
  pci_skip_dev returned 1 for (b,d,f) = (*,0,0).
- on this address (*,0,0) there seems to be a Memory Controller -
  without applying your patch the pci command lists

    => 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   0x168c     0x003c     Network controller      0x80

  with your patch

    => pci
    Scanning PCI devices on bus 0
    BusDevFun  VendorId   DeviceId   Device Class       Sub-Class
    _____________________________________________________________
    00.01.00   0x168c     0x003c     Network controller      0x80

  I would like to know why this memory controller is there and whether
  it should be configured. The pci-mvebu driver in kernel currently
  ignores this Memory Controller. That is the reason why your Intel
  WiFi card and my ath10k wifi card are not working: U-Boot configures
  some addresses in PCIe registers of this Memory Controller, but
  kernel does not reconfigure it.

So all in all:
- your patch solves the issue, but I wonder whether it shouldn't be
  solved in another way, for example by adding pci,no-autoconfig device
  tree property in DTS files in u-boot for these Memory Controllers
- I wonder whether kernel should stop ignoring this device and
  configure it somehow

Marek

[PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports

[Stefan Roese]

Hi Marek,

On 08.02.21 15:00, Marek Behun wrote:
> On Mon, 25 Jan 2021 15:25:31 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> This patch changes the PCI config routines in the Armada XP / 38x driver
>> to not allow access to the PCIe root ports.
>>
>> While updating the Armada XP based theadorable to the latest mainline
>> and testing it with the DM PCI driver I noticed, that the PCI root
>> bridge was being configured incorrectly. Resulting in the PCIe Intel
>> WiFi was not working correctly in Linux. With this patch applied, all
>> PCIe devices work without any issues in Linux again.
>>
> 
> Hi Stefan,
> 
> I overlooked this patch and coincidentally also discovered this bug
> last week. Your patches solves this issue,

Good...

> but
> 
> - previously, when pci-mvebu did not use DM, it was solved differently:
>    pci_skip_dev returned 1 for (b,d,f) = (*,0,0).
> - on this address (*,0,0) there seems to be a Memory Controller -
>    without applying your patch the pci command lists
> 
>      => 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   0x168c     0x003c     Network controller      0x80
> 
>    with your patch
> 
>      => pci
>      Scanning PCI devices on bus 0
>      BusDevFun  VendorId   DeviceId   Device Class       Sub-Class
>      _____________________________________________________________
>      00.01.00   0x168c     0x003c     Network controller      0x80

Yes.

>    I would like to know why this memory controller is there and whether
>    it should be configured. The pci-mvebu driver in kernel currently
>    ignores this Memory Controller. That is the reason why your Intel
>    WiFi card and my ath10k wifi card are not working: U-Boot configures
>    some addresses in PCIe registers of this Memory Controller, but
>    kernel does not reconfigure it.

AFAIR, the U-Boot PCIe host driver configured this "memory controller"
incorrectly resulting in these problems with the attached PCIe devices
(WiFi controller). My presumption is, that the "memory controller" is
the host bridge root port and it's BAR's need to intepreted / configured
differently.

The easy solution was to not configure this host bridge at all, as it
solves these issues (for me and for you) and no further issues are
introduced. At least not that I know of. If there are issues, we of
course need to fix them.

> So all in all:
> - your patch solves the issue, but I wonder whether it shouldn't be
>    solved in another way, for example by adding pci,no-autoconfig device
>    tree property in DTS files in u-boot for these Memory Controllers

Hmmm. If my findings above are correct, then we should fix this
incorrect BAR configuration for all boards using this driver globally
and not introduce a DT property than needs to be added to all boards.

Perhaps it's possible to correctly interpret the "memory controller"
and correctly configure it's BARs. Frankly, I was not able to come up
with such a solution quickly and did choose the solution provided in
this patch instead.

> - I wonder whether kernel should stop ignoring this device and
>    configure it somehow

Why should the kernel do this? Is some functionality missing from your
point of view?

Thanks,
Stefan

[PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports

[Pali Rohár]

Hello Stefan!

On Monday 08 February 2021 15:19:20 Stefan Roese wrote:
> On 08.02.21 15:00, Marek Behun wrote:
> >    I would like to know why this memory controller is there and whether
> >    it should be configured. The pci-mvebu driver in kernel currently
> >    ignores this Memory Controller. That is the reason why your Intel
> >    WiFi card and my ath10k wifi card are not working: U-Boot configures
> >    some addresses in PCIe registers of this Memory Controller, but
> >    kernel does not reconfigure it.
> 
> AFAIR, the U-Boot PCIe host driver configured this "memory controller"
> incorrectly resulting in these problems with the attached PCIe devices
> (WiFi controller). My presumption is, that the "memory controller" is
> the host bridge root port and it's BAR's need to intepreted / configured
> differently.

I'm not sure if this can be a host bridge. Because mvebu has separate
address space for host bridge registers and kernel is creating a virtual
bridge device for accessing host bridge registers.

I think first somebody needs to answer the question what is this device
doing and why kernel has "hacks" which completely hides it from lspci
output.

> The easy solution was to not configure this host bridge at all, as it
> solves these issues (for me and for you) and no further issues are
> introduced. At least not that I know of. If there are issues, we of
> course need to fix them.
> 
> > So all in all:
> > - your patch solves the issue, but I wonder whether it shouldn't be
> >    solved in another way, for example by adding pci,no-autoconfig device
> >    tree property in DTS files in u-boot for these Memory Controllers
> 
> Hmmm. If my findings above are correct, then we should fix this
> incorrect BAR configuration for all boards using this driver globally
> and not introduce a DT property than needs to be added to all boards.
> 
> Perhaps it's possible to correctly interpret the "memory controller"
> and correctly configure it's BARs. Frankly, I was not able to come up
> with such a solution quickly and did choose the solution provided in
> this patch instead.
> 
> > - I wonder whether kernel should stop ignoring this device and
> >    configure it somehow
> 
> Why should the kernel do this? Is some functionality missing from your
> point of view?

Kernel is not only ignoring this device, but it also hides it from PCI
bus. It is not visible in lspci output. And I do not think that kernel
should hide devices. AFAIK lspci show all devices present on bus, even
those which do not have loaded kernel driver...

> 
> Thanks,
> Stefan

[PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports

[Marek Behun]

Maybe we should ask kernel people. It seems that Thomas Petazzoni may
be able to answer, since he is author of the following kernel patch:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4ac99011e542d06ea2bda10063502583c6d7991