linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>,
	"lpieralisi@kernel.org" <lpieralisi@kernel.org>,
	"kw@linux.com" <kw@linux.com>,
	"robh@kernel.org" <robh@kernel.org>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"jingoohan1@gmail.com" <jingoohan1@gmail.com>,
	"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v3 4/6] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers
Date: Wed, 20 Dec 2023 05:10:44 +0000	[thread overview]
Message-ID: <TYBPR01MB5341D7E1D285D1BCA8A4FA13D896A@TYBPR01MB5341.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <xxry4dc4r56vwifkso7g34r3a46tinf2sow7qgc2zemy4g6uvx@7rgtxboob3rh>

Hi Serge,

> From: Serge Semin, Sent: Tuesday, December 19, 2023 6:55 PM
> 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@renesas.com>
> > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > > > ---
> > > > >  .../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.

Thank you for the comment. I understood it.

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

Thank you for your review! I'll fix this patch and submit v4 patch.

Best regards,
Yoshihiro Shimoda

> -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
> > > > >
> > > >
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்

  reply	other threads:[~2023-12-20  5:10 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-15  2:29 [PATCH v3 0/6] PCI: controllers: tidy code up Yoshihiro Shimoda
2023-12-15  2:29 ` [PATCH v3 1/6] PCI: dwc: Drop host prefix from struct dw_pcie_host_ops Yoshihiro Shimoda
2023-12-15  2:29 ` [PATCH v3 2/6] PCI: dwc: Rename to .init in struct dw_pcie_ep_ops Yoshihiro Shimoda
2023-12-15  2:29 ` [PATCH v3 3/6] PCI: dwc: Rename to .get_dbi_offset " Yoshihiro Shimoda
2023-12-15  2:29 ` [PATCH v3 4/6] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers Yoshihiro Shimoda
2023-12-15  9:51   ` Serge Semin
2023-12-15 21:53     ` Krzysztof Wilczyński
2023-12-17 16:32     ` Manivannan Sadhasivam
2023-12-19  0:21       ` Yoshihiro Shimoda
2023-12-19  9:54         ` Serge Semin
2023-12-20  5:10           ` Yoshihiro Shimoda [this message]
2023-12-17 16:34   ` Manivannan Sadhasivam
2023-12-15  2:29 ` [PATCH v3 5/6] PCI: iproc: fix -Wvoid-pointer-to-enum-cast warning Yoshihiro Shimoda
2023-12-15  2:29 ` [PATCH v3 6/6] PCI: rcar-gen4: " Yoshihiro Shimoda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=TYBPR01MB5341D7E1D285D1BCA8A4FA13D896A@TYBPR01MB5341.jpnprd01.prod.outlook.com \
    --to=yoshihiro.shimoda.uh@renesas.com \
    --cc=bhelgaas@google.com \
    --cc=fancer.lancer@gmail.com \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=jingoohan1@gmail.com \
    --cc=kw@linux.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=lpieralisi@kernel.org \
    --cc=mani@kernel.org \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).