| Message ID | 20220902233543.390890-3-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | PCI/PM: Always disable PTM for all devices during suspend | expand |
On 9/2/22 4:35 PM, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/pcie/ptm.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c > index b6a417247ce3..ad283818f37b 100644 > --- a/drivers/pci/pcie/ptm.c > +++ b/drivers/pci/pcie/ptm.c > @@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) > if (!pos) > return -EINVAL; > > - pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > - if (!(cap & PCI_PTM_CAP_REQ)) > - return -EINVAL; > - May be saving PCI_PTM_CAP_REQ enabled state here and using it below only for endpoints checks will reduce the code duplication? > /* > + * Root Ports and Switch Upstream Ports have been configured > + * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and > + * granularity. > + * > * For a PCIe Endpoint, PTM is only useful if the endpoint can > * issue PTM requests to upstream devices that have PTM enabled. > * > @@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) > * device, so there must be some implementation-specific way to > * associate the endpoint with a time source. > */ > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > + pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { > + ups = pci_upstream_bridge(dev); > + if (!ups || !ups->ptm_enabled) > + return -EINVAL; > + } > + > + pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl); Why read PCI_PTM_CTRL state only for root and upstream ports? The same logic will work for endpoints and RC endpoints right? What not use dev->ptm_granularity for root ports? > + ctrl |= PCI_PTM_CTRL_ENABLE; > + } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { > + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > + if (!(cap & PCI_PTM_CAP_REQ)) > + return -EINVAL; > + > ups = pci_upstream_bridge(dev); > if (!ups || !ups->ptm_enabled) > return -EINVAL; > > dev->ptm_granularity = ups->ptm_granularity; > + ctrl = PCI_PTM_CTRL_ENABLE; > + ctrl |= dev->ptm_granularity << 8; > } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) { > + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > + if (!(cap & PCI_PTM_CAP_REQ)) > + return -EINVAL; > + > dev->ptm_granularity = 0; > + ctrl = PCI_PTM_CTRL_ENABLE; > + ctrl |= dev->ptm_granularity << 8; > } else > return -EINVAL; > > - ctrl = PCI_PTM_CTRL_ENABLE; > - ctrl |= dev->ptm_granularity << 8; > pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl); > dev->ptm_enabled = 1; >
On Sat, Sep 3, 2022 at 1:35 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/pcie/ptm.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c > index b6a417247ce3..ad283818f37b 100644 > --- a/drivers/pci/pcie/ptm.c > +++ b/drivers/pci/pcie/ptm.c > @@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) > if (!pos) > return -EINVAL; > > - pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > - if (!(cap & PCI_PTM_CAP_REQ)) > - return -EINVAL; > - > /* > + * Root Ports and Switch Upstream Ports have been configured > + * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and > + * granularity. > + * > * For a PCIe Endpoint, PTM is only useful if the endpoint can > * issue PTM requests to upstream devices that have PTM enabled. > * > @@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) > * device, so there must be some implementation-specific way to > * associate the endpoint with a time source. > */ > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > + pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { > + ups = pci_upstream_bridge(dev); > + if (!ups || !ups->ptm_enabled) > + return -EINVAL; > + } > + > + pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl); > + ctrl |= PCI_PTM_CTRL_ENABLE; > + } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { > + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > + if (!(cap & PCI_PTM_CAP_REQ)) > + return -EINVAL; > + > ups = pci_upstream_bridge(dev); > if (!ups || !ups->ptm_enabled) > return -EINVAL; > > dev->ptm_granularity = ups->ptm_granularity; > + ctrl = PCI_PTM_CTRL_ENABLE; > + ctrl |= dev->ptm_granularity << 8; > } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) { > + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > + if (!(cap & PCI_PTM_CAP_REQ)) > + return -EINVAL; > + > dev->ptm_granularity = 0; > + ctrl = PCI_PTM_CTRL_ENABLE; > + ctrl |= dev->ptm_granularity << 8; > } else > return -EINVAL; I would do if ((pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM || pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT)) { ups = pci_upstream_bridge(dev); if (!ups || !ups->ptm_enabled) return -EINVAL; dev->ptm_granularity = ups->ptm_granularity; } switch(pci_pcie_type(dev)) { case PCI_EXP_TYPE_ROOT_PORT: case PCI_EXP_TYPE_UPSTREAM: pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl); ctrl |= PCI_PTM_CTRL_ENABLE; break; case PCI_EXP_TYPE_ENDPOINT: case PCI_EXP_TYPE_RC_END: ctrl = PCI_PTM_CTRL_ENABLE; break; default: return -EINVAL; } > > - ctrl = PCI_PTM_CTRL_ENABLE; > - ctrl |= dev->ptm_granularity << 8; And I wouldn't remove the line above. Note that for root ports dev->ptm_granularity must be set and reflect the register setting or else the code wouldn't have worked for downstream components. > pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl); > dev->ptm_enabled = 1; > > --
On Sat, Sep 3, 2022 at 7:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Sat, Sep 3, 2022 at 1:35 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > drivers/pci/pcie/ptm.c | 34 +++++++++++++++++++++++++++------- > > 1 file changed, 27 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c > > index b6a417247ce3..ad283818f37b 100644 > > --- a/drivers/pci/pcie/ptm.c > > +++ b/drivers/pci/pcie/ptm.c > > @@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) > > if (!pos) > > return -EINVAL; > > > > - pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > > - if (!(cap & PCI_PTM_CAP_REQ)) > > - return -EINVAL; > > - > > /* > > + * Root Ports and Switch Upstream Ports have been configured > > + * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and > > + * granularity. > > + * > > * For a PCIe Endpoint, PTM is only useful if the endpoint can > > * issue PTM requests to upstream devices that have PTM enabled. > > * > > @@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) > > * device, so there must be some implementation-specific way to > > * associate the endpoint with a time source. > > */ > > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { > > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > > + pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { > > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { > > + ups = pci_upstream_bridge(dev); > > + if (!ups || !ups->ptm_enabled) > > + return -EINVAL; > > + } > > + > > + pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl); > > + ctrl |= PCI_PTM_CTRL_ENABLE; > > + } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { > > + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > > + if (!(cap & PCI_PTM_CAP_REQ)) > > + return -EINVAL; > > + > > ups = pci_upstream_bridge(dev); > > if (!ups || !ups->ptm_enabled) > > return -EINVAL; > > > > dev->ptm_granularity = ups->ptm_granularity; > > + ctrl = PCI_PTM_CTRL_ENABLE; > > + ctrl |= dev->ptm_granularity << 8; > > } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) { > > + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); > > + if (!(cap & PCI_PTM_CAP_REQ)) > > + return -EINVAL; > > + > > dev->ptm_granularity = 0; > > + ctrl = PCI_PTM_CTRL_ENABLE; > > + ctrl |= dev->ptm_granularity << 8; > > } else > > return -EINVAL; > > I would do > > if ((pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM || pci_pcie_type(dev) > == PCI_EXP_TYPE_ENDPOINT)) { > ups = pci_upstream_bridge(dev); > if (!ups || !ups->ptm_enabled) > return -EINVAL; > > dev->ptm_granularity = ups->ptm_granularity; > } > > switch(pci_pcie_type(dev)) { > case PCI_EXP_TYPE_ROOT_PORT: > case PCI_EXP_TYPE_UPSTREAM: > pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl); > ctrl |= PCI_PTM_CTRL_ENABLE; > break; > case PCI_EXP_TYPE_ENDPOINT: > case PCI_EXP_TYPE_RC_END: I missed the cap check here, sorry. > ctrl = PCI_PTM_CTRL_ENABLE; > break; > default: > return -EINVAL; > } > > > > > - ctrl = PCI_PTM_CTRL_ENABLE; > > - ctrl |= dev->ptm_granularity << 8; > > And I wouldn't remove the line above. > > Note that for root ports dev->ptm_granularity must be set and reflect > the register setting or else the code wouldn't have worked for > downstream components. > > > pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl); > > dev->ptm_enabled = 1; > > > > --
diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c index b6a417247ce3..ad283818f37b 100644 --- a/drivers/pci/pcie/ptm.c +++ b/drivers/pci/pcie/ptm.c @@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) if (!pos) return -EINVAL; - pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); - if (!(cap & PCI_PTM_CAP_REQ)) - return -EINVAL; - /* + * Root Ports and Switch Upstream Ports have been configured + * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and + * granularity. + * * For a PCIe Endpoint, PTM is only useful if the endpoint can * issue PTM requests to upstream devices that have PTM enabled. * @@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) * device, so there must be some implementation-specific way to * associate the endpoint with a time source. */ - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || + pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { + if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) { + ups = pci_upstream_bridge(dev); + if (!ups || !ups->ptm_enabled) + return -EINVAL; + } + + pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl); + ctrl |= PCI_PTM_CTRL_ENABLE; + } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) { + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); + if (!(cap & PCI_PTM_CAP_REQ)) + return -EINVAL; + ups = pci_upstream_bridge(dev); if (!ups || !ups->ptm_enabled) return -EINVAL; dev->ptm_granularity = ups->ptm_granularity; + ctrl = PCI_PTM_CTRL_ENABLE; + ctrl |= dev->ptm_granularity << 8; } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) { + pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap); + if (!(cap & PCI_PTM_CAP_REQ)) + return -EINVAL; + dev->ptm_granularity = 0; + ctrl = PCI_PTM_CTRL_ENABLE; + ctrl |= dev->ptm_granularity << 8; } else return -EINVAL; - ctrl = PCI_PTM_CTRL_ENABLE; - ctrl |= dev->ptm_granularity << 8; pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl); dev->ptm_enabled = 1;