| Message ID | 1654240730-31322-1-git-send-email-quic_krichai@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] PCI: qcom: Allow L1 and its sub states on qcom dwc wrapper | expand |
Quoting Krishna chaitanya chundru (2022-06-03 00:18:50) > Allow L1 and its sub-states in the qcom dwc pcie wrapper. > By default its disabled. So enable it explicitly. > Would be good to add some more details about why it's disabled by default. I guess it's disabled by default in the hardware and enabling it is OK to do unconditionally for all qcom dwc pcie devices?
On Fri, Jun 03, 2022 at 12:48:50PM +0530, Krishna chaitanya chundru wrote: > Allow L1 and its sub-states in the qcom dwc pcie wrapper. s/wrapper/driver Also there is no need to use "qcom dwc" in subject. Prefix makes it explicit. > By default its disabled. So enable it explicitly. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 6ab9089..f60645c 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -41,6 +41,9 @@ > #define L23_CLK_RMV_DIS BIT(2) > #define L1_CLK_RMV_DIS BIT(1) > > +#define PCIE20_PARF_PM_CTRL 0x20 > +#define REQ_NOT_ENTR_L1 BIT(5) > + > #define PCIE20_PARF_PHY_CTRL 0x40 > #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK GENMASK(20, 16) > #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) ((x) << 16) > @@ -1267,6 +1270,11 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > val |= BIT(4); > writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL); > > + /* Clear PARF PM REQ_NOT_ENTR_L1 bit to allow L1 states */ Mentioning the field in comment is redundant. Just say "Enable L1 and L1ss" Thanks, Mani > + val = readl(pcie->parf + PCIE20_PARF_PM_CTRL); > + val &= ~REQ_NOT_ENTR_L1; > + writel(val, pcie->parf + PCIE20_PARF_PM_CTRL); > + > if (IS_ENABLED(CONFIG_PCI_MSI)) { > val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT); > val |= BIT(31); > -- > 2.7.4 >
On 6/9/2022 3:47 AM, Stephen Boyd wrote: > Quoting Krishna chaitanya chundru (2022-06-03 00:18:50) >> Allow L1 and its sub-states in the qcom dwc pcie wrapper. >> By default its disabled. So enable it explicitly. >> > Would be good to add some more details about why it's disabled by > default. I guess it's disabled by default in the hardware and enabling > it is OK to do unconditionally for all qcom dwc pcie devices? This is disabled by default in the hardware. We can enable this for all qcom devices unconditionally because Adding this patch alone will not allow aspm transitions we need to enable aspm configs. If particular devices doesn't want aspm they can disable using aspm configs.
On 6/9/2022 4:56 PM, Manivannan Sadhasivam wrote: > On Fri, Jun 03, 2022 at 12:48:50PM +0530, Krishna chaitanya chundru wrote: >> Allow L1 and its sub-states in the qcom dwc pcie wrapper. > s/wrapper/driver > > Also there is no need to use "qcom dwc" in subject. Prefix makes it explicit. Sure will update in the next patch. >> By default its disabled. So enable it explicitly. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 6ab9089..f60645c 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -41,6 +41,9 @@ >> #define L23_CLK_RMV_DIS BIT(2) >> #define L1_CLK_RMV_DIS BIT(1) >> >> +#define PCIE20_PARF_PM_CTRL 0x20 >> +#define REQ_NOT_ENTR_L1 BIT(5) >> + >> #define PCIE20_PARF_PHY_CTRL 0x40 >> #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK GENMASK(20, 16) >> #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) ((x) << 16) >> @@ -1267,6 +1270,11 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) >> val |= BIT(4); >> writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL); >> >> + /* Clear PARF PM REQ_NOT_ENTR_L1 bit to allow L1 states */ > Mentioning the field in comment is redundant. Just say "Enable L1 and L1ss" sure will update in the next patch. > Thanks, > Mani > >> + val = readl(pcie->parf + PCIE20_PARF_PM_CTRL); >> + val &= ~REQ_NOT_ENTR_L1; >> + writel(val, pcie->parf + PCIE20_PARF_PM_CTRL); >> + >> if (IS_ENABLED(CONFIG_PCI_MSI)) { >> val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT); >> val |= BIT(31); >> -- >> 2.7.4 >>
On Wed, Jun 15, 2022 at 06:44:19PM +0530, Krishna Chaitanya Chundru wrote: > On 6/9/2022 3:47 AM, Stephen Boyd wrote: > > Quoting Krishna chaitanya chundru (2022-06-03 00:18:50) > > > Allow L1 and its sub-states in the qcom dwc pcie wrapper. > > > By default its disabled. So enable it explicitly. > > > > > Would be good to add some more details about why it's disabled by > > default. I guess it's disabled by default in the hardware and enabling > > it is OK to do unconditionally for all qcom dwc pcie devices? > > This is disabled by default in the hardware. We can enable this for all qcom > devices unconditionally because > > Adding this patch alone will not allow aspm transitions we need to enable > aspm configs. If particular devices doesn't want aspm > they can disable using aspm configs. This patch only affects qcom. Is PCIE20_PARF_PM_CTRL qcom-specific? Or is this something that should be done for all dwc-based drivers? In fact, it only affects Qcom IP rev 2.7.0 and 1.9.0 (the only users of qcom_pcie_init_2_7_0()). I guess the other revisions don't support ASPM L1 at all? Does this patch affect the Link Capabilities register? Before this patch, does Link Cap advertise L1 support but enabling it doesn't work? Or does it not even advertise L1 support? After this patch, I assume Link Cap advertises L1 support and enabling L1 and L1 substates via PCI_EXP_LNKCTL_ASPM_L1, PCI_L1SS_CTL1_ASPM_L1_1, and PCI_L1SS_CTL1_ASPM_L1_2 works per spec, right? Bjorn
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 6ab9089..f60645c 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -41,6 +41,9 @@ #define L23_CLK_RMV_DIS BIT(2) #define L1_CLK_RMV_DIS BIT(1) +#define PCIE20_PARF_PM_CTRL 0x20 +#define REQ_NOT_ENTR_L1 BIT(5) + #define PCIE20_PARF_PHY_CTRL 0x40 #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK GENMASK(20, 16) #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) ((x) << 16) @@ -1267,6 +1270,11 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) val |= BIT(4); writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL); + /* Clear PARF PM REQ_NOT_ENTR_L1 bit to allow L1 states */ + val = readl(pcie->parf + PCIE20_PARF_PM_CTRL); + val &= ~REQ_NOT_ENTR_L1; + writel(val, pcie->parf + PCIE20_PARF_PM_CTRL); + if (IS_ENABLED(CONFIG_PCI_MSI)) { val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT); val |= BIT(31);
Allow L1 and its sub-states in the qcom dwc pcie wrapper. By default its disabled. So enable it explicitly. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++++ 1 file changed, 8 insertions(+)