Date:   Fri, 24 Sep 2021 13:39:30 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Sergio Paracuellos <sergio.paracuellos@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        linux-pci <linux-pci@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-staging@...ts.linux.dev, gregkh <gregkh@...uxfoundation.org>,
        Liviu Dudau <Liviu.Dudau@....com>,
        Rob Herring <robh@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Subject: Re: [PATCH v3] PCI: of: Avoid pci_remap_iospace() when PCI_IOBASE not defined

On Fri, Sep 24, 2021 at 12:15 PM Sergio Paracuellos
<sergio.paracuellos@...il.com> wrote:
> On Fri, Sep 24, 2021 at 10:53 AM Arnd Bergmann <arnd@...db.de> wrote:
> > On Thu, Sep 23, 2021 at 10:33 PM Sergio Paracuellos
> >
> > Oops, my mistake, I mixed up the CPU address and the PCI address.
> >
> > The correct notation should be
> >
> > <0x01000000 0 0  0 0x1e160000 0x00010000>;
> >
> > i.e. bus address 0 to cpu address 0x1e160000, rather than the other
> > way around as I wrote it.
>
> Mmmm... Do you mean <0x01000000 0 0  0x1e160000 0 0x00010000>; instead?

Yes, sorry for getting it wrong again.

> In any case both of them ends up in a similar trace (the one listed
> here is using your last range as it is):
>
> mt7621-pci 1e140000.pcie: host bridge /pcie@...40000 ranges:
> mt7621-pci 1e140000.pcie:   No bus range found for /pcie@...40000,
> using [bus 00-ff]
> mt7621-pci 1e140000.pcie:      MEM 0x0060000000..0x006fffffff -> 0x0060000000
> mt7621-pci 1e140000.pcie:       IO 0x0000000000..0x1e1600000000ffff ->
> 0x0000000000
>                                                    ^^^^
>                                                     This is the part
> that change between both traces (t7621-pci 1e140000.pcie:       IO
>
> 0x001e160000..0x001e16ffff -> 0x0000000000 in the one I have just put
> above)

Yes, the latter is the expected output.

> mt7621-pci 1e140000.pcie: Writing 0x0 in RALINK_PCI_IOBASE
>                                                   ^^^
>                                                      This is the
> current value written in bridge register as IOBASE for the window
> which is IORESOURCE_IO -> res.start address (in both traces).

The value is correct, but strictly speaking this must be the
raw value from DT, not the translated Linux port number.

As long as io_offset is zero, the two are the same, but if you were
to use multiple host bridge in the system, or pick a different bus
address in DT, you can have a nonzero io_offset. I think this means

   pcie_write(pcie, entry->res->start, RALINK_PCI_IOBASE);

should become

   pcie_write(pcie, entry->res->start + entry->offset, RALINK_PCI_IOBASE);

Try setting some other value as the bus address in DT and see if that
is what gets written to the register.
(I may have the polarity of offset wrong, so this may need to be '-' instead
of '+').

> Also, I noticed that all of these messages have disappeared from the trace:
>
> pci 0000:03:00.0: reg 0x20: initial BAR value 0x00000000 invalid
>
> and now the first tested value seems to be valid....

This just means that the power-on default value of '0' is now within
the configured range of 0...fffff. It's still slightly odd that it warns about
those in the first place if it is going to reassign everything, but that
should be harmless either way.

> > If my assumption is correct here, then you must write the value
> > that you have read from the DT property, which would be
> > 0x001e160000 or 0 in the two versions of the DT property we
> > have listed, but in theory any (properly aligned) number ought
> > to work here, as long as the BAR values, the RALINK_PCI_IOBASE
> > and the io_offset all agree what it is.
>
> RALINK_PCI_IOBASE in that line, is the bridge register you write with
> value from the IO resource created from DT property, yes. So I guess
> you meant PCI_IOBASE for ralink in this sentence, right?
>
> (Prototype of the function -> static inline void pcie_write(struct
> mt7621_pcie *pcie, u32 val, u32 reg))

I meant RALINK_PCI_IOBASE. We do need to write both, to clarify:

RALINK_PCI_IOBASE must be set to match the *bus* address in DT,
so ideally '0', but any value should work as long as these two match.

PCI_IOBASE/mips_io_port_base must be set to the *CPU* address
in DT, so that must be 0x1e160000, possibly converted from
physical to a virtual __iomem address (this is where my MIPS
knowledge ends).

> > pcie_write(pcie, 0xffffffff, RALINK_PCI_MEMBASE)
> >
> > Can you clarify what this does? I would have expected an
> > identity-map for the memory space, which would mean writing
> > the third cell from the ranges property (0x60000000)
> > into this. Is -1 a special value for identity mapping, or does
> > this register do something else?
>
> That was also my understanding at first when I took this code and
> started playing with it since the register is the base address for the
> memory space window. Setting the value you pointed out worked for me
> (and makes sense), but some people seem to have problems with some
> cards when accessing PCI memory resources. That's the only reason I
> have maintained the original value but I don't really understand what
> it means. The same value is used in mt7620 which has a pretty similar
> topology but with one only virtual bridge behind the host-bridge [0].
> However there is no documentation for mt7621 PCI subsystem and the one
> that exists for mt7620 PCI does not clarify much more [1].

Ok, fair enough.

          Arnd

Powered by blists - more mailing lists