Re: [PATCH 3/3] PCI: dw-rockchip: Unify link status checks with FIELD_GET

[Hans Zhang <18255117159@163.com>]

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip