| Message ID | 1481543487-33152-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi, 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. Side note: I think Doug mentioned previously that the default rk3399.dtsi actually leaves the default clock choice (i.e., case 2), so it might be good to patch this property into the rk3399.dtsi instead of the board files. If any board goes with option 1, they can delete the property. I can patch this up myself if you don't, as I'm working on upstreaming the rk3399-gbased Gru/Kevin device trees. > Reported-by: Jeffy Chen <jeffy.chen@rock-chips.com> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > --- > > 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(+) FWIW: Reviewed-by: Brian Norris <briannorris@chromium.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <jeffy.chen@rock-chips.com> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > --- > > 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(+) Acked-by: Rob Herring <robh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <jeffy.chen@rock-chips.com> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > --- > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+ Heiko On Wed, Jan 11, 2017 at 12:28:22PM -0600, Bjorn Helgaas wrote: > On Mon, Dec 12, 2016 at 07:51:27PM +0800, Shawn Lin wrote: > > 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. Ha, wow. FWIW in the tree I'm using, I have both this patch and a DTS patch that uses the matching (but improperly-spelled) property. So *I* have tested it. But I obviously didn't read it well enough. Or maybe I'm mildly dyslexic? Notably, Shawn sent a NON-matching DTS patch already here: https://patchwork.kernel.org/patch/9477651/ So this definitely needs to get straightened out. Preferably by s/apsm/aspm/ in this patch. Regards, Brian -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017/1/12 2:28, Bjorn Helgaas wrote: > 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 <jeffy.chen@rock-chips.com> >> Cc: Brian Norris <briannorris@chromium.org> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> >> --- >> >> 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. > oops, mea culpa. I think I wrote this and tested it on the kernel4.4 chromeOS tree. There were some non-upstream patches there so I slightly amend it when rebasing on your next branch, I guess I made the mistake then. I will fix this. >> + 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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")) { + 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,
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 <jeffy.chen@rock-chips.com> Cc: Brian Norris <briannorris@chromium.org> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- 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(+)