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