| Message ID | 20250103060035.30688-6-jianjun.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: mediatek-gen3: Add MT8196 support | expand |
Il 03/01/25 07:00, Jianjun Wang ha scritto: > If the target system sleep state is suspend-to-idle, the bridge is > supposed to stay in D0, and the framework will not help to restore its > configuration space, so keep its power and clocks during suspend. > > It's recommended to enable L1ss support, so the link can be changed to > L1.2 state during suspend. > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek-gen3.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c > index 48f83c2d91f7..11da68910502 100644 > --- a/drivers/pci/controller/pcie-mediatek-gen3.c > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c > @@ -1291,6 +1291,19 @@ static int mtk_pcie_suspend_noirq(struct device *dev) > int err; > u32 val; > > + /* > + * If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in > + * D0, and the framework will not help to restore its configuration space, so keep it's > + * power and clocks during suspend. > + * > + * It's recommended to enable L1ss support, so the link can be changed to L1.2 state during > + * suspend. > + */ > + if (pm_suspend_default_s2idle()) { AFAIK, this works only if s2idle is the default system sleep state. ...and besides, for good measure (especially in the event that in the future we get hotplug support) you should also check if there is any active PCI-Express device on the controller instance before deciding to leave the controller and link UP, as PCI-Express controllers may be enabled even if there's no actually connected device on a PCIe slot (full PCIe, or mPCIe, or M.2). So, I suggest to: - Add a `bool suspended` member to `struct mtk_gen3_pcie`, then static int mtk_pcie_suspend_noirq(struct device *dev) { struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev); int err; u32 val; val = readl(pcie->base + PCIE_LINK_STATUS_REG); val &= PCIE_PORT_LINKUP; if (val && pm_suspend_target_state == PM_SUSPEND_TO_IDLE) { dev_dbg(....) return 0; } /* Trigger link to L2 state */ err = mtk_pcie_turn_off_link(pcie); if (err) { dev_err(pcie->dev, "cannot enter L2 state\n"); return err; } pcie->suspended = true; /* Pull down the PERST# pin */ .... etc etc and static int mtk_pcie_resume_noirq(struct device *dev) { struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev); int err; if (!pcie->suspended) return 0; err = pcie->soc->power_up .... etc etc ... so you only check if we're going in s2idle in the suspend handler, as that dictates the value of pcie->suspended :-) Cheers, Angelo > + dev_info(dev, "System enter s2idle state, keep PCIe power and clocks\n"); > + return 0; > + } > + > /* Trigger link to L2 state */ > err = mtk_pcie_turn_off_link(pcie); > if (err) { > @@ -1316,6 +1329,11 @@ static int mtk_pcie_resume_noirq(struct device *dev) > struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev); > int err; > > + if (pm_suspend_default_s2idle()) { > + dev_info(dev, "System enter s2idle state, no need to reinitialization\n"); > + return 0; > + } > + > err = pcie->soc->power_up(pcie); > if (err) > return err;
On Fri, Jan 03, 2025 at 02:00:15PM +0800, Jianjun Wang wrote: > If the target system sleep state is suspend-to-idle, the bridge is > supposed to stay in D0, and the framework will not help to restore its > configuration space, so keep its power and clocks during suspend. > > It's recommended to enable L1ss support, so the link can be changed to > L1.2 state during suspend. > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek-gen3.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c > index 48f83c2d91f7..11da68910502 100644 > --- a/drivers/pci/controller/pcie-mediatek-gen3.c > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c > @@ -1291,6 +1291,19 @@ static int mtk_pcie_suspend_noirq(struct device *dev) > int err; > u32 val; > > + /* > + * If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in > + * D0, and the framework will not help to restore its configuration space, so keep it's > + * power and clocks during suspend. s/it's/its/ Where does the requirement for the bridge to stay in D0 come from? Is that part of the Linux power management framework? Or is this something specific to this SoC? I guess "framework" refers to Linux power management, so maybe that assumes nothing needs to be restored when resuming from suspend-to-idle? > + * It's recommended to enable L1ss support, so the link can be changed to L1.2 state during > + * suspend. Wrap to fit in 80 columns like the rest of the file. s/L1ss/L1SS/ Who recommends enabling L1SS support? I suppose enabling L1SS means setting CONFIG_PCIEASPM=y? What causes the transition to L1.2? Is this done by firmware or something else outside of Linux? > + if (pm_suspend_default_s2idle()) { > + dev_info(dev, "System enter s2idle state, keep PCIe power and clocks\n"); 1) I'm a bit dubious about this since there are only two callers of pm_suspend_default_s2idle() in the rest of the kernel. What's unique about this device that requires this? Is this an indication that we're setting mtk_pcie_pm_ops incorrectly, i.e., are we using mtk_pcie_suspend_noirq() for a callback that is *never* supposed to power down the device? 2) The message seems like possible overkill, maybe could be dev_dbg()? I'm not sure the user needs this information at every suspend/resume transition. > + return 0; > + } > + > /* Trigger link to L2 state */ > err = mtk_pcie_turn_off_link(pcie); > if (err) { > @@ -1316,6 +1329,11 @@ static int mtk_pcie_resume_noirq(struct device *dev) > struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev); > int err; > > + if (pm_suspend_default_s2idle()) { > + dev_info(dev, "System enter s2idle state, no need to reinitialization\n"); > + return 0; > + } > + > err = pcie->soc->power_up(pcie); > if (err) > return err; > -- > 2.46.0 >
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c index 48f83c2d91f7..11da68910502 100644 --- a/drivers/pci/controller/pcie-mediatek-gen3.c +++ b/drivers/pci/controller/pcie-mediatek-gen3.c @@ -1291,6 +1291,19 @@ static int mtk_pcie_suspend_noirq(struct device *dev) int err; u32 val; + /* + * If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in + * D0, and the framework will not help to restore its configuration space, so keep it's + * power and clocks during suspend. + * + * It's recommended to enable L1ss support, so the link can be changed to L1.2 state during + * suspend. + */ + if (pm_suspend_default_s2idle()) { + dev_info(dev, "System enter s2idle state, keep PCIe power and clocks\n"); + return 0; + } + /* Trigger link to L2 state */ err = mtk_pcie_turn_off_link(pcie); if (err) { @@ -1316,6 +1329,11 @@ static int mtk_pcie_resume_noirq(struct device *dev) struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev); int err; + if (pm_suspend_default_s2idle()) { + dev_info(dev, "System enter s2idle state, no need to reinitialization\n"); + return 0; + } + err = pcie->soc->power_up(pcie); if (err) return err;
If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in D0, and the framework will not help to restore its configuration space, so keep its power and clocks during suspend. It's recommended to enable L1ss support, so the link can be changed to L1.2 state during suspend. Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> --- drivers/pci/controller/pcie-mediatek-gen3.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)