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,

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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux