On Wed, Oct 18, 2023 at 05:13:41PM +0300, Serge Semin wrote:
> On Tue, Oct 17, 2023 at 11:47:54AM +0530, Manivannan Sadhasivam wrote:
> > From: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> >
> > As per the DWC databook v4.21a, section M.4.1, in order to write some read
> > only and shadow registers through application DBI, the device driver should
> > assert DBI Chip Select 2 (CS2) in addition to DBI Chip Select (CS).
> >
> > This is a requirement at least on the Qcom platforms while programming the
> > BAR size, as the BAR mask registers are marked RO. So let's add two new
> > accessors dw_pcie_dbi_cs2_{en/dis} to enable/disable CS2 access in a vendor
> > specific way while programming the BAR size.
>
> Emm, it's a known thing for all IP-core versions: dbi_cs2 must be
> asserted to access the shadow DW PCIe CSRs space for both RC and
> EP including the BARs mask and their enabling/disabling flag (there
> are many more shadow CSRs available on DW PCIe EPs, and a few in DW
> PCIe RCs). That's why the dw_pcie_ops->writel_dbi2 pointer has been
> defined in the first place (dbi2 suffix means dbi_cs2). You should use
> it to create the platform-specific dbi_cs2 write accessors like it's
> done in pci-keystone.c and pcie-bt1.c. For instance like this:
>
> static void qcom_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val)
> {
> struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> int ret;
>
> writel(1, pcie_ep->elbi + ELBI_CS2_ENABLE);
>
> ret = dw_pcie_write(pci->dbi_base2 + reg, size, val);
> if (ret)
> dev_err(pci->dev, "write DBI address failed\n");
>
> writel(0, pcie_ep->elbi + ELBI_CS2_ENABLE);
> }
>
Hmm, I was not aware that this write_dbi2() callback is supposed to enable the
CS2 access internally. But this approach doesn't look good to me.
We already have accessors for enabling write access to DBI RO registers:
dw_pcie_dbi_ro_wr_en()
dw_pcie_dbi_ro_wr_dis()
And IMO DBI_CS2 access should also be done this way instead of hiding it inside
the register write callback.
- Mani
> /* Common DWC controller ops */
> static const struct dw_pcie_ops pci_ops = {
> .link_up = qcom_pcie_dw_link_up,
> .start_link = qcom_pcie_dw_start_link,
> .stop_link = qcom_pcie_dw_stop_link,
> .write_dbi2 = qcom_pcie_write_dbi2,
> };
>
> For that reason there is absolutely no need in adding the new
> callbacks.
>
> -Serge(y)
>
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-ep.c | 6 ++++++
> > drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++++++
> > 2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index d34a5e87ad18..1874fb3d8df4 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -269,11 +269,17 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >
> > dw_pcie_dbi_ro_wr_en(pci);
> >
> > + dw_pcie_dbi_cs2_en(pci);
> > dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1));
> > + dw_pcie_dbi_cs2_dis(pci);
> > +
> > dw_pcie_writel_dbi(pci, reg, flags);
> >
> > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > + dw_pcie_dbi_cs2_en(pci);
> > dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1));
> > + dw_pcie_dbi_cs2_dis(pci);
> > +
> > dw_pcie_writel_dbi(pci, reg + 4, 0);
> > }
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 55ff76e3d384..3cba27b5bbe5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -379,6 +379,7 @@ struct dw_pcie_ops {
> > size_t size, u32 val);
> > void (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> > size_t size, u32 val);
> > + void (*dbi_cs2_access)(struct dw_pcie *pcie, bool enable);
> > int (*link_up)(struct dw_pcie *pcie);
> > enum dw_pcie_ltssm (*get_ltssm)(struct dw_pcie *pcie);
> > int (*start_link)(struct dw_pcie *pcie);
> > @@ -508,6 +509,18 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
> > dw_pcie_writel_dbi(pci, reg, val);
> > }
> >
> > +static inline void dw_pcie_dbi_cs2_en(struct dw_pcie *pci)
> > +{
> > + if (pci->ops && pci->ops->dbi_cs2_access)
> > + pci->ops->dbi_cs2_access(pci, true);
> > +}
> > +
> > +static inline void dw_pcie_dbi_cs2_dis(struct dw_pcie *pci)
> > +{
> > + if (pci->ops && pci->ops->dbi_cs2_access)
> > + pci->ops->dbi_cs2_access(pci, false);
> > +}
> > +
> > static inline int dw_pcie_start_link(struct dw_pcie *pci)
> > {
> > if (pci->ops && pci->ops->start_link)
> >
> > --
> > 2.25.1
> >
--
மணிவண்ணன் சதாசிவம்
