| Message ID | 20231017-pcie-qcom-bar-v1-2-3e26de07bec0@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: dwc: Fix the BAR size programming | expand |
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 >
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 > >
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
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
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
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
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 > > -- > மணிவண்ணன் சதாசிவம்
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 --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,