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

[Minda Chen]

On 2023/3/31 20:59, Pali Rohár wrote:
> 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.
I am sorry. It is PCIe controller IP revision ID. I have make a mistake for it.
So zero value is correct. I will remove writing IDS_REVISION_ID value.