This enables the Controller [RP] to automatically respond
with Response/ResponseD messages.
Signed-off-by: Christian Gmeiner <[email protected]>
---
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);
+}
+
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
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 <[email protected]>
> ---
> 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 <[email protected]>:
>
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>> ---
>> 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
>>
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 11:56 Uhr schrieb Kishon Vijay Abraham I
<[email protected]>:
>
> 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 <[email protected]>
> >> ---
> >> 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?
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
Hi Bjorn,
Am Fr., 18. Feb. 2022 um 14:20 Uhr schrieb Bjorn Helgaas <[email protected]>:
>
> 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.
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
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 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 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 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
<[email protected]>:
>
> 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.
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
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
<[email protected]>:
>
> 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/[email protected]/
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy