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

[Hans Zhang <18255117159@163.com>]

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
>>