Hello Manivannan,
> From: Manivannan Sadhasivam, Sent: Friday, November 17, 2023 6:19 PM
>
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > > ---
> > > .../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.
Thank you for your comment. And, I'm sorry for delayed the response.
I completely overlooked this email... I'll move them to the header file on v3.
Best regards,
Yoshihiro Shimoda
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
