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

[Frank Li <Frank.li@nxp.com>]

On Thu, Jun 19, 2025 at 01:42:05PM +0800, Hans Zhang wrote:
>
>
> 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?

You are right!

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