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@xxxxxxxxxxx/T/#t > > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx> > > > --- > > > 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 > > > -- மணிவண்ணன் சதாசிவம்
