Re: [PATCH v2 01/13] PCI: dwc: Add dw_pcie_clear_and_set_dword() for register bit manipulation

[Hans Zhang <hans.zhang@cixtech.com>]

On 2025/6/19 12:29, Frank Li wrote:
> EXTERNAL EMAIL
> 
> On Wed, Jun 18, 2025 at 11:21:00PM +0800, Hans Zhang wrote:
>> DesignWare PCIe controller drivers implement register bit manipulation
>> through explicit read-modify-write sequences. These patterns appear
>> repeatedly across multiple drivers with minor variations, creating
>> code duplication and maintenance overhead.
>>
>> Implement dw_pcie_clear_and_set_dword() helper to encapsulate atomic
>> register modification. The function reads the current register value,
>> clears specified bits, sets new bits, and writes back the result in
>> a single operation. This abstraction hides bitwise manipulation details
>> while ensuring consistent behavior across all usage sites.
>>
>> Centralizing this logic reduces future maintenance effort when modifying
>> register access patterns and minimizes the risk of implementation
>> divergence between drivers.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware.h | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index ce9e18554e42..f401c144df0f 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -707,6 +707,17 @@ static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no,
>>        dw_pcie_ep_write_dbi2(ep, func_no, reg, 0x4, val);
>>   }
>>
>> +static inline void dw_pcie_clear_and_set_dword(struct dw_pcie *pci, int pos,
>> +                                            u32 clear, u32 set)
> 
> Can align with regmap_update_bits() argumnet?
> 
> dw_pcie_update_dbi_bits()?
> 

Dear Frank,

Thank you for your reply.


Personally, I think it should be the same as the following API. In this 
way, we can easily know the corresponding parameters and it also 
conforms to the usage habits of PCIe. What do you think?


drivers/pci/access.c

int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
					u32 clear, u32 set)
{
	int ret;
	u32 val;

	ret = pcie_capability_read_dword(dev, pos, &val);
	if (ret)
		return ret;

	val &= ~clear;
	val |= set;
	return pcie_capability_write_dword(dev, pos, val);
}
EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);


void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
				    u32 clear, u32 set)
{
	u32 val;

	pci_read_config_dword(dev, pos, &val);
	val &= ~clear;
	val |= set;
	pci_write_config_dword(dev, pos, val);
}
EXPORT_SYMBOL(pci_clear_and_set_config_dword);


Best regards,
Hans

> Frank
> 
>> +{
>> +     u32 val;
>> +
>> +     val = dw_pcie_readl_dbi(pci, pos);
>> +     val &= ~clear;
>> +     val |= set;
>> +     dw_pcie_writel_dbi(pci, pos, val);
>> +}
>> +
>>   static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci)
>>   {
>>        u32 reg;
>> --
>> 2.25.1
>>
>