Hi Arnd,
On Wed, Sep 22, 2021 at 9:57 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Wed, Sep 22, 2021 at 8:40 PM Sergio Paracuellos
> <sergio.paracuellos@xxxxxxxxx> wrote:
> > On Wed, Sep 22, 2021 at 8:07 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > 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.
>
> > > 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?
>
> 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...
>
> > >
> > > I don't yet understand how you deal with having multiple PCIe
> > > host bridge devices if they have distinct I/O port ranges.
> > > Without remapping to dynamic virtual addresses, does
> > > that mean that every MMIO register between the first and
> > > last PCIe bridge also shows up in /dev/ioport? Or do you
> > > only support port I/O on the first PCIe host bridge?
> >
> > For example, this board is using all available three pci ports [2] and I get:
> >
> > root@gnubee:~# cat /proc/ioports
> > 1e160000-1e16ffff : pcie@1e140000
> > 1e160000-1e160fff : PCI Bus 0000:01
> > 1e160000-1e16000f : 0000:01:00.0
> > 1e160000-1e16000f : ahci
> > 1e160010-1e160017 : 0000:01:00.0
> > 1e160010-1e160017 : ahci
> > 1e160018-1e16001f : 0000:01:00.0
> > 1e160018-1e16001f : ahci
> > 1e160020-1e160023 : 0000:01:00.0
> > 1e160020-1e160023 : ahci
> > 1e160024-1e160027 : 0000:01:00.0
> > 1e160024-1e160027 : ahci
> > 1e161000-1e161fff : PCI Bus 0000:02
> > 1e161000-1e16100f : 0000:02:00.0
> > 1e161000-1e16100f : ahci
> > 1e161010-1e161017 : 0000:02:00.0
> > 1e161010-1e161017 : ahci
> > 1e161018-1e16101f : 0000:02:00.0
> > 1e161018-1e16101f : ahci
> > 1e161020-1e161023 : 0000:02:00.0
> > 1e161020-1e161023 : ahci
> > 1e161024-1e161027 : 0000:02:00.0
> > 1e161024-1e161027 : ahci
> > 1e162000-1e162fff : PCI Bus 0000:03
> > 1e162000-1e16200f : 0000:03:00.0
> > 1e162000-1e16200f : ahci
> > 1e162010-1e162017 : 0000:03:00.0
> > 1e162010-1e162017 : ahci
> > 1e162018-1e16201f : 0000:03:00.0
> > 1e162018-1e16201f : ahci
> > 1e162020-1e162023 : 0000:03:00.0
> > 1e162020-1e162023 : ahci
> > 1e162024-1e162027 : 0000:03:00.0
> > 1e162024-1e162027 : ahci
>
> Ah ok, so there are I/O ports that are at least
> visible (may or may not be accessed by the driver).
Yes, all of them are enumerated on boot and exposed in /proc/ioports.
>
> 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.
>
> 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.
Best regards,
Sergio Paracuellos
>
> Arnd
[0]: https://lore.kernel.org/linux-staging/CAMhs-H-OJyXs+QViJa5_O3wUGhoupmaW4qvVG8WGjxm1haRj9Q@xxxxxxxxxxxxxx/T/#m9e8e8d3afca908d1e1c57539d5e03dd9f5efd850
[1]: https://www.spinics.net/lists/kernel/msg4085596.html
