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

[Hongxing Zhu <hongxing.zhu@nxp.com>]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2023年3月18日 6:25
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>; l.stach@pengutronix.de;
> bhelgaas@google.com; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of RC in suspend
> and resume
> 
> On Fri, Mar 17, 2023 at 07:38:02AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > > Sent: 2023年3月16日 16:11
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > > Cc: Bjorn Helgaas <helgaas@kernel.org>; l.stach@pengutronix.de;
> > > bhelgaas@google.com; linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v2] PCI: imx6: Save and restore MSI control of
> > > RC in suspend and resume
> > >
> > > On Thu, Mar 16, 2023 at 07:37:41AM +0000, Hongxing Zhu wrote:
> > >
> > > [...]
> > >
> > > > > > > > It's not a separate register.
> > > > > > > >
> > > > > > > > The bit I manipulated is the MSI Enable bit of the Message
> > > > > > > > Control Register for MSI (Offset 02h) contained in the
> > > > > > > > MSI-capability of Root Complex.
> > > > > > > >
> > > > > > > > In addition, on i.MX6, the MSI Enable bit controls
> > > > > > > > delivery of MSI interrupts from components below the Root Port.
> > > > > > > >
> > > > > > > > So, set MSI Enable in imx6q-pcie to let the MSI from
> > > > > > > > downstream components works.
> > > > > > >
> > > > > > > My confusion is about this "MSI Capability" found by
> > > > > > > "dw_pcie_find_capability(pci, PCI_CAP_ID_MSI)" in your patch.
> > > > > > >
> > > > > > > The i.MX6 manual might refer to that as an "MSI Capability"
> > > > > > > but as far as I know, the PCIe base spec doesn't document a
> > > > > > > Root Complex MSI
> > > > > Capability.
> > > > > > >
> > > > > > > I don't think it's the same as the one documented in PCIe
> > > > > > > r6.0, sec 7.7.2.  I think it's different because:
> > > > > > >
> > > > > > >   (1) I *think* "pci" here refers to the RC, not to a Root Port.
> > > > > > >
> > > > > > >   (2) The semantics are different.  The MSI-X Enable bit in 7.7.2 only
> > > > > > >   determines whether the Function itself is permitted to use MSI-X.
> > > > > > >   It has nothing to do with devices *below* a Root Port can
> > > > > > > use
> > > MSI-X.
> > > > > > >   It also has nothing to do with whether a Root Port can forward MSI
> > > > > > >   transactions from those downstream devices.
> > > > > > >
> > > > > > > This part of my confusion could be easily resolved via a comment.
> > > > > > >
> > > > > > > I do have a follow-on question, though: the patch seems to
> > > > > > > enable MSI-related functionality using a register in the
> > > > > > > DesignWare IP, not something in the i.MX6-specific IP.  If
> > > > > > > that's true, why don't other DesignWare-based drivers need
> > > > > > > something
> > > similar?
> > > > > > Hi Bjorn:
> > > > > > Thanks a lot for you reply.
> > > > > > This behavior is specific for i.MX PCIe.
> > > > >
> > > > > Which behaviour ? It can't be the root port MSI capability, that
> > > > > would be a HW bug (ie disabling root port MSIs would imply
> > > > > disabling MSIs for all downstream components).
> > > > >
> > > >
> > > > i.MX PCIe designer use this MSI_EN bit to control the MSI trigger
> > > > when integrate  Design Ware PCIe IP.
> > > > Without the MSI_EN bit assertion (1b'1), the devices below this RC
> > > > can't trigger  the MSI successfully.
> > > > Yes, you're right. It should not be the root port MSI capability.
> > >
> > > The question is, it is or it is not the root port MSI capability ?
> > >
> > > If it is, that's a HW bug.
> > >
> > > If it is not there is nothing to do and this patch can be merged.
> > Hi Lorenzo:
> > Thanks for your reply.
> > I think it is not the root port MSI capability actually.
> > Refer to my understands, designer just use the msi_en bit to control
> > the  delivery of MSI interrupts from components below the Root Port.
> > >
> > > > > > i.MX PCIe designer use this MSI_EN bit to control the MSI
> > > > > > trigger when integrate Design Ware PCIe IP.
> > > > >
> > > > > Fair enough but that can't be the MSI Enable bit in the Root
> > > > > Port MSI capability "Message Control" field I am afraid.
> > > > >
> > > > > It is what Bjorn mentioned quite clearly, a root complex
> > > > > configuration register dressed as an MSI capability, the root
> > > > > complex is not a PCI device; either that or that's an HW bug.
> > > > Yes, it is. I agree with you. Had report this situation to the design team.
> > > > Hope to correct this bug in HW design if it's possible.
> > >
> > > I don't understand if it is a HW bug or not, see above. I think it
> > > is legitimate to have MMIO register space that *looks* like an MSI
> > > capability for the root complex to control delivery of MSI
> > > interrupts, as long as it is not the actual root port MSI
> > > capability, in the root port PCI config space in which case this would be a HW
> bug from what you are reporting.
> > I just provide the following suggestions.
> > - Root complex shouldn't have the MSI capability refer to the PCIe Spec
> >   7.7.1 chapter.
> > - Root port MSIs should not imply disabling MSIs for all downstream
> components.
> 
> I think this is all a lot of confusion, mostly on my part, sorry about that.
> 
> Root Complex configuration and behavior is not specified by the PCIe spec, so
> that's completely up to the i.MX designer.  It's fine for the Root Complex to have
> an MSI Capability, and it's fine for that capability to enable/disable the RC fielding
> of MSI MemWr transactions from downstream devices and triggering MSI
> interrupts.
> 
> It's also fine for the RC MSI Capability to be identified with a Capability ID of 0x5,
> although it is slightly confusing to use PCI_CAP_ID_MSI to find it.  It's also
> slightly confusing to use the PCI_MSI_FLAGS offset into the RC MSI Capability.
> 
> Using the PCI_CAP_ID_MSI, PCI_MSI_FLAGS, and PCI_MSI_FLAGS_ENABLE
> macros suggests to the reader that this RC MSI capability is the same as the the
> MSI Capability defined by PCIe r6.0, sec 7.7.1.  Obviously it is *not* the same,
> because we're talking about a *Root Complex* capability, while the sec 7.7.1
> capability can only appear on PCIe functions (Root Ports, Endpoints, Switch Ports,
> etc).
> 
> I suggest a comment to the effect that this is a Root Complex MSI Capability, not
> the MSI Capability defined by PCIe r6.0, sec 7.7.1.
> 
> Possibly even add new #defines in pci-imx6.c with different names, even though
> the values happen to be the same as the PCI_MSI_* #defines.  That would be a
> convenient place to put a comment about what they are.
Hi Bjorn:
Thanks a lot for your dispelling doubts.
How about to add the following comments in the new add function to clarify it?

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1036,6 +1036,18 @@ static void pci_imx_set_msi_en(struct dw_pcie *pci)
        u8 offset;
        u16 val;

+       /*
+        * When i.MX DM PCIe controller is configured as RC mode, it has one
+        * MSI Capability Structure, although PCIe r6.0, sec 7.7.1 doesn't
+        * specify the MSI Capability Structures for Root Complex.
+        *
+        * 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