>
> Caution: EXT Email
>
> On Fri, Mar 17, 2023 at 04:05:28PM -0400, Frank Li wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@xxxxxxx>
> >
> > Add PME_Turn_Off/PME_TO_Ack handshake sequence, and finally
> > put the PCIe controller into D3 state after the L2/L3 ready
> > state transition process completion.
>
> Can you please include a sentence or two about what this means for
> devices below the PCIe controller? Is this guaranteed to be safe for
> them, i.e., can all PCIe devices tolerate PME_Turn_Off, etc., and
> resume correctly afterwards?
We can't guarantee all PCIe devices tolerate PME_Turn_off etc. We just
follow PCI Spec and test some available devices to make sure our implement
is correct.
Anyways, it was still better than nothing.
>
> I suspect other drivers will copy this sort of pattern if it is safe
> and useful.
>
> > struct ls_pcie {
> > struct dw_pcie *pci;
> > + const struct ls_pcie_drvdata *drvdata;
> > + void __iomem *pf_base;
> > + void __iomem *lut_base;
> > + bool big_endian;
> > + bool ep_presence;
>
> This means "any downstream device present", right? Could be an
> Endpoint or could be a Switch Upstream Port? I guess it's basically a
> cache of dw_pcie_link_up() at ls_pcie_host_init()-time.
Should be an Endpoint. Most of our user case is that connect pcie
wifi module.
>
> > + bool pm_support;
> > + struct regmap *scfg;
> > + int index;
> > };
>
> > +static void ls1021a_pcie_send_turnoff_msg(struct ls_pcie *pcie)
> > +{
> > + u32 val;
> > +
> > + if (!pcie->scfg) {
> > + dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > + return;
> > + }
> > +
> > + /* Send Turn_off message */
> > + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > + val |= PMXMTTURNOFF;
> > + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +
> > + /*
> > + * Components with an upstream port must respond to
> > + * PME_Turn_Off with PME_TO_Ack but we can't check.
> > + *
> > + * The standard recommends a 1-10ms timeout after which to
> > + * proceed anyway as if acks were received.
>
> Spec citation please.
>
> > + */
> > + mdelay(10);
> > +
> > + /* Clear Turn_off message */
> > + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > + val &= ~PMXMTTURNOFF;
> > + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +}
>
> > +static bool ls_pcie_pm_check(struct ls_pcie *pcie)
>
> This is used as a boolean ("if (!ls_pcie_pm_check())") so it needs a
> better name. "Check" doesn't give any hint about what a true or false
> return value means. Something like "pm_supported" *would* give a
> hint because "if (!ls_pcie_pm_supported())" is a sensible question to
> ask.
>
> > +{
> > + if (!pcie->ep_presence) {
> > + dev_dbg(pcie->pci->dev, "Endpoint isn't present\n");
> > + return false;
> > + }
> > +
> > + if (!pcie->pm_support)
> > + return false;
>
> Why test the negative ("!pcie->pm_support") and then return false?
> How about:
>
> if (pcie->pm_support)
> return true;
>
> return false;
>
> or even better, just:
>
> return pcie->pm_support;
>
> > + return true;
> > +}
