Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support

[Shawn Lin <shawn.lin@rock-chips.com>]

Hi Mani

在 2025/10/23 星期四 0:03, Manivannan Sadhasivam 写道:
> On Wed, Oct 22, 2025 at 10:27:39PM +0800, Shawn Lin wrote:
>>
>> 在 2025/10/22 星期三 21:04, Manivannan Sadhasivam 写道:
>>> On Wed, Oct 22, 2025 at 07:35:53PM +0800, Shawn Lin wrote:
>>>> The driver should set app_clk_req_n(clkreq ready) of PCIE_CLIENT_POWER reg
>>>> to support L1sub. Otherwise, unset app_clk_req_n and pull down CLKREQ#.
>>>>
>>>
>>> You can definitely improve the commit message on explaining why L1 PM Substates
>>> need to be disabled when the DT property is not present etc... Please refer the
>>> patch from Niklas.
>>
>> Will do.
>>
>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - drop of_pci_clkreq_presnt API
>>>> - drop dependency of Niklas's patch
>>>>
>>>>    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 36 +++++++++++++++++++++++++++
>>>>    1 file changed, 36 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> index 3e2752c..18cd626 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> @@ -62,6 +62,12 @@
>>>>    /* Interrupt Mask Register Related to Miscellaneous Operation */
>>>>    #define PCIE_CLIENT_INTR_MASK_MISC	0x24
>>>> +/* Power Management Control Register */
>>>> +#define PCIE_CLIENT_POWER		0x2c
>>>> +#define  PCIE_CLKREQ_READY		0x10001
>>>> +#define  PCIE_CLKREQ_NOT_READY		0x10000
>>>> +#define  PCIE_CLKREQ_PULL_DOWN		0x30001000
>>>
>>> Can you use bitfields instead of magic values?
>>
>> Of course, will fix.
>>
>>>
>>>> +
>>>>    /* Hot Reset Control Register */
>>>>    #define PCIE_CLIENT_HOT_RESET_CTRL	0x180
>>>>    #define  PCIE_LTSSM_APP_DLY2_EN		BIT(1)
>>>> @@ -85,6 +91,7 @@ struct rockchip_pcie {
>>>>    	struct regulator *vpcie3v3;
>>>>    	struct irq_domain *irq_domain;
>>>>    	const struct rockchip_pcie_of_data *data;
>>>> +	bool supports_clkreq;
>>>>    };
>>>>    struct rockchip_pcie_of_data {
>>>> @@ -200,6 +207,31 @@ static bool rockchip_pcie_link_up(struct dw_pcie *pci)
>>>>    	return FIELD_GET(PCIE_LINKUP_MASK, val) == PCIE_LINKUP;
>>>>    }
>>>> +static void rockchip_pcie_enable_l1sub(struct dw_pcie *pci)
>>>
>>> rockchip_pcie_configure_l1sub()? since this function is not just enabling L1ss.
>>>
>>>> +{
>>>> +	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
>>>> +	u32 cap, l1subcap;
>>>> +
>>>> +	/* Enable L1 substates if CLKREQ# is properly connected */
>>>> +	if (rockchip->supports_clkreq) {
>>>> +		/* Ready to have reference clock removed */
>>>
>>> This comment is misleading (maybe wrong). The presence of this property implies
>>> that the link could enter L1 PM Substates. REFCLK removal only happens when the
>>> link is in L1ss.
>>>
>>> So drop the comment.
>>
>> Will drop.
>>
>>>
>>>> +		rockchip_pcie_writel_apb(rockchip, PCIE_CLKREQ_READY, PCIE_CLIENT_POWER);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	/* Otherwise, pull down CLKREQ# and disable L1 substates */
>>>
>>> "L1 PM Substates"
>>>
>>>> +	rockchip_pcie_writel_apb(rockchip, PCIE_CLKREQ_PULL_DOWN | PCIE_CLKREQ_NOT_READY,
>>>> +				 PCIE_CLIENT_POWER);
>>>> +	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
>>>> +	if (cap) {
>>>> +		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
>>>> +		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
>>>> +			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
>>>> +			      PCI_L1SS_CAP_PCIPM_L1_2);
>>>> +		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
>>>> +	}
>>>> +}
>>>> +
>>>>    static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
>>>>    {
>>>>    	u32 cap, lnkcap;
>>>> @@ -264,6 +296,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>>>>    	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>>>>    					 rockchip);
>>>> +	rockchip_pcie_enable_l1sub(pci);
>>>>    	rockchip_pcie_enable_l0s(pci);
>>>>    	return 0;
>>>> @@ -301,6 +334,7 @@ static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep)
>>>>    	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>>    	enum pci_barno bar;
>>>> +	rockchip_pcie_enable_l1sub(pci);
>>>
>>> I don't think you can decide the CLKREQ# routing on the EP side. The
>>> 'supports-clkreq' property is meant only for the RC afaik.
>>
>> You are right, we cannot decide the CLKREQ# routing on the EP side. But
>> what I have in mind is we at least need to set pinmux to CLKREQ function
>> because on Rockchip platforms, the CLKREQ# of EP mode could also be used
>> as GPIO or other funcions if guys never need L1ss to be supported.
>>
> 
> You don't need to configure pinmux in the driver manually. If the platform
> desginer knows that CLKREQ# is not used in the endpoint, then the corresponding
> pinmux state can be set to non-CLKREQ# function in DT.
> 
> I was assuming that CLKREQ# assert/deassert is handled by the endpoint
> controller hw without endpoint software intervention.

Ok, I got your point now. For the EP part, I'll drop L1ss support after
more internal disscussion as it also need more things to do, e.g,
trigger revelent L1.x irq to do something to actually save power...etc.

> 
> - Mani
>