On Tue, Mar 21, 2023 at 09:39:44PM +0000, Frank Li wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > Sent: Tuesday, March 21, 2023 3:59 PM
> > To: Frank Li <frank.li@xxxxxxx>
> > Cc: bhelgaas@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; dl-linux-imx <linux-
> > imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> > gustavo.pimentel@xxxxxxxxxxxx; kw@xxxxxxxxx; linux-arm-
> > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > pci@xxxxxxxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; M.H. Lian
> > <minghuan.lian@xxxxxxx>; Mingkai Hu <mingkai.hu@xxxxxxx>;
> > robh+dt@xxxxxxxxxx; Roy Zang <roy.zang@xxxxxxx>;
> > shawnguo@xxxxxxxxxx; Z.Q. Hou <zhiqiang.hou@xxxxxxx>
> > 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@xxxxxxx>
> > >
> > > 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
