[PATCH] PCI: rockchip: Mark SLC bit as well as CCC bit for RC

[PATCH] PCI: rockchip: Mark SLC bit as well as CCC bit for RC

[Shawn Lin]

lspci traces CCC to see if the end-2-end supports common
clock, so the current code should work as we mark the CCC bit
of RC. However, ASPM code actually check SLC bit of RC and try to
compare it with the downstream components' SLC instead. So when
enabling ASPM, CCC will be cleared after failing to match SLC with
the corresponding bit of downstream components. On one hand, from the
code of pcie_aspm_configure_common_clock, we could find that what we
actually need to set is SLC. On the other hand, we should also guarantee
that CCC should be marked w/o supporting ASPM. This patch fixes this
issue.

Cc: Brian Norris <briannorris@chromium.org>
Cc: jeffy.chen <jeffy.chen@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/pci/host/pcie-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 26ddd35..7cd4d5c 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -596,7 +596,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 
 	/* Set RC's clock architecture as common clock */
 	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
-	status |= PCI_EXP_LNKCTL_CCC;
+	status |= (PCI_EXP_LNKCTL_CCC | PCI_EXP_LNKSTA_SLC << 16);
 	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
 
 	/* Enable Gen1 training */
-- 
1.9.1

Re: [PATCH] PCI: rockchip: Mark SLC bit as well as CCC bit for RCg

[Brian Norris]

Hi Shawn,

On Fri, Mar 17, 2017 at 03:24:53PM +0800, Shawn Lin wrote:
> lspci traces CCC to see if the end-2-end supports common
> clock, so the current code should work as we mark the CCC bit
> of RC. However, ASPM code actually check SLC bit of RC and try to
> compare it with the downstream components' SLC instead. So when
> enabling ASPM, CCC will be cleared after failing to match SLC with
> the corresponding bit of downstream components. On one hand, from the
> code of pcie_aspm_configure_common_clock, we could find that what we
> actually need to set is SLC. On the other hand, we should also guarantee
> that CCC should be marked w/o supporting ASPM. This patch fixes this
> issue.

This matches my understanding of the code more or less.

This fixes the Common Clock bit for me, with ASPM enabled on the Google
Kevin board; and my EndPoint (Marvell 8997) even reports a lower ASPM
L0s latency now. Nice! Thanks for the patch.

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

Personally, I might even mark this as:

Fixes: b8ab8e041cc0 ("PCI: rockchip: Mark RC as common clock architecture")

Not that it necessarily deserves -stable, but it helps people browsing
the logs, and wondering why the above commit doesn't seem to take effect
:)

> Cc: Brian Norris <briannorris@chromium.org>
> Cc: jeffy.chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/pci/host/pcie-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 26ddd35..7cd4d5c 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -596,7 +596,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  
>  	/* Set RC's clock architecture as common clock */
>  	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> -	status |= PCI_EXP_LNKCTL_CCC;
> +	status |= (PCI_EXP_LNKCTL_CCC | PCI_EXP_LNKSTA_SLC << 16);
>  	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
>  
>  	/* Enable Gen1 training */
> -- 
> 1.9.1
> 
> 

Re: [PATCH] PCI: rockchip: Mark SLC bit as well as CCC bit for RC

[Bjorn Helgaas]

On Fri, Mar 17, 2017 at 03:24:53PM +0800, Shawn Lin wrote:
> lspci traces CCC to see if the end-2-end supports common
> clock, so the current code should work as we mark the CCC bit
> of RC. 

I assume you're talking about lspci code here, where the bit is called
PCI_EXP_LNKCTL_CLOCK.  The only use of PCI_EXP_LNKCTL_CLOCK in lspci
is to decode it in cap_express_link().  I don't know what you mean by
"tracing CCC", since lspci only decodes registers of one device at a
time; it never follows the hierarchy from an endpoint up to a root
port.

I'm not sure how this lspci information is relevant to this patch.

> However, ASPM code actually check SLC bit of RC and try to
> compare it with the downstream components' SLC instead. 

Here I think you're talking about pcie_aspm_configure_common_clock()
in Linux.  That looks at PCI_EXP_LNKSTA_SLC on both ends of a link.
If PCI_EXP_LNKSTA_SLC is set at both ends, we set PCI_EXP_LNKCTL_CCC
on both ends; otherwise we clear PCI_EXP_LNKCTL_CCC on both ends.
This follows the Implementation Note in PCIe r3.1, sec 7.8.7.

Per that note, PCI_EXP_LNKSTA_SLC in the root port tells us whether
the port "uses a clock that has a common source ... to the clock
signal provided on the slot" below the port.

This patch sets PCI_EXP_LNKSTA_SLC based on your knowledge of the
platform topology, i.e., that the root port clock and the slot clock
have the same source.  I assume this is *always* the case for every
possible platform using Rockchip?

If so, just say that, and you can go on to say that this enables the
ASPM code to set PCI_EXP_LNKCTL_CCC if the downstream end also sets
PCI_EXP_LNKSTA_SLC to indicate that it uses the clock provided on the
slot, e.g., something like this:

  All platforms using Rockchip use a common clock for the Root Port
  and the slot connected to it.  Indicate this by setting the Slot
  Clock Configuration (PCI_EXP_LNKSTA_SLC) bit in the Root Port's Link
  Status.

  Per the Implementation Note in the spec (PCIe r3.1, sec 7.8.7), if
  the downstream component also sets PCI_EXP_LNKSTA_SLC, software may
  set the Common Clock Configuration (PCI_EXP_LNKCTL_CCC) bits on both
  ends of the Link.  This is done by pcie_aspm_configure_common_clock().

> So when
> enabling ASPM, CCC will be cleared after failing to match SLC with
> the corresponding bit of downstream components. On one hand, from the
> code of pcie_aspm_configure_common_clock, we could find that what we
> actually need to set is SLC. 

> On the other hand, we should also guarantee
> that CCC should be marked w/o supporting ASPM. 

This patch doesn't change anything with PCI_EXP_LNKCTL_CCC.  I'd
suggest that we should *not* set it here because we have no idea what
(if anything) is at the other end of the link.  Per the implementation
note, I think we should only set PCI_EXP_LNKCTL_CCC if both ends of
the link have PCI_EXP_LNKSTA_SLC set.

Are you suggesting that we should set PCI_EXP_LNKCTL_CCC on both ends
of the link whenever possible, and that today we don't do that unless
ASPM is enabled?  That sounds plausible to me, but that would be a
generic PCIe enhancement, not anything Rockchip-specific, and of
course, this patch doesn't make that happen.

> This patch fixes this
> issue.
> 
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: jeffy.chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/pci/host/pcie-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 26ddd35..7cd4d5c 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -596,7 +596,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  
>  	/* Set RC's clock architecture as common clock */
>  	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> -	status |= PCI_EXP_LNKCTL_CCC;
> +	status |= (PCI_EXP_LNKCTL_CCC | PCI_EXP_LNKSTA_SLC << 16);
>  	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
>  
>  	/* Enable Gen1 training */
> -- 
> 1.9.1
> 
> 

Re: [PATCH] PCI: rockchip: Mark SLC bit as well as CCC bit for RC

[Shawn Lin]

Hi Bjorn,

On 2017/4/1 23:14, Bjorn Helgaas wrote:
> On Fri, Mar 17, 2017 at 03:24:53PM +0800, Shawn Lin wrote:
>> lspci traces CCC to see if the end-2-end supports common
>> clock, so the current code should work as we mark the CCC bit
>> of RC.
>
> I assume you're talking about lspci code here, where the bit is called
> PCI_EXP_LNKCTL_CLOCK.  The only use of PCI_EXP_LNKCTL_CLOCK in lspci
> is to decode it in cap_express_link().  I don't know what you mean by
> "tracing CCC", since lspci only decodes registers of one device at a
> time; it never follows the hierarchy from an endpoint up to a root
> port.
>
> I'm not sure how this lspci information is relevant to this patch.

Actually I meant lscpi found the common clock was cleared but we have
set it in RC's driver.

>
>> However, ASPM code actually check SLC bit of RC and try to
>> compare it with the downstream components' SLC instead.
>
> Here I think you're talking about pcie_aspm_configure_common_clock()
> in Linux.  That looks at PCI_EXP_LNKSTA_SLC on both ends of a link.
> If PCI_EXP_LNKSTA_SLC is set at both ends, we set PCI_EXP_LNKCTL_CCC
> on both ends; otherwise we clear PCI_EXP_LNKCTL_CCC on both ends.
> This follows the Implementation Note in PCIe r3.1, sec 7.8.7.
>
> Per that note, PCI_EXP_LNKSTA_SLC in the root port tells us whether
> the port "uses a clock that has a common source ... to the clock
> signal provided on the slot" below the port.
>
> This patch sets PCI_EXP_LNKSTA_SLC based on your knowledge of the
> platform topology, i.e., that the root port clock and the slot clock
> have the same source.  I assume this is *always* the case for every
> possible platform using Rockchip?

yes.

>
> If so, just say that, and you can go on to say that this enables the
> ASPM code to set PCI_EXP_LNKCTL_CCC if the downstream end also sets
> PCI_EXP_LNKSTA_SLC to indicate that it uses the clock provided on the
> slot, e.g., something like this:
>
>   All platforms using Rockchip use a common clock for the Root Port
>   and the slot connected to it.  Indicate this by setting the Slot
>   Clock Configuration (PCI_EXP_LNKSTA_SLC) bit in the Root Port's Link
>   Status.
>
>   Per the Implementation Note in the spec (PCIe r3.1, sec 7.8.7), if
>   the downstream component also sets PCI_EXP_LNKSTA_SLC, software may
>   set the Common Clock Configuration (PCI_EXP_LNKCTL_CCC) bits on both
>   ends of the Link.  This is done by pcie_aspm_configure_common_clock().
>

It sounds good.

>> So when
>> enabling ASPM, CCC will be cleared after failing to match SLC with
>> the corresponding bit of downstream components. On one hand, from the
>> code of pcie_aspm_configure_common_clock, we could find that what we
>> actually need to set is SLC.
>
>> On the other hand, we should also guarantee
>> that CCC should be marked w/o supporting ASPM.
>
> This patch doesn't change anything with PCI_EXP_LNKCTL_CCC.  I'd
> suggest that we should *not* set it here because we have no idea what
> (if anything) is at the other end of the link.  Per the implementation
> note, I think we should only set PCI_EXP_LNKCTL_CCC if both ends of
> the link have PCI_EXP_LNKSTA_SLC set.
>
> Are you suggesting that we should set PCI_EXP_LNKCTL_CCC on both ends
> of the link whenever possible, and that today we don't do that unless
> ASPM is enabled?  That sounds plausible to me, but that would be a
> generic PCIe enhancement, not anything Rockchip-specific, and of
> course, this patch doesn't make that happen.


It's okay to just set SLC in RC's driver and let aspm code decide the
CCC bit, so I will respin v2 to fix this for rockchip RC.

Then we could go on talking about another issue that should we need to
set PCI_EXP_LNKCTL_CCC on both ends if possbile? I think we need more
evidence from the spec which states that it could benefit for something
else than aspm's latency. However I didn't them.

>
>> This patch fixes this
>> issue.
>>
>> Cc: Brian Norris <briannorris@chromium.org>
>> Cc: jeffy.chen <jeffy.chen@rock-chips.com>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>  drivers/pci/host/pcie-rockchip.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>> index 26ddd35..7cd4d5c 100644
>> --- a/drivers/pci/host/pcie-rockchip.c
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -596,7 +596,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>>
>>  	/* Set RC's clock architecture as common clock */
>>  	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
>> -	status |= PCI_EXP_LNKCTL_CCC;
>> +	status |= (PCI_EXP_LNKCTL_CCC | PCI_EXP_LNKSTA_SLC << 16);
>>  	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
>>
>>  	/* Enable Gen1 training */
>> --
>> 1.9.1
>>
>>
>
>
>