* [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports
@ 2021-01-25 14:25 Stefan Roese
2021-02-08 11:37 ` Stefan Roese
2021-02-08 14:00 ` Marek Behun
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Roese @ 2021-01-25 14:25 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports
2021-01-25 14:25 [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports Stefan Roese
@ 2021-02-08 11:37 ` Stefan Roese
2021-02-08 14:00 ` Marek Behun
1 sibling, 0 replies; 6+ messages in thread
From: Stefan Roese @ 2021-02-08 11:37 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports
2021-01-25 14:25 [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports Stefan Roese
2021-02-08 11:37 ` Stefan Roese
@ 2021-02-08 14:00 ` Marek Behun
2021-02-08 14:19 ` Stefan Roese
1 sibling, 1 reply; 6+ messages in thread
From: Marek Behun @ 2021-02-08 14:00 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports
2021-02-08 14:00 ` Marek Behun
@ 2021-02-08 14:19 ` Stefan Roese
2021-02-08 14:29 ` Pali Rohár
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2021-02-08 14:19 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports
2021-02-08 14:19 ` Stefan Roese
@ 2021-02-08 14:29 ` Pali Rohár
2021-02-08 14:39 ` Marek Behun
0 siblings, 1 reply; 6+ messages in thread
From: Pali Rohár @ 2021-02-08 14:29 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-08 14:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-25 14:25 [PATCH] pci: pci_mvebu: Disable config access to PCI host bridge ports Stefan Roese
2021-02-08 11:37 ` Stefan Roese
2021-02-08 14:00 ` Marek Behun
2021-02-08 14:19 ` Stefan Roese
2021-02-08 14:29 ` Pali Rohár
2021-02-08 14:39 ` Marek Behun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox