diff mbox series

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

Message ID 20231017-pcie-qcom-bar-v1-2-3e26de07bec0@linaro.org (mailing list archive)
State Superseded
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI: dwc: Fix the BAR size programming | expand

Commit Message

Manivannan Sadhasivam Oct. 17, 2023, 6:17 a.m. UTC
From: Manivannan Sadhasivam <mani@kernel.org>

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@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Bjorn Andersson Oct. 17, 2023, 2:24 p.m. UTC | #1
On Tue, Oct 17, 2023 at 11:47:55AM +0530, Manivannan Sadhasivam wrote:
> From: Manivannan Sadhasivam <mani@kernel.org>

Your S-o-b should match this.

> 
> 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@linaro.org>
> ---
>  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?

Regards,
Bjorn

> +	/*
> +	 * Do a dummy read to make sure that the previous write has reached the
> +	 * memory before returning.
> +	 */
> +	readl_relaxed(pcie_ep->elbi + ELBI_CS2_ENABLE);
> +}
> +
>  static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
>  {
>  	struct dw_pcie *pci = &pcie_ep->pci;
> @@ -500,6 +513,7 @@ 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,
> +	.dbi_cs2_access = qcom_pcie_dbi_cs2_access,
>  };
>  
>  static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,
> 
> -- 
> 2.25.1
>
Manivannan Sadhasivam Oct. 17, 2023, 4:21 p.m. UTC | #2
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@kernel.org>
> 
> 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@linaro.org>
> > ---
> >  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.

- Mani

> Regards,
> Bjorn
> 
> > +	/*
> > +	 * Do a dummy read to make sure that the previous write has reached the
> > +	 * memory before returning.
> > +	 */
> > +	readl_relaxed(pcie_ep->elbi + ELBI_CS2_ENABLE);
> > +}
> > +
> >  static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
> >  {
> >  	struct dw_pcie *pci = &pcie_ep->pci;
> > @@ -500,6 +513,7 @@ 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,
> > +	.dbi_cs2_access = qcom_pcie_dbi_cs2_access,
> >  };
> >  
> >  static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,
> > 
> > -- 
> > 2.25.1
> >
Bjorn Andersson Oct. 17, 2023, 4:56 p.m. UTC | #3
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@kernel.org>
> > 
> > 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@linaro.org>
> > > ---
> > >  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".

Not sure that the code is wrong, I just want you to be certain that this
isn't a problem.

Thanks,
Bjorn
Manivannan Sadhasivam Oct. 17, 2023, 5:41 p.m. UTC | #4
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@kernel.org>
> > > 
> > > 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@linaro.org>
> > > > ---
> > > >  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.

Thanks for pointing it out.

- Mani

> Not sure that the code is wrong, I just want you to be certain that this
> isn't a problem.
> 
> Thanks,
> Bjorn
Bjorn Andersson Oct. 17, 2023, 10:18 p.m. UTC | #5
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@kernel.org>
> > > > 
> > > > 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@linaro.org>
> > > > > ---
> > > > >  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
Manivannan Sadhasivam Oct. 18, 2023, 1:27 p.m. UTC | #6
On Tue, Oct 17, 2023 at 03:18:11PM -0700, Bjorn Andersson wrote:
> 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@kernel.org>
> > > > > 
> > > > > 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@linaro.org>
> > > > > > ---
> > > > > >  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.
> 

That shouldn't cause any issues as CS2_ENABLE just opens up the write access to
read only registers. It will cause issues if CPU/compiler reorders this write
with the following writes where we actually write to the read only registers.

For that I initially thought the readl_relaxed() would be sufficient. But
looking more, it may not be enough since CS2_ENABLE register lies in ELBI space
and the read only registers are in DBI space. So the CPU may reorder writes if
this function gets inlined by the compiler since both are in different hardware
space (not sure if CPU considers both regions as one since they are in PCI
domain, in that case the barrier is not required, but I'm not sure).

So to be on the safe side, I should add wmb() after the CS2_ENABLE write.

- Mani

> 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
Bjorn Andersson Oct. 19, 2023, 3:18 a.m. UTC | #7
On Wed, Oct 18, 2023 at 06:57:58PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Oct 17, 2023 at 03:18:11PM -0700, Bjorn Andersson wrote:
> > 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@kernel.org>
> > > > > > 
> > > > > > 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@linaro.org>
> > > > > > > ---
> > > > > > >  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.
> > 
> 
> That shouldn't cause any issues as CS2_ENABLE just opens up the write access to
> read only registers. It will cause issues if CPU/compiler reorders this write
> with the following writes where we actually write to the read only registers.
> 

Wouldn't that cause issues if previous writes are reordered past a
disable?

> For that I initially thought the readl_relaxed() would be sufficient. But
> looking more, it may not be enough since CS2_ENABLE register lies in ELBI space
> and the read only registers are in DBI space. So the CPU may reorder writes if
> this function gets inlined by the compiler since both are in different hardware
> space (not sure if CPU considers both regions as one since they are in PCI
> domain, in that case the barrier is not required, but I'm not sure).

That is a very good question (if the regions are considered the same or
different), I don't know.

> 
> So to be on the safe side, I should add wmb() after the CS2_ENABLE write.
> 

Sounds reasonable, in absence of the answer to above question.

Regards,
Bjorn

> - Mani
> 
> > 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
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam Oct. 19, 2023, 5:19 a.m. UTC | #8
On Wed, Oct 18, 2023 at 08:18:20PM -0700, Bjorn Andersson wrote:
> On Wed, Oct 18, 2023 at 06:57:58PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Oct 17, 2023 at 03:18:11PM -0700, Bjorn Andersson wrote:
> > > 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@kernel.org>
> > > > > > > 
> > > > > > > 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@linaro.org>
> > > > > > > > ---
> > > > > > > >  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.
> > > 
> > 
> > That shouldn't cause any issues as CS2_ENABLE just opens up the write access to
> > read only registers. It will cause issues if CPU/compiler reorders this write
> > with the following writes where we actually write to the read only registers.
> > 
> 
> Wouldn't that cause issues if previous writes are reordered past a
> disable?
> 

That should be fixed by wmb() after CS_ENABLE.

> > For that I initially thought the readl_relaxed() would be sufficient. But
> > looking more, it may not be enough since CS2_ENABLE register lies in ELBI space
> > and the read only registers are in DBI space. So the CPU may reorder writes if
> > this function gets inlined by the compiler since both are in different hardware
> > space (not sure if CPU considers both regions as one since they are in PCI
> > domain, in that case the barrier is not required, but I'm not sure).
> 
> That is a very good question (if the regions are considered the same or
> different), I don't know.
> 
> > 
> > So to be on the safe side, I should add wmb() after the CS2_ENABLE write.
> > 
> 
> Sounds reasonable, in absence of the answer to above question.
> 

Thanks!

- Mani
diff mbox series

Patch

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);
+	/*
+	 * Do a dummy read to make sure that the previous write has reached the
+	 * memory before returning.
+	 */
+	readl_relaxed(pcie_ep->elbi + ELBI_CS2_ENABLE);
+}
+
 static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
 {
 	struct dw_pcie *pci = &pcie_ep->pci;
@@ -500,6 +513,7 @@  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,
+	.dbi_cs2_access = qcom_pcie_dbi_cs2_access,
 };
 
 static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,