Skip to contentSkip to conversation

PCI: emulated PCI bridge common logic

PATCH In review Thomas Petazzoni 5 participants 15 messages Jun 29, 2018 – Jan 10, 2022

Subsystems (4)
PCI PCI native host bridge and endpoint drivers PCI driver for aardvark (Marvell Armada 3700) PCI driver for mvebu (Marvell Armada 370 and Armada XP SoC support)
lore
Share & tools
lore

Thomas Petazzoni

author maintainer
#0 lore raw

cover PCI: emulated PCI bridge common logic

Hello,

The pci-mvebu driver already contains some logic to emulate a root port PCI bridge. It turns out that we have a similar need for the pci-aardvark driver. Instead of duplicating the same logic in two drivers, this patch series starts by adding a small common infrastructure that helps emulate a root port PCI bridge, converts pci-mvebu to use it, and finally extends pci-aardvark to use it as well.

Thanks to this, Marvell Armada 3720 based systems, which use the Aarkvark PCI controller, will have better PCI support, by having a root port PCI bridge exposed.

The emulated PCI bridge common logic is a proposal, I very much welcome comments and suggestions. Also, if you feel that adding a common logic for only two drivers is too early, I'm fine with duplicating a bit of code betwen pci-mvebu and pci-aardvark.

Best regards,

Thomas

Thomas Petazzoni (2):
PCI: Introduce PCI software bridge common logic
PCI: mvebu: Convert to PCI software bridge

Zachary Zhang (1):
PCI: aardvark: Implement emulated root PCI bridge

 drivers/pci/Kconfig                   |   3 +
 drivers/pci/Makefile                  |   1 +
 drivers/pci/controller/Kconfig        |   2 +
 drivers/pci/controller/pci-aardvark.c | 119 ++++++++++-
 drivers/pci/controller/pci-mvebu.c    | 370 ++++++++++------------------------
 drivers/pci/pci-sw-bridge.c           | 149 ++++++++++++++
 include/linux/pci-sw-bridge.h         | 125 ++++++++++++
 7 files changed, 497 insertions(+), 272 deletions(-)
 create mode 100644 drivers/pci/pci-sw-bridge.c
 create mode 100644 include/linux/pci-sw-bridge.h
Signature · 1 line
2.14.4
author maintainer
#1 lore raw

PATCH 1/3 PCI: Introduce PCI software bridge common logic

Some PCI host controllers do not expose at the hardware level a root port PCI bridge. Due to this, the Marvell Armada 370/38x/XP PCI controller driver (pci-mvebu) emulates a root port PCI bridge, and uses that to (among other things?) dynamically create the memory windows that correspond to the PCI MEM and I/O regions.

Since we now need to add a very similar logic for the Marvell Armada 37xx PCI controller driver (pci-aardvark), instead of duplicating the code, we create in this commit a common logic called pci-sw-bridge.

The idea of this logic is to emulate a root port PCI bridge by providing configuration space read/write operations, and faking behind the scenes the configuration space of a PCI bridge. A PCI host controller driver simply has to call pci_sw_bridge_read() and pci_sw_bridge_write() to read/write the configuration space of the bridge.

By default, the PCI bridge configuration space is simply emulated by a chunk of memory, but the PCI host controller can override the behavior of the read and write operations on a per-register basis to do additional actions if needed.

Signed-off-by Thomas Petazzoni <[email protected]>
---
 drivers/pci/Kconfig           |   3 +
 drivers/pci/Makefile          |   1 +
 drivers/pci/pci-sw-bridge.c   | 149 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++
 4 files changed, 278 insertions(+)
 create mode 100644 drivers/pci/pci-sw-bridge.c
 create mode 100644 include/linux/pci-sw-bridge.h
drivers/pci/Kconfig +3 -0
index 56ff8f6d31fc..1f13575d052b 100644
@@ -98,6 +98,9 @@ config PCI_ECAM
 config PCI_LOCKLESS_CONFIG
 	bool
 
+config PCI_SW_BRIDGE
+	bool
+
 config PCI_IOV
 	bool "PCI IOV support"
 	depends on PCI
drivers/pci/Makefile +1 -0
index 535201984b8b..0bd65ddd2c50 100644
@@ -19,6 +19,7 @@ obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
 obj-$(CONFIG_PCI_MSI)		+= msi.o
 obj-$(CONFIG_PCI_ATS)		+= ats.o
 obj-$(CONFIG_PCI_IOV)		+= iov.o
+obj-$(CONFIG_PCI_SW_BRIDGE)	+= pci-sw-bridge.o
 obj-$(CONFIG_ACPI)		+= pci-acpi.o
 obj-$(CONFIG_PCI_LABEL)		+= pci-label.o
 obj-$(CONFIG_X86_INTEL_MID)	+= pci-mid.o
drivers/pci/pci-sw-bridge.c new file +149 -0

149 lines not on this page · raw message

include/linux/pci-sw-bridge.h new file +125 -0

125 lines not on this page · raw message

Signature · 1 line
2.14.4
#2 lore raw

PATCH 1/3 PCI: Introduce PCI software bridge common logic

[+cc Ray since he's doing something similar for iProc,
Ley Foon & Simon for a possible Altera & R-Car bug]

Quoted 10 lines · Thomas Petazzoni

Some PCI host controllers do not expose at the hardware level a root port PCI bridge. Due to this, the Marvell Armada 370/38x/XP PCI controller driver (pci-mvebu) emulates a root port PCI bridge, and uses that to (among other things?) dynamically create the memory windows that correspond to the PCI MEM and I/O regions.

Since we now need to add a very similar logic for the Marvell Armada 37xx PCI controller driver (pci-aardvark), instead of duplicating the code, we create in this commit a common logic called pci-sw-bridge.

Quoted 6 lines

The idea of this logic is to emulate a root port PCI bridge by providing configuration space read/write operations, and faking behind the scenes the configuration space of a PCI bridge. A PCI host controller driver simply has to call pci_sw_bridge_read() and pci_sw_bridge_write() to read/write the configuration space of the bridge.

These systems must actually have a Root Port (there's obviously a Port that originates the link), but it's not visible in a spec-compliant way. So this is basically a wrapper that translates accesses to standard config space into the vendor-specific registers.

Since there really *is* a hardware bridge but it just doesn't have the interface we expect, "sw_bridge" doesn't feel like quite the right name for it. Maybe something related to adapter/emulation/interface/...?

There are several drivers that do this, and it would be really cool to have them all do it in a similar way. I found at least these:

hv_pcifront_read_config()
mvebu_pcie_rd_conf()
thunder_ecam_config_read()
thunder_pem_config_read()
xgene_pcie_config_read32()
iproc_pcie_config_read32()
rcar_pcie_read_conf()

I *really* like the fact that your accessors use the normal register names, e.g., PCI_EXP_DEVCAP instead of PCISWCAP_EXP_DEVCAP. That's huge for greppability.

Quoted 11 lines

By default, the PCI bridge configuration space is simply emulated by a chunk of memory, but the PCI host controller can override the behavior of the read and write operations on a per-register basis to do additional actions if needed.

Signed-off-by Thomas Petazzoni <[email protected]>
---
 drivers/pci/Kconfig           |   3 +
 drivers/pci/Makefile          |   1 +
 drivers/pci/pci-sw-bridge.c   | 149 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++

Could this go in drivers/pci instead of include/linux? I'd prefer to hide it inside the PCI core if that's possible.

drivers/pci/pci-sw-bridge.c line 56

Quoted 6 lines

+ * Should be called by the PCI controller driver when reading the PCI
+ * configuration space of the fake bridge. It will call back the
+ * ->ops->read_base or ->ops->read_pcie operations.
+ */

drivers/pci/pci-sw-bridge.c new file +2 -0
+int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
+		       int size, u32 *value)

We don't really have a strong convention about the names of config
accessors: *_rd_conf(), *_read_config(), *config_read() are all used. Maybe we could at least include "conf" to connect this with config space as opposed to MMIO space.

Quoted 22 lines
drivers/pci/pci-sw-bridge.c new file +22 -0
+{
+	int ret;
+	int reg = where & ~3;
+
+	if (bridge->has_pcie && reg >= PCI_CAP_PCIE_END) {
+		*value = 0;
+		return PCIBIOS_SUCCESSFUL;
+	}
+
+	if (!bridge->has_pcie && reg >= PCI_BRIDGE_CONF_END) {
+		*value = 0;
+		return PCIBIOS_SUCCESSFUL;
+	}
+
+	if (bridge->has_pcie && reg >= PCI_CAP_PCIE_START) {
+		reg -= PCI_CAP_PCIE_START;
+
+		if (bridge->ops->read_pcie)
+			ret = bridge->ops->read_pcie(bridge, reg, value);
+		else
+			ret = PCI_SW_BRIDGE_NOT_HANDLED;
+

drivers/pci/pci-sw-bridge.c line 88

Quoted 10 lines
drivers/pci/pci-sw-bridge.c new file +10 -0
+		if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
+			*value = *((u32*) &bridge->pcie_conf + reg / 4);
+	} else {
+		if (bridge->ops->read_base)
+			ret = bridge->ops->read_base(bridge, reg, value);
+		else
+			ret = PCI_SW_BRIDGE_NOT_HANDLED;
+
+		if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
+			*value = *((u32*) &bridge->conf + reg / 4);

I'm not sure about this part, where the config space that's not handled by the bridge's ops ends up just being RAM that's readable and writable with no effect on the hardware. That seems a little counter-intuitive. I think dropping writes and reading zeroes would simplify my mental model.

drivers/pci/pci-sw-bridge.c line 96

Quoted 8 lines
drivers/pci/pci-sw-bridge.c new file +8 -0
+	}
+
+	if (size == 1)
+		*value = (*value >> (8 * (where & 3))) & 0xff;
+	else if (size == 2)
+		*value = (*value >> (8 * (where & 3))) & 0xffff;
+	else if (size != 4)
+		return PCIBIOS_BAD_REGISTER_NUMBER;

This isn't directly related to your patches, but this is a common pattern with some variations between drivers, which makes me wonder whether there are bugs lurking. These use "where & 3" for size 2:

advk_pcie_rd_conf()
mtk_pcie_hw_rd_cfg()
pci_generic_config_read32()

But these use "where & 2":

_altera_pcie_cfg_read()
rcar_pcie_read_conf()

I *think* this means that unaligned 2-byte config reads on Altera and R-Car will return incorrect data. In both cases, the actual read seen by the hardware is a 32-bit read, but I think we extract the wrong 16 bits from that result.

I wonder if there's a way to use a common helper function to do this.

Quoted 5 lines
include/linux/pci-sw-bridge.h new file +4 -0
+	return PCIBIOS_SUCCESSFUL;
+}
 
+	/*
+	 * Called when writing to the regular PCI bridge configuration

include/linux/pci-sw-bridge.h line 105

Quoted 10 lines
include/linux/pci-sw-bridge.h new file +10 -0
+	 * space. old is the current value, new is the new value being
+	 * written, and mask indicates which parts of the value are
+	 * being changed.
+	 */
+	void (*write_base)(struct pci_sw_bridge *bridge, int reg,
+			   u32 old, u32 new, u32 mask);
+
+	/*
+	 * Same as ->read_base(), except it is for reading from the
+	 * PCIe capability configuration space.

I think you mean "->write_base(), except it is for writing to"

Quoted 4 lines
include/linux/pci-sw-bridge.h new file +4 -0
+	 */
+	void (*write_pcie)(struct pci_sw_bridge *bridge, int reg,
+			   u32 old, u32 new, u32 mask);
+};
author maintainer
#3 lore raw

PATCH 1/3 PCI: Introduce PCI software bridge common logic

Hello Bjorn,

Thanks for your review!

Quoted 11 lines · Bjorn Helgaas
Quoted 6 lines

The idea of this logic is to emulate a root port PCI bridge by providing configuration space read/write operations, and faking behind the scenes the configuration space of a PCI bridge. A PCI host controller driver simply has to call pci_sw_bridge_read() and pci_sw_bridge_write() to read/write the configuration space of the bridge.

These systems must actually have a Root Port (there's obviously a Port that originates the link), but it's not visible in a spec-compliant way. So this is basically a wrapper that translates accesses to standard config space into the vendor-specific registers.

Correct.

Quoted 3 lines

Since there really *is* a hardware bridge but it just doesn't have the interface we expect, "sw_bridge" doesn't feel like quite the right name for it. Maybe something related to adapter/emulation/interface/...?

sw_adapter ? sw_adap ?

Quoted 4 lines

There are several drivers that do this, and it would be really cool to have them all do it in a similar way. I found at least these:

hv_pcifront_read_config()

This one indeed looks potentially a bit similar.

Quoted 1 line

mvebu_pcie_rd_conf()

This one is converted by my patch series.

Quoted 1 line

thunder_ecam_config_read()

This one I'm not sure what is happening,

Quoted 1 line

thunder_pem_config_read()

For this one, it seems like the HW exposes a config space, just that it is bogus. It is a bit of a different situation. It could be handled by the same sw_bridge/sw_adapt stuff, but it's a slightly different use case than "the HW doesn't expose any spec-compliant root port config space".

Quoted 1 line

xgene_pcie_config_read32()

I don't see why this one would need sw_bridge/sw_adap.

Quoted 1 line

iproc_pcie_config_read32()

Not sure about this one either.

Quoted 1 line

rcar_pcie_read_conf()

Perhaps it needs sw_bridge/sw_adap.

Really for all those ones, it would require some involvement from the corresponding driver maintainers, who understand better their HW and see if they would benefit from sw_bridge/sw_adap.

Quoted 3 lines

I *really* like the fact that your accessors use the normal register names, e.g., PCI_EXP_DEVCAP instead of PCISWCAP_EXP_DEVCAP. That's huge for greppability.

Thanks. There's however one down-side: I had to add separate accessors for reading/writing the main PCI config space and the PCIe capability config space. Any additional capability config space would require adding another pair of accessors. But I really wanted to re-use the existing PCI_EXP_* definitions. Another approach is perhaps to pass an "ID" of the config space being accessed, i.e 0x0 for the main config space, and PCI_CAP_ID_EXP for PCI express.

So instead of 4 accessors in pci_sw_bridge_ops, we would have only two:

pci_sw_bridge_read_status_t (*read)(struct pci_sw_bridge *bridge,

				    int id, int reg, u32 *value);
void (*write)(struct pci_sw_bridge *bridge, int id, int reg,
              u32 old, u32 new, u32 mask);

where "id" would allow the accessor to know if we're accessing the main config space, or the PCI express capability config space.

How does that sound ?

Quoted 12 lines
Quoted 11 lines

By default, the PCI bridge configuration space is simply emulated by a chunk of memory, but the PCI host controller can override the behavior of the read and write operations on a per-register basis to do additional actions if needed.

Signed-off-by Thomas Petazzoni <[email protected]>
---
 drivers/pci/Kconfig           |   3 +
 drivers/pci/Makefile          |   1 +
 drivers/pci/pci-sw-bridge.c   | 149 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-sw-bridge.h | 125 +++++++++++++++++++++++++++++++++++
Quoted 2 lines

Could this go in drivers/pci instead of include/linux? I'd prefer to hide it inside the PCI core if that's possible.

Sure, I'll fix that.

Quoted 11 lines

drivers/pci/pci-sw-bridge.c line 56

Quoted 6 lines

+ * Should be called by the PCI controller driver when reading the PCI
+ * configuration space of the fake bridge. It will call back the
+ * ->ops->read_base or ->ops->read_pcie operations.
+ */

drivers/pci/pci-sw-bridge.c new file +2 -0
+int pci_sw_bridge_read(struct pci_sw_bridge *bridge, int where,
+		       int size, u32 *value)

We don't really have a strong convention about the names of config
accessors: *_rd_conf(), *_read_config(), *config_read() are all used. Maybe we could at least include "conf" to connect this with config space as opposed to MMIO space.

No problem, I'll fix that, makes perfect sense.

Quoted 16 lines
Quoted 16 lines
drivers/pci/pci-sw-bridge.c new file +16 -0
+	if (bridge->has_pcie && reg >= PCI_CAP_PCIE_START) {
+		reg -= PCI_CAP_PCIE_START;
+
+		if (bridge->ops->read_pcie)
+			ret = bridge->ops->read_pcie(bridge, reg, value);
+		else
+			ret = PCI_SW_BRIDGE_NOT_HANDLED;
+
+		if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
+			*value = *((u32*) &bridge->pcie_conf + reg / 4);
+	} else {
+		if (bridge->ops->read_base)
+			ret = bridge->ops->read_base(bridge, reg, value);
+		else
+			ret = PCI_SW_BRIDGE_NOT_HANDLED;
+
Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		if (ret == PCI_SW_BRIDGE_NOT_HANDLED)
+			*value = *((u32*) &bridge->conf + reg / 4);

I'm not sure about this part, where the config space that's not handled by the bridge's ops ends up just being RAM that's readable and writable with no effect on the hardware. That seems a little counter-intuitive. I think dropping writes and reading zeroes would simplify my mental model.

The fact that it is handled just like RAM is actually needed for a number of registers. For example, in the current pci-mvebu driver, when reading we have:

        case PCI_VENDOR_ID:
                *value = bridge->device << 16 | bridge->vendor;
                break;

        case PCI_COMMAND:
                *value = bridge->command | bridge->status << 16;
                break;

        case PCI_CLASS_REVISION:
                *value = bridge->class << 16 | bridge->interface << 8 |
                         bridge->revision;
                break;

        case PCI_CACHE_LINE_SIZE:
                *value = bridge->bist << 24 | bridge->header_type << 16 |
                         bridge->latency_timer << 8 | bridge->cache_line_size;
                break;

        case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
                *value = bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4];
                break;

        case PCI_PRIMARY_BUS:
                *value = (bridge->secondary_latency_timer << 24 |
                          bridge->subordinate_bus         << 16 |
                          bridge->secondary_bus           <<  8 |
                          bridge->primary_bus);
                break;

        case PCI_IO_BASE:
                if (!mvebu_has_ioport(port))
                        *value = bridge->secondary_status << 16;
                else
                        *value = (bridge->secondary_status << 16 |
                                  bridge->iolimit          <<  8 |
                                  bridge->iobase);
                break;

We definitely cannot return zeroes for all these values.

And for writes, we have code like this:

        case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
                bridge->bar[((where & ~3) - PCI_BASE_ADDRESS_0) / 4] = value;
                break;

        case PCI_IO_BASE:
                /*
                 * We also keep bit 1 set, it is a read-only bit that
                 * indicates we support 32 bits addressing for the
                 * I/O
                 */
                bridge->iobase = (value & 0xff) | PCI_IO_RANGE_TYPE_32;
                bridge->iolimit = ((value >> 8) & 0xff) | PCI_IO_RANGE_TYPE_32;
                mvebu_pcie_handle_iobase_change(port);
                break;

        case PCI_MEMORY_BASE:
                bridge->membase = value & 0xffff;
                bridge->memlimit = value >> 16;
                mvebu_pcie_handle_membase_change(port);
                break;

        case PCI_IO_BASE_UPPER16:
                bridge->iobaseupper = value & 0xffff;
                bridge->iolimitupper = value >> 16;
                mvebu_pcie_handle_iobase_change(port);
                break;

        case PCI_PRIMARY_BUS:
                bridge->primary_bus             = value & 0xff;
                bridge->secondary_bus           = (value >> 8) & 0xff;
                bridge->subordinate_bus         = (value >> 16) & 0xff;
                bridge->secondary_latency_timer = (value >> 24) & 0xff;
                mvebu_pcie_set_local_bus_nr(port, bridge->secondary_bus);
                break;

The writes being made to those parts of the config space are being reflected back when reading the config space later.

Quoted 17 lines

drivers/pci/pci-sw-bridge.c line 96

Quoted 6 lines
drivers/pci/pci-sw-bridge.c new file +6 -0
+	if (size == 1)
+		*value = (*value >> (8 * (where & 3))) & 0xff;
+	else if (size == 2)
+		*value = (*value >> (8 * (where & 3))) & 0xffff;
+	else if (size != 4)
+		return PCIBIOS_BAD_REGISTER_NUMBER;

This isn't directly related to your patches, but this is a common pattern with some variations between drivers, which makes me wonder whether there are bugs lurking. These use "where & 3" for size 2:

advk_pcie_rd_conf()
mtk_pcie_hw_rd_cfg()
pci_generic_config_read32()

But these use "where & 2":

Quoted 7 lines

_altera_pcie_cfg_read()
rcar_pcie_read_conf()

I *think* this means that unaligned 2-byte config reads on Altera and R-Car will return incorrect data. In both cases, the actual read seen by the hardware is a 32-bit read, but I think we extract the wrong 16 bits from that result.

Indeed, if unaligned 2-byte config reads are made, they will return bogus results. But are such reads allowed ?

Quoted 1 line

I wonder if there's a way to use a common helper function to do this.

Yes, this small bit of logic is duplicated all over the place. I'll see if I can come up with some reasonable helpers for that.

Quoted 17 lines
Quoted 5 lines
include/linux/pci-sw-bridge.h new file +4 -0
+	return PCIBIOS_SUCCESSFUL;
+}  
 
+	/*
+	 * Called when writing to the regular PCI bridge configuration

include/linux/pci-sw-bridge.h line 105

Quoted 10 lines
include/linux/pci-sw-bridge.h new file +10 -0
+	 * space. old is the current value, new is the new value being
+	 * written, and mask indicates which parts of the value are
+	 * being changed.
+	 */
+	void (*write_base)(struct pci_sw_bridge *bridge, int reg,
+			   u32 old, u32 new, u32 mask);
+
+	/*
+	 * Same as ->read_base(), except it is for reading from the
+	 * PCIe capability configuration space.

I think you mean "->write_base(), except it is for writing to"

Indeed, yes. Will fix that.

So, to sum up, there are really three key questions:

(1) Which name do you want for this ?

(2) Agreeing on whether a RAM-like behavior is acceptable, and if not, which solution do you propose instead.

(3) How much do you want me to convert other drivers vs. the maintainers for those drivers being responsible for doing this.

Since "There are only two hard things in Computer Science: cache invalidation and naming things.", I expect question (1) to be the most difficult one :-)

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#4 lore raw

PATCH 1/3 PCI: Introduce PCI software bridge common logic

Quoted 8 lines · Thomas Petazzoni

Indeed, yes. Will fix that.

So, to sum up, there are really three key questions:

(1) Which name do you want for this ?

(2) Agreeing on whether a RAM-like behavior is acceptable, and if not, which solution do you propose instead.

Please take the time to read the PCI(e) specifications and implement what it recommends, rather than doing something else. That's what I did when I originally reworked the mvebu PCIe driver for Armada 388, and it's the only sensible approach to have something that works with the rest of the Linux PCI(e) layer.

Going off and implementing a non-standard behaviour is a recipe for things breaking as the PCIe kernel support evolves.

The spec requires reserved registers to read back as zero and ignore writes, whereas implemented registers have a mixture of behaviours:

- read/write
- read, write to clear
- read-only

Here's the extract(s):

All PCI devices must treat Configuration Space write operations to reserved registers as no-ops; that is, the access must be completed normally on the bus and the data discarded. Read accesses to reserved or unimplemented registers must be completed normally and a data value of 0 returned.

(eg) PCI status:
Reserved bits should be read-only and return zero when read. A one bit is reset (if it is not read-only) whenever the register is written, and the write data in the corresponding bit location is a 1.

[which is why doing the read-modify-write action that some host
bridges that only support 32-bit accesses is dangerous - it leads to various status bits being inadvertently reset.]

Getting this right in a software emulation of the register space for each bit in every register makes for a happy Linux PCIe layer.

Now, configuration read/writes use naturally aligned addresses. The PCI specification defines the PC IO 0xcf8/0xcfc configuration access mechanism. The first register defines the address with a 6 bit "double-word" address to cover the 256 bytes of standard config space. Accesses to 0xcfc are forwarded to the PCI bus as a single configuration request - and that means there is nothing to deal with an attempt to access a mis-aligned word.

Indeed, if 0xcfe was to be accessed as a double word, this would result in the processor reading the two bytes from IO address 0xcfe, 0xcff, as well as 0xd00 and 0xd01 which are not part of the configuration data register - resulting in undefined data being read.

So, basically, misaligned configuration accesses are not supported.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
author maintainer
#5 lore raw

PATCH 1/3 PCI: Introduce PCI software bridge common logic

Hello Russell,

Thanks for your feedback.

Quoted 8 lines · Russell King - ARM Linux
Quoted 2 lines

(2) Agreeing on whether a RAM-like behavior is acceptable, and if not, which solution do you propose instead.

Please take the time to read the PCI(e) specifications and implement what it recommends, rather than doing something else. That's what I did when I originally reworked the mvebu PCIe driver for Armada 388, and it's the only sensible approach to have something that works with the rest of the Linux PCI(e) layer.

Actually, all what I'm doing is generalizing what the mvebu PCIe driver is doing (which you significantly contributed) to, and try to make it usable by other drivers that have the same need.

So, I'm precisely trying to make sure we implement this stuff once, correctly, rather than duplicate it in several drivers. I'm not pretending that my first iteration is entirely correct, and your review of it to make it better is much appreciated.

Quoted 2 lines

Going off and implementing a non-standard behaviour is a recipe for things breaking as the PCIe kernel support evolves.

Sure.

Quoted 15 lines

The spec requires reserved registers to read back as zero and ignore writes, whereas implemented registers have a mixture of behaviours:

- read/write
- read, write to clear
- read-only

Here's the extract(s):

All PCI devices must treat Configuration Space write operations to reserved registers as no-ops; that is, the access must be completed normally on the bus and the data discarded. Read accesses to reserved or unimplemented registers must be completed normally and a data value of 0 returned.

Quoted 5 lines

(eg) PCI status:
Reserved bits should be read-only and return zero when read. A one bit is reset (if it is not read-only) whenever the register is written, and the write data in the corresponding bit location is a 1.

Right. This pci-sw-bridge layer should implement this behavior.

Quoted 6 lines

[which is why doing the read-modify-write action that some host
bridges that only support 32-bit accesses is dangerous - it leads to various status bits being inadvertently reset.]

Getting this right in a software emulation of the register space for each bit in every register makes for a happy Linux PCIe layer.

Absolutely.

Quoted 8 lines

Now, configuration read/writes use naturally aligned addresses. The PCI specification defines the PC IO 0xcf8/0xcfc configuration access mechanism. The first register defines the address with a 6 bit "double-word" address to cover the 256 bytes of standard config space. Accesses to 0xcfc are forwarded to the PCI bus as a single configuration request - and that means there is nothing to deal with an attempt to access a mis-aligned word.

Quoted 6 lines

Indeed, if 0xcfe was to be accessed as a double word, this would result in the processor reading the two bytes from IO address 0xcfe, 0xcff, as well as 0xd00 and 0xd01 which are not part of the configuration data register - resulting in undefined data being read.

So, basically, misaligned configuration accesses are not supported.

OK. So the where & 2 that the RCAR driver is doing for 2 bytes accesses is OK, because bit 0 of where will always be 0 for a 2 bytes access.

Thanks,

Thomas Petazzoni

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author maintainer
#6 lore raw

PATCH 1/3 PCI: Introduce PCI software bridge common logic

In reply to Russell King - ARM Linux

Hello Russell,

Quoted 12 lines · Russell King - ARM Linux

All PCI devices must treat Configuration Space write operations to reserved registers as no-ops; that is, the access must be completed normally on the bus and the data discarded. Read accesses to reserved or unimplemented registers must be completed normally and a data value of 0 returned.

(eg) PCI status:
Reserved bits should be read-only and return zero when read. A one bit is reset (if it is not read-only) whenever the register is written, and the write data in the corresponding bit location is a 1.

Quoted 3 lines

[which is why doing the read-modify-write action that some host
bridges that only support 32-bit accesses is dangerous - it leads to various status bits being inadvertently reset.]

Speaking of this, the generic pci_generic_config_write32() function indeed does this incorrectly, and prints a warning.

However, I just looked at the pci-thunder-pem code, and it seems to correctly account for those W1C bits, by having an explicit list of registers and their W1C bits (see thunder_pem_bridge_w1c_bits). This isn't specific to the Thunder PCIe controller at all, and would benefit from being made generic, no ?

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author maintainer
#7 lore raw

PATCH 1/3 PCI: Introduce PCI software bridge common logic

In reply to Thomas Petazzoni

Bjorn,

Quoted 4 lines · Thomas Petazzoni
Quoted 1 line

I wonder if there's a way to use a common helper function to do this.

Yes, this small bit of logic is duplicated all over the place. I'll see if I can come up with some reasonable helpers for that.

Here is an attempt at doing this:

- Introduce some helpers

https://github.com/MISL-EBU-System-SW/mainline-public/commit/55543d2050d9aa3abe297569be830bde8680e1e9

- Use them in drivers/pci/access.c

https://github.com/MISL-EBU-System-SW/mainline-public/commit/ce6158d5c6a039e0cddf5ee6840dadeb46c5fc4b

- Use them in PCI host controller drivers

https://github.com/MISL-EBU-System-SW/mainline-public/commit/df4d0b5272ea3b4c3226c96466e5360c5a89253f

I'm not a big fan of the naming though. Let me know what you think, if you think it's worth it, I'll submit the patches.

Note: the whole thing is compile-tested only for now.

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author maintainer
#8 lore raw

PATCH 2/3 PCI: mvebu: Convert to PCI software bridge

In reply to Thomas Petazzoni

This commit convers the pci-mvebu driver to use the recently introduced pci-sw-bridge logic, that helps emulating a root port PCI bridge.

It has been tested on Armada GP XP, with a E1000E NIC.

Signed-off-by Thomas Petazzoni <[email protected]>
---
 drivers/pci/controller/Kconfig     |   1 +
 drivers/pci/controller/pci-mvebu.c | 370 ++++++++++---------------------------
 2 files changed, 102 insertions(+), 269 deletions(-)
drivers/pci/controller/Kconfig +1 -0
index 18fa09b3ac8f..0c307f804c8d 100644
@@ -9,6 +9,7 @@ config PCI_MVEBU
 	depends on MVEBU_MBUS
 	depends on ARM
 	depends on OF
+	select PCI_SW_BRIDGE
 
 config PCI_AARDVARK
 	bool "Aardvark PCIe controller"
drivers/pci/controller/pci-mvebu.c +101 -269

539 lines not on this page · raw message

Signature · 1 line
2.14.4
author maintainer
#9 lore raw

PATCH 3/3 PCI: aardvark: Implement emulated root PCI bridge

In reply to Thomas Petazzoni

From: Zachary Zhang <[email protected]>

The PCI controller in the Marvell Armada 3720 does not implement a root port PCI bridge at the hardware level. This causes a number of problems when using PCIe switches or when the Max Payload size needs to be aligned between the root complex and the endpoint.

Implementing an emulated root PCI bridge, like is already done in the pci-mvebu driver for older Marvell platforms allows to solve those issues, and also to support features such as ASR, PME, VC, HP.

Signed-off-by Zachary Zhang <[email protected]>
[Thomas: convert to the common PCI software bridge logic.]
Signed-off-by Thomas Petazzoni <[email protected]>
---
 drivers/pci/controller/Kconfig        |   1 +
 drivers/pci/controller/pci-aardvark.c | 119 +++++++++++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 3 deletions(-)
drivers/pci/controller/Kconfig +1 -0
index 0c307f804c8d..27b29b3f34e8 100644
@@ -16,6 +16,7 @@ config PCI_AARDVARK
 	depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
+	select PCI_SW_BRIDGE
 	help
 	 Add support for Aardvark 64bit PCIe Host Controller. This
 	 controller is part of the South Bridge of the Marvel Armada
drivers/pci/controller/pci-aardvark.c +116 -3

190 lines not on this page · raw message

Signature · 1 line
2.14.4
#10 lore raw

Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge

drivers/pci/controller/pci-aardvark.c static int advk_pcie_wait_pio(struct advk_pcie *pcie) · line 532

Quoted 6 lines · Thomas Petazzoni
drivers/pci/controller/pci-aardvark.c +5 -0
+static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+{
+	struct pci_sw_bridge *bridge = &pcie->bridge;
 
+	/* Support interrupt A for MSI feature */
+	bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;

Only 3.5 years later, IIUC, this is the value you get when you read PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not PCIE_CORE_INT_A_ASSERT_ENABLE.

Readers expect to get the values defined in the PCI spec, i.e.,

PCI_INTERRUPT_UNKNOWN
PCI_INTERRUPT_INTA
PCI_INTERRUPT_INTB
PCI_INTERRUPT_INTC
PCI_INTERRUPT_INTD

Bjorn

#11 lore raw

Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge

[+cc Pali; sorry, I meant to cc you on this but forgot]

Quoted 22 lines · Bjorn Helgaas

drivers/pci/controller/pci-aardvark.c static int advk_pcie_wait_pio(struct advk_pcie *pcie) · line 532

Quoted 6 lines · Thomas Petazzoni
drivers/pci/controller/pci-aardvark.c +5 -0
+static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+{
+	struct pci_sw_bridge *bridge = &pcie->bridge;
 
+	/* Support interrupt A for MSI feature */
+	bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;

Only 3.5 years later, IIUC, this is the value you get when you read PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not PCIE_CORE_INT_A_ASSERT_ENABLE.

Readers expect to get the values defined in the PCI spec, i.e.,

PCI_INTERRUPT_UNKNOWN
PCI_INTERRUPT_INTA
PCI_INTERRUPT_INTB
PCI_INTERRUPT_INTC
PCI_INTERRUPT_INTD

Bjorn

maintainer
#12 lore raw

Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge

Quoted 25 lines · On Friday 07 January 2022 17:17:34 Bjorn Helgaas wrote:

[+cc Pali; sorry, I meant to cc you on this but forgot]

Quoted 22 lines · Bjorn Helgaas

drivers/pci/controller/pci-aardvark.c static int advk_pcie_wait_pio(struct advk_pcie *pcie) · line 532

Quoted 6 lines · Thomas Petazzoni
drivers/pci/controller/pci-aardvark.c +5 -0
+static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+{
+	struct pci_sw_bridge *bridge = &pcie->bridge;
 
+	/* Support interrupt A for MSI feature */
+	bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;

Only 3.5 years later, IIUC, this is the value you get when you read PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not PCIE_CORE_INT_A_ASSERT_ENABLE.

Readers expect to get the values defined in the PCI spec, i.e.,

PCI_INTERRUPT_UNKNOWN
PCI_INTERRUPT_INTA
PCI_INTERRUPT_INTB
PCI_INTERRUPT_INTC
PCI_INTERRUPT_INTD

Bjorn

Yes! We have a prepared patch for it and Marek now sent it:
https://lore.kernel.org/linux-pci/[email protected]/

#13 lore raw

Re: [PATCH 3/3] PCI: aardvark: Implement emulated root PCI bridge

In reply to Bjorn Helgaas
Quoted 13 lines · On Fri, 7 Jan 2022 15:27:36 -0600 Bjorn Helgaas <[email protected]> wrote:

drivers/pci/controller/pci-aardvark.c static int advk_pcie_wait_pio(struct advk_pcie *pcie) · line 532

Quoted 6 lines · Thomas Petazzoni
drivers/pci/controller/pci-aardvark.c +5 -0
+static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+{
+	struct pci_sw_bridge *bridge = &pcie->bridge;  
 
+	/* Support interrupt A for MSI feature */
+	bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;

Only 3.5 years later, IIUC, this is the value you get when you read PCI_INTERRUPT_PIN, so I think this should be PCI_INTERRUPT_INTA, not PCIE_CORE_INT_A_ASSERT_ENABLE.

Quoted 7 lines

Readers expect to get the values defined in the PCI spec, i.e.,

PCI_INTERRUPT_UNKNOWN
PCI_INTERRUPT_INTA
PCI_INTERRUPT_INTB
PCI_INTERRUPT_INTC
PCI_INTERRUPT_INTD

Hello Bjorn,

now sent v2 of batch 4 of fixes for pci-aardvark, where this is fixed in the first patch.
https://lore.kernel.org/linux-pci/[email protected]/
Could you find time to review the series? :-)

Marek

author maintainer
#14 lore raw

cover PCI: emulated PCI bridge common logic

In reply to Thomas Petazzoni

Bjorn, Lorenzo,

Quoted 8 lines · Thomas Petazzoni

The pci-mvebu driver already contains some logic to emulate a root port PCI bridge. It turns out that we have a similar need for the pci-aardvark driver. Instead of duplicating the same logic in two drivers, this patch series starts by adding a small common infrastructure that helps emulate a root port PCI bridge, converts pci-mvebu to use it, and finally extends pci-aardvark to use it as well.

Quoted 8 lines

Thanks to this, Marvell Armada 3720 based systems, which use the Aarkvark PCI controller, will have better PCI support, by having a root port PCI bridge exposed.

The emulated PCI bridge common logic is a proposal, I very much welcome comments and suggestions. Also, if you feel that adding a common logic for only two drivers is too early, I'm fine with duplicating a bit of code betwen pci-mvebu and pci-aardvark.

I was wondering if you had any feedback on this patch series. Not necessarily a detailed review, but at least some general feeling/feedback on the overall approach would be very nice.

Thanks a lot,

Thomas Petazzoni

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com