On Wed, Sep 8, 2021 at 5:48 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> On Fri, Sep 03, 2021 at 05:52:09PM +0800, Huacai Chen wrote:
> > diff --git a/arch/loongarch/pci/acpi.c b/arch/loongarch/pci/acpi.c
> > new file mode 100644
> > index 000000000000..d6e2f69b9503
> > --- /dev/null
> > +++ b/arch/loongarch/pci/acpi.c
> > @@ -0,0 +1,174 @@
> > +// SPDX-License-Identifier: GPL-2.0
A lot of this file appears to duplicate what we already have on other
architectures.
I would suggest moving the other architecture specific code into
drivers/acpi/pci*.c and share as much as possible to make it easier to
make modifications in the future.
> It might be time for default implementations here that can be shared
> with arm64. The functions look the same or similar to the arm64
> version in many cases and why they are different isn't that clear to me
> not being all that familar with the ACPI code.
I think it can be simplified quite a bit if we restructure the acpi pci
code to behave like a normal pci host bridge driver.
> > +
> > +/*
> > + * We need to avoid collisions with `mirrored' VGA ports
> > + * and other strange ISA hardware, so we always want the
> > + * addresses to be allocated in the 0x000-0x0ff region
> > + * modulo 0x400.
> > + *
> > + * Why? Because some silly external IO cards only decode
> > + * the low 10 bits of the IO address. The 0x00-0xff region
> > + * is reserved for motherboard devices that decode all 16
> > + * bits, so it's ok to allocate at, say, 0x2800-0x28ff,
> > + * but we want to try to avoid allocating at 0x2900-0x2bff
> > + * which might have be mirrored at 0x0100-0x03ff..
>
> Is this something you need to worry about on this h/w? arm64 and riscv
> don't seem to need this. If you do need this, then shouldn't everyone?
>
> Don't blindly copy code unless you know you need it. If multiple arches
> have the same code, then that's a sign of blindly copying stuff or a
> need to refactor into common code. It looks like some things are just
> copied from MIPS. The MIPS arch code is a mess and not a good choice to
> draw inspiration from (aka blindly copy). Pick more recently added
> architectures given they will more closely follow current rules as to
> what maintainers want.
It's certainly not architecture specific at all. Maybe what we can do here
is to have the most common implementations provided in drivers/pci/
and then have the host bridge driver pick one of them to match either
the current behavior or whatever we decide is a good default.
Ideally we'd want only one implementation, but some of the older
host bridge implementations may need to be more careful around
ISA bridges than the newer ones. Making it depend on CONFIG_ISA
might work.
> > + */
> > +resource_size_t
> > +pcibios_align_resource(void *data, const struct resource *res,
> > + resource_size_t size, resource_size_t align)
> > +{
> > + resource_size_t start = res->start;
> > +
> > + if (res->flags & IORESOURCE_IO) {
> > + /*
> > + * Put everything into 0x00-0xff region modulo 0x400
> > + */
> > + if (start & 0x300)
> > + start = (start + 0x3ff) & ~0x3ff;
> > + } else if (res->flags & IORESOURCE_MEM) {
> > + /* Make sure we start at our min on all hoses */
> > + if (start < PCIBIOS_MIN_MEM)
> > + start = PCIBIOS_MIN_MEM;
> > + }
> > +
> > + return start;
> > +}
Same here.
Arnd
