[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 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
> 
>