Hello,

On Linux 6.13-rc6 for mipsel in QEMU on the Malta platform, the RTC CMOS
driver does not load and /sys/class/rtc is empty. I have tested this with
"make malta_defconfig", which compiles this driver into the kernel 
(CONFIG_RTC_DRV_CMOS=y).

I have bisected this down to:

commit 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
Author: Jiaxun Yang<jiaxun.yang@flygoat.com>
Date:   Thu Sep 21 19:04:22 2023 +0800

     mips: add <asm-generic/io.h> including
     With the adding, some default ioremap_xx methods defined in
     asm-generic/io.h can be used. E.g the default ioremap_uc() returning
     NULL.
     We also massaged various headers to avoid nested includes.

I have tried to debug this.

The fallout is apparently limited to the CMOS RTC driver, other
drivers that access IO ports seem to function correctly (e.g. the
PATA driver). Also, the read_persistent_clock64 function in
arch/mips/mti-malta/malta-time.c, which accesses the same hardware
works correctly.

The CMOS RTC driver is likely special because this device is defined
in a devicetree (arch/mips/boot/dts/mti/malta.dts) and there it is
the only defined device on the ISA bus.

That driver fails to load because the call to

platform_get_resource(pdev, IORESOURCE_IO, 0);

in cmos_platform_probe in drivers/rtc/rtc-cmos.c returns NULL.

The mediator seems to be that this bad commit causes 
arch/mips/include/asm/io.h
to #include <asm-generic/io.h> at the end. As a side effect, this causes
the PCI_IOBASE macro to be defined:

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *)0)
#endif

That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
defined to mips_io_port_base as far as I can tell), but this does not 
matter much here.

When that macro is defined, pci_address_to_pio() in pci.c calls the
logic_pio_trans_cpuaddr() function, which fails. Removing that ifdef
in this function "fixes" the issue and allows that driver to load and work
apparently correctly:

----------8<------------------
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 661f98c6c63a..368cd9ca6801 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4213,14 +4213,10 @@ EXPORT_SYMBOL_GPL(pci_pio_to_address);
unsigned long __weak pci_address_to_pio(phys_addr_t address)
{
-#ifdef PCI_IOBASE
- return logic_pio_trans_cpuaddr(address);
-#else
if (address > IO_SPACE_LIMIT)
return (unsigned long)-1;
return (unsigned long) address;
-#endif
}
/**
----------8<------------------

Additionally, the following message appears in dmesg on affected kernels:

LOGIC PIO: addr 0x00000070 not registered in io_range_list

(0x0070 is the IO port address of the CMOS RTC).

When I added dump_stack() in logic_pio_trans_cpuaddr(), which prints 
this warning,
I got the following:

----------8<------------------
Call Trace:
[<801185c8>] show_stack+0x38/0x118
[<8010be24>] dump_stack_lvl+0x7c/0xbc
[<80841060>] logic_pio_trans_cpuaddr+0x88/0x98
[<80604bec>] __of_address_to_resource+0x208/0x228
[<805ff45c>] of_device_alloc+0x7c/0x1ac
[<805ff778>] of_platform_device_create_pdata+0x60/0xf8
[<805ff9cc>] of_platform_bus_create+0x1b0/0x238
[<805ffa14>] of_platform_bus_create+0x1f8/0x238
[<805ffbd8>] of_platform_populate+0x80/0xf8
[<80a60008>] of_platform_default_populate_init+0xcc/0xe4
[<8011010c>] do_one_initcall+0x50/0x2b4
[<80a3d0f0>] kernel_init_freeable+0x1f8/0x2a0
[<8086975c>] kernel_init+0x24/0x118
[<801124f8>] ret_from_kernel_thread+0x14/0x1c
----------8<------------------

It appears that some call to logic_pio_register_range() from 
lib/logic_pio.c is missing.
Perhaps the reserve_pio_range() function in arch/mips/loongson64/init.c 
could
be reused, but that's too deep water for me.

Steps to reproduce:

----------8<------------------

wget 
https://ftp.debian.org/debian/dists/Debian12.9/main/installer-mipsel/current/images/malta/netboot/initrd.gz

CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make malta_defconfig

CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make -j4

qemu-system-mipsel -machine malta -cpu 24Kf -m 1g -nographic -kernel 
vmlinuz \
        -initrd initrd.gz \
        -append "console=ttyS0 debug ignore_loglevel"

----------8<------------------


Greetings,

Mateusz

在2025年1月13日一月 下午10:16，Mateusz Jończyk写道：
> Hello,
>
> On Linux 6.13-rc6 for mipsel in QEMU on the Malta platform, the RTC CMOS
> driver does not load and /sys/class/rtc is empty. I have tested this with
> "make malta_defconfig", which compiles this driver into the kernel 
> (CONFIG_RTC_DRV_CMOS=y).

Hi Mateusz,

Thanks for tracking it down, this is indeed a huge effort.

>
> I have bisected this down to:
>
> commit 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
> Author: Jiaxun Yang<jiaxun.yang@flygoat.com>
> Date:   Thu Sep 21 19:04:22 2023 +0800
>
>      mips: add <asm-generic/io.h> including
>      With the adding, some default ioremap_xx methods defined in
>      asm-generic/io.h can be used. E.g the default ioremap_uc() returning
>      NULL.
>      We also massaged various headers to avoid nested includes.

#regzbot introduced: 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc

>
> I have tried to debug this.
>
> The fallout is apparently limited to the CMOS RTC driver, other
> drivers that access IO ports seem to function correctly (e.g. the
> PATA driver). Also, the read_persistent_clock64 function in
> arch/mips/mti-malta/malta-time.c, which accesses the same hardware
> works correctly.
>
> The CMOS RTC driver is likely special because this device is defined
> in a devicetree (arch/mips/boot/dts/mti/malta.dts) and there it is
> the only defined device on the ISA bus.
>
> That driver fails to load because the call to
>
> platform_get_resource(pdev, IORESOURCE_IO, 0);
>
> in cmos_platform_probe in drivers/rtc/rtc-cmos.c returns NULL.
>
> The mediator seems to be that this bad commit causes 
> arch/mips/include/asm/io.h
> to #include <asm-generic/io.h> at the end. As a side effect, this causes
> the PCI_IOBASE macro to be defined:
>
> #ifndef PCI_IOBASE
> #define PCI_IOBASE ((void __iomem *)0)
> #endif
>
> That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
> defined to mips_io_port_base as far as I can tell), but this does not 
> matter much here.

You are right, this is what should be done.

A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
just after including #include <asm-generic/io.h>, with ralink and loongson64
as exception.

In the long term, we should scrutinize platform usage of mips_io_base
following ralink's approach.

>
> When that macro is defined, pci_address_to_pio() in pci.c calls the
> logic_pio_trans_cpuaddr() function, which fails. Removing that ifdef
> in this function "fixes" the issue and allows that driver to load and work
> apparently correctly:
>
> ----------8<------------------
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 661f98c6c63a..368cd9ca6801 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4213,14 +4213,10 @@ EXPORT_SYMBOL_GPL(pci_pio_to_address);
> unsigned long __weak pci_address_to_pio(phys_addr_t address)
> {
> -#ifdef PCI_IOBASE
> - return logic_pio_trans_cpuaddr(address);
> -#else
> if (address > IO_SPACE_LIMIT)
> return (unsigned long)-1;
> return (unsigned long) address;
> -#endif
> }
> /**
> ----------8<------------------
>
> Additionally, the following message appears in dmesg on affected kernels:
>
> LOGIC PIO: addr 0x00000070 not registered in io_range_list
>
> (0x0070 is the IO port address of the CMOS RTC).
>
> When I added dump_stack() in logic_pio_trans_cpuaddr(), which prints 
> this warning,
> I got the following:
>
> ----------8<------------------
> Call Trace:
> [<801185c8>] show_stack+0x38/0x118
> [<8010be24>] dump_stack_lvl+0x7c/0xbc
> [<80841060>] logic_pio_trans_cpuaddr+0x88/0x98
> [<80604bec>] __of_address_to_resource+0x208/0x228
> [<805ff45c>] of_device_alloc+0x7c/0x1ac
> [<805ff778>] of_platform_device_create_pdata+0x60/0xf8
> [<805ff9cc>] of_platform_bus_create+0x1b0/0x238
> [<805ffa14>] of_platform_bus_create+0x1f8/0x238
> [<805ffbd8>] of_platform_populate+0x80/0xf8
> [<80a60008>] of_platform_default_populate_init+0xcc/0xe4
> [<8011010c>] do_one_initcall+0x50/0x2b4
> [<80a3d0f0>] kernel_init_freeable+0x1f8/0x2a0
> [<8086975c>] kernel_init+0x24/0x118
> [<801124f8>] ret_from_kernel_thread+0x14/0x1c
> ----------8<------------------
>
> It appears that some call to logic_pio_register_range() from 
> lib/logic_pio.c is missing.
> Perhaps the reserve_pio_range() function in arch/mips/loongson64/init.c 
> could
> be reused, but that's too deep water for me.

Sadly that is not going to work as those MIPS platforms are not using
vmmap for PCI IO access :-(

>
> Steps to reproduce:
>
> ----------8<------------------
>
> wget 
> https://ftp.debian.org/debian/dists/Debian12.9/main/installer-mipsel/current/images/malta/netboot/initrd.gz
>
> CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make malta_defconfig
>
> CROSS_COMPILE=mipsel-linux-gnu- ARCH=mips make -j4
>
> qemu-system-mipsel -machine malta -cpu 24Kf -m 1g -nographic -kernel 
> vmlinuz \
>         -initrd initrd.gz \
>         -append "console=ttyS0 debug ignore_loglevel"
>
> ----------8<------------------
>
>
> Greetings,
>
> Mateusz

-- 
- Jiaxun

Hi Jiaxun,

On Tue, Jan 14, 2025 at 12:32 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 在2025年1月13日一月 下午10:16，Mateusz Jończyk写道：
> > On Linux 6.13-rc6 for mipsel in QEMU on the Malta platform, the RTC CMOS
> > driver does not load and /sys/class/rtc is empty. I have tested this with
> > "make malta_defconfig", which compiles this driver into the kernel
> > (CONFIG_RTC_DRV_CMOS=y).
>
> Hi Mateusz,
>
> Thanks for tracking it down, this is indeed a huge effort.
>
> >
> > I have bisected this down to:
> >
> > commit 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
> > Author: Jiaxun Yang<jiaxun.yang@flygoat.com>
> > Date:   Thu Sep 21 19:04:22 2023 +0800
> >
> >      mips: add <asm-generic/io.h> including
> >      With the adding, some default ioremap_xx methods defined in
> >      asm-generic/io.h can be used. E.g the default ioremap_uc() returning
> >      NULL.
> >      We also massaged various headers to avoid nested includes.
>
> #regzbot introduced: 4bfb53e7d317c01f296b2feb2fae7c421c1d52dc
>
> >
> > I have tried to debug this.
> >
> > The fallout is apparently limited to the CMOS RTC driver, other
> > drivers that access IO ports seem to function correctly (e.g. the
> > PATA driver). Also, the read_persistent_clock64 function in
> > arch/mips/mti-malta/malta-time.c, which accesses the same hardware
> > works correctly.
> >
> > The CMOS RTC driver is likely special because this device is defined
> > in a devicetree (arch/mips/boot/dts/mti/malta.dts) and there it is
> > the only defined device on the ISA bus.
> >
> > That driver fails to load because the call to
> >
> > platform_get_resource(pdev, IORESOURCE_IO, 0);
> >
> > in cmos_platform_probe in drivers/rtc/rtc-cmos.c returns NULL.
> >
> > The mediator seems to be that this bad commit causes
> > arch/mips/include/asm/io.h
> > to #include <asm-generic/io.h> at the end. As a side effect, this causes
> > the PCI_IOBASE macro to be defined:
> >
> > #ifndef PCI_IOBASE
> > #define PCI_IOBASE ((void __iomem *)0)
> > #endif
> >
> > That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
> > defined to mips_io_port_base as far as I can tell), but this does not
> > matter much here.
>
> You are right, this is what should be done.
>
> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
> just after including #include <asm-generic/io.h>, with ralink and loongson64
> as exception.

Shouldn't arch/mips/include/asm/io.h do

    #define PCI_IOBASE mips_io_port_base

unconditionally, _before_ including  <asm-generic/io.h>?

> In the long term, we should scrutinize platform usage of mips_io_base
> following ralink's approach.

Currently ralink handles that in a mach-specific include:

    arch/mips/include/asm/mach-ralink/spaces.h:#define PCI_IOBASE
mips_io_port_base

Loongson does it differently:

    arch/mips/include/asm/mach-loongson64/spaces.h:#define PCI_IOBASE
     _AC(0xc000000000000000 + SZ_128K, UL)

But still sets mips_io_port_base in prom_init():

    arch/mips/loongson64/init.c:    set_io_port_base(PCI_IOBASE);

so defining PCI_IOBASE to mips_io_port_base in the main <asm/io.h>
should work.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Tue, Jan 14, 2025, at 00:29, Jiaxun Yang wrote:
> 在2025年1月13日一月 下午10:16，Mateusz Jończyk写道：
>> The mediator seems to be that this bad commit causes 
>> arch/mips/include/asm/io.h
>> to #include <asm-generic/io.h> at the end. As a side effect, this causes
>> the PCI_IOBASE macro to be defined:
>>
>> #ifndef PCI_IOBASE
>> #define PCI_IOBASE ((void __iomem *)0)
>> #endif
>>
>> That PCI_IOBASE value above is AFAIK incorrect for MIPS (it should be
>> defined to mips_io_port_base as far as I can tell), but this does not 
>> matter much here.
>
> You are right, this is what should be done.
>
> A quick fix would be #undef PCI_IOBASE in arch/mips/include/asm/io.h
> just after including #include <asm-generic/io.h>, with ralink and loongson64
> as exception.
>
> In the long term, we should scrutinize platform usage of mips_io_base
> following ralink's approach.

I think we are close to the point of being able to remove the broken
default PCI_IOBASE: the NULL pointer here is almost always wrong, and
mainly existed to shut up build failures on architectures that have
no port I/O at all. I know that sparc32 and m68k have cases that
actually rely on the broken PCI_IOBASE, so those need a local workaround,
not sure if some mips platform also falls into this category, as
I have not looked here in detail.

Hopefully we can get to a point where any reference to port I/O
(inb/outb, PCI_IOPORT, mips_io_port_base, ...) is guarded by
an #ifdef CONFIG_HAS_IOPORT check, and this is set exactly on
those platforms that set mips_io_port_base to a valid address.

      Arnd