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