Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220902233543.390890-1-helgaas@kernel.org> <20220902233543.390890-3-helgaas@kernel.org>
In-Reply-To: <20220902233543.390890-3-helgaas@kernel.org>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Sat, 3 Sep 2022 19:40:44 +0200
Message-ID: <CAJZ5v0jV94TwifmoF2UfiDpXNP_Kgt6qNkQH7zwQjo=ZhyU-4A@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] PCI/PTM: Implement pci_enable_ptm() for Root
 Ports, Switch Upstream Ports
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Kai-Heng Feng <kai.heng.feng@canonical.com>,
        Rajvi Jingar <rajvi.jingar@linux.intel.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Koba Ko <koba.ko@canonical.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        "David E . Box" <david.e.box@linux.intel.com>,
        Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Linux PCI <linux-pci@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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;
>
> --