diff mbox series

[12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge

Message ID 20211012164145.14126-13-kabel@kernel.org (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: aardvark controller fixes BATCH 2 | expand

Commit Message

Marek Behún Oct. 12, 2021, 4:41 p.m. UTC 
From: Pali Rohár <pali@kernel.org>

Aardvark controller has something like config space of a Root Port
available at offset 0x0 of internal registers - these registers are used
for implementation of the emulated bridge.

The default value of Class Code of this bridge corresponds to a RAID Mass
storage controller, though. (This is probably intended for when the
controller is used as Endpoint.)

Change the Class Code to correspond to a PCI Bridge.

Add comment explaining this change.

Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Lorenzo Pieralisi Oct. 28, 2021, 6:30 p.m. UTC | #1 
On Tue, Oct 12, 2021 at 06:41:43PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Aardvark controller has something like config space of a Root Port
> available at offset 0x0 of internal registers - these registers are used
> for implementation of the emulated bridge.
> 
> The default value of Class Code of this bridge corresponds to a RAID Mass
> storage controller, though. (This is probably intended for when the
> controller is used as Endpoint.)
> 
> Change the Class Code to correspond to a PCI Bridge.
> 
> Add comment explaining this change.
> 
> Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 289cd45ed1ec..801657e7da93 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -513,6 +513,26 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
>  	advk_writel(pcie, reg, VENDOR_ID_REG);
>  
> +	/*
> +	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
> +	 * because the default value is Mass storage controller (0x010400).
> +	 *
> +	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
> +	 * Configuration Space and it even cannot be accessed via Aardvark's
> +	 * PCI config space access method. Something like config space is
> +	 * available in internal Aardvark registers starting at offset 0x0
> +	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
> +	 * different registers.

Is the RP enumerated as a PCI device with type 0 header ?

> +	 *
> +	 * Therefore driver uses emulation of PCI Bridge which emulates
> +	 * access to configuration space via internal Aardvark registers or
> +	 * emulated configuration buffer.
> +	 */
> +	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
> +	reg &= ~0xffffff00;
> +	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
> +	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);

I remember Bjorn commenting on something similar in the past - he may
have some comments on whether this change is the right thing to do.

Lorenzo

>  	/* Disable Root Bridge I/O space, memory space and bus mastering */
>  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
>  	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> -- 
> 2.32.0
>
Pali Rohár Oct. 28, 2021, 6:45 p.m. UTC | #2 
On Thursday 28 October 2021 19:30:54 Lorenzo Pieralisi wrote:
> On Tue, Oct 12, 2021 at 06:41:43PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Aardvark controller has something like config space of a Root Port
> > available at offset 0x0 of internal registers - these registers are used
> > for implementation of the emulated bridge.
> > 
> > The default value of Class Code of this bridge corresponds to a RAID Mass
> > storage controller, though. (This is probably intended for when the
> > controller is used as Endpoint.)
> > 
> > Change the Class Code to correspond to a PCI Bridge.
> > 
> > Add comment explaining this change.
> > 
> > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 289cd45ed1ec..801657e7da93 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -513,6 +513,26 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
> >  	advk_writel(pcie, reg, VENDOR_ID_REG);
> >  
> > +	/*
> > +	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
> > +	 * because the default value is Mass storage controller (0x010400).
> > +	 *
> > +	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
> > +	 * Configuration Space and it even cannot be accessed via Aardvark's
> > +	 * PCI config space access method. Something like config space is
> > +	 * available in internal Aardvark registers starting at offset 0x0
> > +	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
> > +	 * different registers.
> 
> Is the RP enumerated as a PCI device with type 0 header ?

Yes.

And pci-bridge-emul.c "converts" it to type 1 header. So lspci correctly
see it as type 1.

> > +	 *
> > +	 * Therefore driver uses emulation of PCI Bridge which emulates
> > +	 * access to configuration space via internal Aardvark registers or
> > +	 * emulated configuration buffer.
> > +	 */
> > +	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
> > +	reg &= ~0xffffff00;
> > +	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
> > +	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);
> 
> I remember Bjorn commenting on something similar in the past - he may
> have some comments on whether this change is the right thing to do.

Root Port should have PCI Bridge class code, but aardvark HW initialize
it to class code for Mass Storage (as explained above).

pci-bridge-emul.c again transparently "converts" it to PCI Bridge class
code.

Similar issue has also mvebu hw, see email:
https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/

And similar fixup was applied for kirkwood:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1dc831bf53fddcc6443f74a39e72db5bcea4f15d

Are there any issues with it?

> Lorenzo
> 
> >  	/* Disable Root Bridge I/O space, memory space and bus mastering */
> >  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> >  	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> > -- 
> > 2.32.0
> >
Bjorn Helgaas Oct. 28, 2021, 8:43 p.m. UTC | #3 
On Thu, Oct 28, 2021 at 08:45:57PM +0200, Pali Rohár wrote:
> On Thursday 28 October 2021 19:30:54 Lorenzo Pieralisi wrote:
> > On Tue, Oct 12, 2021 at 06:41:43PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > Aardvark controller has something like config space of a Root Port
> > > available at offset 0x0 of internal registers - these registers are used
> > > for implementation of the emulated bridge.
> > > 
> > > The default value of Class Code of this bridge corresponds to a RAID Mass
> > > storage controller, though. (This is probably intended for when the
> > > controller is used as Endpoint.)
> > > 
> > > Change the Class Code to correspond to a PCI Bridge.
> > > 
> > > Add comment explaining this change.
> > > 
> > > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index 289cd45ed1ec..801657e7da93 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -513,6 +513,26 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > >  	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
> > >  	advk_writel(pcie, reg, VENDOR_ID_REG);
> > >  
> > > +	/*
> > > +	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
> > > +	 * because the default value is Mass storage controller (0x010400).
> > > +	 *
> > > +	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
> > > +	 * Configuration Space and it even cannot be accessed via Aardvark's
> > > +	 * PCI config space access method. Something like config space is
> > > +	 * available in internal Aardvark registers starting at offset 0x0
> > > +	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
> > > +	 * different registers.
> > 
> > Is the RP enumerated as a PCI device with type 0 header ?
> 
> Yes.
> 
> And pci-bridge-emul.c "converts" it to type 1 header. So lspci correctly
> see it as type 1.
> 
> > > +	 *
> > > +	 * Therefore driver uses emulation of PCI Bridge which emulates
> > > +	 * access to configuration space via internal Aardvark registers or
> > > +	 * emulated configuration buffer.
> > > +	 */
> > > +	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
> > > +	reg &= ~0xffffff00;
> > > +	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
> > > +	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);
> > 
> > I remember Bjorn commenting on something similar in the past - he may
> > have some comments on whether this change is the right thing to do.

No comments from me :)

> Root Port should have PCI Bridge class code, but aardvark HW initialize
> it to class code for Mass Storage (as explained above).
>
> pci-bridge-emul.c again transparently "converts" it to PCI Bridge class
> code.
> 
> Similar issue has also mvebu hw, see email:
> https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/
> 
> And similar fixup was applied for kirkwood:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1dc831bf53fddcc6443f74a39e72db5bcea4f15d
> 
> Are there any issues with it?
> 
> > Lorenzo
> > 
> > >  	/* Disable Root Bridge I/O space, memory space and bus mastering */
> > >  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> > >  	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> > > -- 
> > > 2.32.0
> > >
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 289cd45ed1ec..801657e7da93 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -513,6 +513,26 @@  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
 	advk_writel(pcie, reg, VENDOR_ID_REG);
 
+	/*
+	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
+	 * because the default value is Mass storage controller (0x010400).
+	 *
+	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
+	 * Configuration Space and it even cannot be accessed via Aardvark's
+	 * PCI config space access method. Something like config space is
+	 * available in internal Aardvark registers starting at offset 0x0
+	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
+	 * different registers.
+	 *
+	 * Therefore driver uses emulation of PCI Bridge which emulates
+	 * access to configuration space via internal Aardvark registers or
+	 * emulated configuration buffer.
+	 */
+	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
+	reg &= ~0xffffff00;
+	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
+	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);
+
 	/* Disable Root Bridge I/O space, memory space and bus mastering */
 	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);