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;
Message-ID: <21de0e12-5e3d-c27a-d857-23ad3e83dd1c@linux.intel.com>
Date:   Fri, 9 Sep 2022 16:38:42 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [PATCH v4 2/9] PCI/PTM: Add pci_upstream_ptm() helper
Content-Language: en-US
To:     Bjorn Helgaas <helgaas@kernel.org>,
        Kai-Heng Feng <kai.heng.feng@canonical.com>,
        Rajvi Jingar <rajvi.jingar@linux.intel.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>
Cc:     Koba Ko <koba.ko@canonical.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        "David E . Box" <david.e.box@linux.intel.com>,
        linux-pci@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>
References: <20220909202505.314195-1-helgaas@kernel.org>
 <20220909202505.314195-3-helgaas@kernel.org>
From:   Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <20220909202505.314195-3-helgaas@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 9/9/22 1:24 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> PTM requires an unbroken path of PTM-supporting devices between the PTM
> Root and the ultimate PTM Requester, but if a Switch supports PTM, only the
> Upstream Port can have a PTM Capability; the Downstream Ports do not.
> 
> Previously we copied the PTM configuration from the Switch Upstream Port to
> the Downstream Ports so dev->ptm_enabled for any device implied that all
> the upstream devices support PTM.
> 
> Instead of making it look like Downstream Ports have their own PTM config,
> add pci_upstream_ptm(), which returns the upstream device that has a PTM
> Capability (either a Root Port or a Switch Upstream Port).
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/pci/pcie/ptm.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index 85382c135885..0df6cdfe38b4 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -76,6 +76,29 @@ void pci_restore_ptm_state(struct pci_dev *dev)
>  	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
>  }
>  
> +/*
> + * If the next upstream device supports PTM, return it; otherwise return
> + * NULL.  PTM Messages are local, so both link partners must support it.
> + */
> +static struct pci_dev *pci_upstream_ptm(struct pci_dev *dev)
> +{
> +	struct pci_dev *ups = pci_upstream_bridge(dev);
> +
> +	/*
> +	 * Switch Downstream Ports are not permitted to have a PTM
> +	 * capability; their PTM behavior is controlled by the Upstream
> +	 * Port (PCIe r5.0, sec 7.9.16), so if the upstream bridge is a
> +	 * Switch Downstream Port, look up one more level.
> +	 */
> +	if (ups && pci_pcie_type(ups) == PCI_EXP_TYPE_DOWNSTREAM)
> +		ups = pci_upstream_bridge(ups);
> +
> +	if (ups && ups->ptm_cap)
> +		return ups;
> +
> +	return NULL;
> +}
> +
>  void pci_ptm_init(struct pci_dev *dev)
>  {
>  	u16 ptm;
> @@ -95,19 +118,6 @@ void pci_ptm_init(struct pci_dev *dev)
>  	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END))
>  		return;
>  
> -	/*
> -	 * Switch Downstream Ports are not permitted to have a PTM
> -	 * capability; their PTM behavior is controlled by the Upstream
> -	 * Port (PCIe r5.0, sec 7.9.16).
> -	 */
> -	ups = pci_upstream_bridge(dev);
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM &&
> -	    ups && ups->ptm_enabled) {
> -		dev->ptm_granularity = ups->ptm_granularity;
> -		dev->ptm_enabled = 1;
> -		return;
> -	}
> -
>  	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
>  	if (!ptm)
>  		return;
> @@ -124,6 +134,7 @@ void pci_ptm_init(struct pci_dev *dev)
>  	 * the spec recommendation (PCIe r3.1, sec 7.32.3), select the
>  	 * furthest upstream Time Source as the PTM Root.
>  	 */
> +	ups = pci_upstream_ptm(dev);
>  	if (ups && ups->ptm_enabled) {
>  		ctrl = PCI_PTM_CTRL_ENABLE;
>  		if (ups->ptm_granularity == 0)
> @@ -173,7 +184,7 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
>  	 * associate the endpoint with a time source.
>  	 */
>  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> -		ups = pci_upstream_bridge(dev);
> +		ups = pci_upstream_ptm(dev);
>  		if (!ups || !ups->ptm_enabled)
>  			return -EINVAL;
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer