[EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management support

[Bjorn Helgaas]

On Tue, Mar 21, 2023 at 09:39:44PM +0000, Frank Li wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas at kernel.org>
> > Sent: Tuesday, March 21, 2023 3:59 PM
> > To: Frank Li <frank.li at nxp.com>
> > Cc: bhelgaas at google.com; Leo Li <leoyang.li at nxp.com>; dl-linux-imx <linux-
> > imx at nxp.com>; devicetree at vger.kernel.org;
> > gustavo.pimentel at synopsys.com; kw at linux.com; linux-arm-
> > kernel at lists.infradead.org; linux-kernel at vger.kernel.org; linux-
> > pci at vger.kernel.org; lorenzo.pieralisi at arm.com; M.H. Lian
> > <minghuan.lian at nxp.com>; Mingkai Hu <mingkai.hu at nxp.com>;
> > robh+dt at kernel.org; Roy Zang <roy.zang at nxp.com>;
> > shawnguo at kernel.org; Z.Q. Hou <zhiqiang.hou at nxp.com>
> > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management
> > support
> > 
> > Caution: EXT Email
> > 
> > On Tue, Mar 21, 2023 at 12:02:20PM -0400, Frank Li wrote:
> > > From: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
> > >
> > > Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices,
> > such as
> > > NVME or wifi module, and finally put the PCIe controller into D3 state
> > > after the L2/L3 ready state transition process completion.
> > >
> > > However, it's important to note that not all devices may be able to
> > > tolerate the PME_Turn_Off command. In general, fixed PCIe devices
> > > connected to Layerscape, such as NXP wifi devices, are able to handle
> > > this command.
> > 
> > I know this paragraph is here because I asked whether all PCIe devices
> > could tolerate PME_Turn_Off.  I don't know much about that level of
> > the protocol, but it does look to me like PME_Turn_Off is required,
> > e.g., PCIe r6.0, sec 5.3.3.2.1, 5.3.3.4.
> > 
> > So I'm not sure this paragraph adds anything useful.  If the spec
> > requires it, this paragraph is like saying "it's important to note
> > that some PCIe devices may not follow the spec," which is pointless.
> > 
> > This functionality results in any downstream devices being put in
> > D3cold, right?  I think that *would* be worth mentioning.  There are a
> > few cases where we try to avoid putting devices in D3cold, e.g.,
> > no_d3cold, and I suspect this functionality would put them in D3cold
> > regardless of no_d3cold.  Those are corner cases that you would
> > probably never see on your platform, so a brief mention here is
> > probably enough.
> > 
> > > +static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
> > > +{
> > > +     struct dw_pcie *pci = pcie->pci;
> > > +     u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
> > > +     u32 val;
> > > +
> > > +     val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
> > > +     val &= ~PCI_PM_CTRL_STATE_MASK;
> > > +     val |= dstate;
> > > +     dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);
> > 
> > Is this a power management register for the *Root Port*, i.e., as
> > defined by PCIe r6.0 sec 7.5.2?
> > 
> > Or is it a similar register for the *Root Complex* as a whole that is
> > defined by a Layerscape or DesignWare spec and coincidentally uses the
> > same Capability ID and control register layout as the PCIe one?
> 
> I think it is root port. Does linux framework can do that for it
> automatically?  Or need call pci_set_power_state here instead of
> write register directly?

Well, maybe, the linux framework might already put Root Ports in D3 if
the right conditions are satisfied.  I don't understand all of them,
but you can start at pci_dev_check_d3cold() and look at its users and
instrument things to see what actually happens.

But it might be a problem if that has to be synchronized and done in
the right order with respect to the RC things you do here, because I
don't think there's a hook for the PCI core to call your driver to do
the RC stuff.

Bjorn