Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend and resume

[Serge Semin]

On Wed, Apr 05, 2023 at 05:55:48PM +0200, Lorenzo Pieralisi wrote:
> [+Cc Sergey]
> 

Hi Lorenzo

[nip]

> > > 
> > > That's because a PCI root complex is not a PCI device (and this is not an MSI
> > > capability, which lives in PCI config space).
> > > 
> > > I will reword it (and the commit log with it) and merge it in the coming weeks for
> > > v6.4
> > Hi Lorenzo:
> > Thanks a lot for your kindly help.
> 
> I am getting back to this since I am still not convinced and I want to
> understand this once for all.
> 
> We do use dw_pcie_find_capability() in most DWC drivers to find and
> peek/poke at eg PCI express capability of the *Root port* (?),
> 
> eg dw_pcie_wait_for_link()
> 
> so I assume that for iMX6 dw_pcie_find_capability() does just the same,
> which would mean that we are poking the "Message Control" field of the
> Root port MSI capability.

Absolutely right. Besides of the standard PCIe config space which is
filled with the various PCIe capability registers, the DW PCIe Root
Port has the "Port Logic" CSR space with the DW PCIe-specific
settings: GENx Link control and lanes tuning stuff, iMSI-Rx engine,
iATU/eDMA, etc. Some of the capabilities are utilized by their direct
functionality (like the PCI Express capability) meanwhile some are
left unused even though they can be activated during the IP-core
synthesize stage. I don't know why the HW designers left them being
optionally enabled and then available for some setups, DW PCIe RC
HW-manual doesn't have their functionality description. The MSI
capability is one of such PCIe capabilities. It's described in the DW
PCIe RC registers space HW-manual and the DW PCIe RC IP-core
parameters, but the rest of the manual text has nothing about it.

Also note even though the DW PCIe RC HW-manual has _RC_ in the name it
actually describes the PCIe Root Port with the attention remark: "The
Synopsys PCI Express IP does not implement a full root complex."

So from what I can see there can be several reasons of such
configuration:
1. It simplifies the dual-mode device design for the HW-engineers.
Though IMO Synopsys at least should have dropped the unused
capabilities from the RP HW-manual.
2. Synopsys may imply that these capabilities can be used somehow
for the vendor-specific true Root Complex designs. Though they don't
specifically say about that. Instead there is a note: "To implement a
full root complex, you must add your application logic (connected to
the application client interface or AXI interface) and other support
logic (including clock and reset generation)."

> 
> Either that (which would mean that iMX6 has a HW bug because the RP
> Message Control field does not control the delivery of MSIs from
> endpoints but just for the root port itself )

I wouldn't call it a HW bug seeing the MSI capability registers are
actually unused by the RP controllers. So it looks more like the
iMX6-specific feature. See a short explanation in the commit
75cb8d20c112 ("PCI: imx: Enable MSI from downstream components").

Normally MSI event delivery to the CPU doesn't depend on the actual
MSI capability CSRs state in DW PCIe RPs. Instead the MSI is
configured via the Port Logic iMSI-RX registers. That's what is done
in the DW PCIe host driver (see pcie-designware-host.c).

> or all DWC controllers
> modelled the root complex MMIO space as a set of PCI/PCIe capabilities
> that are NOT necessarily mappable to PCI specifications defined ones.

Basically you are right though as I noted above Synopsys doesn't
provide a complete RC solution. It's just a single RP with the MMIO
(DBI) CSRs looking as the PCIe TYPE1 config space.

-Serge(y)

> 
> Can anyone please shed some light on this ? I don't have DWC HW, we need
> to know before merging this code.
> 
> Thanks,
> Lorenzo
> 
> > 
> > Best Regards
> > Richard Zhu
> > > 
> > > Thanks,
> > > Lorenzo
> > > 
> > > > +        *
> > > > +        * The MSI_EN bit of MSI control register contained in this MSI-CAP
> > > > +        * is used control the MSI delivery of MSI interrupts from components
> > > > +        * below the Root Port.
> > > > +        *
> > > > +        * Find it by PCI_CAP_ID_MSI here, and assert the MSI_EN bit to
> > > allow
> > > > +        * the MSI delivery below the Root Port, if the PCI MSI is enabled.
> > > > +        */
> > > >         if (pci_msi_enabled()) {
> > > >                 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > > >                 dw_pcie_dbi_ro_wr_en(pci); Best Regards Richard Zhu
> > > > >
> > > > > Bjorn
>