* [PATCH v2 2/2] arm64: dts: rockchip: Add PCIe clkreq stuff for RK3588 EVB1
2025-10-22 11:35 [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support Shawn Lin
@ 2025-10-22 11:35 ` Shawn Lin
2025-10-22 11:52 ` Hans Zhang
2025-10-22 11:52 ` [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support Hans Zhang
2025-10-22 13:04 ` Manivannan Sadhasivam
2 siblings, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2025-10-22 11:35 UTC (permalink / raw)
To: Heiko Stuebner, Manivannan Sadhasivam, Bjorn Helgaas
Cc: linux-rockchip, Niklas Cassel, linux-pci, Shawn Lin
Add supports-clkreq and pinmux for PCIe ASPM L1 substates.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
index ff1ba5e..c9d284c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
@@ -522,6 +522,7 @@
pinctrl-names = "default";
pinctrl-0 = <&pcie2_0_rst>, <&pcie2_0_wake>, <&pcie2_0_clkreq>, <&wifi_host_wake_irq>;
reset-gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
+ supports-clkreq;
vpcie3v3-supply = <&vcc3v3_wlan>;
status = "okay";
@@ -545,7 +546,8 @@
&pcie2x1l1 {
reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
- pinctrl-0 = <&pcie2_1_rst>, <&rtl8111_isolate>;
+ pinctrl-0 = <&pcie2_1_rst>, <&rtl8111_isolate>, <&pcie30x1m1_1_clkreqn>;
+ supports-clkreq;
status = "okay";
};
@@ -555,7 +557,8 @@
&pcie3x4 {
pinctrl-names = "default";
- pinctrl-0 = <&pcie3_reset>;
+ pinctrl-0 = <&pcie3_reset>, <&pcie30x4m1_clkreqn>;
+ supports-clkreq;
reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
vpcie3v3-supply = <&vcc3v3_pcie30>;
status = "okay";
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/2] arm64: dts: rockchip: Add PCIe clkreq stuff for RK3588 EVB1
2025-10-22 11:35 ` [PATCH v2 2/2] arm64: dts: rockchip: Add PCIe clkreq stuff for RK3588 EVB1 Shawn Lin
@ 2025-10-22 11:52 ` Hans Zhang
0 siblings, 0 replies; 10+ messages in thread
From: Hans Zhang @ 2025-10-22 11:52 UTC (permalink / raw)
To: Shawn Lin, Heiko Stuebner, Manivannan Sadhasivam, Bjorn Helgaas
Cc: linux-rockchip, Niklas Cassel, linux-pci
On 10/22/2025 7:35 PM, Shawn Lin wrote:
> EXTERNAL EMAIL
>
> Add supports-clkreq and pinmux for PCIe ASPM L1 substates.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Reviewed-by: Hans Zhang <hans.zhang@cixtech.com>
Best regards,
Hans
> ---
>
> Changes in v2: None
>
> arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
> index ff1ba5e..c9d284c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
> @@ -522,6 +522,7 @@
> pinctrl-names = "default";
> pinctrl-0 = <&pcie2_0_rst>, <&pcie2_0_wake>, <&pcie2_0_clkreq>, <&wifi_host_wake_irq>;
> reset-gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
> + supports-clkreq;
> vpcie3v3-supply = <&vcc3v3_wlan>;
> status = "okay";
>
> @@ -545,7 +546,8 @@
> &pcie2x1l1 {
> reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
> pinctrl-names = "default";
> - pinctrl-0 = <&pcie2_1_rst>, <&rtl8111_isolate>;
> + pinctrl-0 = <&pcie2_1_rst>, <&rtl8111_isolate>, <&pcie30x1m1_1_clkreqn>;
> + supports-clkreq;
> status = "okay";
> };
>
> @@ -555,7 +557,8 @@
>
> &pcie3x4 {
> pinctrl-names = "default";
> - pinctrl-0 = <&pcie3_reset>;
> + pinctrl-0 = <&pcie3_reset>, <&pcie30x4m1_clkreqn>;
> + supports-clkreq;
> reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> vpcie3v3-supply = <&vcc3v3_pcie30>;
> status = "okay";
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support
2025-10-22 11:35 [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support Shawn Lin
2025-10-22 11:35 ` [PATCH v2 2/2] arm64: dts: rockchip: Add PCIe clkreq stuff for RK3588 EVB1 Shawn Lin
@ 2025-10-22 11:52 ` Hans Zhang
2025-10-22 12:22 ` Shawn Lin
2025-10-22 13:04 ` Manivannan Sadhasivam
2 siblings, 1 reply; 10+ messages in thread
From: Hans Zhang @ 2025-10-22 11:52 UTC (permalink / raw)
To: Shawn Lin, Heiko Stuebner, Manivannan Sadhasivam, Bjorn Helgaas
Cc: linux-rockchip, Niklas Cassel, linux-pci
On 10/22/2025 7:35 PM, Shawn Lin wrote:
> EXTERNAL EMAIL
>
> 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#.
>
> 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
> +
> /* 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)
> +{
> + 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 */
> + rockchip_pcie_writel_apb(rockchip, PCIE_CLKREQ_READY, PCIE_CLIENT_POWER);
> + return;
> + }
> +
> + /* Otherwise, pull down CLKREQ# and disable L1 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);
> rockchip_pcie_enable_l0s(pci);
> rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
>
> @@ -412,6 +446,8 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
> return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
> "failed to get reset lines\n");
>
> + rockchip->supports_clkreq = of_property_read_bool(pdev->dev.of_node, "supports-clkreq");
Hi Shawn,
This line exceeds 80 characters. Can it be like this?
rockchip->supports_clkreq = of_property_read_bool(pdev->dev.of_node,
"supports-clkreq");
I have no doubts about the rest.
Reviewed-by: Hans Zhang <hans.zhang@cixtech.com>
Best regards,
Hans
> +
> return 0;
> }
>
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support
2025-10-22 11:52 ` [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support Hans Zhang
@ 2025-10-22 12:22 ` Shawn Lin
2025-10-22 16:03 ` Bjorn Helgaas
0 siblings, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2025-10-22 12:22 UTC (permalink / raw)
To: Hans Zhang, Heiko Stuebner, Manivannan Sadhasivam, Bjorn Helgaas
Cc: shawn.lin, linux-rockchip, Niklas Cassel, linux-pci
在 2025/10/22 星期三 19:52, Hans Zhang 写道:
>
>
> On 10/22/2025 7:35 PM, Shawn Lin wrote:
>> EXTERNAL EMAIL
>>
>> 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#.
>>
>> 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
>> +
>> /* 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)
>> +{
>> + 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 */
>> + rockchip_pcie_writel_apb(rockchip, PCIE_CLKREQ_READY,
>> PCIE_CLIENT_POWER);
>> + return;
>> + }
>> +
>> + /* Otherwise, pull down CLKREQ# and disable L1 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);
>> rockchip_pcie_enable_l0s(pci);
>> rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
>>
>> @@ -412,6 +446,8 @@ static int rockchip_pcie_resource_get(struct
>> platform_device *pdev,
>> return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
>> "failed to get reset lines\n");
>>
>> + rockchip->supports_clkreq = of_property_read_bool(pdev-
>> >dev.of_node, "supports-clkreq");
>
> Hi Shawn,
>
> This line exceeds 80 characters. Can it be like this?
>
Thanks for the reivew.
I think we've been drop this rule[1] for quite a long time :)
[1]https://www.phoronix.com/news/Linux-Kernel-Deprecates-80-Col
> rockchip->supports_clkreq = of_property_read_bool(pdev->dev.of_node,
> "supports-clkreq");
>
> I have no doubts about the rest.
>
> Reviewed-by: Hans Zhang <hans.zhang@cixtech.com>
>
>
> Best regards,
> Hans
>
>> +
>> return 0;
>> }
>>
>> --
>> 2.7.4
>>
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support
2025-10-22 12:22 ` Shawn Lin
@ 2025-10-22 16:03 ` Bjorn Helgaas
0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Helgaas @ 2025-10-22 16:03 UTC (permalink / raw)
To: Shawn Lin
Cc: Hans Zhang, Heiko Stuebner, Manivannan Sadhasivam, Bjorn Helgaas,
linux-rockchip, Niklas Cassel, linux-pci
On Wed, Oct 22, 2025 at 08:22:39PM +0800, Shawn Lin wrote:
> 在 2025/10/22 星期三 19:52, Hans Zhang 写道:
> > On 10/22/2025 7:35 PM, 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#.
> > > @@ -412,6 +446,8 @@ static int rockchip_pcie_resource_get(struct
> > > platform_device *pdev,
> > > return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
> > > "failed to get reset lines\n");
> > >
> > > + rockchip->supports_clkreq = of_property_read_bool(pdev-
> > > >dev.of_node, "supports-clkreq");
> >
> > This line exceeds 80 characters. Can it be like this?
>
> Thanks for the reivew.
>
> I think we've been drop this rule[1] for quite a long time :)
>
> [1]https://www.phoronix.com/news/Linux-Kernel-Deprecates-80-Col
>
> > rockchip->supports_clkreq = of_property_read_bool(pdev->dev.of_node,
> > "supports-clkreq");
Yes, but that doesn't mean it's OK for the file to be a mishmash where
most of it fits in 80 but random lines go to 100. That just makes it
a pain to read.
If going past 80 makes something significantly easier to read, that's
fine, and this file does that occasionally for #defines and the like.
But in this case, wrapping to fit in 80 really doesn't hurt anything.
Bjorn
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support
2025-10-22 11:35 [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support Shawn Lin
2025-10-22 11:35 ` [PATCH v2 2/2] arm64: dts: rockchip: Add PCIe clkreq stuff for RK3588 EVB1 Shawn Lin
2025-10-22 11:52 ` [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support Hans Zhang
@ 2025-10-22 13:04 ` Manivannan Sadhasivam
2025-10-22 14:27 ` Shawn Lin
2 siblings, 1 reply; 10+ messages in thread
From: Manivannan Sadhasivam @ 2025-10-22 13:04 UTC (permalink / raw)
To: Shawn Lin
Cc: Heiko Stuebner, Bjorn Helgaas, linux-rockchip, Niklas Cassel,
linux-pci
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.
> 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?
> +
> /* 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.
> + 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.
> rockchip_pcie_enable_l0s(pci);
> rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
>
> @@ -412,6 +446,8 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
> return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
> "failed to get reset lines\n");
>
> + rockchip->supports_clkreq = of_property_read_bool(pdev->dev.of_node, "supports-clkreq");
Bjorn still likes to preserve 80 column width for most of the cases.
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support
2025-10-22 13:04 ` Manivannan Sadhasivam
@ 2025-10-22 14:27 ` Shawn Lin
2025-10-22 16:03 ` Manivannan Sadhasivam
0 siblings, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2025-10-22 14:27 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: shawn.lin, Heiko Stuebner, Bjorn Helgaas, linux-rockchip,
Niklas Cassel, linux-pci
在 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.
>
>> rockchip_pcie_enable_l0s(pci);
>> rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
>>
>> @@ -412,6 +446,8 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
>> return dev_err_probe(&pdev->dev, PTR_ERR(rockchip->rst),
>> "failed to get reset lines\n");
>>
>> + rockchip->supports_clkreq = of_property_read_bool(pdev->dev.of_node, "supports-clkreq");
>
> Bjorn still likes to preserve 80 column width for most of the cases.
Ok, will fix it.
>
> - Mani
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support
2025-10-22 14:27 ` Shawn Lin
@ 2025-10-22 16:03 ` Manivannan Sadhasivam
2025-10-23 0:39 ` Shawn Lin
0 siblings, 1 reply; 10+ messages in thread
From: Manivannan Sadhasivam @ 2025-10-22 16:03 UTC (permalink / raw)
To: Shawn Lin
Cc: Heiko Stuebner, Bjorn Helgaas, linux-rockchip, Niklas Cassel,
linux-pci
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.
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/2] PCI: dw-rockchip: Add L1sub support
2025-10-22 16:03 ` Manivannan Sadhasivam
@ 2025-10-23 0:39 ` Shawn Lin
0 siblings, 0 replies; 10+ messages in thread
From: Shawn Lin @ 2025-10-23 0:39 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: shawn.lin, Heiko Stuebner, Bjorn Helgaas, linux-rockchip,
Niklas Cassel, linux-pci
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread