[RESEND] Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree platforms

[RESEND] Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree platforms

[FUKAUMI Naoki]

# Fixes the ML address for linux-rockchip
# Please resend the original patch to linux-rockchip@lists.infradead.org

Hi Bjorn,

On 10/23/25 04:13, Bjorn Helgaas wrote:
> Christian, Naoki, any chance you could test this patch on top of
> v6.18-rc1 to see whether it resolves the problem you reported?
> 
> I'd like to verify that it works before merging it.

I'll be testing now. May I test on v6.18-rc2 without the following patch?

  "PCI: dw-rockchip: Prevent advertising L1 Substates support"

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> On Mon, Oct 20, 2025 at 05:12:07PM -0500, Bjorn Helgaas wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>>
>> f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree
>> platforms") enabled Clock Power Management and L1 Substates, but that
>> caused regressions because these features depend on CLKREQ#, and not all
>> devices and form factors support it.
>>
>> Enable only ASPM L0s and L1, and only when both ends of the link advertise
>> support for them.
>>
>> Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")
>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
>> Link: https://lore.kernel.org/r/db5c95a1-cf3e-46f9-8045-a1b04908051a@xenosoft.de/
>> Reported-by: FUKAUMI Naoki <naoki@radxa.com>
>> Link: https://lore.kernel.org/r/22594781424C5C98+22cb5d61-19b1-4353-9818-3bb2b311da0b@radxa.com/
>> ---
>>
>> Mani, not sure what you think we should do here.  Here's a stab at it as a
>> strawman and in case anybody can test it.
>>
>> Not sure about the message log message.  Maybe OK for testing, but might be
>> overly verbose ultimately.
>>
>> ---
>>   drivers/pci/pcie/aspm.c | 34 +++++++++-------------------------
>>   1 file changed, 9 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 7cc8281e7011..dbc74cc85bcb 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -243,8 +243,7 @@ struct pcie_link_state {
>>   	/* Clock PM state */
>>   	u32 clkpm_capable:1;		/* Clock PM capable? */
>>   	u32 clkpm_enabled:1;		/* Current Clock PM state */
>> -	u32 clkpm_default:1;		/* Default Clock PM state by BIOS or
>> -					   override */
>> +	u32 clkpm_default:1;		/* Default Clock PM state by BIOS */
>>   	u32 clkpm_disable:1;		/* Clock PM disabled */
>>   };
>>   
>> @@ -376,18 +375,6 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>>   	pcie_set_clkpm_nocheck(link, enable);
>>   }
>>   
>> -static void pcie_clkpm_override_default_link_state(struct pcie_link_state *link,
>> -						   int enabled)
>> -{
>> -	struct pci_dev *pdev = link->downstream;
>> -
>> -	/* For devicetree platforms, enable ClockPM by default */
>> -	if (of_have_populated_dt() && !enabled) {
>> -		link->clkpm_default = 1;
>> -		pci_info(pdev, "ASPM: DT platform, enabling ClockPM\n");
>> -	}
>> -}
>> -
>>   static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>>   {
>>   	int capable = 1, enabled = 1;
>> @@ -410,7 +397,6 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>>   	}
>>   	link->clkpm_enabled = enabled;
>>   	link->clkpm_default = enabled;
>> -	pcie_clkpm_override_default_link_state(link, enabled);
>>   	link->clkpm_capable = capable;
>>   	link->clkpm_disable = blacklist ? 1 : 0;
>>   }
>> @@ -811,19 +797,17 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
>>   	struct pci_dev *pdev = link->downstream;
>>   	u32 override;
>>   
>> -	/* For devicetree platforms, enable all ASPM states by default */
>> +	/* For devicetree platforms, enable L0s and L1 by default */
>>   	if (of_have_populated_dt()) {
>> -		link->aspm_default = PCIE_LINK_STATE_ASPM_ALL;
>> +		if (link->aspm_support & PCIE_LINK_STATE_L0S)
>> +			link->aspm_default |= PCIE_LINK_STATE_L0S;
>> +		if (link->aspm_support & PCIE_LINK_STATE_L1)
>> +			link->aspm_default |= PCIE_LINK_STATE_L1;
>>   		override = link->aspm_default & ~link->aspm_enabled;
>>   		if (override)
>> -			pci_info(pdev, "ASPM: DT platform, enabling%s%s%s%s%s%s%s\n",
>> -				 FLAG(override, L0S_UP, " L0s-up"),
>> -				 FLAG(override, L0S_DW, " L0s-dw"),
>> -				 FLAG(override, L1, " L1"),
>> -				 FLAG(override, L1_1, " ASPM-L1.1"),
>> -				 FLAG(override, L1_2, " ASPM-L1.2"),
>> -				 FLAG(override, L1_1_PCIPM, " PCI-PM-L1.1"),
>> -				 FLAG(override, L1_2_PCIPM, " PCI-PM-L1.2"));
>> +			pci_info(pdev, "ASPM: DT platform, enabling%s%s\n",
>> +				 FLAG(override, L0S, " L0s"),
>> +				 FLAG(override, L1, " L1"));
>>   	}
>>   }
>>   
>> -- 
>> 2.43.0
>>
> 



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [RESEND] Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree platforms

[FUKAUMI Naoki]

On 10/23/25 13:25, FUKAUMI Naoki wrote:
> # Fixes the ML address for linux-rockchip
> # Please resend the original patch to linux-rockchip@lists.infradead.org

and linuxppc-dev@lists.ozlabs.org?

> Hi Bjorn,
> 
> On 10/23/25 04:13, Bjorn Helgaas wrote:
>> Christian, Naoki, any chance you could test this patch on top of
>> v6.18-rc1 to see whether it resolves the problem you reported?
>>
>> I'd like to verify that it works before merging it.
> 
> I'll be testing now. May I test on v6.18-rc2 without the following patch?
> 
>   "PCI: dw-rockchip: Prevent advertising L1 Substates support"
> 
> Best regards,
> 
> -- 
> FUKAUMI Naoki
> Radxa Computer (Shenzhen) Co., Ltd.
> 
>> On Mon, Oct 20, 2025 at 05:12:07PM -0500, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for 
>>> devicetree
>>> platforms") enabled Clock Power Management and L1 Substates, but that
>>> caused regressions because these features depend on CLKREQ#, and not all
>>> devices and form factors support it.
>>>
>>> Enable only ASPM L0s and L1, and only when both ends of the link 
>>> advertise
>>> support for them.
>>>
>>> Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states 
>>> for devicetree platforms")
>>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
>>> Link: https://lore.kernel.org/r/db5c95a1-cf3e-46f9-8045- 
>>> a1b04908051a@xenosoft.de/
>>> Reported-by: FUKAUMI Naoki <naoki@radxa.com>
>>> Link: https://lore.kernel.org/ 
>>> r/22594781424C5C98+22cb5d61-19b1-4353-9818-3bb2b311da0b@radxa.com/

and https://lore.kernel.org/all/DDIW7ZP5K1VR.2I7VW56B9CZLF@cknow-tech.com/

maybe https://lore.kernel.org/all/20251015101304.3ec03e6b@bootlin.com/
(then +Cc: linux-arm-kernel@lists.infradead.org?)

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

>>> ---
>>>
>>> Mani, not sure what you think we should do here.  Here's a stab at it 
>>> as a
>>> strawman and in case anybody can test it.
>>>
>>> Not sure about the message log message.  Maybe OK for testing, but 
>>> might be
>>> overly verbose ultimately.
>>>
>>> ---
>>>   drivers/pci/pcie/aspm.c | 34 +++++++++-------------------------
>>>   1 file changed, 9 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index 7cc8281e7011..dbc74cc85bcb 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -243,8 +243,7 @@ struct pcie_link_state {
>>>       /* Clock PM state */
>>>       u32 clkpm_capable:1;        /* Clock PM capable? */
>>>       u32 clkpm_enabled:1;        /* Current Clock PM state */
>>> -    u32 clkpm_default:1;        /* Default Clock PM state by BIOS or
>>> -                       override */
>>> +    u32 clkpm_default:1;        /* Default Clock PM state by BIOS */
>>>       u32 clkpm_disable:1;        /* Clock PM disabled */
>>>   };
>>> @@ -376,18 +375,6 @@ static void pcie_set_clkpm(struct 
>>> pcie_link_state *link, int enable)
>>>       pcie_set_clkpm_nocheck(link, enable);
>>>   }
>>> -static void pcie_clkpm_override_default_link_state(struct 
>>> pcie_link_state *link,
>>> -                           int enabled)
>>> -{
>>> -    struct pci_dev *pdev = link->downstream;
>>> -
>>> -    /* For devicetree platforms, enable ClockPM by default */
>>> -    if (of_have_populated_dt() && !enabled) {
>>> -        link->clkpm_default = 1;
>>> -        pci_info(pdev, "ASPM: DT platform, enabling ClockPM\n");
>>> -    }
>>> -}
>>> -
>>>   static void pcie_clkpm_cap_init(struct pcie_link_state *link, int 
>>> blacklist)
>>>   {
>>>       int capable = 1, enabled = 1;
>>> @@ -410,7 +397,6 @@ static void pcie_clkpm_cap_init(struct 
>>> pcie_link_state *link, int blacklist)
>>>       }
>>>       link->clkpm_enabled = enabled;
>>>       link->clkpm_default = enabled;
>>> -    pcie_clkpm_override_default_link_state(link, enabled);
>>>       link->clkpm_capable = capable;
>>>       link->clkpm_disable = blacklist ? 1 : 0;
>>>   }
>>> @@ -811,19 +797,17 @@ static void 
>>> pcie_aspm_override_default_link_state(struct pcie_link_state *link)
>>>       struct pci_dev *pdev = link->downstream;
>>>       u32 override;
>>> -    /* For devicetree platforms, enable all ASPM states by default */
>>> +    /* For devicetree platforms, enable L0s and L1 by default */
>>>       if (of_have_populated_dt()) {
>>> -        link->aspm_default = PCIE_LINK_STATE_ASPM_ALL;
>>> +        if (link->aspm_support & PCIE_LINK_STATE_L0S)
>>> +            link->aspm_default |= PCIE_LINK_STATE_L0S;
>>> +        if (link->aspm_support & PCIE_LINK_STATE_L1)
>>> +            link->aspm_default |= PCIE_LINK_STATE_L1;
>>>           override = link->aspm_default & ~link->aspm_enabled;
>>>           if (override)
>>> -            pci_info(pdev, "ASPM: DT platform, 
>>> enabling%s%s%s%s%s%s%s\n",
>>> -                 FLAG(override, L0S_UP, " L0s-up"),
>>> -                 FLAG(override, L0S_DW, " L0s-dw"),
>>> -                 FLAG(override, L1, " L1"),
>>> -                 FLAG(override, L1_1, " ASPM-L1.1"),
>>> -                 FLAG(override, L1_2, " ASPM-L1.2"),
>>> -                 FLAG(override, L1_1_PCIPM, " PCI-PM-L1.1"),
>>> -                 FLAG(override, L1_2_PCIPM, " PCI-PM-L1.2"));
>>> +            pci_info(pdev, "ASPM: DT platform, enabling%s%s\n",
>>> +                 FLAG(override, L0S, " L0s"),
>>> +                 FLAG(override, L1, " L1"));
>>>       }
>>>   }
>>> -- 
>>> 2.43.0
>>>
>>
> 
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [RESEND] Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree platforms

[Herve Codina]

Hi,

On Thu, 23 Oct 2025 14:01:59 +0900
FUKAUMI Naoki <naoki@radxa.com> wrote:

...

> 
> maybe https://lore.kernel.org/all/20251015101304.3ec03e6b@bootlin.com/
> (then +Cc: linux-arm-kernel@lists.infradead.org?)
> 

For information, I tested the patch and I can confirm that my issue is
still there with the patch applied.

The only way to fix it seems to either fully disable ASPM or move to
performance ASPM policy.

Maybe my issue is simply a broken ASPM either on my host or my endpoint.

Best regards,
Hervé

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [RESEND] Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree platforms

[Manivannan Sadhasivam]

On Thu, Oct 23, 2025 at 01:25:17PM +0900, FUKAUMI Naoki wrote:
> # Fixes the ML address for linux-rockchip
> # Please resend the original patch to linux-rockchip@lists.infradead.org
> 
> Hi Bjorn,
> 
> On 10/23/25 04:13, Bjorn Helgaas wrote:
> > Christian, Naoki, any chance you could test this patch on top of
> > v6.18-rc1 to see whether it resolves the problem you reported?
> > 
> > I'd like to verify that it works before merging it.
> 
> I'll be testing now. May I test on v6.18-rc2 without the following patch?
> 
>  "PCI: dw-rockchip: Prevent advertising L1 Substates support"
> 

Yes, you should revert the above mentioned commit if you have applied it. It
will remove the L1ss CAP altogether making *this* patch irrelevant.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip