- The code relies on rc_pci_fixup being called, which only happens
when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
this causes a booting failure with a non-obvious cause.
- Update rc_pci_fixup to set the class properly, copying the
more modern style from other places
- Correct the rc_pci_fixup comment
This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
controller.
Signed-off-by: Pali Rohár <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: [email protected]
---
arch/arm/Kconfig | 1 +
arch/arm/mach-dove/pcie.c | 11 ++++++++---
arch/arm/mach-mv78xx0/pcie.c | 11 ++++++++---
arch/arm/mach-orion5x/Kconfig | 1 +
arch/arm/mach-orion5x/pci.c | 12 +++++++++---
arch/mips/Kconfig | 1 +
arch/mips/pci/fixup-cobalt.c | 6 ++++++
7 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index fc196421b2ce..9f157e973555 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -400,6 +400,7 @@ config ARCH_DOVE
select GENERIC_IRQ_MULTI_HANDLER
select GPIOLIB
select HAVE_PCI
+ select PCI_QUIRKS if PCI
select MVEBU_MBUS
select PINCTRL
select PINCTRL_DOVE
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index ee91ac6b5ebf..ecf057a0f5ba 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -135,14 +135,19 @@ static struct pci_ops pcie_ops = {
.write = pcie_wr_conf,
};
+/*
+ * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
+ * is operating as a root complex this needs to be switched to
+ * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
+ * the device. Decoding setup is handled by the orion code.
+ */
static void rc_pci_fixup(struct pci_dev *dev)
{
- /*
- * Prevent enumeration of root complex.
- */
if (dev->bus->parent == NULL && dev->devfn == 0) {
int i;
+ dev->class &= 0xff;
+ dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
dev->resource[i].start = 0;
dev->resource[i].end = 0;
diff --git a/arch/arm/mach-mv78xx0/pcie.c b/arch/arm/mach-mv78xx0/pcie.c
index 636d84b40466..9362b5fc116f 100644
--- a/arch/arm/mach-mv78xx0/pcie.c
+++ b/arch/arm/mach-mv78xx0/pcie.c
@@ -177,14 +177,19 @@ static struct pci_ops pcie_ops = {
.write = pcie_wr_conf,
};
+/*
+ * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
+ * is operating as a root complex this needs to be switched to
+ * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
+ * the device. Decoding setup is handled by the orion code.
+ */
static void rc_pci_fixup(struct pci_dev *dev)
{
- /*
- * Prevent enumeration of root complex.
- */
if (dev->bus->parent == NULL && dev->devfn == 0) {
int i;
+ dev->class &= 0xff;
+ dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
dev->resource[i].start = 0;
dev->resource[i].end = 0;
diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
index e94a61901ffd..7189a5b1ec46 100644
--- a/arch/arm/mach-orion5x/Kconfig
+++ b/arch/arm/mach-orion5x/Kconfig
@@ -6,6 +6,7 @@ menuconfig ARCH_ORION5X
select GPIOLIB
select MVEBU_MBUS
select FORCE_PCI
+ select PCI_QUIRKS
select PHYLIB if NETDEVICES
select PLAT_ORION_LEGACY
help
diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
index 76951bfbacf5..5145fe89702e 100644
--- a/arch/arm/mach-orion5x/pci.c
+++ b/arch/arm/mach-orion5x/pci.c
@@ -509,14 +509,20 @@ static int __init pci_setup(struct pci_sys_data *sys)
/*****************************************************************************
* General PCIe + PCI
****************************************************************************/
+
+/*
+ * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
+ * is operating as a root complex this needs to be switched to
+ * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
+ * the device. Decoding setup is handled by the orion code.
+ */
static void rc_pci_fixup(struct pci_dev *dev)
{
- /*
- * Prevent enumeration of root complex.
- */
if (dev->bus->parent == NULL && dev->devfn == 0) {
int i;
+ dev->class &= 0xff;
+ dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
dev->resource[i].start = 0;
dev->resource[i].end = 0;
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 771ca53af06d..c8d51bd20b84 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -346,6 +346,7 @@ config MIPS_COBALT
select CEVT_GT641XX
select DMA_NONCOHERENT
select FORCE_PCI
+ select PCI_QUIRKS
select I8253
select I8259
select IRQ_MIPS_CPU
diff --git a/arch/mips/pci/fixup-cobalt.c b/arch/mips/pci/fixup-cobalt.c
index 44be65c3e6bb..202f3a0bd97d 100644
--- a/arch/mips/pci/fixup-cobalt.c
+++ b/arch/mips/pci/fixup-cobalt.c
@@ -36,6 +36,12 @@
#define VIA_COBALT_BRD_ID_REG 0x94
#define VIA_COBALT_BRD_REG_to_ID(reg) ((unsigned char)(reg) >> 4)
+/*
+ * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
+ * is operating as a root complex this needs to be switched to
+ * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
+ * the device. Decoding setup is handled by the orion code.
+ */
static void qube_raq_galileo_early_fixup(struct pci_dev *dev)
{
if (dev->devfn == PCI_DEVFN(0, 0) &&
--
2.20.1
On Mon, Nov 01, 2021 at 04:04:05PM +0100, Pali Rohár wrote:
> - The code relies on rc_pci_fixup being called, which only happens
> when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> this causes a booting failure with a non-obvious cause.
> - Update rc_pci_fixup to set the class properly, copying the
> more modern style from other places
> - Correct the rc_pci_fixup comment
>
> This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
> controller.
I wonder if that code is even relevant any more since we started using
CONFIG_PCI_MVEBU
?
Really, these broken controllers should not be used "raw" but always
via their special host bridge driver that fixes all the config space
problems.
Jason
On Monday 01 November 2021 13:27:11 Jason Gunthorpe wrote:
> On Mon, Nov 01, 2021 at 04:04:05PM +0100, Pali Rohár wrote:
> > - The code relies on rc_pci_fixup being called, which only happens
> > when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> > this causes a booting failure with a non-obvious cause.
> > - Update rc_pci_fixup to set the class properly, copying the
> > more modern style from other places
> > - Correct the rc_pci_fixup comment
> >
> > This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> > PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
> > controller.
>
> I wonder if that code is even relevant any more since we started using
> CONFIG_PCI_MVEBU
>
> ?
It is (still) relevant for platforms which do not use CONFIG_PCI_MVEBU
yet.
> Really, these broken controllers should not be used "raw" but always
> via their special host bridge driver that fixes all the config space
> problems.
I agree.
Long-term goal should be to convert these platforms to use pci-mvebu.c
driver. And until it happens simple fixes like in commit 1dc831bf53fd is
needed for all affected Marvell platforms.
Some details how these Marvell PCIe controllers are broken is in email:
https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/
On Mon, Nov 01, 2021 at 06:56:49PM +0100, Pali Rohár wrote:
> On Monday 01 November 2021 13:27:11 Jason Gunthorpe wrote:
> > On Mon, Nov 01, 2021 at 04:04:05PM +0100, Pali Rohár wrote:
> > > - The code relies on rc_pci_fixup being called, which only happens
> > > when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> > > this causes a booting failure with a non-obvious cause.
> > > - Update rc_pci_fixup to set the class properly, copying the
> > > more modern style from other places
> > > - Correct the rc_pci_fixup comment
> > >
> > > This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> > > PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
> > > controller.
> >
> > I wonder if that code is even relevant any more since we started using
> > CONFIG_PCI_MVEBU
> >
> > ?
>
> It is (still) relevant for platforms which do not use CONFIG_PCI_MVEBU
> yet.
I think you should explain this in the commit message
> > Really, these broken controllers should not be used "raw" but always
> > via their special host bridge driver that fixes all the config space
> > problems.
>
> I agree.
>
> Long-term goal should be to convert these platforms to use pci-mvebu.c
> driver. And until it happens simple fixes like in commit 1dc831bf53fd is
> needed for all affected Marvell platforms.
IIRC all these platforms were obsolete before I wrote the above
commit, so I'm not sure why this has suddenly cropped up?
If you want to use a new kernel on this really old HW then update to
use the right PCI driver?
> Some details how these Marvell PCIe controllers are broken is in email:
> https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/
Yes, everything about this controller is broken. It does not function
properly without its driver. The solution to these bugs is to use the
driver, which was specifically made to deal with them.
That said, I can't recall if this driver needs some fixing to work
with pre-kirkwood systems..
Jason
On Mon, Nov 01, 2021 at 04:04:05PM +0100, Pali Roh?r wrote:
> - The code relies on rc_pci_fixup being called, which only happens
> when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> this causes a booting failure with a non-obvious cause.
> - Update rc_pci_fixup to set the class properly, copying the
> more modern style from other places
> - Correct the rc_pci_fixup comment
>
> This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
> controller.
> [..]
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 771ca53af06d..c8d51bd20b84 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -346,6 +346,7 @@ config MIPS_COBALT
> select CEVT_GT641XX
> select DMA_NONCOHERENT
> select FORCE_PCI
> + select PCI_QUIRKS
> select I8253
> select I8259
> select IRQ_MIPS_CPU
this is enabled by default, via drivers/pci/Kconfig
config PCI_QUIRKS
default y
bool "Enable PCI quirk workarounds" if EXPERT
help
This enables workarounds for various PCI chipset bugs/quirks.
Disable this only if your target machine is unaffected by PCI
quirks.
> diff --git a/arch/mips/pci/fixup-cobalt.c b/arch/mips/pci/fixup-cobalt.c
> index 44be65c3e6bb..202f3a0bd97d 100644
> --- a/arch/mips/pci/fixup-cobalt.c
> +++ b/arch/mips/pci/fixup-cobalt.c
> @@ -36,6 +36,12 @@
> #define VIA_COBALT_BRD_ID_REG 0x94
> #define VIA_COBALT_BRD_REG_to_ID(reg) ((unsigned char)(reg) >> 4)
>
> +/*
> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> + * is operating as a root complex this needs to be switched to
> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> + * the device. Decoding setup is handled by the orion code.
> + */
> static void qube_raq_galileo_early_fixup(struct pci_dev *dev)
> {
> if (dev->devfn == PCI_DEVFN(0, 0) &&
this is not a PCIe controller, so how is this patch related ?
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
On Tuesday 02 November 2021 09:42:41 Thomas Bogendoerfer wrote:
> On Mon, Nov 01, 2021 at 04:04:05PM +0100, Pali Rohár wrote:
> > - The code relies on rc_pci_fixup being called, which only happens
> > when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> > this causes a booting failure with a non-obvious cause.
> > - Update rc_pci_fixup to set the class properly, copying the
> > more modern style from other places
> > - Correct the rc_pci_fixup comment
> >
> > This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> > PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
> > controller.
> > [..]
>
> > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> > index 771ca53af06d..c8d51bd20b84 100644
> > --- a/arch/mips/Kconfig
> > +++ b/arch/mips/Kconfig
> > @@ -346,6 +346,7 @@ config MIPS_COBALT
> > select CEVT_GT641XX
> > select DMA_NONCOHERENT
> > select FORCE_PCI
> > + select PCI_QUIRKS
> > select I8253
> > select I8259
> > select IRQ_MIPS_CPU
>
> this is enabled by default, via drivers/pci/Kconfig
IIRC 'default y' can be disabled but 'select' not.
>
> config PCI_QUIRKS
> default y
> bool "Enable PCI quirk workarounds" if EXPERT
> help
> This enables workarounds for various PCI chipset bugs/quirks.
> Disable this only if your target machine is unaffected by PCI
> quirks.
>
> > diff --git a/arch/mips/pci/fixup-cobalt.c b/arch/mips/pci/fixup-cobalt.c
> > index 44be65c3e6bb..202f3a0bd97d 100644
> > --- a/arch/mips/pci/fixup-cobalt.c
> > +++ b/arch/mips/pci/fixup-cobalt.c
> > @@ -36,6 +36,12 @@
> > #define VIA_COBALT_BRD_ID_REG 0x94
> > #define VIA_COBALT_BRD_REG_to_ID(reg) ((unsigned char)(reg) >> 4)
> >
> > +/*
> > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > + * is operating as a root complex this needs to be switched to
> > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > + * the device. Decoding setup is handled by the orion code.
> > + */
> > static void qube_raq_galileo_early_fixup(struct pci_dev *dev)
> > {
> > if (dev->devfn == PCI_DEVFN(0, 0) &&
>
> this is not a PCIe controller, so how is this patch related ?
I put that comment into all quirk code which is related to Marvell PCIe
device XX:00.0 and changes PCI class type from PCI_CLASS_MEMORY_OTHER to
PCI_CLASS_BRIDGE_HOST.
From all what I saw, I'm sure that this device with this specific
characteristics is really (non-compliant) Marvell PCIe controller.
But I do not have this hardware to verify it.
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]
On Tue, Nov 02, 2021 at 10:02:46AM +0100, Pali Roh?r wrote:
> On Tuesday 02 November 2021 09:42:41 Thomas Bogendoerfer wrote:
> > On Mon, Nov 01, 2021 at 04:04:05PM +0100, Pali Roh?r wrote:
> > > - The code relies on rc_pci_fixup being called, which only happens
> > > when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> > > this causes a booting failure with a non-obvious cause.
> > > - Update rc_pci_fixup to set the class properly, copying the
> > > more modern style from other places
> > > - Correct the rc_pci_fixup comment
> > >
> > > This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> > > PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
> > > controller.
> > > [..]
> >
> > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> > > index 771ca53af06d..c8d51bd20b84 100644
> > > --- a/arch/mips/Kconfig
> > > +++ b/arch/mips/Kconfig
> > > @@ -346,6 +346,7 @@ config MIPS_COBALT
> > > select CEVT_GT641XX
> > > select DMA_NONCOHERENT
> > > select FORCE_PCI
> > > + select PCI_QUIRKS
> > > select I8253
> > > select I8259
> > > select IRQ_MIPS_CPU
> >
> > this is enabled by default, via drivers/pci/Kconfig
>
> IIRC 'default y' can be disabled but 'select' not.
overruled only if CONFIG_EXPERT is enabled, which IMHO sounds good enough.
> > config PCI_QUIRKS
> > default y
> > bool "Enable PCI quirk workarounds" if EXPERT
> > help
> > This enables workarounds for various PCI chipset bugs/quirks.
> > Disable this only if your target machine is unaffected by PCI
> > quirks.
> >
> > > diff --git a/arch/mips/pci/fixup-cobalt.c b/arch/mips/pci/fixup-cobalt.c
> > > index 44be65c3e6bb..202f3a0bd97d 100644
> > > --- a/arch/mips/pci/fixup-cobalt.c
> > > +++ b/arch/mips/pci/fixup-cobalt.c
> > > @@ -36,6 +36,12 @@
> > > #define VIA_COBALT_BRD_ID_REG 0x94
> > > #define VIA_COBALT_BRD_REG_to_ID(reg) ((unsigned char)(reg) >> 4)
> > >
> > > +/*
> > > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > > + * is operating as a root complex this needs to be switched to
> > > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > > + * the device. Decoding setup is handled by the orion code.
> > > + */
> > > static void qube_raq_galileo_early_fixup(struct pci_dev *dev)
> > > {
> > > if (dev->devfn == PCI_DEVFN(0, 0) &&
> >
> > this is not a PCIe controller, so how is this patch related ?
>
> I put that comment into all quirk code which is related to Marvell PCIe
> device XX:00.0 and changes PCI class type from PCI_CLASS_MEMORY_OTHER to
> PCI_CLASS_BRIDGE_HOST.
>
> >From all what I saw, I'm sure that this device with this specific
> characteristics is really (non-compliant) Marvell PCIe controller.
just nitpicking, it's a Galileo PCI bridge and not PCIe.
> But I do not have this hardware to verify it.
I still have a few Cobalt systems here.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
On Tuesday 02 November 2021 10:47:00 Thomas Bogendoerfer wrote:
> On Tue, Nov 02, 2021 at 10:02:46AM +0100, Pali Rohár wrote:
> > On Tuesday 02 November 2021 09:42:41 Thomas Bogendoerfer wrote:
> > > On Mon, Nov 01, 2021 at 04:04:05PM +0100, Pali Rohár wrote:
> > > > - The code relies on rc_pci_fixup being called, which only happens
> > > > when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> > > > this causes a booting failure with a non-obvious cause.
> > > > - Update rc_pci_fixup to set the class properly, copying the
> > > > more modern style from other places
> > > > - Correct the rc_pci_fixup comment
> > > >
> > > > This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> > > > PCI-E fixup") for all other Marvell platforms which use same buggy PCIe
> > > > controller.
> > > > [..]
> > >
> > > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> > > > index 771ca53af06d..c8d51bd20b84 100644
> > > > --- a/arch/mips/Kconfig
> > > > +++ b/arch/mips/Kconfig
> > > > @@ -346,6 +346,7 @@ config MIPS_COBALT
> > > > select CEVT_GT641XX
> > > > select DMA_NONCOHERENT
> > > > select FORCE_PCI
> > > > + select PCI_QUIRKS
> > > > select I8253
> > > > select I8259
> > > > select IRQ_MIPS_CPU
> > >
> > > this is enabled by default, via drivers/pci/Kconfig
> >
> > IIRC 'default y' can be disabled but 'select' not.
>
> overruled only if CONFIG_EXPERT is enabled, which IMHO sounds good enough.
Well, if you think this is not needed (anymore), I can drop it. I just
reapplied original fix 1dc831bf53fd.
> > > config PCI_QUIRKS
> > > default y
> > > bool "Enable PCI quirk workarounds" if EXPERT
> > > help
> > > This enables workarounds for various PCI chipset bugs/quirks.
> > > Disable this only if your target machine is unaffected by PCI
> > > quirks.
> > >
> > > > diff --git a/arch/mips/pci/fixup-cobalt.c b/arch/mips/pci/fixup-cobalt.c
> > > > index 44be65c3e6bb..202f3a0bd97d 100644
> > > > --- a/arch/mips/pci/fixup-cobalt.c
> > > > +++ b/arch/mips/pci/fixup-cobalt.c
> > > > @@ -36,6 +36,12 @@
> > > > #define VIA_COBALT_BRD_ID_REG 0x94
> > > > #define VIA_COBALT_BRD_REG_to_ID(reg) ((unsigned char)(reg) >> 4)
> > > >
> > > > +/*
> > > > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > > > + * is operating as a root complex this needs to be switched to
> > > > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > > > + * the device. Decoding setup is handled by the orion code.
> > > > + */
> > > > static void qube_raq_galileo_early_fixup(struct pci_dev *dev)
> > > > {
> > > > if (dev->devfn == PCI_DEVFN(0, 0) &&
> > >
> > > this is not a PCIe controller, so how is this patch related ?
> >
> > I put that comment into all quirk code which is related to Marvell PCIe
> > device XX:00.0 and changes PCI class type from PCI_CLASS_MEMORY_OTHER to
> > PCI_CLASS_BRIDGE_HOST.
> >
> > >From all what I saw, I'm sure that this device with this specific
> > characteristics is really (non-compliant) Marvell PCIe controller.
>
> just nitpicking, it's a Galileo PCI bridge and not PCIe.
Marvell acquired Galileo Technology in the past, so it is possible that
this bad design is originated in Galileo. And maybe same for PCIe from
PCI. At least PCI vendor id for all (new) PCIe controllers is this one.
> > But I do not have this hardware to verify it.
>
> I still have a few Cobalt systems here.
Perfect! It would help if you could provide 'lspci -nn -vv' output from
that system. In case you have very old version of lspci on that system
you could try to run it with '-xxxx' (or '-xxx') which prints hexdump
and I can parse it with local lspci.
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]
On Tue, 2 Nov 2021, Pali Rohár wrote:
> > > >From all what I saw, I'm sure that this device with this specific
> > > characteristics is really (non-compliant) Marvell PCIe controller.
> >
> > just nitpicking, it's a Galileo PCI bridge and not PCIe.
>
> Marvell acquired Galileo Technology in the past, so it is possible that
> this bad design is originated in Galileo. And maybe same for PCIe from
> PCI. At least PCI vendor id for all (new) PCIe controllers is this one.
Umm, PCIe is so different hardware-wise from PCI I doubt there's any
chance for errata to be carried across. Plus the MIPS SysAD bus is so
different from other CPU buses. And we're talking 20+ years old Galileo
devices here.
None of the Galileo system controllers I came across had the class code
set incorrectly.
> > > But I do not have this hardware to verify it.
> >
> > I still have a few Cobalt systems here.
>
> Perfect! It would help if you could provide 'lspci -nn -vv' output from
> that system. In case you have very old version of lspci on that system
> you could try to run it with '-xxxx' (or '-xxx') which prints hexdump
> and I can parse it with local lspci.
For the record here's one from a core card used with a Malta (as with
arch/mips/pci/fixup-malta.c); it has a newer 64120A chip (marked as an
engineering sample BTW):
00:00.0 Host bridge: Marvell Technology Group Ltd. GT-64120/64120A/64121A System Controller (rev 11)
00: ab 11 20 46 47 01 80 22 11 00 00 06 00 20 00 00
10: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 00
20: 00 00 e0 1b 00 00 00 00 00 00 00 00 00 00 00 00
30: 00 00 00 00 00 00 00 00 00 00 00 00 ff 01 00 00
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
The lack of a quirk with a platform does not mean it cannot have a certain
PCI/e device.
As I recall various Atlas/Malta core cards had any of the three device
variants covered by this vendor:device ID and later batches were actually
indeed marked Marvell rather than Galileo.
Maciej
On Tuesday 02 November 2021 12:35:41 Maciej W. Rozycki wrote:
> On Tue, 2 Nov 2021, Pali Rohár wrote:
>
> > > > >From all what I saw, I'm sure that this device with this specific
> > > > characteristics is really (non-compliant) Marvell PCIe controller.
> > >
> > > just nitpicking, it's a Galileo PCI bridge and not PCIe.
> >
> > Marvell acquired Galileo Technology in the past, so it is possible that
> > this bad design is originated in Galileo. And maybe same for PCIe from
> > PCI. At least PCI vendor id for all (new) PCIe controllers is this one.
>
> Umm, PCIe is so different hardware-wise from PCI
Yes hardware is different. But software is same and fully backward
compatible. And base part of config space is same for both PCI an PCIe.
So it is possible to copy+paste HDL code which fills initial values into
config space from old PCI IPs to new PCIe IPs.
> I doubt there's any
> chance for errata to be carried across. Plus the MIPS SysAD bus is so
> different from other CPU buses. And we're talking 20+ years old Galileo
> devices here.
>
> None of the Galileo system controllers I came across had the class code
> set incorrectly.
In kernel there is quirk only for one device with id:
PCI_VENDOR_ID_MARVELL (0x11ab) PCI_DEVICE_ID_MARVELL_GT64111 (0x4146)
So for some reasons quirk is needed... Anyway, patch for this quirk just
adds comment as there is no explanation for it. It does not modify quirk
code.
So it is possible that Marvell (or rather Galileo at that time) included
some config space fixup in some products and 0x4146 did not have it.
Just guessing... We can really only guess what could happen at that time
20 years ago...
Running git blame told me that this class code quirk was introduce in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c4ed38a0c6e2e5c4906296758f816ee71373792f
But there is also no information...
> > > > But I do not have this hardware to verify it.
> > >
> > > I still have a few Cobalt systems here.
> >
> > Perfect! It would help if you could provide 'lspci -nn -vv' output from
> > that system. In case you have very old version of lspci on that system
> > you could try to run it with '-xxxx' (or '-xxx') which prints hexdump
> > and I can parse it with local lspci.
>
> For the record here's one from a core card used with a Malta (as with
> arch/mips/pci/fixup-malta.c); it has a newer 64120A chip (marked as an
> engineering sample BTW):
>
> 00:00.0 Host bridge: Marvell Technology Group Ltd. GT-64120/64120A/64121A System Controller (rev 11)
> 00: ab 11 20 46 47 01 80 22 11 00 00 06 00 20 00 00
> 10: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 00
> 20: 00 00 e0 1b 00 00 00 00 00 00 00 00 00 00 00 00
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 ff 01 00 00
> 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> The lack of a quirk with a platform does not mean it cannot have a certain
> PCI/e device.
This is 11ab:4620 device an there is no PCIe capability in its config
space (you can inspect it via 'lspci -F dump.txt -nn -vv' but there is
nothing interesting).
> As I recall various Atlas/Malta core cards had any of the three device
> variants covered by this vendor:device ID and later batches were actually
> indeed marked Marvell rather than Galileo.
>
> Maciej
On Tue, 2 Nov 2021, Pali Rohár wrote:
> > None of the Galileo system controllers I came across had the class code
> > set incorrectly.
>
> In kernel there is quirk only for one device with id:
> PCI_VENDOR_ID_MARVELL (0x11ab) PCI_DEVICE_ID_MARVELL_GT64111 (0x4146)
>
> So for some reasons quirk is needed... Anyway, patch for this quirk just
> adds comment as there is no explanation for it. It does not modify quirk
> code.
>
> So it is possible that Marvell (or rather Galileo at that time) included
> some config space fixup in some products and 0x4146 did not have it.
> Just guessing... We can really only guess what could happen at that time
> 20 years ago...
Ah, there you go! -- sadly I don't seem to have a copy of the datasheet
for the GT-64111, but the GT-64115 has it[1]:
Table 158: PCI Class Code and Revision ID, Offset: 0x008
Bits Field name Function Initial Value
7:0 RevID Indicates the GT-64115 PCI Revision 0x01
number.
15:8 Reserved Read only. 0x0
23:16 SubClass Indicates the GT-64115 Subclass - Mem- 0x80
ory controller.
31:24 BaseClass Indicates the GT-64115 Base Class - 0x05
memory controller.
and then:
"Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
Header Type (0x00e) fields are read only from the PCI bus. These fields
can be modified and read via the CPU bus."
Likewise with the GT-64120[2]:
Table 208: PCI_0 Class Code and Revision ID, Offset: 0x008 from PCI_0 or CPU; 0x088 from
PCI_1
Bits Field name Function Initial Value
7:0 RevID Indicates the GT-64120 PCI_0 revision number. 0x02
15:8 Reserved Read Only 0. 0x0
23:16 SubClass Indicates the GT-64120 Subclass Depends on value
0x00 - Host Bridge Device. sampled at reset
0x80 - Memory Device. on BankSel[0]. See
Table 44 on page
11-1.
31:24 BaseClass Indicates the GT-64120 Base Class Depends on value
0x06 - Bridge Device. sampled at reset
0x05 - Memory Device. on BankSel[0]. See
Table 44 on page
11-1.
Table 209: PCI_1 Class Code and Revision ID, Offset: 0x088 from PCI_0 or CPU; 0x008 from
PCI_1
Bits Field name Function Initial Value
31:0 Various Same as for PCI_0 Class Code and Revision ID.
and then also:
"Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
Header Type (0x00e) fields are read only from the PCI bus. These fields
can be modified and read via the CPU bus."
-- so this is system-specific and an intended chip feature rather than an
erratum (or rather it is a system erratum if the reset strap or the boot
firmware has got it wrong).
The memory device class code is IIUC meant to be typically chosen when
the Galileo/Marvell device is used without the CPU interface, i.e. as a
PCI memory controller device only[3].
> > The lack of a quirk with a platform does not mean it cannot have a certain
> > PCI/e device.
>
> This is 11ab:4620 device an there is no PCIe capability in its config
> space (you can inspect it via 'lspci -F dump.txt -nn -vv' but there is
> nothing interesting).
Of course, just as Thomas told you about the GT-64111 too. But you were
right in that the memory controller feature seems shared across all the
chip line, whether PCI or PCIe.
References:
[1] "GT-64115 System Controller for RC4640, RM523X, and VR4300 CPUs",
Galileo Technology, Datasheet Revision 1.11, APR 04, 2000, Section
18.16 "PCI Configuration", p. 161
[2] "GT-64120 System Controller For RC4650/4700/5000 and RM526X/527X/7000
CPUs", Galileo Technology, Datasheet Revision 1.4, SEP 14, 1999,
Section 17.16 "PCI Configuration", p. 17-59
[3] same, Chapter 14. "Using the GT-64120 Without the CPU Interface", p.
14-1
Maciej
On Tuesday 02 November 2021 14:01:41 Maciej W. Rozycki wrote:
> On Tue, 2 Nov 2021, Pali Rohár wrote:
>
> > > None of the Galileo system controllers I came across had the class code
> > > set incorrectly.
> >
> > In kernel there is quirk only for one device with id:
> > PCI_VENDOR_ID_MARVELL (0x11ab) PCI_DEVICE_ID_MARVELL_GT64111 (0x4146)
> >
> > So for some reasons quirk is needed... Anyway, patch for this quirk just
> > adds comment as there is no explanation for it. It does not modify quirk
> > code.
> >
> > So it is possible that Marvell (or rather Galileo at that time) included
> > some config space fixup in some products and 0x4146 did not have it.
> > Just guessing... We can really only guess what could happen at that time
> > 20 years ago...
>
> Ah, there you go! -- sadly I don't seem to have a copy of the datasheet
> for the GT-64111, but the GT-64115 has it[1]:
>
> Table 158: PCI Class Code and Revision ID, Offset: 0x008
> Bits Field name Function Initial Value
> 7:0 RevID Indicates the GT-64115 PCI Revision 0x01
> number.
> 15:8 Reserved Read only. 0x0
> 23:16 SubClass Indicates the GT-64115 Subclass - Mem- 0x80
> ory controller.
> 31:24 BaseClass Indicates the GT-64115 Base Class - 0x05
> memory controller.
>
> and then:
>
> "Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
> Header Type (0x00e) fields are read only from the PCI bus. These fields
> can be modified and read via the CPU bus."
>
> Likewise with the GT-64120[2]:
>
> Table 208: PCI_0 Class Code and Revision ID, Offset: 0x008 from PCI_0 or CPU; 0x088 from
> PCI_1
> Bits Field name Function Initial Value
> 7:0 RevID Indicates the GT-64120 PCI_0 revision number. 0x02
> 15:8 Reserved Read Only 0. 0x0
> 23:16 SubClass Indicates the GT-64120 Subclass Depends on value
> 0x00 - Host Bridge Device. sampled at reset
> 0x80 - Memory Device. on BankSel[0]. See
> Table 44 on page
> 11-1.
> 31:24 BaseClass Indicates the GT-64120 Base Class Depends on value
> 0x06 - Bridge Device. sampled at reset
> 0x05 - Memory Device. on BankSel[0]. See
> Table 44 on page
> 11-1.
>
> Table 209: PCI_1 Class Code and Revision ID, Offset: 0x088 from PCI_0 or CPU; 0x008 from
> PCI_1
> Bits Field name Function Initial Value
> 31:0 Various Same as for PCI_0 Class Code and Revision ID.
>
> and then also:
>
> "Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
> Header Type (0x00e) fields are read only from the PCI bus. These fields
> can be modified and read via the CPU bus."
>
> -- so this is system-specific and an intended chip feature rather than an
> erratum (or rather it is a system erratum if the reset strap or the boot
> firmware has got it wrong).
>
> The memory device class code is IIUC meant to be typically chosen when
> the Galileo/Marvell device is used without the CPU interface, i.e. as a
> PCI memory controller device only[3].
>
> > > The lack of a quirk with a platform does not mean it cannot have a certain
> > > PCI/e device.
> >
> > This is 11ab:4620 device an there is no PCIe capability in its config
> > space (you can inspect it via 'lspci -F dump.txt -nn -vv' but there is
> > nothing interesting).
>
> Of course, just as Thomas told you about the GT-64111 too. But you were
> right in that the memory controller feature seems shared across all the
> chip line, whether PCI or PCIe.
>
> References:
>
> [1] "GT-64115 System Controller for RC4640, RM523X, and VR4300 CPUs",
> Galileo Technology, Datasheet Revision 1.11, APR 04, 2000, Section
> 18.16 "PCI Configuration", p. 161
>
> [2] "GT-64120 System Controller For RC4650/4700/5000 and RM526X/527X/7000
> CPUs", Galileo Technology, Datasheet Revision 1.4, SEP 14, 1999,
> Section 17.16 "PCI Configuration", p. 17-59
>
> [3] same, Chapter 14. "Using the GT-64120 Without the CPU Interface", p.
> 14-1
>
> Maciej
Hello Maciej! Thank you very much for the explanation!
Now it makes sense and looks like that for GT-64111 it is "system
erratum" that strapping pins are incorrectly set which leads to wrong
PCI class code.
I will update comment for GT-64111 quirk in v2.
I'm surprised that Marvell copied this 20 years old MIPS Galileo PCI
logic into followup ARM SoC PCIe IPs (and later also into recent ARM64
A3720 SoC PCIe IP), removed configuration of PCI class code via
strapping pins and let default PCI class code value to Memory device,
even also when PCIe controller is running in Root Complex mode. And so
correction can be done only from "CPU bus".
Just Marvell forgot to include chapter about usage without CPU interface
in new ARM and ARM64 SoCs and origin/usage of that Memory Controller
Device was lost in history, even Marvell people was not able to figure
out what was wrong with PCIe IP in their ARM SoCs...
https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/
Maciej, if I had known that you have this kind of information I would
have written you year ago :-)
On Tuesday 02 November 2021 16:02:01 Thomas Bogendoerfer wrote:
> On Tue, Nov 02, 2021 at 11:00:34AM +0100, Pali Rohár wrote:
> > > > But I do not have this hardware to verify it.
> > >
> > > I still have a few Cobalt systems here.
> >
> > Perfect! It would help if you could provide 'lspci -nn -vv' output from
> > that system. In case you have very old version of lspci on that system
> > you could try to run it with '-xxxx' (or '-xxx') which prints hexdump
> > and I can parse it with local lspci.
>
> not sure, if you still needed:
>
> root@raq2:~# lspci -nn -vv
> 00:00.0 Host bridge [0600]: Marvell Technology Group Ltd. Device [11ab:4146] (rev 11)
> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
> Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ >SERR- <PERR+ INTx-
> Latency: 64, Cache Line Size: 32 bytes
> Interrupt: pin A routed to IRQ 0
> Region 1: Memory at 08000000 (32-bit, non-prefetchable) [size=128M]
> Region 2: Memory at 1c000000 (32-bit, non-prefetchable) [size=32M]
> Region 3: Memory at 1f000000 (32-bit, non-prefetchable) [size=16M]
> Region 4: Memory at 14000000 (32-bit, non-prefetchable) [size=4K]
> Region 5: I/O ports at 4000000 [disabled] [size=4K]
>
>
> root@raq2:~# lspci -xxxx
> 00:00.0 Host bridge: Marvell Technology Group Ltd. Device 4146 (rev 11)
> 00: ab 11 46 41 06 00 80 a2 11 00 80 05 08 40 00 00
^^ ^^ ^^
Here is class code
So it confirms that PCI Class code is 0580 which is Memory Controller.
And not Host Bridge as it should be.
If I put this hexdump into dump.txt and run 'lspci -F dump.txt -nn' then I see:
00:00.0 Memory controller [0580]: Marvell Technology Group Ltd. Device [11ab:4146] (rev 11)
In your output above is "Host bridge" which means that quirk was applied:
00:00.0 Host bridge [0600]: Marvell Technology Group Ltd. Device [11ab:4146] (rev 11)
(I guess in 'lspci -nn -vv -b' should be Memory controller as lspci with
'-b' should not see that quirk change)
> 10: 00 00 00 00 00 00 00 08 00 00 00 1c 00 00 00 1f
> 20: 00 00 00 14 01 00 00 14 00 00 00 00 00 00 00 00
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00
> 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]
On Tue, Nov 02, 2021 at 11:00:34AM +0100, Pali Roh?r wrote:
> > > But I do not have this hardware to verify it.
> >
> > I still have a few Cobalt systems here.
>
> Perfect! It would help if you could provide 'lspci -nn -vv' output from
> that system. In case you have very old version of lspci on that system
> you could try to run it with '-xxxx' (or '-xxx') which prints hexdump
> and I can parse it with local lspci.
not sure, if you still needed:
root@raq2:~# lspci -nn -vv
00:00.0 Host bridge [0600]: Marvell Technology Group Ltd. Device [11ab:4146] (rev 11)
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ >SERR- <PERR+ INTx-
Latency: 64, Cache Line Size: 32 bytes
Interrupt: pin A routed to IRQ 0
Region 1: Memory at 08000000 (32-bit, non-prefetchable) [size=128M]
Region 2: Memory at 1c000000 (32-bit, non-prefetchable) [size=32M]
Region 3: Memory at 1f000000 (32-bit, non-prefetchable) [size=16M]
Region 4: Memory at 14000000 (32-bit, non-prefetchable) [size=4K]
Region 5: I/O ports at 4000000 [disabled] [size=4K]
root@raq2:~# lspci -xxxx
00:00.0 Host bridge: Marvell Technology Group Ltd. Device 4146 (rev 11)
00: ab 11 46 41 06 00 80 a2 11 00 80 05 08 40 00 00
10: 00 00 00 00 00 00 00 08 00 00 00 1c 00 00 00 1f
20: 00 00 00 14 01 00 00 14 00 00 00 00 00 00 00 00
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
On Tuesday 02 November 2021 15:49:29 Pali Rohár wrote:
> On Tuesday 02 November 2021 14:01:41 Maciej W. Rozycki wrote:
> > On Tue, 2 Nov 2021, Pali Rohár wrote:
> >
> > > > None of the Galileo system controllers I came across had the class code
> > > > set incorrectly.
> > >
> > > In kernel there is quirk only for one device with id:
> > > PCI_VENDOR_ID_MARVELL (0x11ab) PCI_DEVICE_ID_MARVELL_GT64111 (0x4146)
> > >
> > > So for some reasons quirk is needed... Anyway, patch for this quirk just
> > > adds comment as there is no explanation for it. It does not modify quirk
> > > code.
> > >
> > > So it is possible that Marvell (or rather Galileo at that time) included
> > > some config space fixup in some products and 0x4146 did not have it.
> > > Just guessing... We can really only guess what could happen at that time
> > > 20 years ago...
> >
> > Ah, there you go! -- sadly I don't seem to have a copy of the datasheet
> > for the GT-64111, but the GT-64115 has it[1]:
> >
> > Table 158: PCI Class Code and Revision ID, Offset: 0x008
> > Bits Field name Function Initial Value
> > 7:0 RevID Indicates the GT-64115 PCI Revision 0x01
> > number.
> > 15:8 Reserved Read only. 0x0
> > 23:16 SubClass Indicates the GT-64115 Subclass - Mem- 0x80
> > ory controller.
> > 31:24 BaseClass Indicates the GT-64115 Base Class - 0x05
> > memory controller.
> >
> > and then:
> >
> > "Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
> > Header Type (0x00e) fields are read only from the PCI bus. These fields
> > can be modified and read via the CPU bus."
> >
> > Likewise with the GT-64120[2]:
> >
> > Table 208: PCI_0 Class Code and Revision ID, Offset: 0x008 from PCI_0 or CPU; 0x088 from
> > PCI_1
> > Bits Field name Function Initial Value
> > 7:0 RevID Indicates the GT-64120 PCI_0 revision number. 0x02
> > 15:8 Reserved Read Only 0. 0x0
> > 23:16 SubClass Indicates the GT-64120 Subclass Depends on value
> > 0x00 - Host Bridge Device. sampled at reset
> > 0x80 - Memory Device. on BankSel[0]. See
> > Table 44 on page
> > 11-1.
> > 31:24 BaseClass Indicates the GT-64120 Base Class Depends on value
> > 0x06 - Bridge Device. sampled at reset
> > 0x05 - Memory Device. on BankSel[0]. See
> > Table 44 on page
> > 11-1.
> >
> > Table 209: PCI_1 Class Code and Revision ID, Offset: 0x088 from PCI_0 or CPU; 0x008 from
> > PCI_1
> > Bits Field name Function Initial Value
> > 31:0 Various Same as for PCI_0 Class Code and Revision ID.
> >
> > and then also:
> >
> > "Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
> > Header Type (0x00e) fields are read only from the PCI bus. These fields
> > can be modified and read via the CPU bus."
> >
> > -- so this is system-specific and an intended chip feature rather than an
> > erratum (or rather it is a system erratum if the reset strap or the boot
> > firmware has got it wrong).
> >
> > The memory device class code is IIUC meant to be typically chosen when
> > the Galileo/Marvell device is used without the CPU interface, i.e. as a
> > PCI memory controller device only[3].
I have found on internet some copy of GT64111 datasheet ("GT-64111
System Controller for RC4640, RM523X and VR4300 CPUs", Galileo
Technology, Product Preview Revision 1.1, FEB 4, 1999) and in section
"17.15 PCI Configuration Registers" there is subsection "Class Code and
Revision ID, Offset: 0x008" with content:
Bits Field name Function Initial Value
7:0 RevID Indicates the GT-64111 Revision number. 0x10
GT-64111-P-0 = 0x10
15:8 Reserved 0x0
23:16 SubClass Indicates the GT-64111 Subclass (0x80 - other mem- 0x80
ory controller)
31:24 BaseClass Indicates the GT-64111 Base Class (0x5 - memory 0x05
controller).
And in section "6.5.3 PCI Autoconfiguration at RESET" is following
interesting information:
Eight PCI registers can be automatically loaded after Rst*.
Autoconfiguration mode is enabled by asserting the DMAReq[3]* LOW on
Rst*. Any PCI transactions targeted for the GT-64111 will be retried
while the loading of the PCI configuration registers is in process.
It is highly recommended that all PC applications utilize the PCI
Autoconfiguration at RESET feature. The autoload feature can be easily
implemented with a very low cost EPLD. Galileo provides sample EPLD
equations upon request. (You can always pull the EPLD off your final
product if you find there are no issues during testing.)
NOTE: The GT-64111’s default Class Code is 0x0580 (Memory Controller)
which is a change from the GT-64011.
The GT-64011 used the Class Code 0x0600 which denotes Host Bridge. Some
PCs refuse to configure host bridges if they are found plugged into a
PCI slot (ask the BIOS vendors why...). The “Memory Controller” Class
Code does not cause a problem for these non-compliant BIOSes, so we used
this as the default in the GT-64111. The Class Code can be reporgrammed
in both devices via autoload or CPU register writes.
The PCI register values are loaded from the ROM controlled by BootCS*
are shown in Table 21, below.
TABLE 21. PCI Registers Loaded at RESET
Register Offset Boot Device Address
Device and Vendor ID 0x000 0x1fffffe0
Class Code and Revision ID 0x008 0x1fffffe4
Subsystem Device and Vendor ID 0x02c 0x1fffffe8
Interrupt Pin and Line 0x03c 0x1fffffec
RAS[1:0]* Bank Size 0xc08 0x1ffffff0
RAS[3:2]* Bank Size 0xc0c 0x1ffffff4
CS[2:0]* Bank Size 0xc10 0x1ffffff8
CS[3]* & Boot CS* Bank Size 0xc14 0x1ffffffc
===
So the conclusion is that there is also some RESET configuration via
BootCS (I have no idea what it is or was). And default value (when RESET
configuration is not used) is always "Memory controller" due to
existence of "broken PC BIOSes" (probably x86).
Hence the quirk for GT64111 in kernel is always needed. And Thomas
already confirmed in his pci hexdump that PCI Class code is set to
Memory Controller.
I hope that now this mystery of this GT64111 quirk is solved :-) I will
update patch with correct comment, why quirk is needed.
So due to the fact that 20 years ago there were broken x86 BIOSes which
did not like PCI devices with PCI Class code of Host Bridge, Marvell
changed default PCI Class code to Memory Controller and let it in this
state also for future PCIe-based ARM and AR64 SoCs for next 20 years.
Which generally leaded to broken PCIe support in mvebu SoCs. I have no
more comments about it... :-(
> > > > The lack of a quirk with a platform does not mean it cannot have a certain
> > > > PCI/e device.
> > >
> > > This is 11ab:4620 device an there is no PCIe capability in its config
> > > space (you can inspect it via 'lspci -F dump.txt -nn -vv' but there is
> > > nothing interesting).
> >
> > Of course, just as Thomas told you about the GT-64111 too. But you were
> > right in that the memory controller feature seems shared across all the
> > chip line, whether PCI or PCIe.
> >
> > References:
> >
> > [1] "GT-64115 System Controller for RC4640, RM523X, and VR4300 CPUs",
> > Galileo Technology, Datasheet Revision 1.11, APR 04, 2000, Section
> > 18.16 "PCI Configuration", p. 161
> >
> > [2] "GT-64120 System Controller For RC4650/4700/5000 and RM526X/527X/7000
> > CPUs", Galileo Technology, Datasheet Revision 1.4, SEP 14, 1999,
> > Section 17.16 "PCI Configuration", p. 17-59
> >
> > [3] same, Chapter 14. "Using the GT-64120 Without the CPU Interface", p.
> > 14-1
> >
> > Maciej
>
> Hello Maciej! Thank you very much for the explanation!
>
> Now it makes sense and looks like that for GT-64111 it is "system
> erratum" that strapping pins are incorrectly set which leads to wrong
> PCI class code.
>
> I will update comment for GT-64111 quirk in v2.
>
> I'm surprised that Marvell copied this 20 years old MIPS Galileo PCI
> logic into followup ARM SoC PCIe IPs (and later also into recent ARM64
> A3720 SoC PCIe IP), removed configuration of PCI class code via
> strapping pins and let default PCI class code value to Memory device,
> even also when PCIe controller is running in Root Complex mode. And so
> correction can be done only from "CPU bus".
>
> Just Marvell forgot to include chapter about usage without CPU interface
> in new ARM and ARM64 SoCs and origin/usage of that Memory Controller
> Device was lost in history, even Marvell people was not able to figure
> out what was wrong with PCIe IP in their ARM SoCs...
> https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/
>
> Maciej, if I had known that you have this kind of information I would
> have written you year ago :-)
On 02.11.21 16:48, Pali Rohár wrote:
> On Tuesday 02 November 2021 15:49:29 Pali Rohár wrote:
>> On Tuesday 02 November 2021 14:01:41 Maciej W. Rozycki wrote:
>>> On Tue, 2 Nov 2021, Pali Rohár wrote:
>>>
>>>>> None of the Galileo system controllers I came across had the class code
>>>>> set incorrectly.
>>>>
>>>> In kernel there is quirk only for one device with id:
>>>> PCI_VENDOR_ID_MARVELL (0x11ab) PCI_DEVICE_ID_MARVELL_GT64111 (0x4146)
>>>>
>>>> So for some reasons quirk is needed... Anyway, patch for this quirk just
>>>> adds comment as there is no explanation for it. It does not modify quirk
>>>> code.
>>>>
>>>> So it is possible that Marvell (or rather Galileo at that time) included
>>>> some config space fixup in some products and 0x4146 did not have it.
>>>> Just guessing... We can really only guess what could happen at that time
>>>> 20 years ago...
>>>
>>> Ah, there you go! -- sadly I don't seem to have a copy of the datasheet
>>> for the GT-64111, but the GT-64115 has it[1]:
>>>
>>> Table 158: PCI Class Code and Revision ID, Offset: 0x008
>>> Bits Field name Function Initial Value
>>> 7:0 RevID Indicates the GT-64115 PCI Revision 0x01
>>> number.
>>> 15:8 Reserved Read only. 0x0
>>> 23:16 SubClass Indicates the GT-64115 Subclass - Mem- 0x80
>>> ory controller.
>>> 31:24 BaseClass Indicates the GT-64115 Base Class - 0x05
>>> memory controller.
>>>
>>> and then:
>>>
>>> "Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
>>> Header Type (0x00e) fields are read only from the PCI bus. These fields
>>> can be modified and read via the CPU bus."
>>>
>>> Likewise with the GT-64120[2]:
>>>
>>> Table 208: PCI_0 Class Code and Revision ID, Offset: 0x008 from PCI_0 or CPU; 0x088 from
>>> PCI_1
>>> Bits Field name Function Initial Value
>>> 7:0 RevID Indicates the GT-64120 PCI_0 revision number. 0x02
>>> 15:8 Reserved Read Only 0. 0x0
>>> 23:16 SubClass Indicates the GT-64120 Subclass Depends on value
>>> 0x00 - Host Bridge Device. sampled at reset
>>> 0x80 - Memory Device. on BankSel[0]. See
>>> Table 44 on page
>>> 11-1.
>>> 31:24 BaseClass Indicates the GT-64120 Base Class Depends on value
>>> 0x06 - Bridge Device. sampled at reset
>>> 0x05 - Memory Device. on BankSel[0]. See
>>> Table 44 on page
>>> 11-1.
>>>
>>> Table 209: PCI_1 Class Code and Revision ID, Offset: 0x088 from PCI_0 or CPU; 0x008 from
>>> PCI_1
>>> Bits Field name Function Initial Value
>>> 31:0 Various Same as for PCI_0 Class Code and Revision ID.
>>>
>>> and then also:
>>>
>>> "Device and Vendor ID (0x000), Class Code and Revision ID (0x008), and
>>> Header Type (0x00e) fields are read only from the PCI bus. These fields
>>> can be modified and read via the CPU bus."
>>>
>>> -- so this is system-specific and an intended chip feature rather than an
>>> erratum (or rather it is a system erratum if the reset strap or the boot
>>> firmware has got it wrong).
>>>
>>> The memory device class code is IIUC meant to be typically chosen when
>>> the Galileo/Marvell device is used without the CPU interface, i.e. as a
>>> PCI memory controller device only[3].
>
> I have found on internet some copy of GT64111 datasheet ("GT-64111
> System Controller for RC4640, RM523X and VR4300 CPUs", Galileo
> Technology, Product Preview Revision 1.1, FEB 4, 1999) and in section
> "17.15 PCI Configuration Registers" there is subsection "Class Code and
> Revision ID, Offset: 0x008" with content:
>
> Bits Field name Function Initial Value
> 7:0 RevID Indicates the GT-64111 Revision number. 0x10
> GT-64111-P-0 = 0x10
> 15:8 Reserved 0x0
> 23:16 SubClass Indicates the GT-64111 Subclass (0x80 - other mem- 0x80
> ory controller)
> 31:24 BaseClass Indicates the GT-64111 Base Class (0x5 - memory 0x05
> controller).
>
> And in section "6.5.3 PCI Autoconfiguration at RESET" is following
> interesting information:
>
> Eight PCI registers can be automatically loaded after Rst*.
> Autoconfiguration mode is enabled by asserting the DMAReq[3]* LOW on
> Rst*. Any PCI transactions targeted for the GT-64111 will be retried
> while the loading of the PCI configuration registers is in process.
>
> It is highly recommended that all PC applications utilize the PCI
> Autoconfiguration at RESET feature. The autoload feature can be easily
> implemented with a very low cost EPLD. Galileo provides sample EPLD
> equations upon request. (You can always pull the EPLD off your final
> product if you find there are no issues during testing.)
>
> NOTE: The GT-64111’s default Class Code is 0x0580 (Memory Controller)
> which is a change from the GT-64011.
>
> The GT-64011 used the Class Code 0x0600 which denotes Host Bridge. Some
> PCs refuse to configure host bridges if they are found plugged into a
> PCI slot (ask the BIOS vendors why...). The “Memory Controller” Class
> Code does not cause a problem for these non-compliant BIOSes, so we used
> this as the default in the GT-64111. The Class Code can be reporgrammed
> in both devices via autoload or CPU register writes.
>
> The PCI register values are loaded from the ROM controlled by BootCS*
> are shown in Table 21, below.
>
> TABLE 21. PCI Registers Loaded at RESET
> Register Offset Boot Device Address
> Device and Vendor ID 0x000 0x1fffffe0
> Class Code and Revision ID 0x008 0x1fffffe4
> Subsystem Device and Vendor ID 0x02c 0x1fffffe8
> Interrupt Pin and Line 0x03c 0x1fffffec
> RAS[1:0]* Bank Size 0xc08 0x1ffffff0
> RAS[3:2]* Bank Size 0xc0c 0x1ffffff4
> CS[2:0]* Bank Size 0xc10 0x1ffffff8
> CS[3]* & Boot CS* Bank Size 0xc14 0x1ffffffc
>
> ===
>
> So the conclusion is that there is also some RESET configuration via
> BootCS (I have no idea what it is or was). And default value (when RESET
> configuration is not used) is always "Memory controller" due to
> existence of "broken PC BIOSes" (probably x86).
>
> Hence the quirk for GT64111 in kernel is always needed. And Thomas
> already confirmed in his pci hexdump that PCI Class code is set to
> Memory Controller.
>
> I hope that now this mystery of this GT64111 quirk is solved :-) I will
> update patch with correct comment, why quirk is needed.
>
> So due to the fact that 20 years ago there were broken x86 BIOSes which
> did not like PCI devices with PCI Class code of Host Bridge, Marvell
> changed default PCI Class code to Memory Controller and let it in this
> state also for future PCIe-based ARM and AR64 SoCs for next 20 years.
> Which generally leaded to broken PCIe support in mvebu SoCs. I have no
> more comments about it... :-(
If this is really the case, that all this was "copied" in such a bad
design state into newer SoC's for that many years, which I don't doubt
right now any more, then this is absolutely amazing and pretty sad IMHO.
Pali, many thanks for being persistant enough to dig through this.
Thanks,
Stefan
- The code relies on rc_pci_fixup being called, which only happens
when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
this causes a booting failure with a non-obvious cause.
- Update rc_pci_fixup to set the class properly, copying the
more modern style from other places
- Correct the rc_pci_fixup comment
This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
PCI-E fixup") for all other Marvell ARM platforms which have same buggy
PCIe controller and do not use pci-mvebu.c controller driver yet.
Long-term goal for these Marvell ARM platforms should be conversion to
pci-mvebu.c controller driver and removal of these fixups in arch code.
Signed-off-by: Pali Rohár <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: [email protected]
---
Changes in v2:
* Move MIPS change into separate patch
* Add information that this patch is for platforms which do not use pci-mvebu.c
---
arch/arm/Kconfig | 1 +
arch/arm/mach-dove/pcie.c | 11 ++++++++---
arch/arm/mach-mv78xx0/pcie.c | 11 ++++++++---
arch/arm/mach-orion5x/Kconfig | 1 +
arch/arm/mach-orion5x/pci.c | 12 +++++++++---
5 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index fc196421b2ce..9f157e973555 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -400,6 +400,7 @@ config ARCH_DOVE
select GENERIC_IRQ_MULTI_HANDLER
select GPIOLIB
select HAVE_PCI
+ select PCI_QUIRKS if PCI
select MVEBU_MBUS
select PINCTRL
select PINCTRL_DOVE
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index ee91ac6b5ebf..ecf057a0f5ba 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -135,14 +135,19 @@ static struct pci_ops pcie_ops = {
.write = pcie_wr_conf,
};
+/*
+ * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
+ * is operating as a root complex this needs to be switched to
+ * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
+ * the device. Decoding setup is handled by the orion code.
+ */
static void rc_pci_fixup(struct pci_dev *dev)
{
- /*
- * Prevent enumeration of root complex.
- */
if (dev->bus->parent == NULL && dev->devfn == 0) {
int i;
+ dev->class &= 0xff;
+ dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
dev->resource[i].start = 0;
dev->resource[i].end = 0;
diff --git a/arch/arm/mach-mv78xx0/pcie.c b/arch/arm/mach-mv78xx0/pcie.c
index 636d84b40466..9362b5fc116f 100644
--- a/arch/arm/mach-mv78xx0/pcie.c
+++ b/arch/arm/mach-mv78xx0/pcie.c
@@ -177,14 +177,19 @@ static struct pci_ops pcie_ops = {
.write = pcie_wr_conf,
};
+/*
+ * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
+ * is operating as a root complex this needs to be switched to
+ * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
+ * the device. Decoding setup is handled by the orion code.
+ */
static void rc_pci_fixup(struct pci_dev *dev)
{
- /*
- * Prevent enumeration of root complex.
- */
if (dev->bus->parent == NULL && dev->devfn == 0) {
int i;
+ dev->class &= 0xff;
+ dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
dev->resource[i].start = 0;
dev->resource[i].end = 0;
diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
index e94a61901ffd..7189a5b1ec46 100644
--- a/arch/arm/mach-orion5x/Kconfig
+++ b/arch/arm/mach-orion5x/Kconfig
@@ -6,6 +6,7 @@ menuconfig ARCH_ORION5X
select GPIOLIB
select MVEBU_MBUS
select FORCE_PCI
+ select PCI_QUIRKS
select PHYLIB if NETDEVICES
select PLAT_ORION_LEGACY
help
diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
index 76951bfbacf5..5145fe89702e 100644
--- a/arch/arm/mach-orion5x/pci.c
+++ b/arch/arm/mach-orion5x/pci.c
@@ -509,14 +509,20 @@ static int __init pci_setup(struct pci_sys_data *sys)
/*****************************************************************************
* General PCIe + PCI
****************************************************************************/
+
+/*
+ * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
+ * is operating as a root complex this needs to be switched to
+ * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
+ * the device. Decoding setup is handled by the orion code.
+ */
static void rc_pci_fixup(struct pci_dev *dev)
{
- /*
- * Prevent enumeration of root complex.
- */
if (dev->bus->parent == NULL && dev->devfn == 0) {
int i;
+ dev->class &= 0xff;
+ dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
dev->resource[i].start = 0;
dev->resource[i].end = 0;
--
2.20.1
On Tue, 2 Nov 2021, Pali Rohár wrote:
> Hello Maciej! Thank you very much for the explanation!
You are welcome!
> I'm surprised that Marvell copied this 20 years old MIPS Galileo PCI
> logic into followup ARM SoC PCIe IPs (and later also into recent ARM64
> A3720 SoC PCIe IP), removed configuration of PCI class code via
> strapping pins and let default PCI class code value to Memory device,
> even also when PCIe controller is running in Root Complex mode. And so
> correction can be done only from "CPU bus".
Still the bootstrap firmware (say U-boot, as I can see it mentioned in
your reference) can write the correct value to the class code register.
Or can it?
I guess you can try it and report your findings back. You can poke at
PCI/e registers directly from U-boot (`pci write.w', etc.) as with any
reasonable firmware monitor, no need to write code; I guess you probably
know that already.
I have no such hardware and I have no incentive to chase documentation
for it even if public copies are available for the affected devices.
Also you say it's IP rather than actual discrete chips as it used to be
with the Galileo system controllers, so it could be up to the customer to
get the IP wired/configured correctly.
> Maciej, if I had known that you have this kind of information I would
> have written you year ago :-)
Well, I have all kinds of information.
Maciej
On Tue, 2 Nov 2021, Pali Rohár wrote:
> So the conclusion is that there is also some RESET configuration via
> BootCS (I have no idea what it is or was). And default value (when RESET
> configuration is not used) is always "Memory controller" due to
> existence of "broken PC BIOSes" (probably x86).
BootCS is one of the chip selects for the memory/device bus (AD bus), one
of the three (or four in dual-PCI implementations), along with the SysAD
bus and the PCI bus(es), interconnected, which is where DRAM sits as well
as possibly other locally accessed devices with the Galileo system
controllers. See Figure 5 on page 23 of the GT-64111 document.
Maciej
On Wednesday 03 November 2021 14:49:07 Maciej W. Rozycki wrote:
> On Tue, 2 Nov 2021, Pali Rohár wrote:
>
> > Hello Maciej! Thank you very much for the explanation!
>
> You are welcome!
>
> > I'm surprised that Marvell copied this 20 years old MIPS Galileo PCI
> > logic into followup ARM SoC PCIe IPs (and later also into recent ARM64
> > A3720 SoC PCIe IP), removed configuration of PCI class code via
> > strapping pins and let default PCI class code value to Memory device,
> > even also when PCIe controller is running in Root Complex mode. And so
> > correction can be done only from "CPU bus".
>
> Still the bootstrap firmware (say U-boot, as I can see it mentioned in
> your reference) can write the correct value to the class code register.
> Or can it?
Yes, it can. And I have already sent patches to do it.
On Tuesday 02 November 2021 18:12:58 Pali Rohár wrote:
> - The code relies on rc_pci_fixup being called, which only happens
> when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> this causes a booting failure with a non-obvious cause.
> - Update rc_pci_fixup to set the class properly, copying the
> more modern style from other places
> - Correct the rc_pci_fixup comment
>
> This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> PCI-E fixup") for all other Marvell ARM platforms which have same buggy
> PCIe controller and do not use pci-mvebu.c controller driver yet.
>
> Long-term goal for these Marvell ARM platforms should be conversion to
> pci-mvebu.c controller driver and removal of these fixups in arch code.
>
> Signed-off-by: Pali Rohár <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: [email protected]
Hello! Patch 2/2 was already applied into mips-next. Could you review
also this patch 1/2?
>
> ---
> Changes in v2:
> * Move MIPS change into separate patch
> * Add information that this patch is for platforms which do not use pci-mvebu.c
> ---
> arch/arm/Kconfig | 1 +
> arch/arm/mach-dove/pcie.c | 11 ++++++++---
> arch/arm/mach-mv78xx0/pcie.c | 11 ++++++++---
> arch/arm/mach-orion5x/Kconfig | 1 +
> arch/arm/mach-orion5x/pci.c | 12 +++++++++---
> 5 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index fc196421b2ce..9f157e973555 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -400,6 +400,7 @@ config ARCH_DOVE
> select GENERIC_IRQ_MULTI_HANDLER
> select GPIOLIB
> select HAVE_PCI
> + select PCI_QUIRKS if PCI
> select MVEBU_MBUS
> select PINCTRL
> select PINCTRL_DOVE
> diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
> index ee91ac6b5ebf..ecf057a0f5ba 100644
> --- a/arch/arm/mach-dove/pcie.c
> +++ b/arch/arm/mach-dove/pcie.c
> @@ -135,14 +135,19 @@ static struct pci_ops pcie_ops = {
> .write = pcie_wr_conf,
> };
>
> +/*
> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> + * is operating as a root complex this needs to be switched to
> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> + * the device. Decoding setup is handled by the orion code.
> + */
> static void rc_pci_fixup(struct pci_dev *dev)
> {
> - /*
> - * Prevent enumeration of root complex.
> - */
> if (dev->bus->parent == NULL && dev->devfn == 0) {
> int i;
>
> + dev->class &= 0xff;
> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> dev->resource[i].start = 0;
> dev->resource[i].end = 0;
> diff --git a/arch/arm/mach-mv78xx0/pcie.c b/arch/arm/mach-mv78xx0/pcie.c
> index 636d84b40466..9362b5fc116f 100644
> --- a/arch/arm/mach-mv78xx0/pcie.c
> +++ b/arch/arm/mach-mv78xx0/pcie.c
> @@ -177,14 +177,19 @@ static struct pci_ops pcie_ops = {
> .write = pcie_wr_conf,
> };
>
> +/*
> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> + * is operating as a root complex this needs to be switched to
> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> + * the device. Decoding setup is handled by the orion code.
> + */
> static void rc_pci_fixup(struct pci_dev *dev)
> {
> - /*
> - * Prevent enumeration of root complex.
> - */
> if (dev->bus->parent == NULL && dev->devfn == 0) {
> int i;
>
> + dev->class &= 0xff;
> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> dev->resource[i].start = 0;
> dev->resource[i].end = 0;
> diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
> index e94a61901ffd..7189a5b1ec46 100644
> --- a/arch/arm/mach-orion5x/Kconfig
> +++ b/arch/arm/mach-orion5x/Kconfig
> @@ -6,6 +6,7 @@ menuconfig ARCH_ORION5X
> select GPIOLIB
> select MVEBU_MBUS
> select FORCE_PCI
> + select PCI_QUIRKS
> select PHYLIB if NETDEVICES
> select PLAT_ORION_LEGACY
> help
> diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> index 76951bfbacf5..5145fe89702e 100644
> --- a/arch/arm/mach-orion5x/pci.c
> +++ b/arch/arm/mach-orion5x/pci.c
> @@ -509,14 +509,20 @@ static int __init pci_setup(struct pci_sys_data *sys)
> /*****************************************************************************
> * General PCIe + PCI
> ****************************************************************************/
> +
> +/*
> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> + * is operating as a root complex this needs to be switched to
> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> + * the device. Decoding setup is handled by the orion code.
> + */
> static void rc_pci_fixup(struct pci_dev *dev)
> {
> - /*
> - * Prevent enumeration of root complex.
> - */
> if (dev->bus->parent == NULL && dev->devfn == 0) {
> int i;
>
> + dev->class &= 0xff;
> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> dev->resource[i].start = 0;
> dev->resource[i].end = 0;
> --
> 2.20.1
>
On Tuesday 02 November 2021 16:13:34 Pali Rohár wrote:
> On Tuesday 02 November 2021 16:02:01 Thomas Bogendoerfer wrote:
> > On Tue, Nov 02, 2021 at 11:00:34AM +0100, Pali Rohár wrote:
> > > > > But I do not have this hardware to verify it.
> > > >
> > > > I still have a few Cobalt systems here.
> > >
> > > Perfect! It would help if you could provide 'lspci -nn -vv' output from
> > > that system. In case you have very old version of lspci on that system
> > > you could try to run it with '-xxxx' (or '-xxx') which prints hexdump
> > > and I can parse it with local lspci.
Thomas, one more question, do you have also GT-64115 system which has
PCI device id 0x4611? Based on Maciej quote, GT-64115 probably also
reports itself as "Memory controller" instead of "Host Bridge". So lspci
output from GT-64115 could be also interesting.
> > not sure, if you still needed:
> >
> > root@raq2:~# lspci -nn -vv
> > 00:00.0 Host bridge [0600]: Marvell Technology Group Ltd. Device [11ab:4146] (rev 11)
> > Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
> > Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ >SERR- <PERR+ INTx-
> > Latency: 64, Cache Line Size: 32 bytes
> > Interrupt: pin A routed to IRQ 0
> > Region 1: Memory at 08000000 (32-bit, non-prefetchable) [size=128M]
> > Region 2: Memory at 1c000000 (32-bit, non-prefetchable) [size=32M]
> > Region 3: Memory at 1f000000 (32-bit, non-prefetchable) [size=16M]
> > Region 4: Memory at 14000000 (32-bit, non-prefetchable) [size=4K]
> > Region 5: I/O ports at 4000000 [disabled] [size=4K]
> >
> >
> > root@raq2:~# lspci -xxxx
> > 00:00.0 Host bridge: Marvell Technology Group Ltd. Device 4146 (rev 11)
> > 00: ab 11 46 41 06 00 80 a2 11 00 80 05 08 40 00 00
>
> ^^ ^^ ^^
> Here is class code
>
> So it confirms that PCI Class code is 0580 which is Memory Controller.
> And not Host Bridge as it should be.
>
> If I put this hexdump into dump.txt and run 'lspci -F dump.txt -nn' then I see:
> 00:00.0 Memory controller [0580]: Marvell Technology Group Ltd. Device [11ab:4146] (rev 11)
>
> In your output above is "Host bridge" which means that quirk was applied:
> 00:00.0 Host bridge [0600]: Marvell Technology Group Ltd. Device [11ab:4146] (rev 11)
>
> (I guess in 'lspci -nn -vv -b' should be Memory controller as lspci with
> '-b' should not see that quirk change)
>
> > 10: 00 00 00 00 00 00 00 08 00 00 00 1c 00 00 00 1f
> > 20: 00 00 00 14 01 00 00 14 00 00 00 00 00 00 00 00
> > 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00
> > 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >
> > Thomas.
> >
> > --
> > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> > good idea. [ RFC1925, 2.3 ]
On Wed, Nov 10, 2021 at 12:42:53AM +0100, Pali Roh?r wrote:
> On Tuesday 02 November 2021 16:13:34 Pali Roh?r wrote:
> > On Tuesday 02 November 2021 16:02:01 Thomas Bogendoerfer wrote:
> > > On Tue, Nov 02, 2021 at 11:00:34AM +0100, Pali Roh?r wrote:
> > > > > > But I do not have this hardware to verify it.
> > > > >
> > > > > I still have a few Cobalt systems here.
> > > >
> > > > Perfect! It would help if you could provide 'lspci -nn -vv' output from
> > > > that system. In case you have very old version of lspci on that system
> > > > you could try to run it with '-xxxx' (or '-xxx') which prints hexdump
> > > > and I can parse it with local lspci.
>
> Thomas, one more question, do you have also GT-64115 system which has
> PCI device id 0x4611? Based on Maciej quote, GT-64115 probably also
> reports itself as "Memory controller" instead of "Host Bridge". So lspci
> output from GT-64115 could be also interesting.
The only systems with GT64-xxx chips I have are Cobalt systems, but none of
them has a GT-64115 chip (Raq1 comes with GT-64011 and Raq2 with GT-64111).
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
PING, Gente reminder for patch 1/2.
On Tuesday 02 November 2021 18:12:58 Pali Rohár wrote:
> - The code relies on rc_pci_fixup being called, which only happens
> when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> this causes a booting failure with a non-obvious cause.
> - Update rc_pci_fixup to set the class properly, copying the
> more modern style from other places
> - Correct the rc_pci_fixup comment
>
> This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> PCI-E fixup") for all other Marvell ARM platforms which have same buggy
> PCIe controller and do not use pci-mvebu.c controller driver yet.
>
> Long-term goal for these Marvell ARM platforms should be conversion to
> pci-mvebu.c controller driver and removal of these fixups in arch code.
>
> Signed-off-by: Pali Rohár <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: [email protected]
>
> ---
> Changes in v2:
> * Move MIPS change into separate patch
> * Add information that this patch is for platforms which do not use pci-mvebu.c
> ---
> arch/arm/Kconfig | 1 +
> arch/arm/mach-dove/pcie.c | 11 ++++++++---
> arch/arm/mach-mv78xx0/pcie.c | 11 ++++++++---
> arch/arm/mach-orion5x/Kconfig | 1 +
> arch/arm/mach-orion5x/pci.c | 12 +++++++++---
> 5 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index fc196421b2ce..9f157e973555 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -400,6 +400,7 @@ config ARCH_DOVE
> select GENERIC_IRQ_MULTI_HANDLER
> select GPIOLIB
> select HAVE_PCI
> + select PCI_QUIRKS if PCI
> select MVEBU_MBUS
> select PINCTRL
> select PINCTRL_DOVE
> diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
> index ee91ac6b5ebf..ecf057a0f5ba 100644
> --- a/arch/arm/mach-dove/pcie.c
> +++ b/arch/arm/mach-dove/pcie.c
> @@ -135,14 +135,19 @@ static struct pci_ops pcie_ops = {
> .write = pcie_wr_conf,
> };
>
> +/*
> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> + * is operating as a root complex this needs to be switched to
> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> + * the device. Decoding setup is handled by the orion code.
> + */
> static void rc_pci_fixup(struct pci_dev *dev)
> {
> - /*
> - * Prevent enumeration of root complex.
> - */
> if (dev->bus->parent == NULL && dev->devfn == 0) {
> int i;
>
> + dev->class &= 0xff;
> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> dev->resource[i].start = 0;
> dev->resource[i].end = 0;
> diff --git a/arch/arm/mach-mv78xx0/pcie.c b/arch/arm/mach-mv78xx0/pcie.c
> index 636d84b40466..9362b5fc116f 100644
> --- a/arch/arm/mach-mv78xx0/pcie.c
> +++ b/arch/arm/mach-mv78xx0/pcie.c
> @@ -177,14 +177,19 @@ static struct pci_ops pcie_ops = {
> .write = pcie_wr_conf,
> };
>
> +/*
> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> + * is operating as a root complex this needs to be switched to
> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> + * the device. Decoding setup is handled by the orion code.
> + */
> static void rc_pci_fixup(struct pci_dev *dev)
> {
> - /*
> - * Prevent enumeration of root complex.
> - */
> if (dev->bus->parent == NULL && dev->devfn == 0) {
> int i;
>
> + dev->class &= 0xff;
> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> dev->resource[i].start = 0;
> dev->resource[i].end = 0;
> diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
> index e94a61901ffd..7189a5b1ec46 100644
> --- a/arch/arm/mach-orion5x/Kconfig
> +++ b/arch/arm/mach-orion5x/Kconfig
> @@ -6,6 +6,7 @@ menuconfig ARCH_ORION5X
> select GPIOLIB
> select MVEBU_MBUS
> select FORCE_PCI
> + select PCI_QUIRKS
> select PHYLIB if NETDEVICES
> select PLAT_ORION_LEGACY
> help
> diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> index 76951bfbacf5..5145fe89702e 100644
> --- a/arch/arm/mach-orion5x/pci.c
> +++ b/arch/arm/mach-orion5x/pci.c
> @@ -509,14 +509,20 @@ static int __init pci_setup(struct pci_sys_data *sys)
> /*****************************************************************************
> * General PCIe + PCI
> ****************************************************************************/
> +
> +/*
> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> + * is operating as a root complex this needs to be switched to
> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> + * the device. Decoding setup is handled by the orion code.
> + */
> static void rc_pci_fixup(struct pci_dev *dev)
> {
> - /*
> - * Prevent enumeration of root complex.
> - */
> if (dev->bus->parent == NULL && dev->devfn == 0) {
> int i;
>
> + dev->class &= 0xff;
> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> dev->resource[i].start = 0;
> dev->resource[i].end = 0;
> --
> 2.20.1
>
On Tuesday 09 November 2021 23:53:32 Pali Rohár wrote:
> On Tuesday 02 November 2021 18:12:58 Pali Rohár wrote:
> > - The code relies on rc_pci_fixup being called, which only happens
> > when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> > this causes a booting failure with a non-obvious cause.
> > - Update rc_pci_fixup to set the class properly, copying the
> > more modern style from other places
> > - Correct the rc_pci_fixup comment
> >
> > This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> > PCI-E fixup") for all other Marvell ARM platforms which have same buggy
> > PCIe controller and do not use pci-mvebu.c controller driver yet.
> >
> > Long-term goal for these Marvell ARM platforms should be conversion to
> > pci-mvebu.c controller driver and removal of these fixups in arch code.
> >
> > Signed-off-by: Pali Rohár <[email protected]>
> > Cc: Jason Gunthorpe <[email protected]>
> > Cc: [email protected]
>
> Hello! Patch 2/2 was already applied into mips-next. Could you review
> also this patch 1/2?
PING?
> >
> > ---
> > Changes in v2:
> > * Move MIPS change into separate patch
> > * Add information that this patch is for platforms which do not use pci-mvebu.c
> > ---
> > arch/arm/Kconfig | 1 +
> > arch/arm/mach-dove/pcie.c | 11 ++++++++---
> > arch/arm/mach-mv78xx0/pcie.c | 11 ++++++++---
> > arch/arm/mach-orion5x/Kconfig | 1 +
> > arch/arm/mach-orion5x/pci.c | 12 +++++++++---
> > 5 files changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index fc196421b2ce..9f157e973555 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -400,6 +400,7 @@ config ARCH_DOVE
> > select GENERIC_IRQ_MULTI_HANDLER
> > select GPIOLIB
> > select HAVE_PCI
> > + select PCI_QUIRKS if PCI
> > select MVEBU_MBUS
> > select PINCTRL
> > select PINCTRL_DOVE
> > diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
> > index ee91ac6b5ebf..ecf057a0f5ba 100644
> > --- a/arch/arm/mach-dove/pcie.c
> > +++ b/arch/arm/mach-dove/pcie.c
> > @@ -135,14 +135,19 @@ static struct pci_ops pcie_ops = {
> > .write = pcie_wr_conf,
> > };
> >
> > +/*
> > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > + * is operating as a root complex this needs to be switched to
> > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > + * the device. Decoding setup is handled by the orion code.
> > + */
> > static void rc_pci_fixup(struct pci_dev *dev)
> > {
> > - /*
> > - * Prevent enumeration of root complex.
> > - */
> > if (dev->bus->parent == NULL && dev->devfn == 0) {
> > int i;
> >
> > + dev->class &= 0xff;
> > + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > dev->resource[i].start = 0;
> > dev->resource[i].end = 0;
> > diff --git a/arch/arm/mach-mv78xx0/pcie.c b/arch/arm/mach-mv78xx0/pcie.c
> > index 636d84b40466..9362b5fc116f 100644
> > --- a/arch/arm/mach-mv78xx0/pcie.c
> > +++ b/arch/arm/mach-mv78xx0/pcie.c
> > @@ -177,14 +177,19 @@ static struct pci_ops pcie_ops = {
> > .write = pcie_wr_conf,
> > };
> >
> > +/*
> > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > + * is operating as a root complex this needs to be switched to
> > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > + * the device. Decoding setup is handled by the orion code.
> > + */
> > static void rc_pci_fixup(struct pci_dev *dev)
> > {
> > - /*
> > - * Prevent enumeration of root complex.
> > - */
> > if (dev->bus->parent == NULL && dev->devfn == 0) {
> > int i;
> >
> > + dev->class &= 0xff;
> > + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > dev->resource[i].start = 0;
> > dev->resource[i].end = 0;
> > diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
> > index e94a61901ffd..7189a5b1ec46 100644
> > --- a/arch/arm/mach-orion5x/Kconfig
> > +++ b/arch/arm/mach-orion5x/Kconfig
> > @@ -6,6 +6,7 @@ menuconfig ARCH_ORION5X
> > select GPIOLIB
> > select MVEBU_MBUS
> > select FORCE_PCI
> > + select PCI_QUIRKS
> > select PHYLIB if NETDEVICES
> > select PLAT_ORION_LEGACY
> > help
> > diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> > index 76951bfbacf5..5145fe89702e 100644
> > --- a/arch/arm/mach-orion5x/pci.c
> > +++ b/arch/arm/mach-orion5x/pci.c
> > @@ -509,14 +509,20 @@ static int __init pci_setup(struct pci_sys_data *sys)
> > /*****************************************************************************
> > * General PCIe + PCI
> > ****************************************************************************/
> > +
> > +/*
> > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > + * is operating as a root complex this needs to be switched to
> > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > + * the device. Decoding setup is handled by the orion code.
> > + */
> > static void rc_pci_fixup(struct pci_dev *dev)
> > {
> > - /*
> > - * Prevent enumeration of root complex.
> > - */
> > if (dev->bus->parent == NULL && dev->devfn == 0) {
> > int i;
> >
> > + dev->class &= 0xff;
> > + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > dev->resource[i].start = 0;
> > dev->resource[i].end = 0;
> > --
> > 2.20.1
> >
PING? I have not received any reply!
On Saturday 14 May 2022 20:21:25 Pali Rohár wrote:
> On Tuesday 09 November 2021 23:53:32 Pali Rohár wrote:
> > On Tuesday 02 November 2021 18:12:58 Pali Rohár wrote:
> > > - The code relies on rc_pci_fixup being called, which only happens
> > > when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
> > > this causes a booting failure with a non-obvious cause.
> > > - Update rc_pci_fixup to set the class properly, copying the
> > > more modern style from other places
> > > - Correct the rc_pci_fixup comment
> > >
> > > This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
> > > PCI-E fixup") for all other Marvell ARM platforms which have same buggy
> > > PCIe controller and do not use pci-mvebu.c controller driver yet.
> > >
> > > Long-term goal for these Marvell ARM platforms should be conversion to
> > > pci-mvebu.c controller driver and removal of these fixups in arch code.
> > >
> > > Signed-off-by: Pali Rohár <[email protected]>
> > > Cc: Jason Gunthorpe <[email protected]>
> > > Cc: [email protected]
> >
> > Hello! Patch 2/2 was already applied into mips-next. Could you review
> > also this patch 1/2?
>
> PING?
>
> > >
> > > ---
> > > Changes in v2:
> > > * Move MIPS change into separate patch
> > > * Add information that this patch is for platforms which do not use pci-mvebu.c
> > > ---
> > > arch/arm/Kconfig | 1 +
> > > arch/arm/mach-dove/pcie.c | 11 ++++++++---
> > > arch/arm/mach-mv78xx0/pcie.c | 11 ++++++++---
> > > arch/arm/mach-orion5x/Kconfig | 1 +
> > > arch/arm/mach-orion5x/pci.c | 12 +++++++++---
> > > 5 files changed, 27 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index fc196421b2ce..9f157e973555 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -400,6 +400,7 @@ config ARCH_DOVE
> > > select GENERIC_IRQ_MULTI_HANDLER
> > > select GPIOLIB
> > > select HAVE_PCI
> > > + select PCI_QUIRKS if PCI
> > > select MVEBU_MBUS
> > > select PINCTRL
> > > select PINCTRL_DOVE
> > > diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
> > > index ee91ac6b5ebf..ecf057a0f5ba 100644
> > > --- a/arch/arm/mach-dove/pcie.c
> > > +++ b/arch/arm/mach-dove/pcie.c
> > > @@ -135,14 +135,19 @@ static struct pci_ops pcie_ops = {
> > > .write = pcie_wr_conf,
> > > };
> > >
> > > +/*
> > > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > > + * is operating as a root complex this needs to be switched to
> > > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > > + * the device. Decoding setup is handled by the orion code.
> > > + */
> > > static void rc_pci_fixup(struct pci_dev *dev)
> > > {
> > > - /*
> > > - * Prevent enumeration of root complex.
> > > - */
> > > if (dev->bus->parent == NULL && dev->devfn == 0) {
> > > int i;
> > >
> > > + dev->class &= 0xff;
> > > + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> > > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > dev->resource[i].start = 0;
> > > dev->resource[i].end = 0;
> > > diff --git a/arch/arm/mach-mv78xx0/pcie.c b/arch/arm/mach-mv78xx0/pcie.c
> > > index 636d84b40466..9362b5fc116f 100644
> > > --- a/arch/arm/mach-mv78xx0/pcie.c
> > > +++ b/arch/arm/mach-mv78xx0/pcie.c
> > > @@ -177,14 +177,19 @@ static struct pci_ops pcie_ops = {
> > > .write = pcie_wr_conf,
> > > };
> > >
> > > +/*
> > > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > > + * is operating as a root complex this needs to be switched to
> > > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > > + * the device. Decoding setup is handled by the orion code.
> > > + */
> > > static void rc_pci_fixup(struct pci_dev *dev)
> > > {
> > > - /*
> > > - * Prevent enumeration of root complex.
> > > - */
> > > if (dev->bus->parent == NULL && dev->devfn == 0) {
> > > int i;
> > >
> > > + dev->class &= 0xff;
> > > + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> > > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > dev->resource[i].start = 0;
> > > dev->resource[i].end = 0;
> > > diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
> > > index e94a61901ffd..7189a5b1ec46 100644
> > > --- a/arch/arm/mach-orion5x/Kconfig
> > > +++ b/arch/arm/mach-orion5x/Kconfig
> > > @@ -6,6 +6,7 @@ menuconfig ARCH_ORION5X
> > > select GPIOLIB
> > > select MVEBU_MBUS
> > > select FORCE_PCI
> > > + select PCI_QUIRKS
> > > select PHYLIB if NETDEVICES
> > > select PLAT_ORION_LEGACY
> > > help
> > > diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> > > index 76951bfbacf5..5145fe89702e 100644
> > > --- a/arch/arm/mach-orion5x/pci.c
> > > +++ b/arch/arm/mach-orion5x/pci.c
> > > @@ -509,14 +509,20 @@ static int __init pci_setup(struct pci_sys_data *sys)
> > > /*****************************************************************************
> > > * General PCIe + PCI
> > > ****************************************************************************/
> > > +
> > > +/*
> > > + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
> > > + * is operating as a root complex this needs to be switched to
> > > + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
> > > + * the device. Decoding setup is handled by the orion code.
> > > + */
> > > static void rc_pci_fixup(struct pci_dev *dev)
> > > {
> > > - /*
> > > - * Prevent enumeration of root complex.
> > > - */
> > > if (dev->bus->parent == NULL && dev->devfn == 0) {
> > > int i;
> > >
> > > + dev->class &= 0xff;
> > > + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
> > > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > dev->resource[i].start = 0;
> > > dev->resource[i].end = 0;
> > > --
> > > 2.20.1
> > >
On Thu, Jul 07, 2022 at 08:31:12PM +0200, Pali Roh?r wrote:
> PING? I have not received any reply!
The Dove device I have doesn't make use of PCIe, so I don't think I can
test there, and I don't have the other platforms either.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Pali Rohár <[email protected]> writes:
> PING, Gente reminder for patch 1/2.
Applied on mvebu/arm, it was waiting too long.
Thanks,
Gregory
>
> On Tuesday 02 November 2021 18:12:58 Pali Rohár wrote:
>> - The code relies on rc_pci_fixup being called, which only happens
>> when CONFIG_PCI_QUIRKS is enabled, so add that to Kconfig. Omitting
>> this causes a booting failure with a non-obvious cause.
>> - Update rc_pci_fixup to set the class properly, copying the
>> more modern style from other places
>> - Correct the rc_pci_fixup comment
>>
>> This patch just re-applies commit 1dc831bf53fd ("ARM: Kirkwood: Update
>> PCI-E fixup") for all other Marvell ARM platforms which have same buggy
>> PCIe controller and do not use pci-mvebu.c controller driver yet.
>>
>> Long-term goal for these Marvell ARM platforms should be conversion to
>> pci-mvebu.c controller driver and removal of these fixups in arch code.
>>
>> Signed-off-by: Pali Rohár <[email protected]>
>> Cc: Jason Gunthorpe <[email protected]>
>> Cc: [email protected]
>>
>> ---
>> Changes in v2:
>> * Move MIPS change into separate patch
>> * Add information that this patch is for platforms which do not use pci-mvebu.c
>> ---
>> arch/arm/Kconfig | 1 +
>> arch/arm/mach-dove/pcie.c | 11 ++++++++---
>> arch/arm/mach-mv78xx0/pcie.c | 11 ++++++++---
>> arch/arm/mach-orion5x/Kconfig | 1 +
>> arch/arm/mach-orion5x/pci.c | 12 +++++++++---
>> 5 files changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index fc196421b2ce..9f157e973555 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -400,6 +400,7 @@ config ARCH_DOVE
>> select GENERIC_IRQ_MULTI_HANDLER
>> select GPIOLIB
>> select HAVE_PCI
>> + select PCI_QUIRKS if PCI
>> select MVEBU_MBUS
>> select PINCTRL
>> select PINCTRL_DOVE
>> diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
>> index ee91ac6b5ebf..ecf057a0f5ba 100644
>> --- a/arch/arm/mach-dove/pcie.c
>> +++ b/arch/arm/mach-dove/pcie.c
>> @@ -135,14 +135,19 @@ static struct pci_ops pcie_ops = {
>> .write = pcie_wr_conf,
>> };
>>
>> +/*
>> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
>> + * is operating as a root complex this needs to be switched to
>> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
>> + * the device. Decoding setup is handled by the orion code.
>> + */
>> static void rc_pci_fixup(struct pci_dev *dev)
>> {
>> - /*
>> - * Prevent enumeration of root complex.
>> - */
>> if (dev->bus->parent == NULL && dev->devfn == 0) {
>> int i;
>>
>> + dev->class &= 0xff;
>> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
>> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>> dev->resource[i].start = 0;
>> dev->resource[i].end = 0;
>> diff --git a/arch/arm/mach-mv78xx0/pcie.c b/arch/arm/mach-mv78xx0/pcie.c
>> index 636d84b40466..9362b5fc116f 100644
>> --- a/arch/arm/mach-mv78xx0/pcie.c
>> +++ b/arch/arm/mach-mv78xx0/pcie.c
>> @@ -177,14 +177,19 @@ static struct pci_ops pcie_ops = {
>> .write = pcie_wr_conf,
>> };
>>
>> +/*
>> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
>> + * is operating as a root complex this needs to be switched to
>> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
>> + * the device. Decoding setup is handled by the orion code.
>> + */
>> static void rc_pci_fixup(struct pci_dev *dev)
>> {
>> - /*
>> - * Prevent enumeration of root complex.
>> - */
>> if (dev->bus->parent == NULL && dev->devfn == 0) {
>> int i;
>>
>> + dev->class &= 0xff;
>> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
>> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>> dev->resource[i].start = 0;
>> dev->resource[i].end = 0;
>> diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
>> index e94a61901ffd..7189a5b1ec46 100644
>> --- a/arch/arm/mach-orion5x/Kconfig
>> +++ b/arch/arm/mach-orion5x/Kconfig
>> @@ -6,6 +6,7 @@ menuconfig ARCH_ORION5X
>> select GPIOLIB
>> select MVEBU_MBUS
>> select FORCE_PCI
>> + select PCI_QUIRKS
>> select PHYLIB if NETDEVICES
>> select PLAT_ORION_LEGACY
>> help
>> diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
>> index 76951bfbacf5..5145fe89702e 100644
>> --- a/arch/arm/mach-orion5x/pci.c
>> +++ b/arch/arm/mach-orion5x/pci.c
>> @@ -509,14 +509,20 @@ static int __init pci_setup(struct pci_sys_data *sys)
>> /*****************************************************************************
>> * General PCIe + PCI
>> ****************************************************************************/
>> +
>> +/*
>> + * The root complex has a hardwired class of PCI_CLASS_MEMORY_OTHER, when it
>> + * is operating as a root complex this needs to be switched to
>> + * PCI_CLASS_BRIDGE_HOST or Linux will errantly try to process the BAR's on
>> + * the device. Decoding setup is handled by the orion code.
>> + */
>> static void rc_pci_fixup(struct pci_dev *dev)
>> {
>> - /*
>> - * Prevent enumeration of root complex.
>> - */
>> if (dev->bus->parent == NULL && dev->devfn == 0) {
>> int i;
>>
>> + dev->class &= 0xff;
>> + dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
>> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>> dev->resource[i].start = 0;
>> dev->resource[i].end = 0;
>> --
>> 2.20.1
>>
--
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com