| Message ID | 20250103060035.30688-4-jianjun.wang@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: mediatek-gen3: Add MT8196 support | expand |
Il 03/01/25 07:00, Jianjun Wang ha scritto: > Disable ASPM L0s support because it does not significantly save power > but impacts performance. > That may be a good idea but, without numbers to support your statement, it's a bit difficult to say. How much power does ASPM L0s save on MediaTek SoCs, in microwatts? How is the performance impacted, and on which specific device(s) on the PCIe bus? Cheers, Angelo > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c > index ed3c0614486c..4bd3b39eebe2 100644 > --- a/drivers/pci/controller/pcie-mediatek-gen3.c > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c > @@ -84,6 +84,9 @@ > #define PCIE_MSI_SET_ENABLE_REG 0x190 > #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, 0) > > +#define PCIE_LOW_POWER_CTRL_REG 0x194 > +#define PCIE_FORCE_DIS_L0S BIT(8) > + > #define PCIE_PIPE4_PIE8_REG 0x338 > #define PCIE_K_FINETUNE_MAX GENMASK(5, 0) > #define PCIE_K_FINETUNE_ERR GENMASK(7, 6) > @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie) > val &= ~PCIE_INTX_ENABLE; > writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG); > > + /* > + * Disable L0s support because it does not significantly save power > + * but impacts performance. > + */ > + val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG); > + val |= PCIE_FORCE_DIS_L0S; > + writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG); > + > /* Disable DVFSRC voltage request */ > val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG); > val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote: > Disable ASPM L0s support because it does not significantly save power > but impacts performance. This seems like a user/administrator decision, not a driver decision. L0s reduces power at the cost of performance for *all* PCIe devices, although the actual numbers may vary.
On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote: > Disable ASPM L0s support because it does not significantly save power > but impacts performance. > You should disable ASPM only if it is causing any functional issues to the SoC itself. For other reasons, users will use the existing sysfs/cmdline params to disable ASPM based on usecase if required. - Mani > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c > index ed3c0614486c..4bd3b39eebe2 100644 > --- a/drivers/pci/controller/pcie-mediatek-gen3.c > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c > @@ -84,6 +84,9 @@ > #define PCIE_MSI_SET_ENABLE_REG 0x190 > #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, 0) > > +#define PCIE_LOW_POWER_CTRL_REG 0x194 > +#define PCIE_FORCE_DIS_L0S BIT(8) > + > #define PCIE_PIPE4_PIE8_REG 0x338 > #define PCIE_K_FINETUNE_MAX GENMASK(5, 0) > #define PCIE_K_FINETUNE_ERR GENMASK(7, 6) > @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie) > val &= ~PCIE_INTX_ENABLE; > writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG); > > + /* > + * Disable L0s support because it does not significantly save power > + * but impacts performance. > + */ > + val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG); > + val |= PCIE_FORCE_DIS_L0S; > + writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG); > + > /* Disable DVFSRC voltage request */ > val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG); > val |= PCIE_DISABLE_DVFSRC_VLT_REQ; > -- > 2.46.0 >
On Fri, 2025-01-03 at 10:16 +0100, AngeloGioacchino Del Regno wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Il 03/01/25 07:00, Jianjun Wang ha scritto: > > Disable ASPM L0s support because it does not significantly save > > power > > but impacts performance. > > > > That may be a good idea but, without numbers to support your > statement, it's a bit > difficult to say. > > How much power does ASPM L0s save on MediaTek SoCs, in microwatts? > How is the performance impacted, and on which specific device(s) on > the PCIe bus? It's hard to tell the exact number because it is difficult to measure, and the number of entries into the L0s state may vary even in the same test scenario. However, we have encountered some compatibility issues when connected with some PCIe EPs, and disabling the L0s can fix it. I think disabling L0s might be the better way, since we usually use L1ss for power-saving when the link is idle. Thanks. > > Cheers, > Angelo > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c > > b/drivers/pci/controller/pcie-mediatek-gen3.c > > index ed3c0614486c..4bd3b39eebe2 100644 > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c > > @@ -84,6 +84,9 @@ > > #define PCIE_MSI_SET_ENABLE_REG 0x190 > > #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, > > 0) > > > > +#define PCIE_LOW_POWER_CTRL_REG 0x194 > > +#define PCIE_FORCE_DIS_L0S BIT(8) > > + > > #define PCIE_PIPE4_PIE8_REG 0x338 > > #define PCIE_K_FINETUNE_MAX GENMASK(5, 0) > > #define PCIE_K_FINETUNE_ERR GENMASK(7, 6) > > @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct > > mtk_gen3_pcie *pcie) > > val &= ~PCIE_INTX_ENABLE; > > writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG); > > > > + /* > > + * Disable L0s support because it does not significantly save > > power > > + * but impacts performance. > > + */ > > + val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG); > > + val |= PCIE_FORCE_DIS_L0S; > > + writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG); > > + > > /* Disable DVFSRC voltage request */ > > val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG); > > val |= PCIE_DISABLE_DVFSRC_VLT_REQ; > >
On Fri, 2025-01-03 at 13:15 -0600, Bjorn Helgaas wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote: > > Disable ASPM L0s support because it does not significantly save > > power > > but impacts performance. > > This seems like a user/administrator decision, not a driver decision. > > L0s reduces power at the cost of performance for *all* PCIe devices, > although the actual numbers may vary. We have encountered some compatibility issues when connected with some PCIe EPs, these issues are probabilistic and disabling the L0s can fix them. Users may not be aware of these issues, so I think disabling L0s through the driver might be the better way, since it does not significantly save power and we usually use L1ss for power-saving when the link is idle. Thanks.
Il 07/01/25 03:18, Jianjun Wang (王建军) ha scritto: > On Fri, 2025-01-03 at 10:16 +0100, AngeloGioacchino Del Regno wrote: >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> Il 03/01/25 07:00, Jianjun Wang ha scritto: >>> Disable ASPM L0s support because it does not significantly save >>> power >>> but impacts performance. >>> >> >> That may be a good idea but, without numbers to support your >> statement, it's a bit >> difficult to say. >> >> How much power does ASPM L0s save on MediaTek SoCs, in microwatts? >> How is the performance impacted, and on which specific device(s) on >> the PCIe bus? > > It's hard to tell the exact number because it is difficult to measure, > and the number of entries into the L0s state may vary even in the same > test scenario. > > However, we have encountered some compatibility issues when connected > with some PCIe EPs, and disabling the L0s can fix it. I think disabling > L0s might be the better way, since we usually use L1ss for power-saving > when the link is idle. > To actually decide, we should know what's actually broken, then. Is the MediaTek controller broken, or is the device broken? So, is it a MTK quirk, or a device quirk? If the problem is actually device-related, then this should be handled as a device-specific quirk, as not just MediaTek platforms would be affected by compatibility issues. If the MediaTek PCIe controller is at fault, instead, I agree about just disabling L0s at the controller level - but then this shall be mentioned in the commit message, and should have a Fixes tag as well. Cheers, Angelo > Thanks. > >> >> Cheers, >> Angelo >> >>> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> >>> --- >>> drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c >>> b/drivers/pci/controller/pcie-mediatek-gen3.c >>> index ed3c0614486c..4bd3b39eebe2 100644 >>> --- a/drivers/pci/controller/pcie-mediatek-gen3.c >>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c >>> @@ -84,6 +84,9 @@ >>> #define PCIE_MSI_SET_ENABLE_REG 0x190 >>> #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, >>> 0) >>> >>> +#define PCIE_LOW_POWER_CTRL_REG 0x194 >>> +#define PCIE_FORCE_DIS_L0S BIT(8) >>> + >>> #define PCIE_PIPE4_PIE8_REG 0x338 >>> #define PCIE_K_FINETUNE_MAX GENMASK(5, 0) >>> #define PCIE_K_FINETUNE_ERR GENMASK(7, 6) >>> @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct >>> mtk_gen3_pcie *pcie) >>> val &= ~PCIE_INTX_ENABLE; >>> writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG); >>> >>> + /* >>> + * Disable L0s support because it does not significantly save >>> power >>> + * but impacts performance. >>> + */ >>> + val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG); >>> + val |= PCIE_FORCE_DIS_L0S; >>> + writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG); >>> + >>> /* Disable DVFSRC voltage request */ >>> val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG); >>> val |= PCIE_DISABLE_DVFSRC_VLT_REQ; >> >>
On Tue, Jan 07, 2025 at 02:44:37AM +0000, Jianjun Wang (王建军) wrote: > On Fri, 2025-01-03 at 13:15 -0600, Bjorn Helgaas wrote: > > On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote: > > > Disable ASPM L0s support because it does not significantly save > > > power but impacts performance. > > > > This seems like a user/administrator decision, not a driver > > decision. > > > > L0s reduces power at the cost of performance for *all* PCIe > > devices, although the actual numbers may vary. > > We have encountered some compatibility issues when connected with > some PCIe EPs, these issues are probabilistic and disabling the L0s > can fix them. This sounds like either a software problem in ASPM or a hardware problem in one of the devices. If it's a Linux ASPM issue, obviously we should find and fix that. If it's an endpoint hardware issue, we should fix the driver or quirk it to avoid L0s on all platforms, not just this one. If it's a mediatek-gen3 hardware issue, we should disable L0s as you do here. But if the reason is to work around a hardware erratum, we should describe it as such. Justifying it as "L0s really doesn't save much power, so disable it" is an invitation for somebody to come back and ask why L0s doesn't work when the lspci output claims it *should* work. > Users may not be aware of these issues, so I think disabling L0s > through the driver might be the better way, since it does not > significantly save power and we usually use L1ss for power-saving > when the link is idle. Users should not need to be aware of probabilistic behavior problems related to ASPM. It's *our* problem to make sure users never see issues like that :) Bjorn
On Tue, Jan 07, 2025 at 12:44:43PM +0100, AngeloGioacchino Del Regno wrote: > Il 07/01/25 03:18, Jianjun Wang (王建军) ha scritto: > > On Fri, 2025-01-03 at 10:16 +0100, AngeloGioacchino Del Regno wrote: > > > Il 03/01/25 07:00, Jianjun Wang ha scritto: > > > > Disable ASPM L0s support because it does not significantly save > > > > power > > > > but impacts performance. > > > > > > That may be a good idea but, without numbers to support your > > > statement, it's a bit > > > difficult to say. > > > > > > How much power does ASPM L0s save on MediaTek SoCs, in microwatts? > > > How is the performance impacted, and on which specific device(s) on > > > the PCIe bus? > > > > It's hard to tell the exact number because it is difficult to measure, > > and the number of entries into the L0s state may vary even in the same > > test scenario. > > > > However, we have encountered some compatibility issues when connected > > with some PCIe EPs, and disabling the L0s can fix it. I think disabling > > L0s might be the better way, since we usually use L1ss for power-saving > > when the link is idle. > > To actually decide, we should know what's actually broken, then. > > Is the MediaTek controller broken, or is the device broken? > So, is it a MTK quirk, or a device quirk? > > If the problem is actually device-related, then this should be handled as > a device-specific quirk, as not just MediaTek platforms would be affected > by compatibility issues. > > If the MediaTek PCIe controller is at fault, instead, I agree about just > disabling L0s at the controller level - but then this shall be mentioned > in the commit message, and should have a Fixes tag as well. 100% agreed, sorry for repeating what you just said before I finished reading the thread! Bjorn
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c index ed3c0614486c..4bd3b39eebe2 100644 --- a/drivers/pci/controller/pcie-mediatek-gen3.c +++ b/drivers/pci/controller/pcie-mediatek-gen3.c @@ -84,6 +84,9 @@ #define PCIE_MSI_SET_ENABLE_REG 0x190 #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, 0) +#define PCIE_LOW_POWER_CTRL_REG 0x194 +#define PCIE_FORCE_DIS_L0S BIT(8) + #define PCIE_PIPE4_PIE8_REG 0x338 #define PCIE_K_FINETUNE_MAX GENMASK(5, 0) #define PCIE_K_FINETUNE_ERR GENMASK(7, 6) @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie) val &= ~PCIE_INTX_ENABLE; writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG); + /* + * Disable L0s support because it does not significantly save power + * but impacts performance. + */ + val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG); + val |= PCIE_FORCE_DIS_L0S; + writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG); + /* Disable DVFSRC voltage request */ val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG); val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
Disable ASPM L0s support because it does not significantly save power but impacts performance. Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> --- drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++ 1 file changed, 11 insertions(+)