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

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

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