From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Helgaas Subject: Re: [PATCH v2] PCI: rockchip: Add quirk to disable RC's ASPM L0s Date: Wed, 11 Jan 2017 12:28:22 -0600 Message-ID: <20170111182822.GC14532@bhelgaas-glaptop.roam.corp.google.com> References: <1481543487-33152-1-git-send-email-shawn.lin@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1481543487-33152-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Shawn Lin Cc: Bjorn Helgaas , Rob Herring , linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Wenrui Li , Brian Norris , Jeffy Chen , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org On Mon, Dec 12, 2016 at 07:51:27PM +0800, Shawn Lin wrote: > Rockchip's RC outputs 100MHz reference clock but there are > two methods for PHY to generate it. > > (1)One of them is to use system PLL to generate 100MHz clock and > the PHY will relock it and filter signal noise then outputs the > reference clock. > > (2)Another way is to share Soc's 24MHZ crystal oscillator with > PHY and force PHY's DLL to generate 100MHz internally. > > When using case(2), the exit from L0s doesn't work fine occasionally > due to the broken design of RC receiver's logical circuit. So even if > we use extended-synch, it still fails for PHY to relock the bits from > FTS sometimes. This will hang the system. > > Maybe we could argue that why not use case(1) to avoid it? The reason > is that as we could see the reference clock is derived from system PLL > and the path from it to PHY isn't so clean which means there are some > noise introduced by power-domain and other buses can't be filterd out > by PHY and we could see noise from the frequency spectrum by > oscilloscope. This makes the TX compatibility test a little difficult > to pass the spec. So case(1) and case(2) are both used indeed now. If > using case(2), we should disable RC's L0s support, and that is why we > need this property to indicate this quirk. > > Also after checking quirk.c, I noticed there is already a quirk for > disabling L0s unconditionally, quirk_disable_aspm_l0s. But obviously we > shouldn't do that as mentioned above that case(1) could still works fine > with L0s. > > Reported-by: Jeffy Chen > Cc: Brian Norris > Signed-off-by: Shawn Lin > > --- > > Changes in v2: > - drop the quirk prefix > > Documentation/devicetree/bindings/pci/rockchip-pcie.txt | 2 ++ > drivers/pci/host/pcie-rockchip.c | 9 +++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > index 71aeda1..1453a73 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > @@ -43,6 +43,8 @@ Required properties: > - interrupt-map-mask and interrupt-map: standard PCI properties > > Optional Property: > +- aspm-no-l0s: RC won't support ASPM L0s. This property is needed if > + using 24MHz OSC for RC's PHY. > - ep-gpios: contain the entry for pre-reset gpio > - num-lanes: number of lanes to use > - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe. > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c > index f2dca7b..35988fc 100644 > --- a/drivers/pci/host/pcie-rockchip.c > +++ b/drivers/pci/host/pcie-rockchip.c > @@ -140,6 +140,8 @@ > #define PCIE_RC_CONFIG_DCR_CSPL_SHIFT 18 > #define PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff > #define PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26 > +#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_BASE + 0xcc) > +#define PCIE_RC_CONFIG_LINK_CAP_L0S BIT(10) > #define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0) > #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c) > #define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE + 0x274) > @@ -653,6 +655,13 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) > status &= ~PCIE_RC_CONFIG_THP_CAP_NEXT_MASK; > rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_THP_CAP); > > + /* Clear L0s from RC's link cap */ > + if (of_property_read_bool(dev->of_node, "apsm-no-l0s")) { Did you test this? This string ("apsm-no-l0s") doesn't match the "aspm-no-l0s" documented above. The current tree doesn't contain either string in any DTS. > + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LINK_CAP); > + status &= ~PCIE_RC_CONFIG_LINK_CAP_L0S; > + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LINK_CAP); > + } > + > rockchip_pcie_write(rockchip, 0x0, PCIE_RC_BAR_CONF); > > rockchip_pcie_write(rockchip, > -- > 1.9.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html