[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
       [not found] <20251022191313.GA1265088@bhelgaas>
@ 2025-10-23  4:25 ` FUKAUMI Naoki
  2025-10-23  5:01   ` FUKAUMI Naoki
  2025-10-23  5:03   ` Manivannan Sadhasivam
  0 siblings, 2 replies; 4+ messages in thread
From: FUKAUMI Naoki @ 2025-10-23  4:25 UTC (permalink / raw)
  To: Bjorn Helgaas, Manivannan Sadhasivam
  Cc: Christian Zigotzky, linux-pci, linux-kernel, Bjorn Helgaas,
	linux-rockchip

# 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

^ permalink raw reply	[flat|nested] 4+ messages in thread