| Message ID | 20230825093219.2685912-10-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 1896d17f916819b8b5d145b4feef77109e78c435 |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
On Fri, Aug 25, 2023 at 06:32:09PM +0900, Yoshihiro Shimoda wrote: > Renesas R-Car Gen4 PCIe controllers have an unexpected register value in > the eDMA CTRL register. So, add a new capability flag "EDMA_UNROLL" > which would force the unrolled eDMA mapping for the problematic device. > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-designware.c | 8 +++++++- > drivers/pci/controller/dwc/pcie-designware.h | 5 +++-- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index c4998194fe74..4812ce040f1e 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -883,8 +883,14 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > * Indirect eDMA CSRs access has been completely removed since v5.40a > * thus no space is now reserved for the eDMA channels viewport and > * former DMA CTRL register is no longer fixed to FFs. > + * > + * Note that Renesas R-Car S4-8's PCIe controllers for unknown reason > + * have zeros in the eDMA CTRL register even though the HW-manual > + * explicitly states there must FFs if the unrolled mapping is enabled. > + * For such cases the low-level drivers are supposed to manually > + * activate the unrolled mapping to bypass the auto-detection procedure. > */ > - if (dw_pcie_ver_is_ge(pci, 540A)) > + if (dw_pcie_ver_is_ge(pci, 540A) || dw_pcie_cap_is(pci, EDMA_UNROLL)) > val = 0xFFFFFFFF; > else > val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index b731e38a71fc..c7759a508ca9 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -51,8 +51,9 @@ > > /* DWC PCIe controller capabilities */ > #define DW_PCIE_CAP_REQ_RES 0 > -#define DW_PCIE_CAP_IATU_UNROLL 1 > -#define DW_PCIE_CAP_CDM_CHECK 2 > +#define DW_PCIE_CAP_EDMA_UNROLL 1 > +#define DW_PCIE_CAP_IATU_UNROLL 2 > +#define DW_PCIE_CAP_CDM_CHECK 3 Why did you make the new DW_PCIE_CAP_EDMA_UNROLL "1" and shift all the existing ones down? If they don't need to be ordered like this, leaving the existing ones alone and making DW_PCIE_CAP_EDMA_UNROLL "3" would be a simpler one-line diff. > #define dw_pcie_cap_is(_pci, _cap) \ > test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps) > -- > 2.25.1 >
Hi Bjorn On Thu, Sep 14, 2023 at 11:09:41AM -0500, Bjorn Helgaas wrote: > On Fri, Aug 25, 2023 at 06:32:09PM +0900, Yoshihiro Shimoda wrote: > > Renesas R-Car Gen4 PCIe controllers have an unexpected register value in > > the eDMA CTRL register. So, add a new capability flag "EDMA_UNROLL" > > which would force the unrolled eDMA mapping for the problematic device. > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-designware.c | 8 +++++++- > > drivers/pci/controller/dwc/pcie-designware.h | 5 +++-- > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index c4998194fe74..4812ce040f1e 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -883,8 +883,14 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > * Indirect eDMA CSRs access has been completely removed since v5.40a > > * thus no space is now reserved for the eDMA channels viewport and > > * former DMA CTRL register is no longer fixed to FFs. > > + * > > + * Note that Renesas R-Car S4-8's PCIe controllers for unknown reason > > + * have zeros in the eDMA CTRL register even though the HW-manual > > + * explicitly states there must FFs if the unrolled mapping is enabled. > > + * For such cases the low-level drivers are supposed to manually > > + * activate the unrolled mapping to bypass the auto-detection procedure. > > */ > > - if (dw_pcie_ver_is_ge(pci, 540A)) > > + if (dw_pcie_ver_is_ge(pci, 540A) || dw_pcie_cap_is(pci, EDMA_UNROLL)) > > val = 0xFFFFFFFF; > > else > > val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > index b731e38a71fc..c7759a508ca9 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > @@ -51,8 +51,9 @@ > > > > /* DWC PCIe controller capabilities */ > > #define DW_PCIE_CAP_REQ_RES 0 > > -#define DW_PCIE_CAP_IATU_UNROLL 1 > > -#define DW_PCIE_CAP_CDM_CHECK 2 > > +#define DW_PCIE_CAP_EDMA_UNROLL 1 > > +#define DW_PCIE_CAP_IATU_UNROLL 2 > > +#define DW_PCIE_CAP_CDM_CHECK 3 > > Why did you make the new DW_PCIE_CAP_EDMA_UNROLL "1" and shift all the > existing ones down? If they don't need to be ordered like this, > leaving the existing ones alone and making DW_PCIE_CAP_EDMA_UNROLL "3" > would be a simpler one-line diff. The discussion in framework of which this patch was born is available here: https://lore.kernel.org/linux-pci/20221121124400.1282768-6-yoshihiro.shimoda.uh@renesas.com/ So the patch is mainly what I suggested back then. Though in my case DW_PCIE_CAP_EDMA_UNROLL was intended to have index 2. Why didn't I add the unrolled DMA-capability macros to the tail of the list? My intention was to have the IATU and EDMA unrolled capability flags defined nearby for the sake of having a tiny bit better readability since functionally they look similar but refer to the different controller modules. IMO it was better to have the flags functionally grouped than to save several diff lines. -Serge(y) > > > #define dw_pcie_cap_is(_pci, _cap) \ > > test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps) > > -- > > 2.25.1 > >
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index c4998194fe74..4812ce040f1e 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -883,8 +883,14 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) * Indirect eDMA CSRs access has been completely removed since v5.40a * thus no space is now reserved for the eDMA channels viewport and * former DMA CTRL register is no longer fixed to FFs. + * + * Note that Renesas R-Car S4-8's PCIe controllers for unknown reason + * have zeros in the eDMA CTRL register even though the HW-manual + * explicitly states there must FFs if the unrolled mapping is enabled. + * For such cases the low-level drivers are supposed to manually + * activate the unrolled mapping to bypass the auto-detection procedure. */ - if (dw_pcie_ver_is_ge(pci, 540A)) + if (dw_pcie_ver_is_ge(pci, 540A) || dw_pcie_cap_is(pci, EDMA_UNROLL)) val = 0xFFFFFFFF; else val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index b731e38a71fc..c7759a508ca9 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -51,8 +51,9 @@ /* DWC PCIe controller capabilities */ #define DW_PCIE_CAP_REQ_RES 0 -#define DW_PCIE_CAP_IATU_UNROLL 1 -#define DW_PCIE_CAP_CDM_CHECK 2 +#define DW_PCIE_CAP_EDMA_UNROLL 1 +#define DW_PCIE_CAP_IATU_UNROLL 2 +#define DW_PCIE_CAP_CDM_CHECK 3 #define dw_pcie_cap_is(_pci, _cap) \ test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)