Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 22 Mar 2023 16:19:29 -0500 | From | Bjorn Helgaas <> | Subject | Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management support |
| |
On Tue, Mar 21, 2023 at 09:39:44PM +0000, Frank Li wrote: > > -----Original Message----- > > From: Bjorn Helgaas <helgaas@kernel.org> > > Sent: Tuesday, March 21, 2023 3:59 PM > > To: Frank Li <frank.li@nxp.com> > > Cc: bhelgaas@google.com; Leo Li <leoyang.li@nxp.com>; dl-linux-imx <linux- > > imx@nxp.com>; devicetree@vger.kernel.org; > > gustavo.pimentel@synopsys.com; kw@linux.com; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > > pci@vger.kernel.org; lorenzo.pieralisi@arm.com; M.H. Lian > > <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; > > robh+dt@kernel.org; Roy Zang <roy.zang@nxp.com>; > > shawnguo@kernel.org; Z.Q. Hou <zhiqiang.hou@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@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
| ![\](/images/icornerr.gif) |