| Message ID | 20221121124400.1282768-6-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
+ Serge (who authored EDMA support) Thanks, Mani On Mon, Nov 21, 2022 at 09:43:56PM +0900, Yoshihiro Shimoda wrote: > Since reading value of PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL was > 0x00000000 on one of SoCs (R-Car S4-8), it cannot find the eDMA. > So, directly read the eDMA register if edma.reg_base is not zero. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 637d01807c67..2cc8584da6f4 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -836,8 +836,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > { > u32 val; > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > + if (pci->edma.reg_base) { > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > @@ -845,6 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > pci->edma.mf = EDMA_MF_EDMA_LEGACY; > > pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE; > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > } else { > return -ENODEV; > } > -- > 2.25.1 >
On Tue, Nov 22, 2022 at 07:25:50PM +0530, Manivannan Sadhasivam wrote: > + Serge (who authored EDMA support) Thanks @Mani. It's strange to see a fix for a patch which hasn't been even merged in yet and miss the patch author in the Cc list.) @Yoshihiro, on the next patchset revisions please don't forget to add my email address to the copy list. > > Thanks, > Mani > > On Mon, Nov 21, 2022 at 09:43:56PM +0900, Yoshihiro Shimoda wrote: > > Since reading value of PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL was > > 0x00000000 on one of SoCs (R-Car S4-8), it cannot find the eDMA. > > So, directly read the eDMA register if edma.reg_base is not zero. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/pci/controller/dwc/pcie-designware.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 637d01807c67..2cc8584da6f4 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -836,8 +836,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > { > > u32 val; > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > + if (pci->edma.reg_base) { > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > @@ -845,6 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > pci->edma.mf = EDMA_MF_EDMA_LEGACY; > > > > pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE; > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); Look what you suggest here: < u32 val; < ... < if (pci->edma.reg_base) { < ... < } else if (val != 0xFFFFFFFF) { < ... < } else { < ... It would be strange if your compiler didn't warn about 'val' being used uninitialized here, which in its turn would introduce a regression for the platforms with the indirectly accessible eDMA registers. Anyway you can't just drop something what didn't work for you hardware. The method you suggest to fix here works fine for multiple DW PCIe IP-cores. Judging by the HW manuals it should work at least up to v5.30a. Are you sure that your controller is of v5.20a? I see you overwrite the IP-core version for the PCIe host driver only. Why is that necessary? Does the version auto-detection procedure work incorrectly for you? What does the dbi+0x8f8 CSR contain in the host and EP registers space? Similarly could you also provide a content of the +0x978 register? -Sergey > > } else { > > return -ENODEV; > > } > > -- > > 2.25.1 > > > > -- > மணிவண்ணன் சதாசிவம்
Hi Serge, > From: Serge Semin, Sent: Monday, November 28, 2022 8:56 AM > > On Tue, Nov 22, 2022 at 07:25:50PM +0530, Manivannan Sadhasivam wrote: > > + Serge (who authored EDMA support) > > Thanks @Mani. It's strange to see a fix for a patch which hasn't been even > merged in yet and miss the patch author in the Cc list.) > > @Yoshihiro, on the next patchset revisions please don't forget to add > my email address to the copy list. Sure! I'll add your email address on the next patchset revisions. > > Thanks, > > Mani > > > > On Mon, Nov 21, 2022 at 09:43:56PM +0900, Yoshihiro Shimoda wrote: > > > Since reading value of PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL was > > > 0x00000000 on one of SoCs (R-Car S4-8), it cannot find the eDMA. > > > So, directly read the eDMA register if edma.reg_base is not zero. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > --- > > > drivers/pci/controller/dwc/pcie-designware.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > index 637d01807c67..2cc8584da6f4 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -836,8 +836,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > > { > > > u32 val; > > > > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > + if (pci->edma.reg_base) { > > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > > > > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > @@ -845,6 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > > pci->edma.mf = EDMA_MF_EDMA_LEGACY; > > > > > > pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE; > > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > Look what you suggest here: > < u32 val; > < ... > < if (pci->edma.reg_base) { > < ... > < } else if (val != 0xFFFFFFFF) { > < ... > < } else { > < ... > > It would be strange if your compiler didn't warn about 'val' being used > uninitialized here, which in its turn would introduce a regression for > the platforms with the indirectly accessible eDMA registers. You're correct. It's strange. I don't know why my using compiler [1] didn't warn about the 'val' though... [1] https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/x86_64-gcc-11.1.0-nolibc-aarch64-linux.tar.xz > Anyway you can't just drop something what didn't work for you > hardware. The method you suggest to fix here works fine for multiple > DW PCIe IP-cores. Judging by the HW manuals it should work at least up > to v5.30a. Are you sure that your controller is of v5.20a? I see you > overwrite the IP-core version for the PCIe host driver only. Why is > that necessary? Does the version auto-detection procedure work > incorrectly for you? Thank you for pointed it out! I realized that overwriting the IP-core Version was not needed. So, I'll drop it on v8. --- #define DWC_VERSION 0x520a ... struct rcar_gen4_pcie *rcar_gen4_pcie_devm_alloc(struct device *dev) { ... rcar->dw.version = DWC_VERSION; --- > What does the dbi+0x8f8 CSR contain in the host > and EP registers space? Similarly could you also provide a content of > the +0x978 register? The dbi+0x8f8 and the +0x978 registers' values are 0x00000000. --------------- (sorry, replace tabs with spaces...)--------------- --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) { u32 val; + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, + dw_pcie_readl_dma(pci, 0x8f8), + dw_pcie_readl_dma(pci, 0x978)); + if (pci->edma.reg_base) { pci->edma.mf = EDMA_MF_EDMA_UNROLL; ------------------------------------------------------------------- ----- Output ----- # dmesg | grep edma [ 1.108709] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 00000000, +0x978 = 00000000 ------------------ So, should I change the condition like below? --- - if (val == 0xFFFFFFFF && pci->edma.reg_base) { + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { ... - } else if (val != 0xFFFFFFFF) { - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { --- Best regards, Yoshihiro Shimoda > -Sergey > > > > } else { > > > return -ENODEV; > > > } > > > -- > > > 2.25.1 > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
On Mon, Nov 28, 2022 at 02:52:56AM +0000, Yoshihiro Shimoda wrote: > Hi Serge, > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:56 AM > > > > On Tue, Nov 22, 2022 at 07:25:50PM +0530, Manivannan Sadhasivam wrote: > > > + Serge (who authored EDMA support) > > > > Thanks @Mani. It's strange to see a fix for a patch which hasn't been even > > merged in yet and miss the patch author in the Cc list.) > > > > @Yoshihiro, on the next patchset revisions please don't forget to add > > my email address to the copy list. > > Sure! I'll add your email address on the next patchset revisions. > > > > Thanks, > > > Mani > > > > > > On Mon, Nov 21, 2022 at 09:43:56PM +0900, Yoshihiro Shimoda wrote: > > > > Since reading value of PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL was > > > > 0x00000000 on one of SoCs (R-Car S4-8), it cannot find the eDMA. > > > > So, directly read the eDMA register if edma.reg_base is not zero. > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > --- > > > > drivers/pci/controller/dwc/pcie-designware.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > index 637d01807c67..2cc8584da6f4 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > @@ -836,8 +836,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > > > { > > > > u32 val; > > > > > > > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > > + if (pci->edma.reg_base) { > > > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > > > > > > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > > @@ -845,6 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > > > pci->edma.mf = EDMA_MF_EDMA_LEGACY; > > > > > > > > pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE; > > > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > > Look what you suggest here: > > < u32 val; > > < ... > > < if (pci->edma.reg_base) { > > < ... > > < } else if (val != 0xFFFFFFFF) { > > < ... > > < } else { > > < ... > > > > It would be strange if your compiler didn't warn about 'val' being used > > uninitialized here, which in its turn would introduce a regression for > > the platforms with the indirectly accessible eDMA registers. > > You're correct. It's strange. I don't know why my using compiler [1] didn't > warn about the 'val' though... > > [1] > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/x86_64-gcc-11.1.0-nolibc-aarch64-linux.tar.xz > > > Anyway you can't just drop something what didn't work for you > > hardware. The method you suggest to fix here works fine for multiple > > DW PCIe IP-cores. Judging by the HW manuals it should work at least up > > to v5.30a. Are you sure that your controller is of v5.20a? I see you > > overwrite the IP-core version for the PCIe host driver only. Why is > > that necessary? Does the version auto-detection procedure work > > incorrectly for you? > > Thank you for pointed it out! I realized that overwriting the IP-core > Version was not needed. So, I'll drop it on v8. > --- > #define DWC_VERSION 0x520a > ... > struct rcar_gen4_pcie *rcar_gen4_pcie_devm_alloc(struct device *dev) > { > ... > rcar->dw.version = DWC_VERSION; > --- > > > What does the dbi+0x8f8 CSR contain in the host > > and EP registers space? Similarly could you also provide a content of > > the +0x978 register? > > The dbi+0x8f8 and the +0x978 registers' values are 0x00000000. > --------------- (sorry, replace tabs with spaces...)--------------- > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > { > u32 val; > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > + dw_pcie_readl_dma(pci, 0x8f8), > + dw_pcie_readl_dma(pci, 0x978)); > + No, this should have been the dw_pcie_readl_dbi() calls instead of dw_pcie_readl_!dma!(). What I try to understand from these values is the real version of your controller (dbi+0x8f8) and whether the legacy eDMA viewport registers range follows the documented convention of having FFs in the dbi+0x978 register. My current assumption that either your IP-core is newer than v5.30a or has some vendor-specific modification. But let's see the value first. > if (pci->edma.reg_base) { > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > ------------------------------------------------------------------- > > ----- Output ----- > # dmesg | grep edma > [ 1.108709] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 00000000, +0x978 = 00000000 > ------------------ > > So, should I change the condition like below? > > --- > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { > ... > - } else if (val != 0xFFFFFFFF) { > - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { > --- Definitely no. Even though it's impossible to have the eDMA controller configured with zero number of read and write channels we shouldn't assume that getting a zero value from the DMA_CTRL_VIEWPORT_OFF offset means having the unrolled eDMA CSRs mapping. Let's have a look at the content of the dbi+0x8f8 and dbi+0x978 offsets first. Based on these values we'll come up with what to do next. -Serge(y) > > Best regards, > Yoshihiro Shimoda > > > -Sergey > > > > > > } else { > > > > return -ENODEV; > > > > } > > > > -- > > > > 2.25.1 > > > > > > > > > > -- > > > மணிவண்ணன் சதாசிவம்
Hi Serge, > From: Serge Semin, Sent: Monday, November 28, 2022 8:59 PM > > On Mon, Nov 28, 2022 at 02:52:56AM +0000, Yoshihiro Shimoda wrote: > > Hi Serge, > > > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:56 AM > > > <snip> > > > What does the dbi+0x8f8 CSR contain in the host > > > and EP registers space? Similarly could you also provide a content of > > > the +0x978 register? > > > > The dbi+0x8f8 and the +0x978 registers' values are 0x00000000. > > --------------- (sorry, replace tabs with spaces...)--------------- > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > { > > u32 val; > > > > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > > + dw_pcie_readl_dma(pci, 0x8f8), > > + dw_pcie_readl_dma(pci, 0x978)); > > + > > No, this should have been the dw_pcie_readl_dbi() calls instead of > dw_pcie_readl_!dma!(). What I try to understand from these values is > the real version of your controller (dbi+0x8f8) and whether the legacy > eDMA viewport registers range follows the documented convention of > having FFs in the dbi+0x978 register. My current assumption that > either your IP-core is newer than v5.30a or has some vendor-specific > modification. But let's see the value first. Oops! I'm sorry for my bad code. After fixed the code, the values are: --- [ 1.108943] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 3532302a, +0x978 = 00000000 --- <snip> > > So, should I change the condition like below? > > > > --- > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { > > ... > > - } else if (val != 0xFFFFFFFF) { > > - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { > > --- > > Definitely no. Even though it's impossible to have the eDMA controller > configured with zero number of read and write channels we shouldn't > assume that getting a zero value from the DMA_CTRL_VIEWPORT_OFF offset > means having the unrolled eDMA CSRs mapping. Let's have a look at the > content of the dbi+0x8f8 and dbi+0x978 offsets first. Based on these > values we'll come up with what to do next. I got it. Best regards, Yoshihiro Shimoda > -Serge(y) > > > > > Best regards, > > Yoshihiro Shimoda > > > > > -Sergey > > > > > > > > } else { > > > > > return -ENODEV; > > > > > } > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம்
On Mon, Nov 28, 2022 at 12:41:11PM +0000, Yoshihiro Shimoda wrote: > Hi Serge, > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:59 PM > > > > On Mon, Nov 28, 2022 at 02:52:56AM +0000, Yoshihiro Shimoda wrote: > > > Hi Serge, > > > > > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:56 AM > > > > > <snip> > > > > What does the dbi+0x8f8 CSR contain in the host > > > > and EP registers space? Similarly could you also provide a content of > > > > the +0x978 register? > > > > > > The dbi+0x8f8 and the +0x978 registers' values are 0x00000000. > > > --------------- (sorry, replace tabs with spaces...)--------------- > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > > { > > > u32 val; > > > > > > > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > > > + dw_pcie_readl_dma(pci, 0x8f8), > > > + dw_pcie_readl_dma(pci, 0x978)); > > > + > > > > No, this should have been the dw_pcie_readl_dbi() calls instead of > > dw_pcie_readl_!dma!(). What I try to understand from these values is > > the real version of your controller (dbi+0x8f8) and whether the legacy > > eDMA viewport registers range follows the documented convention of > > having FFs in the dbi+0x978 register. My current assumption that > > either your IP-core is newer than v5.30a or has some vendor-specific > > modification. But let's see the value first. > > Oops! I'm sorry for my bad code. After fixed the code, the values are: > --- > [ 1.108943] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 3532302a, +0x978 = 00000000 > --- @Yoshihiro. Got it. Thanks. Could you please confirm (double-check) that what the content of the +0x978 CSR was really read by means of the dw_pcie_readl_dbi() method? @Mani, could you please have a look at the content of the +0x8f8 and +0x978 CSRs in the CDM space of the available to you controllers? I've reproduced the behavior what discovered by @Yoshihiro, but on v5.40a IP-core only. It was expected for that controller version, but v5.20a was supposed to have FFs in +0x978 for the unrolled iATU/eDMA space. It's strange to see it didn't. -Sergey > > <snip> > > > So, should I change the condition like below? > > > > > > --- > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { > > > ... > > > - } else if (val != 0xFFFFFFFF) { > > > - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { > > > --- > > > > Definitely no. Even though it's impossible to have the eDMA controller > > configured with zero number of read and write channels we shouldn't > > assume that getting a zero value from the DMA_CTRL_VIEWPORT_OFF offset > > means having the unrolled eDMA CSRs mapping. Let's have a look at the > > content of the dbi+0x8f8 and dbi+0x978 offsets first. Based on these > > values we'll come up with what to do next. > > I got it. > > Best regards, > Yoshihiro Shimoda > > > -Serge(y) > > > > > > > > Best regards, > > > Yoshihiro Shimoda > > > > > > > -Sergey > > > > > > > > > > } else { > > > > > > return -ENODEV; > > > > > > } > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > > > > > -- > > > > > மணிவண்ணன் சதாசிவம்
Hi Serge, > From: Serge Semin, Sent: Tuesday, November 29, 2022 1:11 AM > > On Mon, Nov 28, 2022 at 12:41:11PM +0000, Yoshihiro Shimoda wrote: > > Hi Serge, > > > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:59 PM > > > > > > On Mon, Nov 28, 2022 at 02:52:56AM +0000, Yoshihiro Shimoda wrote: > > > > Hi Serge, <snip> > > > No, this should have been the dw_pcie_readl_dbi() calls instead of > > > dw_pcie_readl_!dma!(). What I try to understand from these values is > > > the real version of your controller (dbi+0x8f8) and whether the legacy > > > eDMA viewport registers range follows the documented convention of > > > having FFs in the dbi+0x978 register. My current assumption that > > > either your IP-core is newer than v5.30a or has some vendor-specific > > > modification. But let's see the value first. > > > > > Oops! I'm sorry for my bad code. After fixed the code, the values are: > > --- > > [ 1.108943] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 3532302a, +0x978 = 00000000 > > --- > > @Yoshihiro. Got it. Thanks. Could you please confirm (double-check) > that what the content of the +0x978 CSR was really read by means of the > dw_pcie_readl_dbi() method? Yes, I used dw_pcie_readl_dbi() like below. --------------- (sorry, tabs replaced with spaces) --------------- --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) { u32 val; + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, + dw_pcie_readl_dbi(pci, 0x8f8), + dw_pcie_readl_dbi(pci, 0x978)); + if (pci->edma.reg_base) { pci->edma.mf = EDMA_MF_EDMA_UNROLL; ------------------------------------------------------------------ Best regards, Yoshihiro Shimoda > @Mani, could you please have a look at the content of the +0x8f8 and > +0x978 CSRs in the CDM space of the available to you controllers? > > I've reproduced the behavior what discovered by @Yoshihiro, but on > v5.40a IP-core only. It was expected for that controller version, but > v5.20a was supposed to have FFs in +0x978 for the unrolled iATU/eDMA > space. It's strange to see it didn't. > > -Sergey > > > > > <snip> > > > > So, should I change the condition like below? > > > > > > > > --- > > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > > + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { > > > > ... > > > > - } else if (val != 0xFFFFFFFF) { > > > > - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { > > > > --- > > > > > > Definitely no. Even though it's impossible to have the eDMA controller > > > configured with zero number of read and write channels we shouldn't > > > assume that getting a zero value from the DMA_CTRL_VIEWPORT_OFF offset > > > means having the unrolled eDMA CSRs mapping. Let's have a look at the > > > content of the dbi+0x8f8 and dbi+0x978 offsets first. Based on these > > > values we'll come up with what to do next. > > > > I got it. > > > > Best regards, > > Yoshihiro Shimoda > > > > > -Serge(y) > > > > > > > > > > > Best regards, > > > > Yoshihiro Shimoda > > > > > > > > > -Sergey > > > > > > > > > > > > } else { > > > > > > > return -ENODEV; > > > > > > > } > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > -- > > > > > > மணிவண்ணன் சதாசிவம்
Hi Serge, Manivannan, > From: Yoshihiro Shimoda, Sent: Tuesday, November 29, 2022 9:22 AM > > > From: Serge Semin, Sent: Tuesday, November 29, 2022 1:11 AM > > > > On Mon, Nov 28, 2022 at 12:41:11PM +0000, Yoshihiro Shimoda wrote: > > > Hi Serge, > > > > > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:59 PM > > > > > > > > On Mon, Nov 28, 2022 at 02:52:56AM +0000, Yoshihiro Shimoda wrote: > > > > > Hi Serge, > <snip> > > > > No, this should have been the dw_pcie_readl_dbi() calls instead of > > > > dw_pcie_readl_!dma!(). What I try to understand from these values is > > > > the real version of your controller (dbi+0x8f8) and whether the legacy > > > > eDMA viewport registers range follows the documented convention of > > > > having FFs in the dbi+0x978 register. My current assumption that > > > > either your IP-core is newer than v5.30a or has some vendor-specific > > > > modification. But let's see the value first. > > > > > > > > Oops! I'm sorry for my bad code. After fixed the code, the values are: > > > --- > > > [ 1.108943] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 3532302a, +0x978 = 00000000 > > > --- > > > > @Yoshihiro. Got it. Thanks. Could you please confirm (double-check) > > that what the content of the +0x978 CSR was really read by means of the > > dw_pcie_readl_dbi() method? > > Yes, I used dw_pcie_readl_dbi() like below. > > --------------- (sorry, tabs replaced with spaces) --------------- > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > { > u32 val; > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > + dw_pcie_readl_dbi(pci, 0x8f8), > + dw_pcie_readl_dbi(pci, 0x978)); > + > if (pci->edma.reg_base) { > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > ------------------------------------------------------------------ > > > @Mani, could you please have a look at the content of the +0x8f8 and > > +0x978 CSRs in the CDM space of the available to you controllers? > > > > I've reproduced the behavior what discovered by @Yoshihiro, but on > > v5.40a IP-core only. It was expected for that controller version, but > > v5.20a was supposed to have FFs in +0x978 for the unrolled iATU/eDMA > > space. It's strange to see it didn't. So, should I add a quirk flag for my controller like below? --- diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 69665a8a002e..384bd2c0ea75 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -844,7 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) u32 val; val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); - if (val == 0xFFFFFFFF && pci->edma.reg_base) { + if ((pci->no_dma_ctrl_reg || val == 0xFFFFFFFF) && pci->edma.reg_base) { pci->edma.mf = EDMA_MF_EDMA_UNROLL; val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); --- Best regards, Yoshihiro Shimoda > > -Sergey > > > > > > > > <snip> > > > > > So, should I change the condition like below? > > > > > > > > > > --- > > > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > > > + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { > > > > > ... > > > > > - } else if (val != 0xFFFFFFFF) { > > > > > - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { > > > > > --- > > > > > > > > Definitely no. Even though it's impossible to have the eDMA controller > > > > configured with zero number of read and write channels we shouldn't > > > > assume that getting a zero value from the DMA_CTRL_VIEWPORT_OFF offset > > > > means having the unrolled eDMA CSRs mapping. Let's have a look at the > > > > content of the dbi+0x8f8 and dbi+0x978 offsets first. Based on these > > > > values we'll come up with what to do next. > > > > > > I got it. > > > > > > Best regards, > > > Yoshihiro Shimoda > > > > > > > -Serge(y) > > > > > > > > > > > > > > Best regards, > > > > > Yoshihiro Shimoda > > > > > > > > > > > -Sergey > > > > > > > > > > > > > > } else { > > > > > > > > return -ENODEV; > > > > > > > > } > > > > > > > > -- > > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > மணிவண்ணன் சதாசிவம்
Cc += Frank Li @Frank could you have a look at the thread and check the content of the CSRs dbi+0x8f8 and dbi+0x978 on available to you DW PCIe +EDMA devices? On Thu, Dec 08, 2022 at 12:26:18PM +0000, Yoshihiro Shimoda wrote: > Hi Serge, Manivannan, > > > From: Yoshihiro Shimoda, Sent: Tuesday, November 29, 2022 9:22 AM > > > > > From: Serge Semin, Sent: Tuesday, November 29, 2022 1:11 AM > > > > > > On Mon, Nov 28, 2022 at 12:41:11PM +0000, Yoshihiro Shimoda wrote: > > > > Hi Serge, > > > > > > > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:59 PM > > > > > > > > > > On Mon, Nov 28, 2022 at 02:52:56AM +0000, Yoshihiro Shimoda wrote: > > > > > > Hi Serge, > > <snip> > > > > > No, this should have been the dw_pcie_readl_dbi() calls instead of > > > > > dw_pcie_readl_!dma!(). What I try to understand from these values is > > > > > the real version of your controller (dbi+0x8f8) and whether the legacy > > > > > eDMA viewport registers range follows the documented convention of > > > > > having FFs in the dbi+0x978 register. My current assumption that > > > > > either your IP-core is newer than v5.30a or has some vendor-specific > > > > > modification. But let's see the value first. > > > > > > > > > > > Oops! I'm sorry for my bad code. After fixed the code, the values are: > > > > --- > > > > [ 1.108943] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 3532302a, +0x978 = 00000000 > > > > --- > > > > > > @Yoshihiro. Got it. Thanks. Could you please confirm (double-check) > > > that what the content of the +0x978 CSR was really read by means of the > > > dw_pcie_readl_dbi() method? > > > > Yes, I used dw_pcie_readl_dbi() like below. > > > > --------------- (sorry, tabs replaced with spaces) --------------- > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > { > > u32 val; > > > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > > + dw_pcie_readl_dbi(pci, 0x8f8), > > + dw_pcie_readl_dbi(pci, 0x978)); > > + > > if (pci->edma.reg_base) { > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > ------------------------------------------------------------------ > > > > > @Mani, could you please have a look at the content of the +0x8f8 and > > > +0x978 CSRs in the CDM space of the available to you controllers? > > > > > > I've reproduced the behavior what discovered by @Yoshihiro, but on > > > v5.40a IP-core only. It was expected for that controller version, but > > > v5.20a was supposed to have FFs in +0x978 for the unrolled iATU/eDMA > > > space. It's strange to see it didn't. > > So, should I add a quirk flag for my controller like below? > --- > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 69665a8a002e..384bd2c0ea75 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -844,7 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > u32 val; > > val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > + if ((pci->no_dma_ctrl_reg || val == 0xFFFFFFFF) && pci->edma.reg_base) { > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); Very much no. I have just got rid from such boolean flags from the DW PCI private data. Please be patient. Maintainers not always respond as soon as we would want. Please note my patchset also stalls due to your discovery (DW eDMA patches still under review). What we have discovered here is the undocumented behavior. The HW-manuals from 4.80 up to 5.30 say that the register dbi+0x978 must have FFs if the register doesn't exist. A similar rule is defined for the iATU CSRs space and it's working well at least up to IP-core 5.40a. The viewport-based eDMA CSRs space has been removed from the HW-manuals since IP-core 5.40a and I can assure that IP-core 5.40a has zeros in dbi+0x978 on the real HW. I do have another devices with eDMA but all of them have been synthesized with the legacy viewport-based eDMA access, so I can't check whether the FFs statement is correct for the devices older than 5.40. You detected the problem on the IP-core 5.20a, but @Mani didn't see it on his devices. Neither does Frank AFAIU. That's why I asked him and @Frank to check what value the dbi+0x8f8 and dbi+0x978 registers have in their devices at hand. After that we'll either add the IP-core version based test here: < - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); < + if (dw_pcie_ver_is_ge(pci, 5?0A)) { < + val = 0xFFFFFFFF; < + else < + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); or add a new capability flag if @Mani has IP-core 5.20a with FFs in the CSRs dbi+0x978. Like this: < - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); < + if (dw_pcie_cap_is(pci, EDMA_UNROLL)) { < + val = 0xFFFFFFFF; < + else < + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); So the main goal of this activity is to check whether your discovery is a bug on your particular device or is a bug in the HW-manuals. If the later is correct then the eDMA CSRs space auto-detection procedure should be fixed to be executed up to the corresponding IP-core version. If the former is correct then we'll add a new capability flag. In anyway having the new boolean field in the device private data wouldn't be a good option since there is the capabilities API available in the driver. -Serge(y) > --- > > Best regards, > Yoshihiro Shimoda > > > > -Sergey > > > > > > > > > > > <snip> > > > > > > So, should I change the condition like below? > > > > > > > > > > > > --- > > > > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > > > > + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { > > > > > > ... > > > > > > - } else if (val != 0xFFFFFFFF) { > > > > > > - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { > > > > > > --- > > > > > > > > > > Definitely no. Even though it's impossible to have the eDMA controller > > > > > configured with zero number of read and write channels we shouldn't > > > > > assume that getting a zero value from the DMA_CTRL_VIEWPORT_OFF offset > > > > > means having the unrolled eDMA CSRs mapping. Let's have a look at the > > > > > content of the dbi+0x8f8 and dbi+0x978 offsets first. Based on these > > > > > values we'll come up with what to do next. > > > > > > > > I got it. > > > > > > > > Best regards, > > > > Yoshihiro Shimoda > > > > > > > > > -Serge(y) > > > > > > > > > > > > > > > > > Best regards, > > > > > > Yoshihiro Shimoda > > > > > > > > > > > > > -Sergey > > > > > > > > > > > > > > > > } else { > > > > > > > > > return -ENODEV; > > > > > > > > > } > > > > > > > > > -- > > > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > மணிவண்ணன் சதாசிவம்
Hi Serge, > From: Serge Semin, Sent: Thursday, December 8, 2022 11:01 PM > > Cc += Frank Li > > @Frank could you have a look at the thread and check the content of > the CSRs dbi+0x8f8 and dbi+0x978 on available to you DW PCIe +EDMA > devices? > > On Thu, Dec 08, 2022 at 12:26:18PM +0000, Yoshihiro Shimoda wrote: > > Hi Serge, Manivannan, > > > > > From: Yoshihiro Shimoda, Sent: Tuesday, November 29, 2022 9:22 AM > > > > > > > From: Serge Semin, Sent: Tuesday, November 29, 2022 1:11 AM > > > > > > > > On Mon, Nov 28, 2022 at 12:41:11PM +0000, Yoshihiro Shimoda wrote: > > > > > Hi Serge, > > > > > > > > > > > From: Serge Semin, Sent: Monday, November 28, 2022 8:59 PM > > > > > > > > > > > > On Mon, Nov 28, 2022 at 02:52:56AM +0000, Yoshihiro Shimoda wrote: > > > > > > > Hi Serge, > > > <snip> > > > > > > No, this should have been the dw_pcie_readl_dbi() calls instead of > > > > > > dw_pcie_readl_!dma!(). What I try to understand from these values is > > > > > > the real version of your controller (dbi+0x8f8) and whether the legacy > > > > > > eDMA viewport registers range follows the documented convention of > > > > > > having FFs in the dbi+0x978 register. My current assumption that > > > > > > either your IP-core is newer than v5.30a or has some vendor-specific > > > > > > modification. But let's see the value first. > > > > > > > > > > > > > > Oops! I'm sorry for my bad code. After fixed the code, the values are: > > > > > --- > > > > > [ 1.108943] pcie-rcar-gen4 e65d0000.pcie: dw_pcie_edma_find_chip: +0x8f8 = 3532302a, +0x978 = 00000000 > > > > > --- > > > > > > > > @Yoshihiro. Got it. Thanks. Could you please confirm (double-check) > > > > that what the content of the +0x978 CSR was really read by means of the > > > > dw_pcie_readl_dbi() method? > > > > > > Yes, I used dw_pcie_readl_dbi() like below. > > > > > > --------------- (sorry, tabs replaced with spaces) --------------- > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -843,6 +843,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > > { > > > u32 val; > > > > > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > > > + dw_pcie_readl_dbi(pci, 0x8f8), > > > + dw_pcie_readl_dbi(pci, 0x978)); > > > + > > > if (pci->edma.reg_base) { > > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > > ------------------------------------------------------------------ > > > > > > > @Mani, could you please have a look at the content of the +0x8f8 and > > > > +0x978 CSRs in the CDM space of the available to you controllers? > > > > > > > > I've reproduced the behavior what discovered by @Yoshihiro, but on > > > > v5.40a IP-core only. It was expected for that controller version, but > > > > v5.20a was supposed to have FFs in +0x978 for the unrolled iATU/eDMA > > > > space. It's strange to see it didn't. > > > > > So, should I add a quirk flag for my controller like below? > > --- > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 69665a8a002e..384bd2c0ea75 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -844,7 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > u32 val; > > > > val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > + if ((pci->no_dma_ctrl_reg || val == 0xFFFFFFFF) && pci->edma.reg_base) { > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > Very much no. I have just got rid from such boolean flags from the DW > PCI private data. > > Please be patient. Maintainers not always respond as soon as we would > want. Please note my patchset also stalls due to your discovery (DW > eDMA patches still under review). > > What we have discovered here is the undocumented behavior. The > HW-manuals from 4.80 up to 5.30 say that the register dbi+0x978 must > have FFs if the register doesn't exist. A similar rule is defined for > the iATU CSRs space and it's working well at least up to IP-core > 5.40a. The viewport-based eDMA CSRs space has been removed from the > HW-manuals since IP-core 5.40a and I can assure that IP-core 5.40a has > zeros in dbi+0x978 on the real HW. I do have another devices with eDMA > but all of them have been synthesized with the legacy viewport-based > eDMA access, so I can't check whether the FFs statement is correct for > the devices older than 5.40. You detected the problem on the IP-core > 5.20a, but @Mani didn't see it on his devices. Neither does Frank > AFAIU. That's why I asked him and @Frank to check what value the > dbi+0x8f8 and dbi+0x978 registers have in their devices at hand. After > that we'll either add the IP-core version based test here: > < - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > < + if (dw_pcie_ver_is_ge(pci, 5?0A)) { > < + val = 0xFFFFFFFF; > < + else > < + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > or add a new capability flag if @Mani has IP-core 5.20a with FFs in > the CSRs dbi+0x978. Like this: > < - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > < + if (dw_pcie_cap_is(pci, EDMA_UNROLL)) { > < + val = 0xFFFFFFFF; > < + else > < + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > So the main goal of this activity is to check whether your discovery > is a bug on your particular device or is a bug in the HW-manuals. If > the later is correct then the eDMA CSRs space auto-detection procedure > should be fixed to be executed up to the corresponding IP-core > version. If the former is correct then we'll add a new capability > flag. In anyway having the new boolean field in the device private > data wouldn't be a good option since there is the capabilities API > available in the driver. Thank you very much for your detailed explanation! I understood. # At least, I should have realized that the PCI dwc driver has # the capabilities API before I send an email... Best regards, Yoshihiro Shimoda > -Serge(y) > > > --- > > > > Best regards, > > Yoshihiro Shimoda > > > > > > -Sergey > > > > > > > > > > > > > > <snip> > > > > > > > So, should I change the condition like below? > > > > > > > > > > > > > > --- > > > > > > > - if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > > > > > + if ((val == 0xFFFFFFFF || val == 0x00000000) && pci->edma.reg_base) { > > > > > > > ... > > > > > > > - } else if (val != 0xFFFFFFFF) { > > > > > > > - } else if (!(val == 0xFFFFFFFF || val == 0x00000000)) { > > > > > > > --- > > > > > > > > > > > > Definitely no. Even though it's impossible to have the eDMA controller > > > > > > configured with zero number of read and write channels we shouldn't > > > > > > assume that getting a zero value from the DMA_CTRL_VIEWPORT_OFF offset > > > > > > means having the unrolled eDMA CSRs mapping. Let's have a look at the > > > > > > content of the dbi+0x8f8 and dbi+0x978 offsets first. Based on these > > > > > > values we'll come up with what to do next. > > > > > > > > > > I got it. > > > > > > > > > > Best regards, > > > > > Yoshihiro Shimoda > > > > > > > > > > > -Serge(y) > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > Yoshihiro Shimoda > > > > > > > > > > > > > > > -Sergey > > > > > > > > > > > > > > > > > > } else { > > > > > > > > > > return -ENODEV; > > > > > > > > > > } > > > > > > > > > > -- > > > > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > மணிவண்ணன் சதாசிவம்
Hi Frank On Fri, Dec 09, 2022 at 03:52:42PM +0000, Frank Li wrote: > Hi Serge, > > > From: Serge Semin, Sent: Thursday, December 8, 2022 11:01 PM > > > > Cc += Frank Li > > > > @Frank could you have a look at the thread and check the content of > > the CSRs dbi+0x8f8 and dbi+0x978 on available to you DW PCIe +EDMA > > devices? > > [ 2.598038] imx6q-pcie 5f010000.pcie_ep: imx_add_pcie_ep: +0x8f8 = 3438302a, +0x978 = 00010001 Thanks for the reply. So it's 4.80a with the legacy viewport-based access. Alas it isn't what we need in this thread. We'll need @Mani's respond in order to decide how to fix the auto-detection procedure. -Serge(y) > > Frank Li > >
Hi Serge, On Sun, Dec 11, 2022 at 06:28:49PM +0300, Serge Semin wrote: > Hi Frank > > On Fri, Dec 09, 2022 at 03:52:42PM +0000, Frank Li wrote: > > Hi Serge, > > > > > From: Serge Semin, Sent: Thursday, December 8, 2022 11:01 PM > > > > > > Cc += Frank Li > > > > > > @Frank could you have a look at the thread and check the content of > > > the CSRs dbi+0x8f8 and dbi+0x978 on available to you DW PCIe +EDMA > > > devices? > > > > > [ 2.598038] imx6q-pcie 5f010000.pcie_ep: imx_add_pcie_ep: +0x8f8 = 3438302a, +0x978 = 00010001 > > Thanks for the reply. So it's 4.80a with the legacy viewport-based > access. Alas it isn't what we need in this thread. We'll need > @Mani's respond in order to decide how to fix the auto-detection > procedure. > Sorry for the late reply! With below diff on the EP: diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 6f3805228a18..0eb4d3218738 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -665,6 +665,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) if (val == 0xFFFFFFFF && pci->edma.reg_base) { pci->edma.mf = EDMA_MF_EDMA_UNROLL; + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, + dw_pcie_readl_dbi(pci, 0x8f8), + dw_pcie_readl_dbi(pci, 0x978)); + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); } else if (val != 0xFFFFFFFF) { pci->edma.mf = EDMA_MF_EDMA_LEGACY; The output was: qcom-pcie-ep 1c08000.pcie-ep: dw_pcie_edma_find_chip: +0x8f8 = 3533302a, +0x978 = ffffffff Hope this helps! Thanks, Mani > -Serge(y) > > > > > Frank Li > > > >
On Mon, Dec 12, 2022 at 06:26:58PM +0530, Manivannan Sadhasivam wrote: > Hi Serge, > > On Sun, Dec 11, 2022 at 06:28:49PM +0300, Serge Semin wrote: > > Hi Frank > > > > On Fri, Dec 09, 2022 at 03:52:42PM +0000, Frank Li wrote: > > > Hi Serge, > > > > > > > From: Serge Semin, Sent: Thursday, December 8, 2022 11:01 PM > > > > > > > > Cc += Frank Li > > > > > > > > @Frank could you have a look at the thread and check the content of > > > > the CSRs dbi+0x8f8 and dbi+0x978 on available to you DW PCIe +EDMA > > > > devices? > > > > > > > > [ 2.598038] imx6q-pcie 5f010000.pcie_ep: imx_add_pcie_ep: +0x8f8 = 3438302a, +0x978 = 00010001 > > > > Thanks for the reply. So it's 4.80a with the legacy viewport-based > > access. Alas it isn't what we need in this thread. We'll need > > @Mani's respond in order to decide how to fix the auto-detection > > procedure. > > > > Sorry for the late reply! > > With below diff on the EP: > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 6f3805228a18..0eb4d3218738 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -665,6 +665,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > if (val == 0xFFFFFFFF && pci->edma.reg_base) { > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > + dw_pcie_readl_dbi(pci, 0x8f8), > + dw_pcie_readl_dbi(pci, 0x978)); > + > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > } else if (val != 0xFFFFFFFF) { > pci->edma.mf = EDMA_MF_EDMA_LEGACY; > > > The output was: > > qcom-pcie-ep 1c08000.pcie-ep: dw_pcie_edma_find_chip: +0x8f8 = 3533302a, +0x978 = ffffffff > > Hope this helps! Great! Thanks. This indeed helps. So it's 5.30a IP-core. Just one quick question. Does that device have eDMA embedded into the DW PCIe controller? -Serge(y) > > Thanks, > Mani > > > -Serge(y) > > > > > > > > Frank Li > > > > > > > > -- > மணிவண்ணன் சதாசிவம்
On Mon, Dec 12, 2022 at 07:56:00PM +0300, Serge Semin wrote: > On Mon, Dec 12, 2022 at 06:26:58PM +0530, Manivannan Sadhasivam wrote: > > Hi Serge, > > > > On Sun, Dec 11, 2022 at 06:28:49PM +0300, Serge Semin wrote: > > > Hi Frank > > > > > > On Fri, Dec 09, 2022 at 03:52:42PM +0000, Frank Li wrote: > > > > Hi Serge, > > > > > > > > > From: Serge Semin, Sent: Thursday, December 8, 2022 11:01 PM > > > > > > > > > > Cc += Frank Li > > > > > > > > > > @Frank could you have a look at the thread and check the content of > > > > > the CSRs dbi+0x8f8 and dbi+0x978 on available to you DW PCIe +EDMA > > > > > devices? > > > > > > > > > > > [ 2.598038] imx6q-pcie 5f010000.pcie_ep: imx_add_pcie_ep: +0x8f8 = 3438302a, +0x978 = 00010001 > > > > > > Thanks for the reply. So it's 4.80a with the legacy viewport-based > > > access. Alas it isn't what we need in this thread. We'll need > > > @Mani's respond in order to decide how to fix the auto-detection > > > procedure. > > > > > > > > Sorry for the late reply! > > > > With below diff on the EP: > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 6f3805228a18..0eb4d3218738 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -665,6 +665,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > > + dw_pcie_readl_dbi(pci, 0x8f8), > > + dw_pcie_readl_dbi(pci, 0x978)); > > + > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > } else if (val != 0xFFFFFFFF) { > > pci->edma.mf = EDMA_MF_EDMA_LEGACY; > > > > > > The output was: > > > > qcom-pcie-ep 1c08000.pcie-ep: dw_pcie_edma_find_chip: +0x8f8 = 3533302a, +0x978 = ffffffff > > > > Hope this helps! > > Great! Thanks. This indeed helps. So it's 5.30a IP-core. Just one > quick question. Does that device have eDMA embedded into the DW PCIe > controller? > Yes it is and it is the test platform I use for eDMA/PCI_EP work. Thanks, Mani > -Serge(y) > > > > > Thanks, > > Mani > > > > > -Serge(y) > > > > > > > > > > > Frank Li > > > > > > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
On Mon, Dec 12, 2022 at 10:41:02PM +0530, Manivannan Sadhasivam wrote: > On Mon, Dec 12, 2022 at 07:56:00PM +0300, Serge Semin wrote: > > On Mon, Dec 12, 2022 at 06:26:58PM +0530, Manivannan Sadhasivam wrote: > > > Hi Serge, > > > > > > On Sun, Dec 11, 2022 at 06:28:49PM +0300, Serge Semin wrote: > > > > Hi Frank > > > > > > > > On Fri, Dec 09, 2022 at 03:52:42PM +0000, Frank Li wrote: > > > > > Hi Serge, > > > > > > > > > > > From: Serge Semin, Sent: Thursday, December 8, 2022 11:01 PM > > > > > > > > > > > > Cc += Frank Li > > > > > > > > > > > > @Frank could you have a look at the thread and check the content of > > > > > > the CSRs dbi+0x8f8 and dbi+0x978 on available to you DW PCIe +EDMA > > > > > > devices? > > > > > > > > > > > > > > [ 2.598038] imx6q-pcie 5f010000.pcie_ep: imx_add_pcie_ep: +0x8f8 = 3438302a, +0x978 = 00010001 > > > > > > > > Thanks for the reply. So it's 4.80a with the legacy viewport-based > > > > access. Alas it isn't what we need in this thread. We'll need > > > > @Mani's respond in order to decide how to fix the auto-detection > > > > procedure. > > > > > > > > > > > > Sorry for the late reply! > > > > > > With below diff on the EP: > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > index 6f3805228a18..0eb4d3218738 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -665,6 +665,10 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) > > > if (val == 0xFFFFFFFF && pci->edma.reg_base) { > > > pci->edma.mf = EDMA_MF_EDMA_UNROLL; > > > > > > + dev_info(pci->dev, "%s: +0x8f8 = %08x, +0x978 = %08x\n", __func__, > > > + dw_pcie_readl_dbi(pci, 0x8f8), > > > + dw_pcie_readl_dbi(pci, 0x978)); > > > + > > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > } else if (val != 0xFFFFFFFF) { > > > pci->edma.mf = EDMA_MF_EDMA_LEGACY; > > > > > > > > > The output was: > > > > > > qcom-pcie-ep 1c08000.pcie-ep: dw_pcie_edma_find_chip: +0x8f8 = 3533302a, +0x978 = ffffffff > > > > > > Hope this helps! > > > > Great! Thanks. This indeed helps. So it's 5.30a IP-core. Just one > > quick question. Does that device have eDMA embedded into the DW PCIe > > controller? > > > > Yes it is and it is the test platform I use for eDMA/PCI_EP work. So the procedure works well for IP-core 5.30a and AFAICS it doesn't for 5.40a (eDMA viewport-based CSRs are missing in the HW-manual) and for an unexpected reason in IP-core 5.20a synthesized for Renesas R-Car Gen4 PCIe. Thus this seems more like a vendor-specific problem, than a version-specific one since the HW-manual in both 5.20a and 5.30a cases state that the dbi+0x978 register must have FFs if the CSR doesn't exist. It doesn't exist if the next statement is false: !CX_PL_REG_DISABLE && CC_DMA_ENABLE && !CC_UNROLL_ENABLE && CC_DEVICE_TYPE!=3 So seeing the R-Car Gen4 PCIe has the unrolled eDMA mapping the dbi+0x978 registers must contain FFs. The best solution in this case would be to have a special capability flag which would force the unrolled eDMA mapping for the problematic devices. Like this: --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -840,8 +840,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 some devices for unknown reason may 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); --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -52,7 +52,8 @@ /* 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 2 +#define DW_PCIE_CAP_CDM_CHECK 3 #define dw_pcie_cap_is(_pci, _cap) \ test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps) The patch above is based on the updated version of my patchset, which I'll resubmit for review tomorrow. I'll add @Yoshihiro in Cc-list of the series. -Serge(y) > > Thanks, > Mani > > > -Serge(y) > > > > > > > > Thanks, > > > Mani > > > > > > > -Serge(y) > > > > > > > > > > > > > > Frank Li > > > > > > > > > > > > > > > > -- > > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 637d01807c67..2cc8584da6f4 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -836,8 +836,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) { u32 val; - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); - if (val == 0xFFFFFFFF && pci->edma.reg_base) { + if (pci->edma.reg_base) { pci->edma.mf = EDMA_MF_EDMA_UNROLL; val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); @@ -845,6 +844,7 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci) pci->edma.mf = EDMA_MF_EDMA_LEGACY; pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE; + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); } else { return -ENODEV; }
Since reading value of PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL was 0x00000000 on one of SoCs (R-Car S4-8), it cannot find the eDMA. So, directly read the eDMA register if edma.reg_base is not zero. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pci/controller/dwc/pcie-designware.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)