| Message ID | 1474578027-64292-1-git-send-email-rajatja@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
(Nit: I think the $subject is typically 'PCI: rockchip: ...'.) Hi Rajat, On Thu, Sep 22, 2016 at 02:00:27PM -0700, Rajat Jain wrote: > The register value gets lost on a Link speed/width change, and the > ideal fix should reprogram this on that event (refer "Link Bandwidth > Management Interrupt Enable" & "Link Autonomous Bandwidth Interrupt Enable" > in link control reg?). Oh, that that reminds me (sorry for not noticing this earlier): Rockchip did add handling of those two interrupt bits. See: } else if (reg & PCIE_CLIENT_INT_PHY) { dev_dbg(dev, "phy link changes\n"); rockchip_pcie_clr_bw_int(rockchip); } Do you ever see such interrupts? In any case, it's possible we could do the re-programming there, just to be sure, though I'm not sure we can test it well. 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 Thu, Sep 22, 2016 at 2:14 PM, Brian Norris <briannorris@chromium.org> wrote: > (Nit: I think the $subject is typically 'PCI: rockchip: ...'.) > > Hi Rajat, > > On Thu, Sep 22, 2016 at 02:00:27PM -0700, Rajat Jain wrote: >> The register value gets lost on a Link speed/width change, and the >> ideal fix should reprogram this on that event (refer "Link Bandwidth >> Management Interrupt Enable" & "Link Autonomous Bandwidth Interrupt Enable" >> in link control reg?). > > Oh, that that reminds me (sorry for not noticing this earlier): Rockchip > did add handling of those two interrupt bits. See: > > } else if (reg & PCIE_CLIENT_INT_PHY) { > dev_dbg(dev, "phy link changes\n"); > rockchip_pcie_clr_bw_int(rockchip); > } Oh I did not notice this. I'll try it out.. > > Do you ever see such interrupts? In any case, it's possible we could do > the re-programming there, just to be sure, though I'm not sure we can > test it well. > > 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
Thanks for the tip Brian, I was able to test it too (for some reason my system didn't fall apart on retraining the link to 5GT/s). Sending out new one shortly. On Thu, Sep 22, 2016 at 2:56 PM, Rajat Jain <rajatja@google.com> wrote: > On Thu, Sep 22, 2016 at 2:14 PM, Brian Norris <briannorris@chromium.org> wrote: >> (Nit: I think the $subject is typically 'PCI: rockchip: ...'.) >> >> Hi Rajat, >> >> On Thu, Sep 22, 2016 at 02:00:27PM -0700, Rajat Jain wrote: >>> The register value gets lost on a Link speed/width change, and the >>> ideal fix should reprogram this on that event (refer "Link Bandwidth >>> Management Interrupt Enable" & "Link Autonomous Bandwidth Interrupt Enable" >>> in link control reg?). >> >> Oh, that that reminds me (sorry for not noticing this earlier): Rockchip >> did add handling of those two interrupt bits. See: >> >> } else if (reg & PCIE_CLIENT_INT_PHY) { >> dev_dbg(dev, "phy link changes\n"); >> rockchip_pcie_clr_bw_int(rockchip); >> } > > Oh I did not notice this. I'll try it out.. > >> >> Do you ever see such interrupts? In any case, it's possible we could do >> the re-programming there, just to be sure, though I'm not sure we can >> test it well. >> >> 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
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c index c3593e6..e537413 100644 --- a/drivers/pci/host/pcie-rockchip.c +++ b/drivers/pci/host/pcie-rockchip.c @@ -95,6 +95,11 @@ #define PCIE_CORE_PL_CONF_SPEED_MASK 0x00000018 #define PCIE_CORE_PL_CONF_LANE_MASK 0x00000006 #define PCIE_CORE_PL_CONF_LANE_SHIFT 1 +#define PCIE_CORE_TXCREDIT_CFG1 (PCIE_CORE_CTRL_MGMT_BASE + 0x020) +#define PCIE_CORE_TXCREDIT_CFG1_MUI_MASK 0xFFFF0000 +#define PCIE_CORE_TXCREDIT_CFG1_MUI_SHIFT 16 +#define PCIE_CORE_TXCREDIT_CFG1_MUI_ENCODE(x) \ + (((x) >> 3) << PCIE_CORE_TXCREDIT_CFG1_MUI_SHIFT) #define PCIE_CORE_INT_STATUS (PCIE_CORE_CTRL_MGMT_BASE + 0x20c) #define PCIE_CORE_INT_PRFPE BIT(0) #define PCIE_CORE_INT_CRFPE BIT(1) @@ -523,6 +528,12 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) PCIE_CORE_PL_CONF_LANE_MASK); dev_dbg(dev, "current link width is x%d\n", status); + /* Update credit update interval */ + status = rockchip_pcie_read(rockchip, PCIE_CORE_TXCREDIT_CFG1); + status &= ~PCIE_CORE_TXCREDIT_CFG1_MUI_MASK; + status |= PCIE_CORE_TXCREDIT_CFG1_MUI_ENCODE(24000); /* ns */ + rockchip_pcie_write(rockchip, status, PCIE_CORE_TXCREDIT_CFG1); + rockchip_pcie_write(rockchip, ROCKCHIP_VENDOR_ID, PCIE_RC_CONFIG_VENDOR); rockchip_pcie_write(rockchip,
This increases the likelihood of link state to automatically go to L1 and save some power. The default credit update interval of 7.5 us results in the rootport sending UpdateFC packets too often, thus reulting in the link never going to L1, and always staying in L0/L0s. The value 24 us was chosen after some experiments and peeking over the PCIe bus to see that we do enter L1 substate when there is not enough traffic on the PCIe bus. The register value gets lost on a Link speed/width change, and the ideal fix should reprogram this on that event (refer "Link Bandwidth Management Interrupt Enable" & "Link Autonomous Bandwidth Interrupt Enable" in link control reg?). But the rockchip platforms doesn't support hotplug and I've verified that the retraining doesn't work as well, so this should be OK. Signed-off-by: Rajat Jain <rajatja@google.com> --- drivers/pci/host/pcie-rockchip.c | 11 +++++++++++ 1 file changed, 11 insertions(+)