Re: [PATCH v4 02/13] PCI: dwc: Refactor code by using dw_pcie_clear_and_set_dword()

[Hans Zhang <18255117159@163.com>]

On 2025/8/27 21:51, Manivannan Sadhasivam wrote:
> On Wed, Aug 13, 2025 at 12:45:20PM GMT, Hans Zhang wrote:
>> DesignWare core modules contain multiple instances of manual
>> read-modify-write operations for register bit manipulation.
>> These patterns duplicate functionality now provided by
>> dw_pcie_clear_and_set_dword(), particularly in debugfs, endpoint,
>> host, and core initialization paths.
>>
>> Replace open-coded bit manipulation sequences with calls to
>> dw_pcie_clear_and_set_dword(). Affected areas include debugfs register
>> control, endpoint capability configuration, host setup routines, and
>> core link initialization logic. The changes simplify power management
>> handling, capability masking, and feature configuration.
>>
>> Standardizing on the helper function reduces code duplication by ~140
>> lines across core modules while improving readability. The refactoring
>> also ensures consistent error handling for register operations and
>> provides a single point of control for future bit manipulation logi
>> updates.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   .../controller/dwc/pcie-designware-debugfs.c  | 66 +++++++---------
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 20 +++--
>>   .../pci/controller/dwc/pcie-designware-host.c | 26 +++----
>>   drivers/pci/controller/dwc/pcie-designware.c  | 75 +++++++------------
>>   drivers/pci/controller/dwc/pcie-designware.h  | 18 +----
>>   5 files changed, 76 insertions(+), 129 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
>> index 0fbf86c0b97e..ff185b8977f3 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-debugfs.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
>> @@ -213,10 +213,8 @@ static ssize_t lane_detect_write(struct file *file, const char __user *buf,
>>   	if (val)
>>   		return val;
>>   
>> -	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
>> -	val &= ~(LANE_SELECT);
>> -	val |= FIELD_PREP(LANE_SELECT, lane);
>> -	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG, val);
>> +	dw_pcie_clear_and_set_dword(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG,
>> +				    LANE_SELECT, FIELD_PREP(LANE_SELECT, lane));
>>   
>>   	return count;
>>   }
>> @@ -309,12 +307,11 @@ static void set_event_number(struct dwc_pcie_rasdes_priv *pdata,
>>   {
>>   	u32 val;
>>   
>> -	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
>> -	val &= ~EVENT_COUNTER_ENABLE;
>> -	val &= ~(EVENT_COUNTER_GROUP_SELECT | EVENT_COUNTER_EVENT_SELECT);
>> -	val |= FIELD_PREP(EVENT_COUNTER_GROUP_SELECT, event_list[pdata->idx].group_no);
>> +	val = FIELD_PREP(EVENT_COUNTER_GROUP_SELECT, event_list[pdata->idx].group_no);
>>   	val |= FIELD_PREP(EVENT_COUNTER_EVENT_SELECT, event_list[pdata->idx].event_no);
>> -	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
>> +	dw_pcie_clear_and_set_dword(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG,
>> +				    EVENT_COUNTER_ENABLE | EVENT_COUNTER_GROUP_SELECT |
>> +				    EVENT_COUNTER_EVENT_SELECT, val);
>>   }
>>   
>>   static ssize_t counter_enable_read(struct file *file, char __user *buf,
>> @@ -354,13 +351,10 @@ static ssize_t counter_enable_write(struct file *file, const char __user *buf,
>>   
>>   	mutex_lock(&rinfo->reg_event_lock);
>>   	set_event_number(pdata, pci, rinfo);
>> -	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
>> -	if (enable)
>> -		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_ON);
>> -	else
>> -		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_OFF);
>>   
>> -	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
>> +	val |= FIELD_PREP(EVENT_COUNTER_ENABLE, enable ? PER_EVENT_ON : PER_EVENT_OFF);
> 
> So you just added the bitfields to the existing 'val' variable which was storing
> the return value of kstrtou32_from_user().
> 

Dear Mani,

Thank you very much for your reply and for taking the time to point out 
the problem. I double-checked today, identified the problem and will fix 
it in the next version.


> What makes me nervous about this series is these kind of subtle bugs. It is
> really hard to spot all with too many drivers/changes :/
> 
> I would suggest you to just convert whatever drivers you can test with and leave
> the rest to platforms maintainers to convert later. I do not want to regress
> platforms for cleanups.

My original intention was to hope that the code would become 
increasingly concise and that a lot of repetitive code would be removed. 
I also believe it will provide convenience for the subsequent 
development. I hope there won't be any problems with my next version.

> 
>> +	dw_pcie_clear_and_set_dword(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG,
>> +				    0, val);
> 
> Similar to what Lukas suggested here: https://lore.kernel.org/linux-pci/aKDpIeQgt7I9Ts8F@wunner.de
> 
> Please use separate API for just setting the word instead of passing 0 to this
> API.
> 

Will add dw_pcie_clear_dword and dw_pcie_set_dword.

Best regards,
Hans

> - Mani
>