Message-ID: <9f80ba5e-726b-ad68-b71f-ab23470bfa36@linux.intel.com>
Date: Tue, 14 Oct 2025 15:41:42 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>, 
    Guenter Roeck <linux@...ck-us.net>, Bjorn Helgaas <bhelgaas@...gle.com>, 
    linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 03/24] MIPS: PCI: Use pci_enable_resources()

On Tue, 14 Oct 2025, Maciej W. Rozycki wrote:

> On Tue, 14 Oct 2025, Ilpo Järvinen wrote:
> 
> > > As you can see there are holes in the map below 0x100, so e.g. if the bus 
> > > master IDE I/O space registers (claimed last in the list by `ata_piix') 
> > > were assigned to 00000030-0000003f, then all hell would break loose.  It 
> > > is exactly the mapping that happened in the absence of the code piece in 
> > > question IIRC.
> > 
> > Are you sure pci-malta.c has to do anything like this as 
> > pcibios_align_resource() does lower bound IO resource start addresses if 
> > PCIBIOS_MIN_IO is set?
> 
>  Well, PCIBIOS_MIN_IO is never set for Malta and therefore stays at 0.

I meant whether pci-malta.c has to play with the ->start address at all 
if it would use PCIBIOS_MIN_IO.

> I could boot 2.6.11 with the hunk reverted and see what happens, not a big 
> deal (I should have old GCC somewhere as a kernel such old would surely be 
> a pain to build with modern GCC).  This stuff was badly broken before 
> commit ae81aad5c2e1 (and there was support there too for the Atlas board, 
> a weird one with a Philips SAA9730 southbridge and supporting a subset of 
> the same CPU core cards as the Malta board does).
> 
> > How about this patch below?
> > 
> > (I'm not sure if it should actually be
> > 	PCIBIOS_MIN_IO = 0x1000 - hose->io_resource->start;
> > to allow resources starting from 0x1000 if ->start is not at 0.)
> 
>  I'd have to go through the relevant datasheets to see whether it can 
> actually happen in reality.  Perhaps we can just hardwire PCIBIOS_MIN_IO 
> to 0x1000 instead, similarly to what other MIPS platforms do.

My patch did hardcode set it to 0x1000, I just noted before the patch that 
I'm not sure if the code should actually try to align the resulting "real 
start address" to 0x1000 if hose->io_resource->start != 0.

Or are you saying also the the if () check should be removed as well?

> After all 
> Malta's southbridge is on the mainboard and therefore always the same, 
> regardless of the northbridge (system controller in MIPS-speak) which 
> comes with the pluggable CPU core card.
> 
>  NB there are commit c5de50dada14 ("MIPS: Malta: Change start address to 
> avoid conflicts.") and commit 27547abf36af ("MIPS: malta: Incorporate 
> PIIX4 ACPI I/O region in PCI controller resources") that fiddled with this 
> code piece.  Especially the latter one refers additional commits that may 
> give further insights.  And the former one removed a "FIXME" annotation, 
> which suggests I didn't consider the solution perfect back 20 years ago, 
> but given how long it stayed there it was surely good enough for its time.

It was "good enough" only because the arch specific 
pcibios_enable_resources() was lacking the check for whether the resource 
truly got assigned or not. The PIIX4 driver must worked just fine without 
those IO resources which is what most drivers do despite using 
pci(m)_enable_device() and not pci_enable_device_mem() (the latter 
doesn't even seem to come with m variant).

-- 
 i.

Powered by blists - more mailing lists