Re: [PATCH v2 2/3] PCI/PTM: Implement pci_enable_ptm() for Root Ports, Switch Upstream Ports

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Sep 3, 2022 at 1:35 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
>  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;
>
> --



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux