| Message ID | 1692239684-12697-1-git-send-email-quic_krichai@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v1] PCI: qcom: Add sysfs entry to change link speed dynamically | expand |
Hi Krishna, kernel test robot noticed the following build warnings: [auto build test WARNING on pci/next] [also build test WARNING on pci/for-linus linus/master v6.5-rc6 next-20230816] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/PCI-qcom-Add-sysfs-entry-to-change-link-speed-dynamically/20230817-103734 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next patch link: https://lore.kernel.org/r/1692239684-12697-1-git-send-email-quic_krichai%40quicinc.com patch subject: [PATCH v1] PCI: qcom: Add sysfs entry to change link speed dynamically config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20230817/202308171155.o5viLJ3O-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171155.o5viLJ3O-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308171155.o5viLJ3O-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/pci/controller/dwc/pcie-qcom.c:249:13: warning: 'qcom_pcie_opp_update' used but never defined 249 | static void qcom_pcie_opp_update(struct qcom_pcie *pcie); | ^~~~~~~~~~~~~~~~~~~~ vim +/qcom_pcie_opp_update +249 drivers/pci/controller/dwc/pcie-qcom.c 247 248 static void qcom_pcie_icc_update(struct qcom_pcie *pcie); > 249 static void qcom_pcie_opp_update(struct qcom_pcie *pcie); 250
On Thu, Aug 17, 2023 at 08:04:43AM +0530, Krishna chaitanya chundru wrote: > PCIe can operate on lower GEN speed if client decided based upon > the bandwidth & latency requirements. To support dynamic GEN speed > switch adding this sysfs support. Who does "client" refer to? I assume it's the system administrator, but of course the endpoint is involved in the hardware speed negotiation, so one could think of the endpoint as a "client" with its own speed capabilities and requirements. > To change the GEN speed the link should be in L0, so first disable > L0s & L1. > > L0s needs to be disabled at both RC & EP because L0s entry is > independent. For enabling L0s both ends of the link needs to support > it, so first check if L0s is supported on both ends and then enable > L0s. Is there a place to document this sysfs knob? Why should it be qcom-specific? This sounds like generic PCIe functionality. The ASPM stuff looks like it should be done by aspm.c, not done behind its back. Everything here looks generic (not qcom-specific) except the qcom_pcie_icc_update() and qcom_pcie_opp_update(). Maybe we need some core infrastructure around this. > This patch is dependent on "PCI: qcom: Add support for OPP" > https://lore.kernel.org/linux-arm-msm/1692192264-18515-1-git-send-email-quic_krichai@quicinc.com/T/#t > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 141 +++++++++++++++++++++++++++++++++ > 1 file changed, 141 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 831d158..ad67d17 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -241,10 +241,150 @@ struct qcom_pcie { > const struct qcom_pcie_cfg *cfg; > struct dentry *debugfs; > bool suspended; > + bool l0s_supported; > }; > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) > > +static void qcom_pcie_icc_update(struct qcom_pcie *pcie); > +static void qcom_pcie_opp_update(struct qcom_pcie *pcie); > + > +static int qcom_pcie_disable_l0s(struct pci_dev *pdev, void *userdata) > +{ > + int lnkctl; > + > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); > + lnkctl &= ~(PCI_EXP_LNKCTL_ASPM_L0S); > + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); > + > + return 0; > +} > + > +static int qcom_pcie_check_l0s_support(struct pci_dev *pdev, void *userdata) > +{ > + struct pci_dev *parent = pdev->bus->self; > + struct qcom_pcie *pcie = userdata; > + struct dw_pcie *pci = pcie->pci; > + int lnkcap; > + > + /* check parent supports L0s */ > + if (parent) { > + dev_err(pci->dev, "parent\n"); > + pci_read_config_dword(parent, pci_pcie_cap(parent) + PCI_EXP_LNKCAP, > + &lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { > + dev_info(pci->dev, "Parent does not support L0s\n"); > + pcie->l0s_supported = false; > + return 0; > + } > + } > + > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP, > + &lnkcap); > + dev_err(pci->dev, "child %x\n", lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { > + dev_info(pci->dev, "Device does not support L0s\n"); > + pcie->l0s_supported = false; > + return 0; > + } > + > + return 0; > +} > + > +static int qcom_pcie_enable_l0s(struct pci_dev *pdev, void *userdata) > +{ > + int lnkctl; > + > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); > + lnkctl |= (PCI_EXP_LNKCTL_ASPM_L0S); > + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); > + > + return 0; > +} > + > +static ssize_t qcom_pcie_speed_change_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + unsigned int current_speed, target_speed, max_speed; > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > + struct pci_bus *child, *root_bus = NULL; > + struct dw_pcie_rp *pp = &pcie->pci->pp; > + struct dw_pcie *pci = pcie->pci; > + struct pci_dev *pdev; > + u16 offset; > + u32 val; > + int ret; > + > + list_for_each_entry(child, &pp->bridge->bus->children, node) { > + if (child->parent == pp->bridge->bus) { > + root_bus = child; > + break; > + } > + } > + > + pdev = root_bus->self; > + > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > + > + val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP); > + max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); > + > + val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA); > + current_speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val); > + > + ret = kstrtouint(buf, 10, &target_speed); > + if (ret) > + return ret; > + > + if (target_speed > max_speed) > + return -EINVAL; > + > + if (current_speed == target_speed) > + return count; > + > + pci_walk_bus(pp->bridge->bus, qcom_pcie_disable_l0s, pcie); > + > + /* Disable L1 */ > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); > + val &= ~(PCI_EXP_LNKCTL_ASPM_L1); > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); > + > + /* Set target GEN speed */ > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2); > + val &= ~PCI_EXP_LNKCTL2_TLS; > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, val | target_speed); > + > + ret = pcie_retrain_link(pdev, true); > + if (ret) > + dev_err(dev, "Link retrain failed %d\n", ret); > + > + /* Enable L1 */ > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); > + val |= (PCI_EXP_LNKCTL_ASPM_L1); > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); > + > + pcie->l0s_supported = true; > + pci_walk_bus(pp->bridge->bus, qcom_pcie_check_l0s_support, pcie); > + > + if (pcie->l0s_supported) > + pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_l0s, pcie); > + > + qcom_pcie_icc_update(pcie); > + > + qcom_pcie_opp_update(pcie); > + > + return count; > +} > +static DEVICE_ATTR_WO(qcom_pcie_speed_change); > + > +static struct attribute *qcom_pcie_attrs[] = { > + &dev_attr_qcom_pcie_speed_change.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(qcom_pcie); > + > static void qcom_ep_reset_assert(struct qcom_pcie *pcie) > { > gpiod_set_value_cansleep(pcie->reset, 1); > @@ -1716,6 +1856,7 @@ static struct platform_driver qcom_pcie_driver = { > .of_match_table = qcom_pcie_match, > .pm = &qcom_pcie_pm_ops, > .probe_type = PROBE_PREFER_ASYNCHRONOUS, > + .dev_groups = qcom_pcie_groups, > }, > }; > builtin_platform_driver(qcom_pcie_driver); > -- > 2.7.4 >
On 8/17/2023 10:50 PM, Bjorn Helgaas wrote: > On Thu, Aug 17, 2023 at 08:04:43AM +0530, Krishna chaitanya chundru wrote: >> PCIe can operate on lower GEN speed if client decided based upon >> the bandwidth & latency requirements. To support dynamic GEN speed >> switch adding this sysfs support. > Who does "client" refer to? I assume it's the system administrator, > but of course the endpoint is involved in the hardware speed > negotiation, so one could think of the endpoint as a "client" with its > own speed capabilities and requirements. The client here refers to either system administrator or endpoint both can decide to go to lower gen speed for reducing power consumption. >> To change the GEN speed the link should be in L0, so first disable >> L0s & L1. >> >> L0s needs to be disabled at both RC & EP because L0s entry is >> independent. For enabling L0s both ends of the link needs to support >> it, so first check if L0s is supported on both ends and then enable >> L0s. > Is there a place to document this sysfs knob? Why should it be > qcom-specific? This sounds like generic PCIe functionality. > > The ASPM stuff looks like it should be done by aspm.c, not done behind > its back. > > Everything here looks generic (not qcom-specific) except the > qcom_pcie_icc_update() and qcom_pcie_opp_update(). Maybe we need some > core infrastructure around this. I will try to move the logic to the pci generic and add infrastructure to call controller specific calls. - KC > >> This patch is dependent on "PCI: qcom: Add support for OPP" >> https://lore.kernel.org/linux-arm-msm/1692192264-18515-1-git-send-email-quic_krichai@quicinc.com/T/#t >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 141 +++++++++++++++++++++++++++++++++ >> 1 file changed, 141 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 831d158..ad67d17 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -241,10 +241,150 @@ struct qcom_pcie { >> const struct qcom_pcie_cfg *cfg; >> struct dentry *debugfs; >> bool suspended; >> + bool l0s_supported; >> }; >> >> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) >> >> +static void qcom_pcie_icc_update(struct qcom_pcie *pcie); >> +static void qcom_pcie_opp_update(struct qcom_pcie *pcie); >> + >> +static int qcom_pcie_disable_l0s(struct pci_dev *pdev, void *userdata) >> +{ >> + int lnkctl; >> + >> + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); >> + lnkctl &= ~(PCI_EXP_LNKCTL_ASPM_L0S); >> + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); >> + >> + return 0; >> +} >> + >> +static int qcom_pcie_check_l0s_support(struct pci_dev *pdev, void *userdata) >> +{ >> + struct pci_dev *parent = pdev->bus->self; >> + struct qcom_pcie *pcie = userdata; >> + struct dw_pcie *pci = pcie->pci; >> + int lnkcap; >> + >> + /* check parent supports L0s */ >> + if (parent) { >> + dev_err(pci->dev, "parent\n"); >> + pci_read_config_dword(parent, pci_pcie_cap(parent) + PCI_EXP_LNKCAP, >> + &lnkcap); >> + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { >> + dev_info(pci->dev, "Parent does not support L0s\n"); >> + pcie->l0s_supported = false; >> + return 0; >> + } >> + } >> + >> + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP, >> + &lnkcap); >> + dev_err(pci->dev, "child %x\n", lnkcap); >> + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { >> + dev_info(pci->dev, "Device does not support L0s\n"); >> + pcie->l0s_supported = false; >> + return 0; >> + } >> + >> + return 0; >> +} >> + >> +static int qcom_pcie_enable_l0s(struct pci_dev *pdev, void *userdata) >> +{ >> + int lnkctl; >> + >> + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); >> + lnkctl |= (PCI_EXP_LNKCTL_ASPM_L0S); >> + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); >> + >> + return 0; >> +} >> + >> +static ssize_t qcom_pcie_speed_change_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t count) >> +{ >> + unsigned int current_speed, target_speed, max_speed; >> + struct qcom_pcie *pcie = dev_get_drvdata(dev); >> + struct pci_bus *child, *root_bus = NULL; >> + struct dw_pcie_rp *pp = &pcie->pci->pp; >> + struct dw_pcie *pci = pcie->pci; >> + struct pci_dev *pdev; >> + u16 offset; >> + u32 val; >> + int ret; >> + >> + list_for_each_entry(child, &pp->bridge->bus->children, node) { >> + if (child->parent == pp->bridge->bus) { >> + root_bus = child; >> + break; >> + } >> + } >> + >> + pdev = root_bus->self; >> + >> + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); >> + >> + val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP); >> + max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); >> + >> + val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA); >> + current_speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val); >> + >> + ret = kstrtouint(buf, 10, &target_speed); >> + if (ret) >> + return ret; >> + >> + if (target_speed > max_speed) >> + return -EINVAL; >> + >> + if (current_speed == target_speed) >> + return count; >> + >> + pci_walk_bus(pp->bridge->bus, qcom_pcie_disable_l0s, pcie); >> + >> + /* Disable L1 */ >> + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); >> + val &= ~(PCI_EXP_LNKCTL_ASPM_L1); >> + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); >> + >> + /* Set target GEN speed */ >> + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2); >> + val &= ~PCI_EXP_LNKCTL2_TLS; >> + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, val | target_speed); >> + >> + ret = pcie_retrain_link(pdev, true); >> + if (ret) >> + dev_err(dev, "Link retrain failed %d\n", ret); >> + >> + /* Enable L1 */ >> + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); >> + val |= (PCI_EXP_LNKCTL_ASPM_L1); >> + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); >> + >> + pcie->l0s_supported = true; >> + pci_walk_bus(pp->bridge->bus, qcom_pcie_check_l0s_support, pcie); >> + >> + if (pcie->l0s_supported) >> + pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_l0s, pcie); >> + >> + qcom_pcie_icc_update(pcie); >> + >> + qcom_pcie_opp_update(pcie); >> + >> + return count; >> +} >> +static DEVICE_ATTR_WO(qcom_pcie_speed_change); >> + >> +static struct attribute *qcom_pcie_attrs[] = { >> + &dev_attr_qcom_pcie_speed_change.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(qcom_pcie); >> + >> static void qcom_ep_reset_assert(struct qcom_pcie *pcie) >> { >> gpiod_set_value_cansleep(pcie->reset, 1); >> @@ -1716,6 +1856,7 @@ static struct platform_driver qcom_pcie_driver = { >> .of_match_table = qcom_pcie_match, >> .pm = &qcom_pcie_pm_ops, >> .probe_type = PROBE_PREFER_ASYNCHRONOUS, >> + .dev_groups = qcom_pcie_groups, >> }, >> }; >> builtin_platform_driver(qcom_pcie_driver); >> -- >> 2.7.4 >>
Hi Krishna, kernel test robot noticed the following build errors: [auto build test ERROR on pci/next] [also build test ERROR on pci/for-linus linus/master v6.5-rc7 next-20230822] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/PCI-qcom-Add-sysfs-entry-to-change-link-speed-dynamically/20230817-103734 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next patch link: https://lore.kernel.org/r/1692239684-12697-1-git-send-email-quic_krichai%40quicinc.com patch subject: [PATCH v1] PCI: qcom: Add sysfs entry to change link speed dynamically config: loongarch-randconfig-r005-20230822 (https://download.01.org/0day-ci/archive/20230822/202308222302.9EPeENqh-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230822/202308222302.9EPeENqh-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308222302.9EPeENqh-lkp@intel.com/ All errors (new ones prefixed by >>): loongarch64-linux-ld: drivers/pci/controller/dwc/pcie-qcom.o: in function `qcom_pcie_speed_change_store': >> drivers/pci/controller/dwc/pcie-qcom.c:375:(.text+0xc14): undefined reference to `qcom_pcie_opp_update' vim +375 drivers/pci/controller/dwc/pcie-qcom.c 303 304 static ssize_t qcom_pcie_speed_change_store(struct device *dev, 305 struct device_attribute *attr, 306 const char *buf, 307 size_t count) 308 { 309 unsigned int current_speed, target_speed, max_speed; 310 struct qcom_pcie *pcie = dev_get_drvdata(dev); 311 struct pci_bus *child, *root_bus = NULL; 312 struct dw_pcie_rp *pp = &pcie->pci->pp; 313 struct dw_pcie *pci = pcie->pci; 314 struct pci_dev *pdev; 315 u16 offset; 316 u32 val; 317 int ret; 318 319 list_for_each_entry(child, &pp->bridge->bus->children, node) { 320 if (child->parent == pp->bridge->bus) { 321 root_bus = child; 322 break; 323 } 324 } 325 326 pdev = root_bus->self; 327 328 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); 329 330 val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP); 331 max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); 332 333 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA); 334 current_speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val); 335 336 ret = kstrtouint(buf, 10, &target_speed); 337 if (ret) 338 return ret; 339 340 if (target_speed > max_speed) 341 return -EINVAL; 342 343 if (current_speed == target_speed) 344 return count; 345 346 pci_walk_bus(pp->bridge->bus, qcom_pcie_disable_l0s, pcie); 347 348 /* Disable L1 */ 349 val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); 350 val &= ~(PCI_EXP_LNKCTL_ASPM_L1); 351 dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); 352 353 /* Set target GEN speed */ 354 val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2); 355 val &= ~PCI_EXP_LNKCTL2_TLS; 356 dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, val | target_speed); 357 358 ret = pcie_retrain_link(pdev, true); 359 if (ret) 360 dev_err(dev, "Link retrain failed %d\n", ret); 361 362 /* Enable L1 */ 363 val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); 364 val |= (PCI_EXP_LNKCTL_ASPM_L1); 365 dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); 366 367 pcie->l0s_supported = true; 368 pci_walk_bus(pp->bridge->bus, qcom_pcie_check_l0s_support, pcie); 369 370 if (pcie->l0s_supported) 371 pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_l0s, pcie); 372 373 qcom_pcie_icc_update(pcie); 374 > 375 qcom_pcie_opp_update(pcie); 376 377 return count; 378 } 379 static DEVICE_ATTR_WO(qcom_pcie_speed_change); 380
On Fri, Aug 18, 2023 at 07:56:02AM +0530, Krishna Chaitanya Chundru wrote: > > On 8/17/2023 10:50 PM, Bjorn Helgaas wrote: > > On Thu, Aug 17, 2023 at 08:04:43AM +0530, Krishna chaitanya chundru wrote: > > > PCIe can operate on lower GEN speed if client decided based upon > > > the bandwidth & latency requirements. To support dynamic GEN speed > > > switch adding this sysfs support. > > Who does "client" refer to? I assume it's the system administrator, > > but of course the endpoint is involved in the hardware speed > > negotiation, so one could think of the endpoint as a "client" with its > > own speed capabilities and requirements. > > The client here refers to either system administrator or endpoint both can > decide to go > > to lower gen speed for reducing power consumption. > Since this sounds like a debug knob, I'd prefer to go with debugfs than sysfs. - Mani > > > To change the GEN speed the link should be in L0, so first disable > > > L0s & L1. > > > > > > L0s needs to be disabled at both RC & EP because L0s entry is > > > independent. For enabling L0s both ends of the link needs to support > > > it, so first check if L0s is supported on both ends and then enable > > > L0s. > > Is there a place to document this sysfs knob? Why should it be > > qcom-specific? This sounds like generic PCIe functionality. > > > > The ASPM stuff looks like it should be done by aspm.c, not done behind > > its back. > > > > Everything here looks generic (not qcom-specific) except the > > qcom_pcie_icc_update() and qcom_pcie_opp_update(). Maybe we need some > > core infrastructure around this. > > I will try to move the logic to the pci generic and add infrastructure to > call controller specific calls. > > - KC > > > > > > This patch is dependent on "PCI: qcom: Add support for OPP" > > > https://lore.kernel.org/linux-arm-msm/1692192264-18515-1-git-send-email-quic_krichai@quicinc.com/T/#t > > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > > > --- > > > drivers/pci/controller/dwc/pcie-qcom.c | 141 +++++++++++++++++++++++++++++++++ > > > 1 file changed, 141 insertions(+) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > index 831d158..ad67d17 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > @@ -241,10 +241,150 @@ struct qcom_pcie { > > > const struct qcom_pcie_cfg *cfg; > > > struct dentry *debugfs; > > > bool suspended; > > > + bool l0s_supported; > > > }; > > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) > > > +static void qcom_pcie_icc_update(struct qcom_pcie *pcie); > > > +static void qcom_pcie_opp_update(struct qcom_pcie *pcie); > > > + > > > +static int qcom_pcie_disable_l0s(struct pci_dev *pdev, void *userdata) > > > +{ > > > + int lnkctl; > > > + > > > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); > > > + lnkctl &= ~(PCI_EXP_LNKCTL_ASPM_L0S); > > > + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); > > > + > > > + return 0; > > > +} > > > + > > > +static int qcom_pcie_check_l0s_support(struct pci_dev *pdev, void *userdata) > > > +{ > > > + struct pci_dev *parent = pdev->bus->self; > > > + struct qcom_pcie *pcie = userdata; > > > + struct dw_pcie *pci = pcie->pci; > > > + int lnkcap; > > > + > > > + /* check parent supports L0s */ > > > + if (parent) { > > > + dev_err(pci->dev, "parent\n"); > > > + pci_read_config_dword(parent, pci_pcie_cap(parent) + PCI_EXP_LNKCAP, > > > + &lnkcap); > > > + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { > > > + dev_info(pci->dev, "Parent does not support L0s\n"); > > > + pcie->l0s_supported = false; > > > + return 0; > > > + } > > > + } > > > + > > > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP, > > > + &lnkcap); > > > + dev_err(pci->dev, "child %x\n", lnkcap); > > > + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { > > > + dev_info(pci->dev, "Device does not support L0s\n"); > > > + pcie->l0s_supported = false; > > > + return 0; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int qcom_pcie_enable_l0s(struct pci_dev *pdev, void *userdata) > > > +{ > > > + int lnkctl; > > > + > > > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); > > > + lnkctl |= (PCI_EXP_LNKCTL_ASPM_L0S); > > > + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); > > > + > > > + return 0; > > > +} > > > + > > > +static ssize_t qcom_pcie_speed_change_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, > > > + size_t count) > > > +{ > > > + unsigned int current_speed, target_speed, max_speed; > > > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > > > + struct pci_bus *child, *root_bus = NULL; > > > + struct dw_pcie_rp *pp = &pcie->pci->pp; > > > + struct dw_pcie *pci = pcie->pci; > > > + struct pci_dev *pdev; > > > + u16 offset; > > > + u32 val; > > > + int ret; > > > + > > > + list_for_each_entry(child, &pp->bridge->bus->children, node) { > > > + if (child->parent == pp->bridge->bus) { > > > + root_bus = child; > > > + break; > > > + } > > > + } > > > + > > > + pdev = root_bus->self; > > > + > > > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > + > > > + val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP); > > > + max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); > > > + > > > + val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA); > > > + current_speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val); > > > + > > > + ret = kstrtouint(buf, 10, &target_speed); > > > + if (ret) > > > + return ret; > > > + > > > + if (target_speed > max_speed) > > > + return -EINVAL; > > > + > > > + if (current_speed == target_speed) > > > + return count; > > > + > > > + pci_walk_bus(pp->bridge->bus, qcom_pcie_disable_l0s, pcie); > > > + > > > + /* Disable L1 */ > > > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); > > > + val &= ~(PCI_EXP_LNKCTL_ASPM_L1); > > > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); > > > + > > > + /* Set target GEN speed */ > > > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2); > > > + val &= ~PCI_EXP_LNKCTL2_TLS; > > > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, val | target_speed); > > > + > > > + ret = pcie_retrain_link(pdev, true); > > > + if (ret) > > > + dev_err(dev, "Link retrain failed %d\n", ret); > > > + > > > + /* Enable L1 */ > > > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); > > > + val |= (PCI_EXP_LNKCTL_ASPM_L1); > > > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); > > > + > > > + pcie->l0s_supported = true; > > > + pci_walk_bus(pp->bridge->bus, qcom_pcie_check_l0s_support, pcie); > > > + > > > + if (pcie->l0s_supported) > > > + pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_l0s, pcie); > > > + > > > + qcom_pcie_icc_update(pcie); > > > + > > > + qcom_pcie_opp_update(pcie); > > > + > > > + return count; > > > +} > > > +static DEVICE_ATTR_WO(qcom_pcie_speed_change); > > > + > > > +static struct attribute *qcom_pcie_attrs[] = { > > > + &dev_attr_qcom_pcie_speed_change.attr, > > > + NULL, > > > +}; > > > +ATTRIBUTE_GROUPS(qcom_pcie); > > > + > > > static void qcom_ep_reset_assert(struct qcom_pcie *pcie) > > > { > > > gpiod_set_value_cansleep(pcie->reset, 1); > > > @@ -1716,6 +1856,7 @@ static struct platform_driver qcom_pcie_driver = { > > > .of_match_table = qcom_pcie_match, > > > .pm = &qcom_pcie_pm_ops, > > > .probe_type = PROBE_PREFER_ASYNCHRONOUS, > > > + .dev_groups = qcom_pcie_groups, > > > }, > > > }; > > > builtin_platform_driver(qcom_pcie_driver); > > > -- > > > 2.7.4 > > >
On Thu, Aug 17, 2023 at 08:04:43AM +0530, Krishna chaitanya chundru wrote: > PCIe can operate on lower GEN speed if client decided based upon > the bandwidth & latency requirements. To support dynamic GEN speed > switch adding this sysfs support. > > To change the GEN speed the link should be in L0, so first disable > L0s & L1. > > L0s needs to be disabled at both RC & EP because L0s entry is > independent. For enabling L0s both ends of the link needs to support > it, so first check if L0s is supported on both ends and then enable > L0s. > > This patch is dependent on "PCI: qcom: Add support for OPP" > https://lore.kernel.org/linux-arm-msm/1692192264-18515-1-git-send-email-quic_krichai@quicinc.com/T/#t > There is also an ongoing proposal to add PCIe bandwidth controller driver [1] which more or less does the same thing. Check if you can make use of it. - Mani [1] https://lore.kernel.org/linux-pci/20230817121708.53213-1-ilpo.jarvinen@linux.intel.com/ > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 141 +++++++++++++++++++++++++++++++++ > 1 file changed, 141 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 831d158..ad67d17 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -241,10 +241,150 @@ struct qcom_pcie { > const struct qcom_pcie_cfg *cfg; > struct dentry *debugfs; > bool suspended; > + bool l0s_supported; > }; > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) > > +static void qcom_pcie_icc_update(struct qcom_pcie *pcie); > +static void qcom_pcie_opp_update(struct qcom_pcie *pcie); > + > +static int qcom_pcie_disable_l0s(struct pci_dev *pdev, void *userdata) > +{ > + int lnkctl; > + > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); > + lnkctl &= ~(PCI_EXP_LNKCTL_ASPM_L0S); > + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); > + > + return 0; > +} > + > +static int qcom_pcie_check_l0s_support(struct pci_dev *pdev, void *userdata) > +{ > + struct pci_dev *parent = pdev->bus->self; > + struct qcom_pcie *pcie = userdata; > + struct dw_pcie *pci = pcie->pci; > + int lnkcap; > + > + /* check parent supports L0s */ > + if (parent) { > + dev_err(pci->dev, "parent\n"); > + pci_read_config_dword(parent, pci_pcie_cap(parent) + PCI_EXP_LNKCAP, > + &lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { > + dev_info(pci->dev, "Parent does not support L0s\n"); > + pcie->l0s_supported = false; > + return 0; > + } > + } > + > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP, > + &lnkcap); > + dev_err(pci->dev, "child %x\n", lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { > + dev_info(pci->dev, "Device does not support L0s\n"); > + pcie->l0s_supported = false; > + return 0; > + } > + > + return 0; > +} > + > +static int qcom_pcie_enable_l0s(struct pci_dev *pdev, void *userdata) > +{ > + int lnkctl; > + > + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); > + lnkctl |= (PCI_EXP_LNKCTL_ASPM_L0S); > + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); > + > + return 0; > +} > + > +static ssize_t qcom_pcie_speed_change_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + unsigned int current_speed, target_speed, max_speed; > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > + struct pci_bus *child, *root_bus = NULL; > + struct dw_pcie_rp *pp = &pcie->pci->pp; > + struct dw_pcie *pci = pcie->pci; > + struct pci_dev *pdev; > + u16 offset; > + u32 val; > + int ret; > + > + list_for_each_entry(child, &pp->bridge->bus->children, node) { > + if (child->parent == pp->bridge->bus) { > + root_bus = child; > + break; > + } > + } > + > + pdev = root_bus->self; > + > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > + > + val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP); > + max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); > + > + val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA); > + current_speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val); > + > + ret = kstrtouint(buf, 10, &target_speed); > + if (ret) > + return ret; > + > + if (target_speed > max_speed) > + return -EINVAL; > + > + if (current_speed == target_speed) > + return count; > + > + pci_walk_bus(pp->bridge->bus, qcom_pcie_disable_l0s, pcie); > + > + /* Disable L1 */ > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); > + val &= ~(PCI_EXP_LNKCTL_ASPM_L1); > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); > + > + /* Set target GEN speed */ > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2); > + val &= ~PCI_EXP_LNKCTL2_TLS; > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, val | target_speed); > + > + ret = pcie_retrain_link(pdev, true); > + if (ret) > + dev_err(dev, "Link retrain failed %d\n", ret); > + > + /* Enable L1 */ > + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); > + val |= (PCI_EXP_LNKCTL_ASPM_L1); > + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); > + > + pcie->l0s_supported = true; > + pci_walk_bus(pp->bridge->bus, qcom_pcie_check_l0s_support, pcie); > + > + if (pcie->l0s_supported) > + pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_l0s, pcie); > + > + qcom_pcie_icc_update(pcie); > + > + qcom_pcie_opp_update(pcie); > + > + return count; > +} > +static DEVICE_ATTR_WO(qcom_pcie_speed_change); > + > +static struct attribute *qcom_pcie_attrs[] = { > + &dev_attr_qcom_pcie_speed_change.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(qcom_pcie); > + > static void qcom_ep_reset_assert(struct qcom_pcie *pcie) > { > gpiod_set_value_cansleep(pcie->reset, 1); > @@ -1716,6 +1856,7 @@ static struct platform_driver qcom_pcie_driver = { > .of_match_table = qcom_pcie_match, > .pm = &qcom_pcie_pm_ops, > .probe_type = PROBE_PREFER_ASYNCHRONOUS, > + .dev_groups = qcom_pcie_groups, > }, > }; > builtin_platform_driver(qcom_pcie_driver); > -- > 2.7.4 >
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 831d158..ad67d17 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -241,10 +241,150 @@ struct qcom_pcie { const struct qcom_pcie_cfg *cfg; struct dentry *debugfs; bool suspended; + bool l0s_supported; }; #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) +static void qcom_pcie_icc_update(struct qcom_pcie *pcie); +static void qcom_pcie_opp_update(struct qcom_pcie *pcie); + +static int qcom_pcie_disable_l0s(struct pci_dev *pdev, void *userdata) +{ + int lnkctl; + + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); + lnkctl &= ~(PCI_EXP_LNKCTL_ASPM_L0S); + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); + + return 0; +} + +static int qcom_pcie_check_l0s_support(struct pci_dev *pdev, void *userdata) +{ + struct pci_dev *parent = pdev->bus->self; + struct qcom_pcie *pcie = userdata; + struct dw_pcie *pci = pcie->pci; + int lnkcap; + + /* check parent supports L0s */ + if (parent) { + dev_err(pci->dev, "parent\n"); + pci_read_config_dword(parent, pci_pcie_cap(parent) + PCI_EXP_LNKCAP, + &lnkcap); + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { + dev_info(pci->dev, "Parent does not support L0s\n"); + pcie->l0s_supported = false; + return 0; + } + } + + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP, + &lnkcap); + dev_err(pci->dev, "child %x\n", lnkcap); + if (!(lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)) { + dev_info(pci->dev, "Device does not support L0s\n"); + pcie->l0s_supported = false; + return 0; + } + + return 0; +} + +static int qcom_pcie_enable_l0s(struct pci_dev *pdev, void *userdata) +{ + int lnkctl; + + pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, &lnkctl); + lnkctl |= (PCI_EXP_LNKCTL_ASPM_L0S); + pci_write_config_word(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCTL, lnkctl); + + return 0; +} + +static ssize_t qcom_pcie_speed_change_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t count) +{ + unsigned int current_speed, target_speed, max_speed; + struct qcom_pcie *pcie = dev_get_drvdata(dev); + struct pci_bus *child, *root_bus = NULL; + struct dw_pcie_rp *pp = &pcie->pci->pp; + struct dw_pcie *pci = pcie->pci; + struct pci_dev *pdev; + u16 offset; + u32 val; + int ret; + + list_for_each_entry(child, &pp->bridge->bus->children, node) { + if (child->parent == pp->bridge->bus) { + root_bus = child; + break; + } + } + + pdev = root_bus->self; + + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); + + val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP); + max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); + + val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA); + current_speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val); + + ret = kstrtouint(buf, 10, &target_speed); + if (ret) + return ret; + + if (target_speed > max_speed) + return -EINVAL; + + if (current_speed == target_speed) + return count; + + pci_walk_bus(pp->bridge->bus, qcom_pcie_disable_l0s, pcie); + + /* Disable L1 */ + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); + val &= ~(PCI_EXP_LNKCTL_ASPM_L1); + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); + + /* Set target GEN speed */ + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2); + val &= ~PCI_EXP_LNKCTL2_TLS; + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, val | target_speed); + + ret = pcie_retrain_link(pdev, true); + if (ret) + dev_err(dev, "Link retrain failed %d\n", ret); + + /* Enable L1 */ + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL); + val |= (PCI_EXP_LNKCTL_ASPM_L1); + dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL, val); + + pcie->l0s_supported = true; + pci_walk_bus(pp->bridge->bus, qcom_pcie_check_l0s_support, pcie); + + if (pcie->l0s_supported) + pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_l0s, pcie); + + qcom_pcie_icc_update(pcie); + + qcom_pcie_opp_update(pcie); + + return count; +} +static DEVICE_ATTR_WO(qcom_pcie_speed_change); + +static struct attribute *qcom_pcie_attrs[] = { + &dev_attr_qcom_pcie_speed_change.attr, + NULL, +}; +ATTRIBUTE_GROUPS(qcom_pcie); + static void qcom_ep_reset_assert(struct qcom_pcie *pcie) { gpiod_set_value_cansleep(pcie->reset, 1); @@ -1716,6 +1856,7 @@ static struct platform_driver qcom_pcie_driver = { .of_match_table = qcom_pcie_match, .pm = &qcom_pcie_pm_ops, .probe_type = PROBE_PREFER_ASYNCHRONOUS, + .dev_groups = qcom_pcie_groups, }, }; builtin_platform_driver(qcom_pcie_driver);
PCIe can operate on lower GEN speed if client decided based upon the bandwidth & latency requirements. To support dynamic GEN speed switch adding this sysfs support. To change the GEN speed the link should be in L0, so first disable L0s & L1. L0s needs to be disabled at both RC & EP because L0s entry is independent. For enabling L0s both ends of the link needs to support it, so first check if L0s is supported on both ends and then enable L0s. This patch is dependent on "PCI: qcom: Add support for OPP" https://lore.kernel.org/linux-arm-msm/1692192264-18515-1-git-send-email-quic_krichai@quicinc.com/T/#t Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- drivers/pci/controller/dwc/pcie-qcom.c | 141 +++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+)