Re: [PATCH v7 5/9] PCI: dwc: Avoid reading a register to detect whether eDMA exists

[Serge Semin <fancer.lancer@gmail.com>]

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