[PATCH 3/3] PCI: dw-rockchip: Unify link status checks with FIELD_GET
Hans Zhang
18255117159 at 163.com
Tue Apr 22 05:29:33 PDT 2025
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 at 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 at 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
More information about the Linux-rockchip
mailing list