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