Re: [PATCH 2/2] PCI: qcom-ep: Implement dbi_cs2_access() function callback for DBI CS2 access

[Bjorn Andersson]

On Tue, Oct 17, 2023 at 11:11:00PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Oct 17, 2023 at 09:56:09AM -0700, Bjorn Andersson wrote:
> > On Tue, Oct 17, 2023 at 09:51:29PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Oct 17, 2023 at 07:24:31AM -0700, Bjorn Andersson wrote:
> > > > On Tue, Oct 17, 2023 at 11:47:55AM +0530, Manivannan Sadhasivam wrote:
> > > > > From: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> > > > 
> > > > Your S-o-b should match this.
> > > > 
> > > 
> > > I gave b4 a shot for sending the patches and missed this. Will fix it in next
> > > version.
> > > 
> > > > > 
> > > > > Qcom EP platforms require enabling/disabling the DBI CS2 access while
> > > > > programming some read only and shadow registers through DBI. So let's
> > > > > implement the dbi_cs2_access() callback that will be called by the DWC core
> > > > > while programming such registers like BAR mask register.
> > > > > 
> > > > > Without DBI CS2 access, writes to those registers will not be reflected.
> > > > > 
> > > > > Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > > > > ---
> > > > >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 14 ++++++++++++++
> > > > >  1 file changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > > index 32c8d9e37876..4653cbf7f9ed 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > > @@ -124,6 +124,7 @@
> > > > >  
> > > > >  /* ELBI registers */
> > > > >  #define ELBI_SYS_STTS				0x08
> > > > > +#define ELBI_CS2_ENABLE				0xa4
> > > > >  
> > > > >  /* DBI registers */
> > > > >  #define DBI_CON_STATUS				0x44
> > > > > @@ -262,6 +263,18 @@ static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
> > > > >  	disable_irq(pcie_ep->perst_irq);
> > > > >  }
> > > > >  
> > > > > +static void qcom_pcie_dbi_cs2_access(struct dw_pcie *pci, bool enable)
> > > > > +{
> > > > > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > > > > +
> > > > > +	writel_relaxed(enable, pcie_ep->elbi + ELBI_CS2_ENABLE);
> > > > 
> > > > Don't you want to maintain the ordering of whatever write came before
> > > > this?
> > > > 
> > > 
> > > Since this in a dedicated function, I did not care about the ordering w.r.t
> > > previous writes. Even if it gets inlined, the order should not matter since it
> > > only enables/disables the CS2 access for the forthcoming writes.
> > > 
> > 
> > The wmb() - in a non-relaxed writel -  would ensure that no earlier
> > writes are reordered and end up in your expected set of "forthcoming
> > writes".
> > 
> 
> I was under the impression that the readl_relaxed() here serves as an implicit
> barrier. But reading the holy memory-barriers documentation doesn't explicitly
> say so. So I'm going to add wmb() to be on the safe side as you suggested.
> 

I'm talking about writes prior to this function is being called.

In other words, if you write:

writel_relaxed(A, ptr); (or writel, it doesn't matter)
writel_relaxed(X, ELBI_CS2_ENABLE);
readl_relaxed(ELBI_CS2_ENABLE);

Then there are circumstances where the write to ptr might be performed
after ELBI_CS2_ENABLE.

Iiuc, the way to avoid that is to either be certain that none of those
circumstances applies, or to add a wmb(), like:

writel_relaxed(A, ptr); (or writel, it doesn't matter)
wmb();
writel_relaxed(X, ELBI_CS2_ENABLE);
readl_relaxed(ELBI_CS2_ENABLE);

or short hand:

writel_relaxed(A, ptr); (or writel, it doesn't matter)
writel(X, ELBI_CS2_ENABLE);
readl_relaxed(ELBI_CS2_ENABLE);

Where the wmb() will ensure the two writes happen in order.

The read in your code will ensure that execution won't proceed until the
write has hit the hardware, so that's good. But writing this makes me
uncertain if there's sufficient guarantees for the CPU not reordering
later operations.

Regards,
Bjorn