Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

AngeloGioacchino Del Regno
2025-01-07T11:44:46Z

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;
>>
>>