mbox series

[v3,00/3] MIPS: SGI-IP27 rework part2

Message ID 20190319154755.31049-1-tbogendoerfer@suse.de (mailing list archive)
Headers show
Series MIPS: SGI-IP27 rework part2 | expand

Message

Thomas Bogendoerfer March 19, 2019, 3:47 p.m. UTC
SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
architecture and share some hardware (ioc3/bridge). To share
the software parts this patchset reworks SGI IP27 interrupt
and pci bridge code. By using features Linux gained during the
many years since SGI IP27 code was integrated this even results
in code reduction and IMHO cleaner code.

Tests have been done on a two module O200 (4 CPUs) and an
Origin 2000 (8 CPUs).

My next step in integrating SGI IP30 support is splitting ioc3eth
into a MFD and subdevice drivers, which will be submitted soon.

Changes in v3:

- dropped patches accepted by Paul
- moved IP27 specific __phys_to_dma/__dma_to_phys into its own file
- moved pcibios_to_node into IP27 specific file
- moved PCI bus address resources setup out of pci-xtalk code into
  IP27 specific code
- dropped bit from hub_irq_data and use hwirq from irq_data
- introduced intr_addr for setting up bridge interrupts (IP30 preperation)

Changes in v2:

- replaced HUB_L/HUB_S by __raw_readq/__raw_writeq
- removed union bridge_ate
- replaced remaing fields in slice_data by per_cpu data
- use generic_handle_irq instead of do_IRQ
- use hierarchy irq domain for stacking bridge and hub interrupt
- moved __dma_to_phys/__phy_to_dma to mach-ip27/dma-direct.h
- use dev_to_node() for pcibus_to_node() implementation

Thomas Bogendoerfer (3):
  MIPS: SGI-IP27: move IP27 specific code out of pci-ip27.c into new
    file
  MIPS: SGI-IP27: use generic PCI driver
  MIPS: SGI-IP27: abstract chipset irq from bridge

 arch/mips/Kconfig                          |   3 +
 arch/mips/include/asm/mach-ip27/topology.h |  11 +-
 arch/mips/include/asm/pci/bridge.h         |  14 +-
 arch/mips/include/asm/sn/irq_alloc.h       |  11 +
 arch/mips/include/asm/xtalk/xtalk.h        |   9 -
 arch/mips/pci/Makefile                     |   1 -
 arch/mips/pci/ops-bridge.c                 | 302 --------------
 arch/mips/pci/pci-ip27.c                   | 214 ----------
 arch/mips/sgi-ip27/Makefile                |   4 +-
 arch/mips/sgi-ip27/ip27-init.c             |   2 +
 arch/mips/sgi-ip27/ip27-irq.c              | 191 ++++-----
 arch/mips/sgi-ip27/ip27-pci.c              |  30 ++
 arch/mips/sgi-ip27/ip27-xtalk.c            |  61 ++-
 drivers/pci/controller/Kconfig             |   3 +
 drivers/pci/controller/Makefile            |   1 +
 drivers/pci/controller/pci-xtalk-bridge.c  | 610 +++++++++++++++++++++++++++++
 include/linux/platform_data/xtalk-bridge.h |  22 ++
 17 files changed, 822 insertions(+), 667 deletions(-)
 create mode 100644 arch/mips/include/asm/sn/irq_alloc.h
 delete mode 100644 arch/mips/pci/ops-bridge.c
 delete mode 100644 arch/mips/pci/pci-ip27.c
 create mode 100644 arch/mips/sgi-ip27/ip27-pci.c
 create mode 100644 drivers/pci/controller/pci-xtalk-bridge.c
 create mode 100644 include/linux/platform_data/xtalk-bridge.h

Comments

Thomas Bogendoerfer March 19, 2019, 3:47 p.m. UTC | #1
SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
architecture and share some hardware (ioc3/bridge). To share
the software parts this patchset reworks SGI IP27 interrupt
and pci bridge code. By using features Linux gained during the
many years since SGI IP27 code was integrated this even results
in code reduction and IMHO cleaner code.

Tests have been done on a two module O200 (4 CPUs) and an
Origin 2000 (8 CPUs).

My next step in integrating SGI IP30 support is splitting ioc3eth
into a MFD and subdevice drivers, which will be submitted soon.

Changes in v3:

- dropped patches accepted by Paul
- moved IP27 specific __phys_to_dma/__dma_to_phys into its own file
- moved pcibios_to_node into IP27 specific file
- moved PCI bus address resources setup out of pci-xtalk code into
  IP27 specific code
- dropped bit from hub_irq_data and use hwirq from irq_data
- introduced intr_addr for setting up bridge interrupts (IP30 preperation)

Changes in v2:

- replaced HUB_L/HUB_S by __raw_readq/__raw_writeq
- removed union bridge_ate
- replaced remaing fields in slice_data by per_cpu data
- use generic_handle_irq instead of do_IRQ
- use hierarchy irq domain for stacking bridge and hub interrupt
- moved __dma_to_phys/__phy_to_dma to mach-ip27/dma-direct.h
- use dev_to_node() for pcibus_to_node() implementation

Thomas Bogendoerfer (10):
  MIPS: SGI-IP27: get rid of volatile and hubreg_t
  MIPS: SGI-IP27: clean up bridge access and header files
  MIPS: SGI-IP27: use pr_info/pr_emerg and pr_cont to fix output
  MIPS: SGI-IP27: do xtalk scanning later
  MIPS: SGI-IP27: do boot CPU init later
  MIPS: SGI-IP27: rework HUB interrupts
  PCI: call add_bus method also for root bus
  MIPS: SGI-IP27: use generic PCI driver
  genirq/irqdomain: fall back to default domain when creating hierarchy
    domain
  MIPS: SGI-IP27: abstract chipset irq from bridge

 arch/mips/Kconfig                               |   4 +
 arch/mips/include/asm/dma-direct.h              |   2 +
 arch/mips/include/asm/mach-generic/dma-direct.h |   7 +
 arch/mips/include/asm/mach-ip27/dma-direct.h    |  20 +
 arch/mips/include/asm/mach-ip27/irq.h           |  12 +-
 arch/mips/include/asm/mach-ip27/mmzone.h        |   9 -
 arch/mips/include/asm/mach-ip27/topology.h      |  13 +-
 arch/mips/include/asm/pci.h                     |   8 +
 arch/mips/include/asm/pci/bridge.h              | 211 ++++----
 arch/mips/include/asm/smp-ops.h                 |   1 +
 arch/mips/include/asm/sn/addrs.h                |  72 +--
 arch/mips/include/asm/sn/arch.h                 |   2 -
 arch/mips/include/asm/sn/intr.h                 |   6 +
 arch/mips/include/asm/sn/io.h                   |   2 +-
 arch/mips/include/asm/sn/sn0/addrs.h            |   5 -
 arch/mips/include/asm/xtalk/xtalk.h             |   9 -
 arch/mips/kernel/smp.c                          |   2 +
 arch/mips/pci/Makefile                          |   1 -
 arch/mips/pci/ops-bridge.c                      | 322 ------------
 arch/mips/pci/pci-ip27.c                        | 233 ---------
 arch/mips/sgi-ip27/Makefile                     |   3 +-
 arch/mips/sgi-ip27/ip27-hubio.c                 |   4 +-
 arch/mips/sgi-ip27/ip27-init.c                  |  41 +-
 arch/mips/sgi-ip27/ip27-irq-pci.c               | 266 ----------
 arch/mips/sgi-ip27/ip27-irq.c                   | 331 ++++++++-----
 arch/mips/sgi-ip27/ip27-irqno.c                 |  48 --
 arch/mips/sgi-ip27/ip27-memory.c                |  34 +-
 arch/mips/sgi-ip27/ip27-nmi.c                   |  64 +--
 arch/mips/sgi-ip27/ip27-smp.c                   |   5 +-
 arch/mips/sgi-ip27/ip27-timer.c                 |  42 +-
 arch/mips/sgi-ip27/ip27-xtalk.c                 |  44 +-
 drivers/pci/controller/Kconfig                  |   3 +
 drivers/pci/controller/Makefile                 |   1 +
 drivers/pci/controller/pci-xtalk-bridge.c       | 623 ++++++++++++++++++++++++
 drivers/pci/probe.c                             |   6 +
 include/linux/platform_data/xtalk-bridge.h      |  17 +
 kernel/irq/irqdomain.c                          |   5 +-
 37 files changed, 1126 insertions(+), 1352 deletions(-)
 create mode 100644 arch/mips/include/asm/mach-generic/dma-direct.h
 create mode 100644 arch/mips/include/asm/mach-ip27/dma-direct.h
 delete mode 100644 arch/mips/pci/ops-bridge.c
 delete mode 100644 arch/mips/pci/pci-ip27.c
 delete mode 100644 arch/mips/sgi-ip27/ip27-irq-pci.c
 delete mode 100644 arch/mips/sgi-ip27/ip27-irqno.c
 create mode 100644 drivers/pci/controller/pci-xtalk-bridge.c
 create mode 100644 include/linux/platform_data/xtalk-bridge.h
Bjorn Helgaas April 18, 2019, 8:57 p.m. UTC | #2
Hi Thomas,

On Tue, Mar 19, 2019 at 04:47:49PM +0100, Thomas Bogendoerfer wrote:
> SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
> architecture and share some hardware (ioc3/bridge). To share
> the software parts this patchset reworks SGI IP27 interrupt
> and pci bridge code. By using features Linux gained during the
> many years since SGI IP27 code was integrated this even results
> in code reduction and IMHO cleaner code.
> 
> Tests have been done on a two module O200 (4 CPUs) and an
> Origin 2000 (8 CPUs).

Thanks for doing all this work!  It seems like it basically converts
some of the SGI PCI code to the structure typical of current host
controller drivers and moves it to drivers/pci/controller, which all
seems great to me.

The patches were kind of in limbo as far as Patchwork.  Lorenzo
handles the native host controller drivers, so I just delegated them
to him, so now they should be on his radar.

Bjorn

> My next step in integrating SGI IP30 support is splitting ioc3eth
> into a MFD and subdevice drivers, which will be submitted soon.
> 
> Changes in v3:
> 
> - dropped patches accepted by Paul
> - moved IP27 specific __phys_to_dma/__dma_to_phys into its own file
> - moved pcibios_to_node into IP27 specific file
> - moved PCI bus address resources setup out of pci-xtalk code into
>   IP27 specific code
> - dropped bit from hub_irq_data and use hwirq from irq_data
> - introduced intr_addr for setting up bridge interrupts (IP30 preperation)
> 
> Changes in v2:
> 
> - replaced HUB_L/HUB_S by __raw_readq/__raw_writeq
> - removed union bridge_ate
> - replaced remaing fields in slice_data by per_cpu data
> - use generic_handle_irq instead of do_IRQ
> - use hierarchy irq domain for stacking bridge and hub interrupt
> - moved __dma_to_phys/__phy_to_dma to mach-ip27/dma-direct.h
> - use dev_to_node() for pcibus_to_node() implementation
> 
> Thomas Bogendoerfer (3):
>   MIPS: SGI-IP27: move IP27 specific code out of pci-ip27.c into new
>     file
>   MIPS: SGI-IP27: use generic PCI driver
>   MIPS: SGI-IP27: abstract chipset irq from bridge
> 
>  arch/mips/Kconfig                          |   3 +
>  arch/mips/include/asm/mach-ip27/topology.h |  11 +-
>  arch/mips/include/asm/pci/bridge.h         |  14 +-
>  arch/mips/include/asm/sn/irq_alloc.h       |  11 +
>  arch/mips/include/asm/xtalk/xtalk.h        |   9 -
>  arch/mips/pci/Makefile                     |   1 -
>  arch/mips/pci/ops-bridge.c                 | 302 --------------
>  arch/mips/pci/pci-ip27.c                   | 214 ----------
>  arch/mips/sgi-ip27/Makefile                |   4 +-
>  arch/mips/sgi-ip27/ip27-init.c             |   2 +
>  arch/mips/sgi-ip27/ip27-irq.c              | 191 ++++-----
>  arch/mips/sgi-ip27/ip27-pci.c              |  30 ++
>  arch/mips/sgi-ip27/ip27-xtalk.c            |  61 ++-
>  drivers/pci/controller/Kconfig             |   3 +
>  drivers/pci/controller/Makefile            |   1 +
>  drivers/pci/controller/pci-xtalk-bridge.c  | 610 +++++++++++++++++++++++++++++
>  include/linux/platform_data/xtalk-bridge.h |  22 ++
>  17 files changed, 822 insertions(+), 667 deletions(-)
>  create mode 100644 arch/mips/include/asm/sn/irq_alloc.h
>  delete mode 100644 arch/mips/pci/ops-bridge.c
>  delete mode 100644 arch/mips/pci/pci-ip27.c
>  create mode 100644 arch/mips/sgi-ip27/ip27-pci.c
>  create mode 100644 drivers/pci/controller/pci-xtalk-bridge.c
>  create mode 100644 include/linux/platform_data/xtalk-bridge.h
> 
> -- 
> 2.13.7
>
Thomas Bogendoerfer May 6, 2019, 10:37 a.m. UTC | #3
On Thu, 18 Apr 2019 15:57:26 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> Hi Thomas,
> 
> On Tue, Mar 19, 2019 at 04:47:49PM +0100, Thomas Bogendoerfer wrote:
> > SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
> > architecture and share some hardware (ioc3/bridge). To share
> > the software parts this patchset reworks SGI IP27 interrupt
> > and pci bridge code. By using features Linux gained during the
> > many years since SGI IP27 code was integrated this even results
> > in code reduction and IMHO cleaner code.
> > 
> > Tests have been done on a two module O200 (4 CPUs) and an
> > Origin 2000 (8 CPUs).
> 
> Thanks for doing all this work!  It seems like it basically converts
> some of the SGI PCI code to the structure typical of current host
> controller drivers and moves it to drivers/pci/controller, which all
> seems great to me.
> 
> The patches were kind of in limbo as far as Patchwork.  Lorenzo
> handles the native host controller drivers, so I just delegated them
> to him, so now they should be on his radar.

Is there a chance to still get this into v5.2 ?

Thomas.
Lorenzo Pieralisi May 7, 2019, 9:42 a.m. UTC | #4
On Mon, May 06, 2019 at 12:37:24PM +0200, Thomas Bogendoerfer wrote:
> On Thu, 18 Apr 2019 15:57:26 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > Hi Thomas,
> > 
> > On Tue, Mar 19, 2019 at 04:47:49PM +0100, Thomas Bogendoerfer wrote:
> > > SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
> > > architecture and share some hardware (ioc3/bridge). To share
> > > the software parts this patchset reworks SGI IP27 interrupt
> > > and pci bridge code. By using features Linux gained during the
> > > many years since SGI IP27 code was integrated this even results
> > > in code reduction and IMHO cleaner code.
> > > 
> > > Tests have been done on a two module O200 (4 CPUs) and an
> > > Origin 2000 (8 CPUs).
> > 
> > Thanks for doing all this work!  It seems like it basically converts
> > some of the SGI PCI code to the structure typical of current host
> > controller drivers and moves it to drivers/pci/controller, which all
> > seems great to me.
> > 
> > The patches were kind of in limbo as far as Patchwork.  Lorenzo
> > handles the native host controller drivers, so I just delegated them
> > to him, so now they should be on his radar.
> 
> Is there a chance to still get this into v5.2 ?

I do not think so, I will have a look as soon as possible, sorry
for the delay.

Lorenzo
Lorenzo Pieralisi May 7, 2019, 3:31 p.m. UTC | #5
On Thu, Apr 18, 2019 at 03:57:26PM -0500, Bjorn Helgaas wrote:
> Hi Thomas,
> 
> On Tue, Mar 19, 2019 at 04:47:49PM +0100, Thomas Bogendoerfer wrote:
> > SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
> > architecture and share some hardware (ioc3/bridge). To share
> > the software parts this patchset reworks SGI IP27 interrupt
> > and pci bridge code. By using features Linux gained during the
> > many years since SGI IP27 code was integrated this even results
> > in code reduction and IMHO cleaner code.
> > 
> > Tests have been done on a two module O200 (4 CPUs) and an
> > Origin 2000 (8 CPUs).
> 
> Thanks for doing all this work!  It seems like it basically converts
> some of the SGI PCI code to the structure typical of current host
> controller drivers and moves it to drivers/pci/controller, which all
> seems great to me.

I had a look and the code is really, really MIPS specific, actually
I would be interested in understanding how many platforms it supports,
it is not even FW configurable.

To give another piece of evidence why some PCI driver code is still in
arch code, look at arch/arm, we did not move it to
drivers/pci/controller for the same reasons, eg lack of DT support.

With hard-coded resources, <asm/...> includes in driver code and MIPS
specific kludges even if it does reuse some APIs shared with controller
drivers I am not 100% certain that moving it to drivers/pci/controller
buys us anything, this is really arch specific code, however we slice
it.

The line between what stays in arch and what goes to
drivers/pci/controller is thin but this code is definitely more on the
arch side IMHO.

I do not question Thomas' effort, which I appreciate, I question
the end result and its usefulness, this series is even increasing
lines of kernel code, I would like to see the benefits.

Lorenzo

> The patches were kind of in limbo as far as Patchwork.  Lorenzo
> handles the native host controller drivers, so I just delegated them
> to him, so now they should be on his radar.
> 
> Bjorn
> 
> > My next step in integrating SGI IP30 support is splitting ioc3eth
> > into a MFD and subdevice drivers, which will be submitted soon.
> > 
> > Changes in v3:
> > 
> > - dropped patches accepted by Paul
> > - moved IP27 specific __phys_to_dma/__dma_to_phys into its own file
> > - moved pcibios_to_node into IP27 specific file
> > - moved PCI bus address resources setup out of pci-xtalk code into
> >   IP27 specific code
> > - dropped bit from hub_irq_data and use hwirq from irq_data
> > - introduced intr_addr for setting up bridge interrupts (IP30 preperation)
> > 
> > Changes in v2:
> > 
> > - replaced HUB_L/HUB_S by __raw_readq/__raw_writeq
> > - removed union bridge_ate
> > - replaced remaing fields in slice_data by per_cpu data
> > - use generic_handle_irq instead of do_IRQ
> > - use hierarchy irq domain for stacking bridge and hub interrupt
> > - moved __dma_to_phys/__phy_to_dma to mach-ip27/dma-direct.h
> > - use dev_to_node() for pcibus_to_node() implementation
> > 
> > Thomas Bogendoerfer (3):
> >   MIPS: SGI-IP27: move IP27 specific code out of pci-ip27.c into new
> >     file
> >   MIPS: SGI-IP27: use generic PCI driver
> >   MIPS: SGI-IP27: abstract chipset irq from bridge
> > 
> >  arch/mips/Kconfig                          |   3 +
> >  arch/mips/include/asm/mach-ip27/topology.h |  11 +-
> >  arch/mips/include/asm/pci/bridge.h         |  14 +-
> >  arch/mips/include/asm/sn/irq_alloc.h       |  11 +
> >  arch/mips/include/asm/xtalk/xtalk.h        |   9 -
> >  arch/mips/pci/Makefile                     |   1 -
> >  arch/mips/pci/ops-bridge.c                 | 302 --------------
> >  arch/mips/pci/pci-ip27.c                   | 214 ----------
> >  arch/mips/sgi-ip27/Makefile                |   4 +-
> >  arch/mips/sgi-ip27/ip27-init.c             |   2 +
> >  arch/mips/sgi-ip27/ip27-irq.c              | 191 ++++-----
> >  arch/mips/sgi-ip27/ip27-pci.c              |  30 ++
> >  arch/mips/sgi-ip27/ip27-xtalk.c            |  61 ++-
> >  drivers/pci/controller/Kconfig             |   3 +
> >  drivers/pci/controller/Makefile            |   1 +
> >  drivers/pci/controller/pci-xtalk-bridge.c  | 610 +++++++++++++++++++++++++++++
> >  include/linux/platform_data/xtalk-bridge.h |  22 ++
> >  17 files changed, 822 insertions(+), 667 deletions(-)
> >  create mode 100644 arch/mips/include/asm/sn/irq_alloc.h
> >  delete mode 100644 arch/mips/pci/ops-bridge.c
> >  delete mode 100644 arch/mips/pci/pci-ip27.c
> >  create mode 100644 arch/mips/sgi-ip27/ip27-pci.c
> >  create mode 100644 drivers/pci/controller/pci-xtalk-bridge.c
> >  create mode 100644 include/linux/platform_data/xtalk-bridge.h
> > 
> > -- 
> > 2.13.7
> >
Thomas Bogendoerfer May 7, 2019, 3:48 p.m. UTC | #6
On Tue, 7 May 2019 16:31:17 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> On Thu, Apr 18, 2019 at 03:57:26PM -0500, Bjorn Helgaas wrote:
> > Hi Thomas,
> > 
> > On Tue, Mar 19, 2019 at 04:47:49PM +0100, Thomas Bogendoerfer wrote:
> > > SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
> > > architecture and share some hardware (ioc3/bridge). To share
> > > the software parts this patchset reworks SGI IP27 interrupt
> > > and pci bridge code. By using features Linux gained during the
> > > many years since SGI IP27 code was integrated this even results
> > > in code reduction and IMHO cleaner code.
> > > 
> > > Tests have been done on a two module O200 (4 CPUs) and an
> > > Origin 2000 (8 CPUs).
> > 
> > Thanks for doing all this work!  It seems like it basically converts
> > some of the SGI PCI code to the structure typical of current host
> > controller drivers and moves it to drivers/pci/controller, which all
> > seems great to me.
> 
> I had a look and the code is really, really MIPS specific, actually
> I would be interested in understanding how many platforms it supports,
> it is not even FW configurable.

it's MIPS only and used in basically 3 different SGI platforms.

> With hard-coded resources, <asm/...> includes in driver code and MIPS
> specific kludges even if it does reuse some APIs shared with controller
> drivers I am not 100% certain that moving it to drivers/pci/controller
> buys us anything, this is really arch specific code, however we slice
> it.

hmm, I thought the idea of having one drivers/pci/controller directory
is to have all of them in one place.

> The line between what stays in arch and what goes to
> drivers/pci/controller is thin but this code is definitely more on the
> arch side IMHO.

what makes the xgene driver different from the xtalk-bridge driver ? Ok
it used DT, but it's still just for a specific type of SOCs from one
vendor, isn't it ?

> I do not question Thomas' effort, which I appreciate, I question
> the end result and its usefulness, this series is even increasing
> lines of kernel code, I would like to see the benefits.

the move from arch/mips/pci to drivers/pci/controller increases lines of
code by two lines. The whole patchset adds 155 lines, but also adds
functionality to be able to use the driver with different platforms.

Anyway I can live with not moving to drivers/pci/controller if you don't
like it there.

Thomas.
Lorenzo Pieralisi May 7, 2019, 4:30 p.m. UTC | #7
On Tue, May 07, 2019 at 05:48:20PM +0200, Thomas Bogendoerfer wrote:
> On Tue, 7 May 2019 16:31:17 +0100
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> > On Thu, Apr 18, 2019 at 03:57:26PM -0500, Bjorn Helgaas wrote:
> > > Hi Thomas,
> > > 
> > > On Tue, Mar 19, 2019 at 04:47:49PM +0100, Thomas Bogendoerfer wrote:
> > > > SGI IP27 (Origin/Onyx2) and SGI IP30 (Octane) have a similair
> > > > architecture and share some hardware (ioc3/bridge). To share
> > > > the software parts this patchset reworks SGI IP27 interrupt
> > > > and pci bridge code. By using features Linux gained during the
> > > > many years since SGI IP27 code was integrated this even results
> > > > in code reduction and IMHO cleaner code.
> > > > 
> > > > Tests have been done on a two module O200 (4 CPUs) and an
> > > > Origin 2000 (8 CPUs).
> > > 
> > > Thanks for doing all this work!  It seems like it basically converts
> > > some of the SGI PCI code to the structure typical of current host
> > > controller drivers and moves it to drivers/pci/controller, which all
> > > seems great to me.
> > 
> > I had a look and the code is really, really MIPS specific, actually
> > I would be interested in understanding how many platforms it supports,
> > it is not even FW configurable.
> 
> it's MIPS only and used in basically 3 different SGI platforms.
> 
> > With hard-coded resources, <asm/...> includes in driver code and MIPS
> > specific kludges even if it does reuse some APIs shared with controller
> > drivers I am not 100% certain that moving it to drivers/pci/controller
> > buys us anything, this is really arch specific code, however we slice
> > it.
> 
> hmm, I thought the idea of having one drivers/pci/controller directory
> is to have all of them in one place.

Well, drivers/pci/controller code should at least try to be arch
agnostic to belong there, there are exceptions, as I said but
they are the exception not the rule.

> > The line between what stays in arch and what goes to
> > drivers/pci/controller is thin but this code is definitely more on the
> > arch side IMHO.
> 
> what makes the xgene driver different from the xtalk-bridge driver ? Ok
> it used DT, but it's still just for a specific type of SOCs from one
> vendor, isn't it ?

Eg no <asm/...> includes required; its code can in principle be
reused on other arches, this series can't, it is arch code IMO.

> > I do not question Thomas' effort, which I appreciate, I question
> > the end result and its usefulness, this series is even increasing
> > lines of kernel code, I would like to see the benefits.
> 
> the move from arch/mips/pci to drivers/pci/controller increases lines of
> code by two lines. The whole patchset adds 155 lines, but also adds
> functionality to be able to use the driver with different platforms.

I am not convinced it will buy you much to be honest, I might be wrong.

If we move the code we have to have a compelling reason for it to
happen and at the moment I do not necessarily see one, happy to
be proven wrong if you extend it to support other platforms cleanly
(eg with firmware bindings) on top of this series.

> Anyway I can live with not moving to drivers/pci/controller if you don't
> like it there.

I can't certainly maintain it, it is too MIPS specific and I do not like
this code either but that's certainly not your fault.

- Hardcoded resources
- No firmware bindings
- Lots of MIPS specific workarounds
- arch code dependency

I do not think that as it stands it belongs in driver/pci/controller,
no.

Thanks,
Lorenzo
Christoph Hellwig May 8, 2019, 6:24 a.m. UTC | #8
On Tue, May 07, 2019 at 05:48:20PM +0200, Thomas Bogendoerfer wrote:
> > I had a look and the code is really, really MIPS specific, actually
> > I would be interested in understanding how many platforms it supports,
> > it is not even FW configurable.
> 
> it's MIPS only and used in basically 3 different SGI platforms.

Actually this isn't quite true, this whole infraѕtructure was also
shared by the SGI IA64 systems.

The early SN1 prototypes were basically just IP35 systems with IA64
CPUs glued in, and even the later SN2 that were released as Altix
shared the same xtalk + bridge infrastructure, just with some hardware
updates.  It just turned out SGI at some point decided to hide all that
in firmware.  Linux 2.4 was still accessing this natively, but the code
was a mess.  I tried to consolidate it with the mips version at that
point, but SGI decided that hiding it under firmware calls was easier
as they could just move the messy code there.

So, not it isn't really MIPS only in theory, although in practice it
very much is..