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@xxxxxxxxxxxx> > Cc: jeffy.chen <jeffy.chen@xxxxxxxxxxxxxx> > Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> > --- > > 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 > >
