Re: [PATCH 1/2] PCI: dwc: Add new accessors to enable/disable DBI CS2 while setting the BAR size

[Serge Semin]

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

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