[PATCH 0/2] PCI: dwc: Fix the BAR size programming

[PATCH 0/2] PCI: dwc: Fix the BAR size programming

[Manivannan Sadhasivam]

Hello,

This series fixes the issue seen on Qcom EP platforms implementing the DWC
core while setting the BAR size. Currently, whatever the BAR size getting
programmed through pci_epc_set_bar() is not reflected on the host side
during enumeration.

Debugging that issue revealed that the DWC Spec mandates asserting the DBI
CS2 register in addition to DBI CS while programming some read only and
shadow registers. On the Qcom EP platforms, the BAR mask register used to
program the BAR size is marked as read only (RO). So the driver needs to
assert DBI CS2 before programming and deassert it once done.

Hence, this series adds two new macros for asserting/deasserting the DBI
CS2 while programming BAR size and also introduces a new host callback,
dbi_cs2_access() that the vendor drivers can implement.

For platforms not requiring the DBI CS2 access, this is a no-op.

This series has been tested on Qcom SM8450 based development platform.

---
Manivannan Sadhasivam (2):
      PCI: dwc: Add new accessors to enable/disable DBI CS2 while setting the BAR size
      PCI: qcom-ep: Implement dbi_cs2_access() function callback for DBI CS2 access

 drivers/pci/controller/dwc/pcie-designware-ep.c |  6 ++++++
 drivers/pci/controller/dwc/pcie-designware.h    | 13 +++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c       | 14 ++++++++++++++
 3 files changed, 33 insertions(+)
---
base-commit: a286439bbc71e8c9bb1660b7d4775efcab6011fa
change-id: 20231017-pcie-qcom-bar-c4863c33c0e4

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <mani@kernel.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.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.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

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

[Serge Semin]

On Tue, Oct 17, 2023 at 11:47:54AM +0530, Manivannan Sadhasivam wrote:
> From: Manivannan Sadhasivam <mani@kernel.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);
}

/* 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@linaro.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
> 

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

[Manivannan Sadhasivam]

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

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

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

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

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

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

[Manivannan Sadhasivam]

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

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

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

[Manivannan Sadhasivam]

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(+)

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,

-- 
2.25.1

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

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

[Manivannan Sadhasivam]

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

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

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

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

[Manivannan Sadhasivam]

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

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

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

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

[Manivannan Sadhasivam]

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

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

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

[Bjorn Andersson]

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

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

[Manivannan Sadhasivam]

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

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