Re: [PATCH v2 4/6] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers

[Manivannan Sadhasivam <mani@kernel.org>]

On Tue, Nov 14, 2023 at 01:52:24PM +0300, Serge Semin wrote:
> On Tue, Nov 14, 2023 at 02:54:54PM +0900, Yoshihiro Shimoda wrote:
> > The current code calculated some dbi[2] registers' offset by calling
> > dw_pcie_ep_get_dbi[2]_offset() in each function. To improve code
> > readability, add dw_pcie_ep_{read,write}_dbi[2} and some data-width
> > related helpers.
> 
> Nice update. Thanks!
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> 
> I'll replicate my v1 nitpick regarding the accessors location here so
> the maintainers would decide whether it worth being taken into
> account.
> 
> > 
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 231 ++++++++++--------
> >  1 file changed, 129 insertions(+), 102 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 1100671db887..2b5b5b0fa7a9 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -65,24 +65,88 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep, u8 func_no
> >  	return dbi2_offset;
> >  }
> >  
> > +static u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg,
> > +			       size_t size)
> > +{
> > +	unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +	return dw_pcie_read_dbi(pci, offset + reg, size);
> > +}
> > +
> > +static void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg,
> > +				 size_t size, u32 val)
> > +{
> > +	unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no);
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +	dw_pcie_write_dbi(pci, offset + reg, size, val);
> > +}
> > +
> > +static void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg,
> > +				  size_t size, u32 val)
> > +{
> > +	unsigned int offset = dw_pcie_ep_get_dbi2_offset(ep, func_no);
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +	dw_pcie_write_dbi2(pci, offset + reg, size, val);
> > +}
> > +
> > +static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +					 u32 reg, u32 val)
> > +{
> > +	dw_pcie_ep_write_dbi(ep, func_no, reg, 0x4, val);
> > +}
> > +
> > +static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +				       u32 reg)
> > +{
> > +	return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x4);
> > +}
> > +
> > +static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +					 u32 reg, u16 val)
> > +{
> > +	dw_pcie_ep_write_dbi(ep, func_no, reg, 0x2, val);
> > +}
> > +
> > +static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +				       u32 reg)
> > +{
> > +	return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x2);
> > +}
> > +
> > +static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +					 u32 reg, u8 val)
> > +{
> > +	dw_pcie_ep_write_dbi(ep, func_no, reg, 0x1, val);
> > +}
> > +
> > +static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no,
> > +				      u32 reg)
> > +{
> > +	return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x1);
> > +}
> > +
> > +static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no,
> > +					  u32 reg, u32 val)
> > +{
> > +	dw_pcie_ep_write_dbi2(ep, func_no, reg, 0x4, val);
> > +}
> > +
> 
> My comment was:
> 
> > From: Serge Semin, Sent: Monday, November 13, 2023 9:41 PM
> > > I am not sure whether the methods above are supposed to be defined
> > > here instead of being moved to the "pcie-designware.h" header file
> > > together with dw_pcie_ep_get_dbi2_offset() and
> > > dw_pcie_ep_get_dbi_offset(). The later place seems more suitable
> > > seeing the accessors are generic, look similar to the
> > > dw_pcie_{write,read}_dbi{,2}() functions and might be useful in the
> > > platform drivers. On the other hand no LLDDs would have used it
> > > currently. So I'll leave this as a food for thoughts for the driver
> > > and subsystem maintainers.
> 
> Yoshihiro replied:
> > Perhaps, when a device driver needs to use these functions actually,
> > we can move these functions to pcie-designware.h, I think.
> 

I agree with you. Since these are read/write accessors, it would be better to
move them to the header file instead.

- Mani

-- 
மணிவண்ணன் சதாசிவம்