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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Serge, Manivannan,

> From: Manivannan Sadhasivam, Sent: Monday, December 18, 2023 1:33 AM
> 
> On Fri, Dec 15, 2023 at 12:51:28PM +0300, Serge Semin wrote:
> > Hi Yoshihiro
> >
> > On Fri, Dec 15, 2023 at 11:29:53AM +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.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > > ---
> > >  .../pci/controller/dwc/pcie-designware-ep.c   | 184 ++++++------------
> > >  drivers/pci/controller/dwc/pcie-designware.h  |  93 +++++++++
> > >  2 files changed, 153 insertions(+), 124 deletions(-)
> > >
<snip>
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 5e36da166ffe..b92e69041fe8 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -534,6 +534,99 @@ static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci)
> > >  	return (enum dw_pcie_ltssm)FIELD_GET(PORT_LOGIC_LTSSM_STATE_MASK, val);
> > >  }
> > >
> >
> > > +static inline unsigned int dw_pcie_ep_get_dbi_offset(struct dw_pcie_ep *ep,
> > > +						     u8 func_no)
> > > +{
> > > +	unsigned int dbi_offset = 0;
> > > +
> > > +	if (ep->ops->get_dbi_offset)
> > > +		dbi_offset = ep->ops->get_dbi_offset(ep, func_no);
> > > +
> > > +	return dbi_offset;
> > > +}
> > > +
> > > +static inline unsigned int dw_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep,
> > > +						      u8 func_no)
> > > +{
> > > +	unsigned int dbi2_offset = 0;
> > > +
> > > +	if (ep->ops->get_dbi2_offset)
> > > +		dbi2_offset = ep->ops->get_dbi2_offset(ep, func_no);
> > > +	else if (ep->ops->get_dbi_offset)     /* for backward compatibility */
> > > +		dbi2_offset = ep->ops->get_dbi_offset(ep, func_no);
> > > +
> > > +	return dbi2_offset;
> > > +}
> > > +
> > > +static inline 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 inline 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 inline 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);
> > > +}
> > > +
> >
> > A tiny nitpick. Since these are CSRs accessors it would be better for
> > readability to have them grouped with the rest of the IO-accessors
> > dw_pcie_writel_dbi()..dw_pcie_writel_dbi2(). Particularly have them
> > defined below the already available ones. So first normal
> > DBI-accessors would be placed and the EP-specific DBI-accessors
> > afterwords. Not sure whether it's that much required. So it's up to
> > Mani to decide. Perhaps the subsystem maintainers could fix it on
> > merge in? Bjorn, Krzysztof, Lorenzo?
> >
> 
> +1

Thank you for your comment and a vote.
To be honest, I don't understand what grouping is better for readability...
Anyway, perhaps, I should modify the header file as v4 patches. On v3,
the IO-accessors are the following:
---
static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
static inline u32 dw_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
static inline void dw_pcie_writew_dbi(struct dw_pcie *pci, u32 reg, u16 val)
static inline u16 dw_pcie_readw_dbi(struct dw_pcie *pci, u32 reg)
static inline void dw_pcie_writeb_dbi(struct dw_pcie *pci, u32 reg, u8 val)
static inline u8 dw_pcie_readb_dbi(struct dw_pcie *pci, u32 reg)
static inline void dw_pcie_writel_dbi2(struct dw_pcie *pci, u32 reg, u32 val)

static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci) // not IO-accessors
static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) // not IO-accessors
static inline int dw_pcie_start_link(struct dw_pcie *pci) // not IO-accessors
static inline void dw_pcie_stop_link(struct dw_pcie *pci) // not IO-accessors
static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci) // not IO-accessors

static inline unsigned int dw_pcie_ep_get_dbi_offset(struct dw_pcie_ep *ep, u8 func_no)
static inline unsigned int dw_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep, u8 func_no)
static inline u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, size_t size)
static inline void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, size_t size, u32 val)
static inline void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg, size_t size, u32 val) // for dbi2
static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u32 val)
static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg)
static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u16 val)
static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg)
static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u8 val)
static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg)
static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u32 val) // for dbi2
---

Perhaps the following order is better?
---
// normal DBI-accessors
static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
static inline u32 dw_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
static inline void dw_pcie_writew_dbi(struct dw_pcie *pci, u32 reg, u16 val)
static inline u16 dw_pcie_readw_dbi(struct dw_pcie *pci, u32 reg)
static inline void dw_pcie_writeb_dbi(struct dw_pcie *pci, u32 reg, u8 val)
static inline u8 dw_pcie_readb_dbi(struct dw_pcie *pci, u32 reg)
static inline void dw_pcie_writel_dbi2(struct dw_pcie *pci, u32 reg, u32 val)

// EP-specific DBI-accessors for dbi
static inline unsigned int dw_pcie_ep_get_dbi_offset(struct dw_pcie_ep *ep, u8 func_no)
static inline u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, size_t size)
static inline void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, size_t size, u32 val)
static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u32 val)
static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg)
static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u16 val)
static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg)
static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u8 val)
static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg)

// EP-specific DBI-accessors for dbi2
static inline unsigned int dw_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep, u8 func_no)
static inline void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg, size_t size, u32 val)
static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u32 val)

// rest of inline functions
static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci)
static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
static inline int dw_pcie_start_link(struct dw_pcie *pci)
static inline void dw_pcie_stop_link(struct dw_pcie *pci)
static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci)
---

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> > -Serge(y)
> >
> > >  #ifdef CONFIG_PCIE_DW_HOST
> > >  irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp);
> > >  int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
> > > --
> > > 2.34.1
> > >
> >
> 
> --
> மணிவண்ணன் சதாசிவம்




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux