Message-ID: <20231019165013.GB7254@thinkpad>
Date:   Thu, 19 Oct 2023 22:20:13 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Manivannan Sadhasivam <mani@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Rob Herring <robh@...nel.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI: dwc: Add new accessors to enable/disable DBI
 CS2 while setting the BAR size

On Thu, Oct 19, 2023 at 05:37:39PM +0300, Serge Semin wrote:
> On Thu, Oct 19, 2023 at 10:58:35AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Oct 18, 2023 at 05:13:41PM +0300, Serge Semin wrote:
> > > On Tue, Oct 17, 2023 at 11:47:54AM +0530, Manivannan Sadhasivam wrote:
> > > > From: Manivannan Sadhasivam <mani@...nel.org>
> > > > 
> > > > As per the DWC databook v4.21a, section M.4.1, in order to write some read
> > > > only and shadow registers through application DBI, the device driver should
> > > > assert DBI Chip Select 2 (CS2) in addition to DBI Chip Select (CS).
> > > > 
> > > > This is a requirement at least on the Qcom platforms while programming the
> > > > BAR size, as the BAR mask registers are marked RO. So let's add two new
> > > > accessors dw_pcie_dbi_cs2_{en/dis} to enable/disable CS2 access in a vendor
> > > > specific way while programming the BAR size.
> > > 
> > > Emm, it's a known thing for all IP-core versions: dbi_cs2 must be
> > > asserted to access the shadow DW PCIe CSRs space for both RC and
> > > EP including the BARs mask and their enabling/disabling flag (there
> > > are many more shadow CSRs available on DW PCIe EPs, and a few in DW
> > > PCIe RCs). That's why the dw_pcie_ops->writel_dbi2 pointer has been
> > > defined in the first place (dbi2 suffix means dbi_cs2). You should use
> > > it to create the platform-specific dbi_cs2 write accessors like it's
> > > done in pci-keystone.c and pcie-bt1.c. For instance like this:
> > > 
> > > static void qcom_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val)
> > > {
> > > 	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > > 	int ret;
> > > 
> > > 	writel(1, pcie_ep->elbi + ELBI_CS2_ENABLE);
> > > 
> > > 	ret = dw_pcie_write(pci->dbi_base2 + reg, size, val);
> > > 	if (ret)
> > > 		dev_err(pci->dev, "write DBI address failed\n");
> > > 
> > > 	writel(0, pcie_ep->elbi + ELBI_CS2_ENABLE);
> > > }
> > > 
> > 
> 
> > Hmm, I was not aware that this write_dbi2() callback is supposed to enable the
> > CS2 access internally. But this approach doesn't look good to me.
> > 
> > We already have accessors for enabling write access to DBI RO registers:
> > 
> > dw_pcie_dbi_ro_wr_en()
> > dw_pcie_dbi_ro_wr_dis()
> > 
> > And IMO DBI_CS2 access should also be done this way instead of hiding it inside
> > the register write callback.
> 
> No, for many-many reasons.
> 
> First of all, DBI RO/RW switch and DBI/DBI2 are absolutely different
> things. Former one switches the CSRs access attributes in both DBI and
> DBI2 CSR spaces. The later one are two CSR spaces, which access to is
> platform-specific. They can't and shouldn't be compared.  DBI2 space
> is a shadow DBI CSRs space, which registers aren't just the RW
> versions of the DBI space, but its CSRs normally have different
> semantics with respect to the normal DBI CSRs available on the same
> offsets. I.e. BAR0 contains MEM/IO, TYPE, PREF flags and base address,
> meanwhile DBI2-BAR0 - BAR enable/disable flag, BAR mask. From that
> perspective having the dw_pcie_ops.write_dbi2 callback utilized for
> the DBI2 space access would provide an interface looking similar to
> the just DBI space - dw_pcie_ops.{write_dbi,read_dbi}. Thus the
> unified access interface would provide much more readable code, where
> based on the method name you'll be able to immediately infer the space
> being accessed to.
> 
> Secondly, DBI RO/RW switch is a straight-forward CSR flag
> clearing/setting DBI_RO_WR_EN. This mechanism is available on all DW
> PCIe IP-cores and works in the _same_ way on all of them: just the
> MISC_CONTROL_1_OFF.DBI_RO_WR_EN flag switching. It switches RO/RW
> access attributes on both DBI_CS and DBI_CS2. So it's a cross-platform
> thing independent from the vendor-specific IP-core settings. That's
> why having generic functions for the RO/RW switch was the best choice:
> it's cross-platform so no need in the platform-specific callbacks, it
> works for both DBI and DBI2 so instead of implementing two additional
> RW-accessors you can call the RW en/dis method around the DBI and DBI2
> accessors whenever you need to switch the access attributes.
> 
> On the contrary DBI_CS2 is the DW PCIe IP-core input signal which
> activation is platform-specific. Some platforms have it switchable via
> a system-controller, some - via an additional elbi CSRs space, some -
> provide an additional CSR space mapping DBI2 with no need in the
> direct DBI_CS2 flag toggle, some may have an intermix of these
> setups or may need some additional manipulation to access the DBI2
> space. So your case of having the DBI_CS2 flag available via the elbi
> space and having the DBI/DBI2 space mapped within the 4K offset with
> respect to DBI is just a single possible option.
> 
> Finally, it's all about simplicity, maintainability and KIS principle.
> Your approach would imply having additional platform-specific
> callbacks, meanwhile there is already available DBI2 space accessor
> which is more than enough to implement what you need.  Even if you
> drop the later one (and convert all the already available LLDDs to
> supporting what you want), having two callbacks instead of one will
> still make things harder to read, because the kernel hacker would need
> to keep in mind the DBI/DBI2 access context. Additionally calling
> _three_ methods instead of a _single_ one will look much more complex.
> Moreover having on/off antagonists prune to errors since a developer
> may forget to disable the DBI2 flag, which on some platforms will
> change the DBI CSRs semantics. Such error will be just impossible to
> meet should the current interface is preserved unless the
> platform-specific DBI2 accessor is incorrectly implemented, which
> would be still specific to the particular platform, but not for the
> entire DW PCIe driver. The last but not least, as I already mentioned
> dw_pcie_ops.write_dbi2 and respective wrappers look as much like the
> normal DBI accessors dw_pcie_ops.{write_dbi,read_dbi} which greatly
> improves the code readability.
> 

Hmm, thanks for the detailed clarification, really appreciated.

My understanding about the DBI2 space was not clear and your reply clarified
that. I will implement the write_dbi2() callback as suggested.

- Mani

> So no, I failed to find any firm justification for the approach you
> suggest.
> 
> -Serge(y)
> 
> > 
> > - Mani
> > 
> > > /* Common DWC controller ops */
> > > static const struct dw_pcie_ops pci_ops = {
> > > 	.link_up = qcom_pcie_dw_link_up,
> > > 	.start_link = qcom_pcie_dw_start_link,
> > > 	.stop_link = qcom_pcie_dw_stop_link,
> > > 	.write_dbi2 = qcom_pcie_write_dbi2,
> > > };
> > > 
> > > For that reason there is absolutely no need in adding the new
> > > callbacks.
> > > 
> > > -Serge(y)
> > > 
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-designware-ep.c |  6 ++++++
> > > >  drivers/pci/controller/dwc/pcie-designware.h    | 13 +++++++++++++
> > > >  2 files changed, 19 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index d34a5e87ad18..1874fb3d8df4 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -269,11 +269,17 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > > >  
> > > >  	dw_pcie_dbi_ro_wr_en(pci);
> > > >  
> > > > +	dw_pcie_dbi_cs2_en(pci);
> > > >  	dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1));
> > > > +	dw_pcie_dbi_cs2_dis(pci);
> > > > +
> > > >  	dw_pcie_writel_dbi(pci, reg, flags);
> > > >  
> > > >  	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > > > +		dw_pcie_dbi_cs2_en(pci);
> > > >  		dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1));
> > > > +		dw_pcie_dbi_cs2_dis(pci);
> > > > +
> > > >  		dw_pcie_writel_dbi(pci, reg + 4, 0);
> > > >  	}
> > > >  
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index 55ff76e3d384..3cba27b5bbe5 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -379,6 +379,7 @@ struct dw_pcie_ops {
> > > >  			     size_t size, u32 val);
> > > >  	void    (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> > > >  			      size_t size, u32 val);
> > > > +	void	(*dbi_cs2_access)(struct dw_pcie *pcie, bool enable);
> > > >  	int	(*link_up)(struct dw_pcie *pcie);
> > > >  	enum dw_pcie_ltssm (*get_ltssm)(struct dw_pcie *pcie);
> > > >  	int	(*start_link)(struct dw_pcie *pcie);
> > > > @@ -508,6 +509,18 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
> > > >  	dw_pcie_writel_dbi(pci, reg, val);
> > > >  }
> > > >  
> > > > +static inline void dw_pcie_dbi_cs2_en(struct dw_pcie *pci)
> > > > +{
> > > > +	if (pci->ops && pci->ops->dbi_cs2_access)
> > > > +		pci->ops->dbi_cs2_access(pci, true);
> > > > +}
> > > > +
> > > > +static inline void dw_pcie_dbi_cs2_dis(struct dw_pcie *pci)
> > > > +{
> > > > +	if (pci->ops && pci->ops->dbi_cs2_access)
> > > > +		pci->ops->dbi_cs2_access(pci, false);
> > > > +}
> > > > +
> > > >  static inline int dw_pcie_start_link(struct dw_pcie *pci)
> > > >  {
> > > >  	if (pci->ops && pci->ops->start_link)
> > > > 
> > > > -- 
> > > > 2.25.1
> > > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

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

Powered by blists - more mailing lists