Re: [PATCH v3 1/3] starfive: pci: Add StarFive JH7110 pcie driver

["Pali Rohár" <pali@kernel.org>]

On Friday 31 March 2023 18:35:00 Minda Chen wrote:
> On 2023/3/30 1:27, Pali Rohár wrote:
> > Hello!
> > 
> > On Wednesday 29 March 2023 18:01:41 Minda Chen wrote:
> >> +	/* PCIe PCI Standard Configuration Identification Settings. */
> >> +	value = (PCI_CLASS_BRIDGE_PCI_NORMAL << IDS_CLASS_CODE_SHIFT) | IDS_REVISION_ID;
> >> +	writel(value, priv->reg_base + PCIE_PCI_IDS);
> > 
> > This looks like configuration of the PCI_CLASS_REVISION read-only
> > register. Is there any reason why you are removing the original
> > "revision" information by hardcoded IDS_REVISION_ID constant?
> > 
> This register is not read-only register, consist  resion ID and class code.
>  Bit [39:32]: Revision ID
>  Bit [63:40]: Class code

I mean that this "priv->reg_base + PCIE_PCI_IDS" read-write register
configures what is visible when reading read-only standard PCI register
PCI_CLASS_REVISION.

PCIe Root Port is by definition of PCI-to-PCI Bridge class and therefore
reading from PCI_CLASS_REVISION must return PCI_CLASS_BRIDGE_PCI_* value.
This is required by PCIe spec.

With above driver init code it looks like that your PCIe controller does
not set correct value into PCI_CLASS_REVISION register after power-on
and manually setting correct value via "priv->reg_base + PCIE_PCI_IDS"
is a workaround.

Hopefully it is a more clear what I mean in my previous email.

Also mvebu controller has this issue and has similar workaround in
pci_mvebu.c driver.

> And the register reset value is zero, Our PCIe version is 2.0. So set value 2.
> Maybe I will add comment to  this.

Well, Revision ID (low 8 bits of PCI_CLASS_REVISION) is the extension to
the vendor / device id register. It is not version of PCIe standard.

I would expect that this Vendor ID, Device ID and Revision ID values are
not being changed by the driver as they identify PCI and PCIe devices.