On Fri, Sep 24, 2021 at 12:15 PM Sergio Paracuellos
[off-list ref] wrote:

On Fri, Sep 24, 2021 at 10:53 AM Arnd Bergmann [off-list ref] wrote:

quoted

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@1e140000 ranges:
mt7621-pci 1e140000.pcie:   No bus range found for /pcie@1e140000,
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.

quoted

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).

quoted

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