Link-up detection manually checked PCIE_LINKUP bits across RC/EP modes,
leading to code duplication. Centralize the logic using FIELD_GET. This
removes redundancy and abstracts hardware-specific bit masking, ensuring
consistent link state handling.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index cdc8afc6cfc1..2b26060af5c2 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -196,10 +196,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
u32 val = rockchip_pcie_get_ltssm(rockchip);
- if ((val & PCIE_LINKUP) == PCIE_LINKUP)
- return 1;
-
- return 0;
+ return FIELD_GET(PCIE_LINKUP_MASK, val) == 3;
}
static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
@@ -499,7 +496,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
struct dw_pcie *pci = &rockchip->pci;
struct dw_pcie_rp *pp = &pci->pp;
struct device *dev = pci->dev;
- u32 reg, val;
+ u32 reg;
reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
@@ -508,8 +505,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
if (reg & PCIE_RDLH_LINK_UP_CHGED) {
- val = rockchip_pcie_get_ltssm(rockchip);
- if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
+ if (rockchip_pcie_link_up(pci)) {
dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
/* Rescan the bus to enumerate endpoint devices */
pci_lock_rescan_remove();
@@ -526,7 +522,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
struct rockchip_pcie *rockchip = arg;
struct dw_pcie *pci = &rockchip->pci;
struct device *dev = pci->dev;
- u32 reg, val;
+ u32 reg;
reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
@@ -540,8 +536,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
}
if (reg & PCIE_RDLH_LINK_UP_CHGED) {
- val = rockchip_pcie_get_ltssm(rockchip);
- if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
+ if (rockchip_pcie_link_up(pci)) {
dev_dbg(dev, "link up\n");
dw_pcie_ep_linkup(&pci->ep);
}
--
2.25.1On Tue, Apr 22, 2025 at 07:28:30PM +0800, Hans Zhang wrote:
> Link-up detection manually checked PCIE_LINKUP bits across RC/EP modes,
> leading to code duplication. Centralize the logic using FIELD_GET. This
> removes redundancy and abstracts hardware-specific bit masking, ensuring
> consistent link state handling.
>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index cdc8afc6cfc1..2b26060af5c2 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -196,10 +196,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
> struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> u32 val = rockchip_pcie_get_ltssm(rockchip);
>
> - if ((val & PCIE_LINKUP) == PCIE_LINKUP)
> - return 1;
> -
> - return 0;
> + return FIELD_GET(PCIE_LINKUP_MASK, val) == 3;
While I like the idea of your patch, here you are replacing something that
is easy to read (PCIE_LINKUP) with a magic value, which IMO is a step in
the wrong direction.
> }
>
> static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
> @@ -499,7 +496,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
> struct dw_pcie *pci = &rockchip->pci;
> struct dw_pcie_rp *pp = &pci->pp;
> struct device *dev = pci->dev;
> - u32 reg, val;
> + u32 reg;
>
> reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
> rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> @@ -508,8 +505,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
> dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
>
> if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> - val = rockchip_pcie_get_ltssm(rockchip);
> - if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> + if (rockchip_pcie_link_up(pci)) {
> dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
> /* Rescan the bus to enumerate endpoint devices */
> pci_lock_rescan_remove();
> @@ -526,7 +522,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> struct rockchip_pcie *rockchip = arg;
> struct dw_pcie *pci = &rockchip->pci;
> struct device *dev = pci->dev;
> - u32 reg, val;
> + u32 reg;
>
> reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
> rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> @@ -540,8 +536,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> }
>
> if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> - val = rockchip_pcie_get_ltssm(rockchip);
> - if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> + if (rockchip_pcie_link_up(pci)) {
> dev_dbg(dev, "link up\n");
> dw_pcie_ep_linkup(&pci->ep);
> }
> --
> 2.25.1
>
On 2025/4/22 19:39, Niklas Cassel wrote:
> On Tue, Apr 22, 2025 at 07:28:30PM +0800, Hans Zhang wrote:
>> Link-up detection manually checked PCIE_LINKUP bits across RC/EP modes,
>> leading to code duplication. Centralize the logic using FIELD_GET. This
>> removes redundancy and abstracts hardware-specific bit masking, ensuring
>> consistent link state handling.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 15 +++++----------
>> 1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> index cdc8afc6cfc1..2b26060af5c2 100644
>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> @@ -196,10 +196,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
>> struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
>> u32 val = rockchip_pcie_get_ltssm(rockchip);
>>
>> - if ((val & PCIE_LINKUP) == PCIE_LINKUP)
>> - return 1;
>> -
>> - return 0;
>> + return FIELD_GET(PCIE_LINKUP_MASK, val) == 3;
>
> While I like the idea of your patch, here you are replacing something that
> is easy to read (PCIE_LINKUP) with a magic value, which IMO is a step in
> the wrong direction.
>
Hi Niklas,
Thank you very much for your reply. How about I add another macro
definition?
#define PCIE_LINKUP 3
If not, I'll restore it. At least the following can use the
rockchip_pcie_get_ltssm function to avoid duplicate code.
Best regards,
Hans
>
>> }
>>
>> static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
>> @@ -499,7 +496,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
>> struct dw_pcie *pci = &rockchip->pci;
>> struct dw_pcie_rp *pp = &pci->pp;
>> struct device *dev = pci->dev;
>> - u32 reg, val;
>> + u32 reg;
>>
>> reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
>> rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
>> @@ -508,8 +505,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
>> dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
>>
>> if (reg & PCIE_RDLH_LINK_UP_CHGED) {
>> - val = rockchip_pcie_get_ltssm(rockchip);
>> - if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
>> + if (rockchip_pcie_link_up(pci)) {
>> dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
>> /* Rescan the bus to enumerate endpoint devices */
>> pci_lock_rescan_remove();
>> @@ -526,7 +522,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
>> struct rockchip_pcie *rockchip = arg;
>> struct dw_pcie *pci = &rockchip->pci;
>> struct device *dev = pci->dev;
>> - u32 reg, val;
>> + u32 reg;
>>
>> reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
>> rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
>> @@ -540,8 +536,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
>> }
>>
>> if (reg & PCIE_RDLH_LINK_UP_CHGED) {
>> - val = rockchip_pcie_get_ltssm(rockchip);
>> - if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
>> + if (rockchip_pcie_link_up(pci)) {
>> dev_dbg(dev, "link up\n");
>> dw_pcie_ep_linkup(&pci->ep);
>> }
>> --
>> 2.25.1
>>
On Tue, Apr 22, 2025 at 07:50:50PM +0800, Hans Zhang wrote: > > > On 2025/4/22 19:39, Niklas Cassel wrote: > > On Tue, Apr 22, 2025 at 07:28:30PM +0800, Hans Zhang wrote: > > > Link-up detection manually checked PCIE_LINKUP bits across RC/EP modes, > > > leading to code duplication. Centralize the logic using FIELD_GET. This > > > removes redundancy and abstracts hardware-specific bit masking, ensuring > > > consistent link state handling. > > > > > > Signed-off-by: Hans Zhang <18255117159@163.com> > > > --- > > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 15 +++++---------- > > > 1 file changed, 5 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > > index cdc8afc6cfc1..2b26060af5c2 100644 > > > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > > @@ -196,10 +196,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci) > > > struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > > > u32 val = rockchip_pcie_get_ltssm(rockchip); > > > - if ((val & PCIE_LINKUP) == PCIE_LINKUP) > > > - return 1; > > > - > > > - return 0; > > > + return FIELD_GET(PCIE_LINKUP_MASK, val) == 3; > > > > While I like the idea of your patch, here you are replacing something that > > is easy to read (PCIE_LINKUP) with a magic value, which IMO is a step in > > the wrong direction. > > > > Hi Niklas, > > Thank you very much for your reply. How about I add another macro > definition? > > #define PCIE_LINKUP 3 Yes, adding another macro for it is what I meant. Kind regards, Niklas
On 2025/4/22 20:24, Niklas Cassel wrote:
> On Tue, Apr 22, 2025 at 07:50:50PM +0800, Hans Zhang wrote:
>>
>>
>> On 2025/4/22 19:39, Niklas Cassel wrote:
>>> On Tue, Apr 22, 2025 at 07:28:30PM +0800, Hans Zhang wrote:
>>>> Link-up detection manually checked PCIE_LINKUP bits across RC/EP modes,
>>>> leading to code duplication. Centralize the logic using FIELD_GET. This
>>>> removes redundancy and abstracts hardware-specific bit masking, ensuring
>>>> consistent link state handling.
>>>>
>>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>>> ---
>>>> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 15 +++++----------
>>>> 1 file changed, 5 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> index cdc8afc6cfc1..2b26060af5c2 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> @@ -196,10 +196,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
>>>> struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
>>>> u32 val = rockchip_pcie_get_ltssm(rockchip);
>>>> - if ((val & PCIE_LINKUP) == PCIE_LINKUP)
>>>> - return 1;
>>>> -
>>>> - return 0;
>>>> + return FIELD_GET(PCIE_LINKUP_MASK, val) == 3;
>>>
>>> While I like the idea of your patch, here you are replacing something that
>>> is easy to read (PCIE_LINKUP) with a magic value, which IMO is a step in
>>> the wrong direction.
>>>
>>
>> Hi Niklas,
>>
>> Thank you very much for your reply. How about I add another macro
>> definition?
>>
>> #define PCIE_LINKUP 3
>
> Yes, adding another macro for it is what I meant.
>
Hi Niklas,
Thank you very much for your reply and reminder. The second and third
patches will be changed as follows:
patch 0002:
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index fd5827bbfae3..4d5c8ed18953 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -34,30 +34,37 @@
#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev)
-#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40)
-#define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0)
-#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc)
-#define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8)
-#define PCIE_CLIENT_INTR_STATUS_MSG_RX 0x04
+#define PCIE_CLIENT_GENERAL_CONTROL 0x0
+#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40)
+#define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0)
+#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc)
+#define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8)
+
+#define PCIE_CLIENT_INTR_STATUS_MSG_RX 0x4
+#define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8
+
#define PCIE_CLIENT_INTR_STATUS_MISC 0x10
+#define PCIE_RDLH_LINK_UP_CHGED BIT(1)
+#define PCIE_LINK_REQ_RST_NOT_INT BIT(2)
+
+#define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c
#define PCIE_CLIENT_INTR_MASK_MISC 0x24
+
#define PCIE_CLIENT_POWER 0x2c
+#define PME_READY_ENTER_L23 BIT(3)
+
#define PCIE_CLIENT_MSG_GEN 0x34
-#define PME_READY_ENTER_L23 BIT(3)
-#define PME_TURN_OFF (BIT(4) | BIT(20))
-#define PME_TO_ACK (BIT(9) | BIT(25))
-#define PCIE_SMLH_LINKUP BIT(16)
-#define PCIE_RDLH_LINKUP BIT(17)
-#define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
-#define PCIE_RDLH_LINK_UP_CHGED BIT(1)
-#define PCIE_LINK_REQ_RST_NOT_INT BIT(2)
-#define PCIE_CLIENT_GENERAL_CONTROL 0x0
-#define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8
-#define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c
+#define PME_TURN_OFF HIWORD_UPDATE_BIT(BIT(4))
+#define PME_TO_ACK HIWORD_UPDATE_BIT(BIT(9))
+
#define PCIE_CLIENT_HOT_RESET_CTRL 0x180
+#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4)
+
#define PCIE_CLIENT_LTSSM_STATUS 0x300
-#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4)
-#define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0)
+#define PCIE_SMLH_LINKUP BIT(16)
+#define PCIE_RDLH_LINKUP BIT(17)
+#define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
+#define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0)
struct rockchip_pcie {
struct dw_pcie pci;
patch 0003:
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 4d5c8ed18953..0553e7e80e1b 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -8,6 +8,7 @@
* Author: Simon Xue <xxm@rock-chips.com>
*/
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/gpio/consumer.h>
#include <linux/irqchip/chained_irq.h>
@@ -61,9 +62,8 @@
#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4)
#define PCIE_CLIENT_LTSSM_STATUS 0x300
-#define PCIE_SMLH_LINKUP BIT(16)
-#define PCIE_RDLH_LINKUP BIT(17)
-#define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
+#define PCIE_LINKUP 0x3
+#define PCIE_LINKUP_MASK GENMASK(17, 16)
#define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0)
struct rockchip_pcie {
@@ -197,10 +197,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
u32 val = rockchip_pcie_get_ltssm(rockchip);
- if ((val & PCIE_LINKUP) == PCIE_LINKUP)
- return 1;
-
- return 0;
+ return FIELD_GET(PCIE_LINKUP_MASK, val) == PCIE_LINKUP;
}
static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
@@ -500,7 +497,7 @@ static irqreturn_t
rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
struct dw_pcie *pci = &rockchip->pci;
struct dw_pcie_rp *pp = &pci->pp;
struct device *dev = pci->dev;
- u32 reg, val;
+ u32 reg;
reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
@@ -509,8 +506,7 @@ static irqreturn_t
rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
if (reg & PCIE_RDLH_LINK_UP_CHGED) {
- val = rockchip_pcie_get_ltssm(rockchip);
- if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
+ if (rockchip_pcie_link_up(pci)) {
dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
/* Rescan the bus to enumerate endpoint devices */
pci_lock_rescan_remove();
@@ -527,7 +523,7 @@ static irqreturn_t
rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
struct rockchip_pcie *rockchip = arg;
struct dw_pcie *pci = &rockchip->pci;
struct device *dev = pci->dev;
- u32 reg, val;
+ u32 reg;
reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
@@ -541,8 +537,7 @@ static irqreturn_t
rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
}
if (reg & PCIE_RDLH_LINK_UP_CHGED) {
- val = rockchip_pcie_get_ltssm(rockchip);
- if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
+ if (rockchip_pcie_link_up(pci)) {
dev_dbg(dev, "link up\n");
dw_pcie_ep_linkup(&pci->ep);
}
Best regards,
Hans
© 2016 - 2026 Red Hat, Inc.