isOn Wed, Sep 22, 2021 at 10:55 PM Sergio Paracuellos
[off-list ref] wrote:

On Wed, Sep 22, 2021 at 9:57 PM Arnd Bergmann [off-list ref] wrote:

quoted

On Wed, Sep 22, 2021 at 8:40 PM Sergio Paracuellos
[off-list ref] wrote:

quoted

On Wed, Sep 22, 2021 at 8:07 PM Arnd Bergmann [off-list ref] wrote:

quoted

On Wed, Sep 22, 2021 at 7:42 PM Sergio Paracuellos
Ok, thank you for the detailed explanation.

I suppose you can use the generic infrastructure in asm-generic/io.h
if you "#define PCI_IOBASE mips_io_port_base". In this case, you
should have an architecture specific implementation of
pci_remap_iospace() that assigns mips_io_port_base.

No, that is what I tried originally defining PCI_IOBASE as
_AC(0xa0000000, UL) [0] which is the same as KSEG1 [1] that ends in
'mips_io_port_base'.

Defining it as KSEG1 would be problematic because that means that
the Linux-visible port numbers are offset from the bus-visible ones.

You really want PCI_IOBASE to point to the address of port 0.

Do you mean that doing

#define PCI_IOBASE mips_io_port_base

would have different result that doing what I did

#define PCI_IOBASE _AC(0xa0000000, UL)

?

I am not really understanding this yet (I think I need a bit of sleep
time :)), but I will test this tomorrow and come back to you again
with results.

Both would let devices access the registers, but they are different
regarding the bus translations you have to program into the
host bridge, and how to access the hardcoded port numbers.

quoted

quoted

quoted

pci_remap_iospace() was originally meant as an architecture
specific helper, but it moved into generic code after all architectures
had the same requirements. If MIPS has different requirements,
then it should not be shared.

I see. So, if it can not be shared, would defining 'pci_remap_iospace'
as 'weak' acceptable? Doing in this way I guess I can redefine the
symbol for mips to have the same I currently have but without the
ifdef in the core APIs...

I would hope to kill off the __weak functions, and prefer using an #ifdef
around the generic implementation. One way to do it is to define
a macro with the same name, such as

#define pci_remap_iospace pci_remap_iospace

I guess this should be defined in arch/mips/include/asm/pci.h?

Yes, that would be a good place for that, possibly next to
the (static inline) definition.

quoted

and then use #ifdef around the C function to see if that's already defined.

I see. That would work, I guess. But I don't really understand why
this approach would be better than this patch changes itself. Looks
more hacky to me. As Bjorn pointed out in a previous version of this
patch [0], this case is the same as the one in
'acpi_pci_root_remap_iospace' and the same approach is used there...

The acpi_pci_root_remap_iospace() does this because on that code is
shared with x86 and ia64, where the port numbers are accessed using
separate instructions that do not translate into MMIO addresses at all.

On MIPS, the port access eventually does translate into MMIO, and
you need a way to communicate the mapping between the host
bridge and the architecture specific code.

This is particularly important since we want the host bridge driver
to be portable. If you set up the mapping differently between e.g.
mt7621 and mt7623, they are not able to use the same driver
code for setting pci_host_bridge->io_offset and for programming
the inbound translation registers.

quoted

I only see one host bridge here though, and it has a single
I/O port range, so maybe all three ports are inside of
a single PCI domain?

See this cover letter [1] with a fantastic ascii art :) to a better
understanding of this pci topology. Yes, there is one host bridge and
from here three virtual bridges where at most three endpoints can be
connected.

Ok, so you don't have the problem I was referring to. A lot of
SoCs actually have multiple host bridges, but only one root
port per host bridge, because they are based on licensed IP
blocks that don't support a normal topology like the one you have.

quoted

Having high numbers for the I/O ports is definitely a
problem as I mentioned. Anything that tries to access
PC-style legacy devices on the low port numbers
will now directly go on the bus accessing MMIO
registers that it shouldn't, either causing a CPU exception
or (worse) undefined behavior from random register
accesses.

All I/O port addresses for ralink SoCs are in higher addresses than
default IO_SPACE_LIMIT 0xFFFF, that's why we have to also change this
limit together with this patch changes. Nothing to do with this, is an
architectural thing of these SoCs.

I don't understand. What I see above is that the host bridge
has the region 1e160000-1e16ffff registered, so presumably
1e160000 is actually the start of the window into the host bridge.
If you set PCI_IOBASE to that location, the highest port number
would become 0x2027, which is under 0xffff.

       Arnd