linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: Marvell: Update PCIe fixup
@ 2021-11-01 15:04 Pali Rohár
  2021-11-01 16:27 ` Jason Gunthorpe
                   ` (2 more replies)
  0 siblings, 3 replies; 30+ messages in thread
From: Pali Rohár @ 2021-11-01 15:04 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Thomas Bogendoerfer, Jason Gunthorpe
  Cc: linux-arm-kernel, linux-mips, linux-pci, linux-kernel

- 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 <pali@kernel.org>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: stable@vger.kernel.org
---
 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


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-01 15:04 [PATCH] PCI: Marvell: Update PCIe fixup Pali Rohár
@ 2021-11-01 16:27 ` Jason Gunthorpe
  2021-11-01 17:56   ` Pali Rohár
  2021-11-02  8:42 ` Thomas Bogendoerfer
  2021-11-02 17:12 ` [PATCH v2 1/2] ARM: " Pali Rohár
  2 siblings, 1 reply; 30+ messages in thread
From: Jason Gunthorpe @ 2021-11-01 16:27 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Thomas Bogendoerfer, linux-arm-kernel,
	linux-mips, linux-pci, linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-01 16:27 ` Jason Gunthorpe
@ 2021-11-01 17:56   ` Pali Rohár
  2021-11-01 18:03     ` Jason Gunthorpe
  0 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2021-11-01 17:56 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Thomas Bogendoerfer, linux-arm-kernel,
	linux-mips, linux-pci, linux-kernel

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/

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-01 17:56   ` Pali Rohár
@ 2021-11-01 18:03     ` Jason Gunthorpe
  0 siblings, 0 replies; 30+ messages in thread
From: Jason Gunthorpe @ 2021-11-01 18:03 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Thomas Bogendoerfer, linux-arm-kernel,
	linux-mips, linux-pci, linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-01 15:04 [PATCH] PCI: Marvell: Update PCIe fixup Pali Rohár
  2021-11-01 16:27 ` Jason Gunthorpe
@ 2021-11-02  8:42 ` Thomas Bogendoerfer
  2021-11-02  9:02   ` Pali Rohár
  2021-11-02 17:12 ` [PATCH v2 1/2] ARM: " Pali Rohár
  2 siblings, 1 reply; 30+ messages in thread
From: Thomas Bogendoerfer @ 2021-11-02  8:42 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02  8:42 ` Thomas Bogendoerfer
@ 2021-11-02  9:02   ` Pali Rohár
  2021-11-02  9:47     ` Thomas Bogendoerfer
  0 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2021-11-02  9:02 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02  9:02   ` Pali Rohár
@ 2021-11-02  9:47     ` Thomas Bogendoerfer
  2021-11-02 10:00       ` Pali Rohár
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Bogendoerfer @ 2021-11-02  9:47 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02  9:47     ` Thomas Bogendoerfer
@ 2021-11-02 10:00       ` Pali Rohár
  2021-11-02 12:35         ` Maciej W. Rozycki
  2021-11-02 15:02         ` Thomas Bogendoerfer
  0 siblings, 2 replies; 30+ messages in thread
From: Pali Rohár @ 2021-11-02 10:00 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 10:00       ` Pali Rohár
@ 2021-11-02 12:35         ` Maciej W. Rozycki
  2021-11-02 12:58           ` Pali Rohár
  2021-11-02 15:02         ` Thomas Bogendoerfer
  1 sibling, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2021-11-02 12:35 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	linux-arm-kernel, linux-mips, linux-pci, linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 12:35         ` Maciej W. Rozycki
@ 2021-11-02 12:58           ` Pali Rohár
  2021-11-02 14:01             ` Maciej W. Rozycki
  0 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2021-11-02 12:58 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	linux-arm-kernel, linux-mips, linux-pci, linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 12:58           ` Pali Rohár
@ 2021-11-02 14:01             ` Maciej W. Rozycki
  2021-11-02 14:49               ` Pali Rohár
  0 siblings, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2021-11-02 14:01 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	linux-arm-kernel, linux-mips, linux-pci, linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 14:01             ` Maciej W. Rozycki
@ 2021-11-02 14:49               ` Pali Rohár
  2021-11-02 15:48                 ` Pali Rohár
  2021-11-03 14:49                 ` Maciej W. Rozycki
  0 siblings, 2 replies; 30+ messages in thread
From: Pali Rohár @ 2021-11-02 14:49 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	Marek Behún, linux-arm-kernel, linux-mips, linux-pci,
	linux-kernel

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 :-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 10:00       ` Pali Rohár
  2021-11-02 12:35         ` Maciej W. Rozycki
@ 2021-11-02 15:02         ` Thomas Bogendoerfer
  2021-11-02 15:13           ` Pali Rohár
  1 sibling, 1 reply; 30+ messages in thread
From: Thomas Bogendoerfer @ 2021-11-02 15:02 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 15:02         ` Thomas Bogendoerfer
@ 2021-11-02 15:13           ` Pali Rohár
  2021-11-09 23:42             ` Pali Rohár
  0 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2021-11-02 15:13 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 14:49               ` Pali Rohár
@ 2021-11-02 15:48                 ` Pali Rohár
  2021-11-02 17:03                   ` Stefan Roese
  2021-11-03 14:59                   ` Maciej W. Rozycki
  2021-11-03 14:49                 ` Maciej W. Rozycki
  1 sibling, 2 replies; 30+ messages in thread
From: Pali Rohár @ 2021-11-02 15:48 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	Marek Behún, linux-arm-kernel, linux-mips, linux-pci,
	linux-kernel

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 :-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 15:48                 ` Pali Rohár
@ 2021-11-02 17:03                   ` Stefan Roese
  2021-11-03 14:59                   ` Maciej W. Rozycki
  1 sibling, 0 replies; 30+ messages in thread
From: Stefan Roese @ 2021-11-02 17:03 UTC (permalink / raw)
  To: Pali Rohár, Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	Marek Behún, linux-arm-kernel, linux-mips, linux-pci,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup
  2021-11-01 15:04 [PATCH] PCI: Marvell: Update PCIe fixup Pali Rohár
  2021-11-01 16:27 ` Jason Gunthorpe
  2021-11-02  8:42 ` Thomas Bogendoerfer
@ 2021-11-02 17:12 ` Pali Rohár
  2021-11-02 17:12   ` [PATCH v2 2/2] MIPS: Cobalt: Explain GT64111 early PCI fixup Pali Rohár
                     ` (2 more replies)
  2 siblings, 3 replies; 30+ messages in thread
From: Pali Rohár @ 2021-11-02 17:12 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe
  Cc: linux-arm-kernel, linux-pci, linux-kernel

- 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 <pali@kernel.org>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: stable@vger.kernel.org

---
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


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 2/2] MIPS: Cobalt: Explain GT64111 early PCI fixup
  2021-11-02 17:12 ` [PATCH v2 1/2] ARM: " Pali Rohár
@ 2021-11-02 17:12   ` Pali Rohár
  2021-11-03 16:36     ` Thomas Bogendoerfer
  2021-11-09 22:53   ` [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup Pali Rohár
  2022-02-19 14:30   ` Pali Rohár
  2 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2021-11-02 17:12 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Maciej W. Rozycki; +Cc: linux-mips, linux-kernel

Properly document why changing PCI Class Code for GT64111 device to Host
Bridge is required as important details were after 20 years forgotten.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Split from ARM changes
* Removal of Kconfig changes
* Explanation is completely rewritten as as this MIPS Cobalt device
  predates ARM Orion devices and reason is slightly different.
---
 arch/mips/pci/fixup-cobalt.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/mips/pci/fixup-cobalt.c b/arch/mips/pci/fixup-cobalt.c
index 44be65c3e6bb..00206ff52988 100644
--- a/arch/mips/pci/fixup-cobalt.c
+++ b/arch/mips/pci/fixup-cobalt.c
@@ -36,6 +36,21 @@
 #define VIA_COBALT_BRD_ID_REG  0x94
 #define VIA_COBALT_BRD_REG_to_ID(reg)	((unsigned char)(reg) >> 4)
 
+/*
+ * Default value of PCI Class Code on GT64111 is PCI_CLASS_MEMORY_OTHER (0x0580)
+ * instead of PCI_CLASS_BRIDGE_HOST (0x0600). Galileo explained this choice in
+ * document "GT-64111 System Controller for RC4640, RM523X and VR4300 CPUs",
+ * section "6.5.3 PCI Autoconfiguration at RESET":
+ *
+ *   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.
+ *
+ * So fix the incorrect default value of PCI Class Code. More details are on:
+ * https://lore.kernel.org/r/20211102154831.xtrlgrmrizl5eidl@pali/
+ * https://lore.kernel.org/r/20211102150201.GA11675@alpha.franken.de/
+ */
 static void qube_raq_galileo_early_fixup(struct pci_dev *dev)
 {
 	if (dev->devfn == PCI_DEVFN(0, 0) &&
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 14:49               ` Pali Rohár
  2021-11-02 15:48                 ` Pali Rohár
@ 2021-11-03 14:49                 ` Maciej W. Rozycki
  2021-11-03 15:03                   ` Pali Rohár
  1 sibling, 1 reply; 30+ messages in thread
From: Maciej W. Rozycki @ 2021-11-03 14:49 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	Marek Behún, linux-arm-kernel, linux-mips, linux-pci,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 15:48                 ` Pali Rohár
  2021-11-02 17:03                   ` Stefan Roese
@ 2021-11-03 14:59                   ` Maciej W. Rozycki
  1 sibling, 0 replies; 30+ messages in thread
From: Maciej W. Rozycki @ 2021-11-03 14:59 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	Marek Behún, linux-arm-kernel, linux-mips, linux-pci,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-03 14:49                 ` Maciej W. Rozycki
@ 2021-11-03 15:03                   ` Pali Rohár
  0 siblings, 0 replies; 30+ messages in thread
From: Pali Rohár @ 2021-11-03 15:03 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Gregory Clement, Jason Gunthorpe,
	Marek Behún, linux-arm-kernel, linux-mips, linux-pci,
	linux-kernel

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/2] MIPS: Cobalt: Explain GT64111 early PCI fixup
  2021-11-02 17:12   ` [PATCH v2 2/2] MIPS: Cobalt: Explain GT64111 early PCI fixup Pali Rohár
@ 2021-11-03 16:36     ` Thomas Bogendoerfer
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Bogendoerfer @ 2021-11-03 16:36 UTC (permalink / raw)
  To: Pali Rohár; +Cc: Maciej W. Rozycki, linux-mips, linux-kernel

On Tue, Nov 02, 2021 at 06:12:59PM +0100, Pali Rohár wrote:
> Properly document why changing PCI Class Code for GT64111 device to Host
> Bridge is required as important details were after 20 years forgotten.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v2:
> * Split from ARM changes
> * Removal of Kconfig changes
> * Explanation is completely rewritten as as this MIPS Cobalt device
>   predates ARM Orion devices and reason is slightly different.
> ---
>  arch/mips/pci/fixup-cobalt.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup
  2021-11-02 17:12 ` [PATCH v2 1/2] ARM: " Pali Rohár
  2021-11-02 17:12   ` [PATCH v2 2/2] MIPS: Cobalt: Explain GT64111 early PCI fixup Pali Rohár
@ 2021-11-09 22:53   ` Pali Rohár
  2022-05-14 18:21     ` Pali Rohár
  2022-02-19 14:30   ` Pali Rohár
  2 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2021-11-09 22:53 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe
  Cc: linux-arm-kernel, linux-pci, linux-kernel

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 <pali@kernel.org>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: stable@vger.kernel.org

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-02 15:13           ` Pali Rohár
@ 2021-11-09 23:42             ` Pali Rohár
  2021-11-10  8:55               ` Thomas Bogendoerfer
  0 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2021-11-09 23:42 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: Marvell: Update PCIe fixup
  2021-11-09 23:42             ` Pali Rohár
@ 2021-11-10  8:55               ` Thomas Bogendoerfer
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Bogendoerfer @ 2021-11-10  8:55 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe, linux-arm-kernel, linux-mips,
	linux-pci, linux-kernel

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 ]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup
  2021-11-02 17:12 ` [PATCH v2 1/2] ARM: " Pali Rohár
  2021-11-02 17:12   ` [PATCH v2 2/2] MIPS: Cobalt: Explain GT64111 early PCI fixup Pali Rohár
  2021-11-09 22:53   ` [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup Pali Rohár
@ 2022-02-19 14:30   ` Pali Rohár
  2022-07-18 10:34     ` Gregory CLEMENT
  2 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2022-02-19 14:30 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe
  Cc: linux-arm-kernel, linux-pci, linux-kernel

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 <pali@kernel.org>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: stable@vger.kernel.org
> 
> ---
> 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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup
  2021-11-09 22:53   ` [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup Pali Rohár
@ 2022-05-14 18:21     ` Pali Rohár
  2022-07-07 18:31       ` Pali Rohár
  0 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2022-05-14 18:21 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe
  Cc: linux-arm-kernel, linux-pci, linux-kernel

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 <pali@kernel.org>
> > Cc: Jason Gunthorpe <jgg@nvidia.com>
> > Cc: stable@vger.kernel.org
> 
> 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
> > 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup
  2022-05-14 18:21     ` Pali Rohár
@ 2022-07-07 18:31       ` Pali Rohár
  2022-07-07 19:22         ` Russell King (Oracle)
  0 siblings, 1 reply; 30+ messages in thread
From: Pali Rohár @ 2022-07-07 18:31 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Sebastian Hesselbarth,
	Gregory Clement, Jason Gunthorpe
  Cc: linux-arm-kernel, linux-pci, linux-kernel

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 <pali@kernel.org>
> > > Cc: Jason Gunthorpe <jgg@nvidia.com>
> > > Cc: stable@vger.kernel.org
> > 
> > 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
> > > 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup
  2022-07-07 18:31       ` Pali Rohár
@ 2022-07-07 19:22         ` Russell King (Oracle)
  0 siblings, 0 replies; 30+ messages in thread
From: Russell King (Oracle) @ 2022-07-07 19:22 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Sebastian Hesselbarth, Gregory Clement,
	Jason Gunthorpe, linux-arm-kernel, linux-pci, linux-kernel

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!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup
  2022-02-19 14:30   ` Pali Rohár
@ 2022-07-18 10:34     ` Gregory CLEMENT
  0 siblings, 0 replies; 30+ messages in thread
From: Gregory CLEMENT @ 2022-07-18 10:34 UTC (permalink / raw)
  To: Pali Rohár, Russell King, Andrew Lunn,
	Sebastian Hesselbarth, Jason Gunthorpe
  Cc: linux-arm-kernel, linux-pci, linux-kernel

Pali Rohár <pali@kernel.org> 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 <pali@kernel.org>
>> Cc: Jason Gunthorpe <jgg@nvidia.com>
>> Cc: stable@vger.kernel.org
>> 
>> ---
>> 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

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2022-07-18 10:34 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-01 15:04 [PATCH] PCI: Marvell: Update PCIe fixup Pali Rohár
2021-11-01 16:27 ` Jason Gunthorpe
2021-11-01 17:56   ` Pali Rohár
2021-11-01 18:03     ` Jason Gunthorpe
2021-11-02  8:42 ` Thomas Bogendoerfer
2021-11-02  9:02   ` Pali Rohár
2021-11-02  9:47     ` Thomas Bogendoerfer
2021-11-02 10:00       ` Pali Rohár
2021-11-02 12:35         ` Maciej W. Rozycki
2021-11-02 12:58           ` Pali Rohár
2021-11-02 14:01             ` Maciej W. Rozycki
2021-11-02 14:49               ` Pali Rohár
2021-11-02 15:48                 ` Pali Rohár
2021-11-02 17:03                   ` Stefan Roese
2021-11-03 14:59                   ` Maciej W. Rozycki
2021-11-03 14:49                 ` Maciej W. Rozycki
2021-11-03 15:03                   ` Pali Rohár
2021-11-02 15:02         ` Thomas Bogendoerfer
2021-11-02 15:13           ` Pali Rohár
2021-11-09 23:42             ` Pali Rohár
2021-11-10  8:55               ` Thomas Bogendoerfer
2021-11-02 17:12 ` [PATCH v2 1/2] ARM: " Pali Rohár
2021-11-02 17:12   ` [PATCH v2 2/2] MIPS: Cobalt: Explain GT64111 early PCI fixup Pali Rohár
2021-11-03 16:36     ` Thomas Bogendoerfer
2021-11-09 22:53   ` [PATCH v2 1/2] ARM: Marvell: Update PCIe fixup Pali Rohár
2022-05-14 18:21     ` Pali Rohár
2022-07-07 18:31       ` Pali Rohár
2022-07-07 19:22         ` Russell King (Oracle)
2022-02-19 14:30   ` Pali Rohár
2022-07-18 10:34     ` Gregory CLEMENT

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).