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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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