> -----Original Message----- > From: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx> > Sent: 2023年3月16日 16:11 > To: Hongxing Zhu <hongxing.zhu@xxxxxxx> > Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx>; l.stach@xxxxxxxxxxxxxx; > bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > 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. Thanks. Best Regards Richard > > Please clarify, thank you very much. > > Lorenzo > > > Thanks. > > > > Best Regards > > Richard Zhu > > > > > > Lorenzo > > > > > > > So, the other DesignWare-base PCIe driver doesn't need this beahvior. > > > > > > > > Best Regards > > > > Richard Zhu > > > > > > > > > > > > > It seems that some device might shutdown msi when do the > > > > > > > > suspend > > > > > > > operations. > > > > > > > > > > > > > > > > > > Would you mind investigating it please ? > > > > > > > > Sure, I did further investigation on i.MX6QP platform. > > > > > > > > The MSI_EN bit of RC MSI capability would be cleared to > > > > > > > > zero, when > > > > > > > > PCIE_RESET(BIT29 of IOMUXC_GPR1) is toggled (assertion > > > > > > > > 1b'1, then de-assertion 1b'0). > > > > > > > > > > > > > > > > Verification steps: > > > > > > > > MSI_EN of RC is set to 1b'1 when system is boot up. > > > > > > > > ./memtool 1ffc050 1 > > > > > > > > 0x01FFC050: 01017005 > > > > > > > > > > > > > > > > Toggle PCIe reset of i.MX6QP. > > > > > > > > root@imx6qpdlsolox:~# ./memtool 20e0004=68691005 Writing > > > > > > > > 32-bit value > > > > > > > > 0x68691005 to address 0x020E0004 root@imx6qpdlsolox:~# > > > > > > > > ./memtool > > > > > > > > 20e0004=48691005 Writing 32-bit value 0x48691005 to > > > > > > > > address > > > > > > > 0x020E0004 > > > > > > > > > > > > > > > > The MSI_EN bit of RC had been cleared to 1b'0. > > > > > > > > ./memtool 1ffc050 1 > > > > > > > > 0x01FFC050: 01807005 > > > > > > > > > > > > > > > > This is why I used to reply to Bjorn the MSI_EN of RC is > > > > > > > > cleared when RESETs are toggled during the > > > > > > > > imx6_pcie_host_init() in > > > > > > > > imx6_pcie_resume_noirq() callback. > > > > > > > > > > > > > > > > Best Regards > > > > > > > > Richard Zhu > > > > > > > > > > > > > > > > > > Lorenzo > > > > > > > > > > > > > > > > > > > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx> > > > > > > > > > > --- > > > > > > > > > > Changes v1-->v2: > > > > > > > > > > New create one save/restore function, used save the > > > > > > > > > > setting in suspend and restore the configuration in resume. > > > > > > > > > > v1 > > > > > > > > > > https://eur01.safelinks.protection.outlook.com/?url=ht > > > > > > > > > > tps% > > > > > > > > > > 3A%2 > > > > > > > > > > F%2F > > > > > > > > > > > > > > > > > > > > > > > > > > patc%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C24971d8de9b54b > > > > > > > 0b10 > > > > > > > > > > > > > > > > > > > > > > > > > > ad08db2182774d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6 > > > > > > > 38140 > > > > > > > > > > > > > > > > > > > > > > > > > > 616456052078%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ > > > > > > > QIjoiV > > > > > > > > > > > > > > > > > > > > > > > > > > 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vE > > > > > > > tRxL > > > > > > > > > > > BVi5lYmpwTNZfafMms3263LZXodneLChjEaOM%3D&reserved=0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > hwork.kernel.org%2Fproject%2Flinux-pci%2Fpatch%2F1667289595-12440-1- > > > > > > > > > g > > > > > > > > > i > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > t-send-email-hongxing.zhu%40nxp.com%2F&data=05%7C01%7Chongxing.zhu > > > > > > > > > %40n > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > xp.com%7C3aeb1d128f854dad1a5608daea77706d%7C686ea1d3bc2b4c6fa9 > > > > > > > 2 > > > > > > > > > cd99c5c > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 301635%7C0%7C0%7C638080095954881374%7CUnknown%7CTWFpbGZsb3 > > > > > > > > > d8eyJWIjoiMC > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000 > > > > > > > % > > > > > > > > > 7C%7C% > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 7C&sdata=V8yVvvpTKGoR1UyQP5HD2IdlSjJdznBeD12bdI67dEI%3D&reserved > > > > > > > = > > > > > > > > > 0 > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > drivers/pci/controller/dwc/pci-imx6.c | 23 > > > > > > > > > > +++++++++++++++++++++++ > > > > > > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > > > > > > > > > b/drivers/pci/controller/dwc/pci-imx6.c > > > > > > > > > > index 1dde5c579edc..aa3096890c3b 100644 > > > > > > > > > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > > > > > > > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > > > > > > > > > @@ -76,6 +76,7 @@ struct imx6_pcie { > > > > > > > > > > struct clk *pcie; > > > > > > > > > > struct clk *pcie_aux; > > > > > > > > > > struct regmap *iomuxc_gpr; > > > > > > > > > > + u16 msi_ctrl; > > > > > > > > > > u32 controller_id; > > > > > > > > > > struct reset_control *pciephy_reset; > > > > > > > > > > struct reset_control *apps_reset; > > > > > > > > > > @@ -1042,6 +1043,26 @@ static void > > > > > > > > > > imx6_pcie_pm_turnoff(struct > > > > > > > > > imx6_pcie *imx6_pcie) > > > > > > > > > > usleep_range(1000, 10000); } > > > > > > > > > > > > > > > > > > > > +static void imx6_pcie_msi_save_restore(struct > > > > > > > > > > +imx6_pcie *imx6_pcie, bool save) { > > > > > > > > > > + u8 offset; > > > > > > > > > > + u16 val; > > > > > > > > > > + struct dw_pcie *pci = imx6_pcie->pci; > > > > > > > > > > + > > > > > > > > > > + if (pci_msi_enabled()) { > > > > > > > > > > + offset = dw_pcie_find_capability(pci, > PCI_CAP_ID_MSI); > > > > > > > > > > + if (save) { > > > > > > > > > > + val = dw_pcie_readw_dbi(pci, offset + > > > > > PCI_MSI_FLAGS); > > > > > > > > > > + imx6_pcie->msi_ctrl = val; > > > > > > > > > > + } else { > > > > > > > > > > + dw_pcie_dbi_ro_wr_en(pci); > > > > > > > > > > + val = imx6_pcie->msi_ctrl; > > > > > > > > > > + dw_pcie_writew_dbi(pci, offset + > PCI_MSI_FLAGS, > > > > > val); > > > > > > > > > > + dw_pcie_dbi_ro_wr_dis(pci); > > > > > > > > > > + } > > > > > > > > > > + } > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > static int imx6_pcie_suspend_noirq(struct device *dev) { > > > > > > > > > > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > > > @@ > > > > > > > > > > -1050,6 > > > > > > > > > > +1071,7 @@ static int imx6_pcie_suspend_noirq(struct > > > > > > > > > > +device > > > > > > > > > > +*dev) > > > > > > > > > > if (!(imx6_pcie->drvdata->flags & > > > > > > > > > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND)) > > > > > > > > > > return 0; > > > > > > > > > > > > > > > > > > > > + imx6_pcie_msi_save_restore(imx6_pcie, true); > > > > > > > > > > imx6_pcie_pm_turnoff(imx6_pcie); > > > > > > > > > > imx6_pcie_stop_link(imx6_pcie->pci); > > > > > > > > > > imx6_pcie_host_exit(pp); @@ -1069,6 +1091,7 @@ > > > > > > > > > > static int imx6_pcie_resume_noirq(struct device > > > > > > > > > *dev) > > > > > > > > > > ret = imx6_pcie_host_init(pp); > > > > > > > > > > if (ret) > > > > > > > > > > return ret; > > > > > > > > > > + imx6_pcie_msi_save_restore(imx6_pcie, false); > > > > > > > > > > dw_pcie_setup_rc(pp); > > > > > > > > > > > > > > > > > > > > if (imx6_pcie->link_is_up) > > > > > > > > > > -- > > > > > > > > > > 2.25.1
