| Message ID | 20220131120841.118833-1-christian.gmeiner@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI: cadence: respond to received PTM Requests | expand |
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.
Update to imperative mood, e.g., "Enable Controller to ...":
https://chris.beams.io/posts/git-commit/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v5.16#n134
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
> drivers/pci/controller/cadence/pcie-cadence-host.c | 10 ++++++++++
> drivers/pci/controller/cadence/pcie-cadence.h | 4 ++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index fb96d37a135c..940c7dd701d6 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -123,6 +123,14 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
> return ret;
> }
>
> +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?
> +}
> +
> static int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc)
> {
> struct cdns_pcie *pcie = &rc->pcie;
> @@ -501,6 +509,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> if (rc->quirk_detect_quiet_flag)
> cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
>
> + cdns_pcie_host_enable_ptm_response(pcie);
> +
> ret = cdns_pcie_start_link(pcie);
> if (ret) {
> dev_err(dev, "Failed to start link\n");
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index c8a27b6290ce..9510ea513b8a 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -116,6 +116,10 @@
> #define LM_RC_BAR_CFG_APERTURE(bar, aperture) \
> (((aperture) - 2) << ((bar) * 8))
>
> +/* PTM Control Register */
> +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8)
> +#define CDNS_PCIE_LM_TPM_CTRL_PTMRSEN BIT(17)
> +
> /*
> * Endpoint Function Registers (PCI configuration space for endpoint functions)
> */
> --
> 2.34.1
>
Hi Am Mo., 31. Jan. 2022 um 23:05 Uhr schrieb Bjorn Helgaas <helgaas@kernel.org>: > > Update subject line to match previous conventions ("git log --oneline > drivers/pci/controller/cadence/pcie-cadence-host.c" to see). > Sure > 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. > > Update to imperative mood, e.g., "Enable Controller to ...": > > https://chris.beams.io/posts/git-commit/ > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v5.16#n134 > Sure > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > > --- > > drivers/pci/controller/cadence/pcie-cadence-host.c | 10 ++++++++++ > > drivers/pci/controller/cadence/pcie-cadence.h | 4 ++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c > > index fb96d37a135c..940c7dd701d6 100644 > > --- a/drivers/pci/controller/cadence/pcie-cadence-host.c > > +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c > > @@ -123,6 +123,14 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie) > > return ret; > > } > > > > +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 is a great question to which I have no answer yet. All I have is a PCIe based FPGA that wants a working PTM. My target platform is an TI AM642 and I do not have a full picture of the pci layering used (cadence, pci-j721e and pci-keystone). All I have is the AM64x Technical Reference Manual (SPRUIM2C – SEPTEMBER 2021): https://www.ti.com/lit/pdf/SPRUIM2 There I found this register and the meaning of that bit. I think it is a good idea to get some people with knowledge on board. So I will add Kishon Vijay Abraham I and Swapnil Jakhade to CC. > > +} > > + > > static int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc) > > { > > struct cdns_pcie *pcie = &rc->pcie; > > @@ -501,6 +509,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) > > if (rc->quirk_detect_quiet_flag) > > cdns_pcie_detect_quiet_min_delay_set(&rc->pcie); > > > > + cdns_pcie_host_enable_ptm_response(pcie); > > + > > ret = cdns_pcie_start_link(pcie); > > if (ret) { > > dev_err(dev, "Failed to start link\n"); > > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h > > index c8a27b6290ce..9510ea513b8a 100644 > > --- a/drivers/pci/controller/cadence/pcie-cadence.h > > +++ b/drivers/pci/controller/cadence/pcie-cadence.h > > @@ -116,6 +116,10 @@ > > #define LM_RC_BAR_CFG_APERTURE(bar, aperture) \ > > (((aperture) - 2) << ((bar) * 8)) > > > > +/* PTM Control Register */ > > +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8) > > +#define CDNS_PCIE_LM_TPM_CTRL_PTMRSEN BIT(17) > > + > > /* > > * Endpoint Function Registers (PCI configuration space for endpoint functions) > > */ > > -- > > 2.34.1 > > -- greets -- Christian Gmeiner, MSc https://christian-gmeiner.info/privacypolicy
Hi Bjorn, 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. > > Update to imperative mood, e.g., "Enable Controller to ...": > > https://chris.beams.io/posts/git-commit/ > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v5.16#n134 > >> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> >> --- >> drivers/pci/controller/cadence/pcie-cadence-host.c | 10 ++++++++++ >> drivers/pci/controller/cadence/pcie-cadence.h | 4 ++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c >> index fb96d37a135c..940c7dd701d6 100644 >> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c >> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c >> @@ -123,6 +123,14 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie) >> return ret; >> } >> >> +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. Thanks, Kishon > >> +} >> + >> static int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc) >> { >> struct cdns_pcie *pcie = &rc->pcie; >> @@ -501,6 +509,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) >> if (rc->quirk_detect_quiet_flag) >> cdns_pcie_detect_quiet_min_delay_set(&rc->pcie); >> >> + cdns_pcie_host_enable_ptm_response(pcie); >> + >> ret = cdns_pcie_start_link(pcie); >> if (ret) { >> dev_err(dev, "Failed to start link\n"); >> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h >> index c8a27b6290ce..9510ea513b8a 100644 >> --- a/drivers/pci/controller/cadence/pcie-cadence.h >> +++ b/drivers/pci/controller/cadence/pcie-cadence.h >> @@ -116,6 +116,10 @@ >> #define LM_RC_BAR_CFG_APERTURE(bar, aperture) \ >> (((aperture) - 2) << ((bar) * 8)) >> >> +/* PTM Control Register */ >> +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8) >> +#define CDNS_PCIE_LM_TPM_CTRL_PTMRSEN BIT(17) >> + >> /* >> * Endpoint Function Registers (PCI configuration space for endpoint functions) >> */ >> -- >> 2.34.1 >>
Hi Bjorn, Am Fr., 18. Feb. 2022 um 11:56 Uhr schrieb Kishon Vijay Abraham I <kishon@ti.com>: > > Hi Bjorn, > > 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. > > > > Update to imperative mood, e.g., "Enable Controller to ...": > > > > https://chris.beams.io/posts/git-commit/ > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v5.16#n134 > > > >> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > >> --- > >> drivers/pci/controller/cadence/pcie-cadence-host.c | 10 ++++++++++ > >> drivers/pci/controller/cadence/pcie-cadence.h | 4 ++++ > >> 2 files changed, 14 insertions(+) > >> > >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c > >> index fb96d37a135c..940c7dd701d6 100644 > >> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c > >> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c > >> @@ -123,6 +123,14 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie) > >> return ret; > >> } > >> > >> +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. > With that information is it okay for you that I send a V2 of this patch with an improved commit message or do you see any other problems that I need to take into account?
On Fri, Feb 18, 2022 at 04:26:48PM +0530, Kishon Vijay Abraham I wrote: > Hi Bjorn, > > 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 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. > >> +/* PTM Control Register */ > >> +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8) Other #defines in this file use lower-case hex. Bjorn
Hi Bjorn, Am Fr., 18. Feb. 2022 um 14:20 Uhr schrieb Bjorn Helgaas <helgaas@kernel.org>: > > On Fri, Feb 18, 2022 at 04:26:48PM +0530, Kishon Vijay Abraham I wrote: > > Hi Bjorn, > > > > 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? > Would something like this work for you? ---8<--- PCI: cadence: Enable Controller to respond to received PTM Requests This enables the Controller [RP] to automatically respond with Response/ResponseD messages if CDNS_PCIE_LM_TPM_CTRL_PTMRSEN and PCI_PTM_CTRL_ENABLE bits are both set. ---8<--- > 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 can test this out if needed for this change.. but that won't happen before Monday. > 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. > > > >> +/* PTM Control Register */ > > >> +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8) > > Other #defines in this file use lower-case hex. I changed this in V2.
Hi Bjorn, 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: >> Hi Bjorn, >> >> 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'. 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. But with this patch which enables PTM by default for RC, it wouldn't be required to clear those bits. Thanks, Kishon > > 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. > >>>> +/* PTM Control Register */ >>>> +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8) > > Other #defines in this file use lower-case hex. > > Bjorn >
On Tue, Feb 22, 2022 at 07:40:54PM +0530, Kishon Vijay Abraham I wrote: > Hi Bjorn, > > 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: > >> Hi Bjorn, > >> > >> 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. Thanks, Lorenzo > Thanks, > Kishon > > > > 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. > > > >>>> +/* PTM Control Register */ > >>>> +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8) > > > > Other #defines in this file use lower-case hex. > > > > Bjorn > >
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.
On Fri, Apr 29, 2022 at 02:28:25PM -0500, Bjorn Helgaas wrote: > 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 ? Yes. The capability bits default to '1', even if CDNS_PCIE_LM_TPM_CTRL_PTMRSEN is cleared. Strictly speaking the current behavior is wrong, and the capability bits should be cleared instead. > > > 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 ? I believe "these bits" would have been more correct. Since these are capability bits, the AM64* TRM lists them as read-only, but of course they can be modified. To write them, you need to set bit [21], e.g. you can write the PTM capability register at address 0xD200A24. At the TRM address 0xD000A24 it is read-only. > > > > > 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. This is correct. > > 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'll ask Christian for permission to resend his patch with the subject changed. > > > > 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.
Am Do., 5. Mai 2022 um 16:43 Uhr schrieb Dominic Rath <dominic.rath@ibv-augsburg.net>: > > On Fri, Apr 29, 2022 at 02:28:25PM -0500, Bjorn Helgaas wrote: > > 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 ? > > Yes. The capability bits default to '1', even if > CDNS_PCIE_LM_TPM_CTRL_PTMRSEN is cleared. Strictly speaking the current > behavior is wrong, and the capability bits should be cleared instead. > > > > > 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 ? > > I believe "these bits" would have been more correct. Since these are > capability bits, the AM64* TRM lists them as read-only, but of course > they can be modified. To write them, you need to set bit [21], e.g. > you can write the PTM capability register at address 0xD200A24. At > the TRM address 0xD000A24 it is read-only. > > > > > > > > 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. > > This is correct. > > > > > 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'll ask Christian for permission to resend his patch with the subject > changed. > I will send a new version tomorrow with an improved subject line.
On Thu, May 05, 2022 at 04:48:17PM +0200, Christian Gmeiner wrote: [...] > > > 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'll ask Christian for permission to resend his patch with the subject > > changed. > > > > I will send a new version tomorrow with an improved subject line. Hi Christian, just asking if you are about to post a new version to pick it up. Thanks, Lorenzo
Am Mi., 11. Mai 2022 um 14:37 Uhr schrieb Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>: > > On Thu, May 05, 2022 at 04:48:17PM +0200, Christian Gmeiner wrote: > > [...] > > > > > 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'll ask Christian for permission to resend his patch with the subject > > > changed. > > > > > > > I will send a new version tomorrow with an improved subject line. > > Hi Christian, Hi Lorenzo > > just asking if you are about to post a new version to pick it up. > Sorry for the delay .. send out V3: https://lore.kernel.org/lkml/20220512055539.1782437-1-christian.gmeiner@gmail.com/
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c index fb96d37a135c..940c7dd701d6 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-host.c +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c @@ -123,6 +123,14 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie) return ret; } +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); +} + static int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc) { struct cdns_pcie *pcie = &rc->pcie; @@ -501,6 +509,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) if (rc->quirk_detect_quiet_flag) cdns_pcie_detect_quiet_min_delay_set(&rc->pcie); + cdns_pcie_host_enable_ptm_response(pcie); + ret = cdns_pcie_start_link(pcie); if (ret) { dev_err(dev, "Failed to start link\n"); diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h index c8a27b6290ce..9510ea513b8a 100644 --- a/drivers/pci/controller/cadence/pcie-cadence.h +++ b/drivers/pci/controller/cadence/pcie-cadence.h @@ -116,6 +116,10 @@ #define LM_RC_BAR_CFG_APERTURE(bar, aperture) \ (((aperture) - 2) << ((bar) * 8)) +/* PTM Control Register */ +#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0DA8) +#define CDNS_PCIE_LM_TPM_CTRL_PTMRSEN BIT(17) + /* * Endpoint Function Registers (PCI configuration space for endpoint functions) */
This enables the Controller [RP] to automatically respond with Response/ResponseD messages. Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> --- drivers/pci/controller/cadence/pcie-cadence-host.c | 10 ++++++++++ drivers/pci/controller/cadence/pcie-cadence.h | 4 ++++ 2 files changed, 14 insertions(+)