Re: [PATCH] PCI: Marvell: Update PCIe fixup

[Stefan Roese]

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