Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 29 Apr 2022 14:28:25 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc:     Kishon Vijay Abraham I <kishon@ti.com>,
        Christian Gmeiner <christian.gmeiner@gmail.com>,
        linux-kernel@vger.kernel.org, Tom Joseph <tjoseph@cadence.com>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org
Subject: Re: [PATCH] PCI: cadence: respond to received PTM Requests
Message-ID: <20220429192825.GA82239@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220429105627.GA28438@lpieralisi>
Precedence: bulk

On Fri, Apr 29, 2022 at 11:56:27AM +0100, Lorenzo Pieralisi wrote:
> On Tue, Feb 22, 2022 at 07:40:54PM +0530, Kishon Vijay Abraham I wrote:
> > On 18/02/22 6:50 pm, Bjorn Helgaas wrote:
> > > On Fri, Feb 18, 2022 at 04:26:48PM +0530, Kishon Vijay Abraham I wrote:
> > >> On 01/02/22 3:35 am, Bjorn Helgaas wrote:
> > >>> Update subject line to match previous conventions ("git log --oneline
> > >>> drivers/pci/controller/cadence/pcie-cadence-host.c" to see).
> > >>>
> > >>> On Mon, Jan 31, 2022 at 01:08:27PM +0100, Christian Gmeiner wrote:
> > >>>> This enables the Controller [RP] to automatically respond
> > >>>> with Response/ResponseD messages.
> > > 
> > >>>> +static void cdns_pcie_host_enable_ptm_response(struct cdns_pcie *pcie)
> > >>>> +{
> > >>>> +	u32 val;
> > >>>> +
> > >>>> +	val = cdns_pcie_readl(pcie, CDNS_PCIE_LM_PTM_CTRL);
> > >>>> +	cdns_pcie_writel(pcie, CDNS_PCIE_LM_PTM_CTRL, val | CDNS_PCIE_LM_TPM_CTRL_PTMRSEN);
> > >>>
> > >>> I assume this is some device-specific enable bit that is effectively
> > >>> ANDed with PCI_PTM_CTRL_ENABLE in the Precision Time Measurement
> > >>> Capability?
> > >>
> > >> That's correct. This bit enables Controller [RP] to respond to the
> > >> received PTM Requests.
> > > 
> > > Great!  Christian, can you update the commit log to reflect that
> > > both this bit *and* PCI_PTM_CTRL_ENABLE must be set for the RP to
> > > respond to received PTM Requests?
> > > 
> > > When CDNS_PCIE_LM_TPM_CTRL_PTMRSEN is cleared, do PCI_PTM_CAP_ROOT
> > > and the PTM Responder Capable bit (for which we don't have a #define)
> > > read as zero?
> > 
> > I see both PTM Responder Capable bit and PTM Root Capable is
> > by-default set to '1'.
> 
> Without this patch applied and with no other SW setting
> CDNS_PCIE_LM_TPM_CTRL_PTMRSEN, correct ?
> 
> > root@am64xx-evm:~# devmem2 0xD000A24
> > 
> > /dev/mem opened.
> > Memory mapped at address 0xffffa8980000.
> > Read at address  0x0D000A24 (0xffffa8980a24): 0x00000406
> > 
> > And this bit can be programmed through the local management APB
> > interface if required.
> 
> Which bit ? CDNS_PCIE_LM_TPM_CTRL_PTMRSEN ?
> 
> > But with this patch which enables PTM by default for RC, it
> > wouldn't be required to clear those bits.
> 
> Yes but that does not comply with the specifications as Bjorn
> pointed out below.
> 
> We can merge this patch but it would be good to investigate on this
> point.

I *think* this is OK.  Correct me if I'm wrong:

  - We're talking about a Root Port.

  - The Root Port's PTM Capability reads as 0x00000406 (PTM Responder
    Capable and PTM Root Capable set).

  - Without this patch, setting PTM Enable does nothing, and the Root
    Port does not send PTM Responses.

    This is the non-conforming situation because the Port claims that
    it implements the PTM Responder role, but it can't actually be
    enabled.

  - With this patch that sets CDNS_PCIE_LM_TPM_CTRL_PTMRSEN, the PTM
    Enable bit still powers up as zero, so the Port does not send PTM
    Responses, but setting PTM Enable enables PTM Responses from the
    Root Port.

So I think that after setting CDNS_PCIE_LM_TPM_CTRL_PTMRSEN, the PTM
capability works as per spec.

I think the proposed subject of "Enable Controller to respond to
received PTM Requests" is somewhat misleading, though, because PTM
responses still aren't enabled until we set PTM Enable.  I suggest
something like:

  PCI: cadence: Allow PTM Responder to be enabled

> > > I think that would be the correct behavior per PCIe r6.0, sec
> > > 7.9.15.2, and it would avoid the confusion of having the PTM
> > > Capability register advertise functionality that cannot be enabled via
> > > the PTM Control register.