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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux