[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 at chromium.org>
>> Cc: jeffy.chen <jeffy.chen at rock-chips.com>
>> Signed-off-by: Shawn Lin <shawn.lin at 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
>>
>>
>
>
>