Re: [PATCH v3 2/2] asm-generic: Add new pci.h and use it

[Palmer Dabbelt <palmer@dabbelt.com>]

On Thu, 21 Jul 2022 15:05:34 PDT (-0700), shorne@gmail.com wrote:
> On Tue, Jul 19, 2022 at 08:58:39AM -0700, Palmer Dabbelt wrote:
>> On Sun, 17 Jul 2022 17:41:14 PDT (-0700), shorne@gmail.com wrote:
>> > The asm/pci.h used for many newer architectures share similar
>> > definitions.  Move the common parts to asm-generic/pci.h to allow for
>> > sharing code.
>> >
>> > Two things to note are:
>> >
>> >  - isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
>> >    these architectures avoid creating that file and add the definition
>> >    to asm/pci.h.
>> >  - ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
>> >    undefine it after including asm-generic/pci.h.  Why doesn't csky
>> >    define it?
>> >  - pci_get_legacy_ide_irq, This function is only used on architectures
>> >    that support PNP.  It is only maintained for arm64, in other
>> >    architectures it is removed.
>> >
>> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> > Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
>> > Signed-off-by: Stafford Horne <shorne@gmail.com>
>> > ---
>> > Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
>> > pci/quirks.c.  We discussed limiting it only to x86 though as its a general
>> > quick triggered by pci ids I think it will be more tricky than we thought so I
>> > will leave as is.  It might be nice to move it out of asm/dma.h and into
>> > asm/pci.h though.
>> >
>> > Since v2:
>> >  - Nothing
>> > Since v1:
>> >  - Remove definition of pci_get_legacy_ide_irq
>> >
>> >  arch/arm64/include/asm/pci.h | 12 +++---------
>> >  arch/csky/include/asm/pci.h  | 24 ++++--------------------
>> >  arch/riscv/include/asm/pci.h | 25 +++----------------------
>> >  arch/um/include/asm/pci.h    | 24 ++----------------------
>> >  include/asm-generic/pci.h    | 36 ++++++++++++++++++++++++++++++++++++
>> >  5 files changed, 48 insertions(+), 73 deletions(-)
>> >  create mode 100644 include/asm-generic/pci.h
>> >
>> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
>> > index b33ca260e3c9..1180e83712f5 100644
>> > --- a/arch/arm64/include/asm/pci.h
>> > +++ b/arch/arm64/include/asm/pci.h
>> > @@ -9,7 +9,6 @@
>> >  #include <asm/io.h>
>> >
>> >  #define PCIBIOS_MIN_IO		0x1000
>> > -#define PCIBIOS_MIN_MEM		0
>> >
>> >  /*
>> >   * Set to 1 if the kernel should re-assign all PCI bus numbers
>> > @@ -18,9 +17,6 @@
>> >  	(pci_has_flag(PCI_REASSIGN_ALL_BUS))
>> >
>> >  #define arch_can_pci_mmap_wc() 1
>> > -#define ARCH_GENERIC_PCI_MMAP_RESOURCE	1
>> > -
>> > -extern int isa_dma_bridge_buggy;
>> >
>> >  #ifdef CONFIG_PCI
>> >  static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > @@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> >  	/* no legacy IRQ on arm64 */
>> >  	return -ENODEV;
>> >  }
>> > -
>> > -static inline int pci_proc_domain(struct pci_bus *bus)
>> > -{
>> > -	return 1;
>> > -}
>> >  #endif  /* CONFIG_PCI */
>> >
>> > +/* Generic PCI */
>> > +#include <asm-generic/pci.h>
>> > +
>> >  #endif  /* __ASM_PCI_H */
>> > diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
>> > index ebc765b1f78b..44866c1ad461 100644
>> > --- a/arch/csky/include/asm/pci.h
>> > +++ b/arch/csky/include/asm/pci.h
>> > @@ -9,26 +9,10 @@
>> >
>> >  #include <asm/io.h>
>> >
>> > -#define PCIBIOS_MIN_IO		0
>> > -#define PCIBIOS_MIN_MEM		0
>> > +/* Generic PCI */
>> > +#include <asm-generic/pci.h>
>> >
>> > -/* C-SKY shim does not initialize PCI bus */
>> > -#define pcibios_assign_all_busses() 1
>> > -
>> > -extern int isa_dma_bridge_buggy;
>> > -
>> > -#ifdef CONFIG_PCI
>> > -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > -{
>> > -	/* no legacy IRQ on csky */
>> > -	return -ENODEV;
>> > -}
>> > -
>> > -static inline int pci_proc_domain(struct pci_bus *bus)
>> > -{
>> > -	/* always show the domain in /proc */
>> > -	return 1;
>> > -}
>> > -#endif  /* CONFIG_PCI */
>> > +/* csky doesn't use generic pci resource mapping */
>> > +#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
>> >
>> >  #endif  /* __ASM_CSKY_PCI_H */
>> > diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
>> > index 7fd52a30e605..12ce8150cfb0 100644
>> > --- a/arch/riscv/include/asm/pci.h
>> > +++ b/arch/riscv/include/asm/pci.h
>> > @@ -12,29 +12,7 @@
>> >
>> >  #include <asm/io.h>
>> >
>> > -#define PCIBIOS_MIN_IO		0
>> > -#define PCIBIOS_MIN_MEM		0
>>
>> My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing out
>> address 0 to devices").  Do you mind either splitting out the arch/riscv
>> bits or having this in via some sort of shared tag?
>
> Hi Palmer,
>
> I replied last on my phone but since it produces HTML multi-part email it got
> rejected from a few places and I am not sure if you saw my reply.
>
> It might be a bit hard to separate out the architecture specific bits as it
> requires a bit of coordination with the asm-generic/pci.h move.
>
> Some options:
>   1 I can produce a tag for you to merge into your for-next.
>   2 I can skip touching riscv at all in this patch and you can take care of it
>     in a future patch.
>   3 I could cherry pick your change bb356ddb78b2 ("RISC-V: PCI: Avoid handing out
>     address 0 to devices") to my for-next branch.  It should help avoid conflict.
>
> Seeing that I am still doing small updates here and there I am not sure when I
> will be ablt to create a stable tag.  So maybe 3 would work best?

If you cherry pick it then we're going to both end up with the patch, 
which IMO is worse than the conflict (and the patch is pretty far back, 
so I don't really want to rebase everything this late).

Since this isn't in yet, would it be OK just landing it as a single 
patch on top of 5.19-rc1 that gets merged into whatever tree it ends up 
in (ie, yours or Arnd's)?  Then as long as the commit is stable when it 
lands in for-next I can just merge it down to my tree and sort out the 
conflict from there.

If that's not possible then I think the best bet is to just drop the 
RISC-V diff, I can deal with it when the rest lands in Linus' tree.

> -Stafford