Hi Yoshihiro
On Tue, Dec 19, 2023 at 12:21:12AM +0000, Yoshihiro Shimoda wrote:
> 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...
It's better to group the function definitions up by their
functionality so the code reader wouldn't need to jump over the
unrelated methods if one is looking for something particular, like
only EP-specific DBI accessors or EP-specific DBI2 accessors, etc.
> 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)
LGTM. Thanks.
-Serge(y)
>
> // 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
> > > >
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
