Re: [PATCH v2 04/19] powerpc: Create pci_controller_ops.dma_dev_setup and shim

Re: [PATCH v2 04/19] powerpc: Create pci_controller_ops.dma_dev_setup and shim

[Arnd Bergmann]

On Tuesday 31 March 2015 16:00:42 Daniel Axtens wrote:
> Introduces the pci_controller_ops structure.
> Add pci_controller_ops.dma_dev_setup, shadowing ppc_md.pci_dma_dev_setup.
> Add a shim, and change the callsites to use the shim.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> 
> ---
> 
> v1 --> v2:
>  - Better commit message
>  - Use phb in favour of hose
>  - Make shim name match ppc_md name, not pci_controller_ops name.
> ---
>  arch/powerpc/include/asm/pci-bridge.h | 21 +++++++++++++++++++++
>  arch/powerpc/kernel/pci-common.c      |  3 +--
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index 546d036..347d49d 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -15,6 +15,13 @@
>  struct device_node;
>  
>  /*
> + * PCI controller operations
> + */
> +struct pci_controller_ops {
> +       void            (*dma_dev_setup)(struct pci_dev *dev);
> +};
> +
> +/*

Please see https://patchwork.ozlabs.org/patch/431333/ for related work.

I think it would be better not to introduce another architecture-specific
pci host bridge operations structure, but instead consolidate into
the one that is already there. We are also adding a generic way to set up
PCI DMA, so it would seems reasonable to hook into that place.

	Arnd

Re: [PATCH v2 04/19] powerpc: Create pci_controller_ops.dma_dev_setup and shim

[Daniel Axtens]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

> Please see https://patchwork.ozlabs.org/patch/431333/ for related work.
> 
I'm familiar with that patch series - I've been helping Yijing get it up
to speed on PowerPC.


> I think it would be better not to introduce another architecture-specific
> pci host bridge operations structure, but instead consolidate into
> the one that is already there. We are also adding a generic way to set up
> PCI DMA, so it would seems reasonable to hook into that place.
I see what you're getting at, and I agree that we want to move towards
generic operations. 

However, I think this should go in as is at this point, for two main
reasons:

1) This is a good midpoint that makes it easier to move to a generic
structure. Our arch specific stuff is quirky and difficult. This patch
series does a lot to reduce the complexity, and would make it very easy
to move these ops into a generic structure at some future point. 

2) Trying to go generic at this point risks making the change set so
complex and wide ranging that it will really struggle to get in. For
example, Yijing's patch set, despite not changing any of the quirky
stuff in PowerPC, is already quite long, and will require agreement from
a lot of people before it can go in.

Much as I would like to have everything as generic as possible, if we
were to try to do the whole job in one go, it'd become a big, difficult,
messy patch set, and would be less likely to happen than if we were to
do it in two steps.

Regards,
Daniel



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]

Re: [PATCH v2 04/19] powerpc: Create pci_controller_ops.dma_dev_setup and shim

[Arnd Bergmann]

On Tuesday 07 April 2015 10:31:36 Daniel Axtens wrote:
> > Please see https://patchwork.ozlabs.org/patch/431333/ for related work.
> > 
> I'm familiar with that patch series - I've been helping Yijing get it up
> to speed on PowerPC.
> 
> 
> > I think it would be better not to introduce another architecture-specific
> > pci host bridge operations structure, but instead consolidate into
> > the one that is already there. We are also adding a generic way to set up
> > PCI DMA, so it would seems reasonable to hook into that place.
> I see what you're getting at, and I agree that we want to move towards
> generic operations. 
> 
> However, I think this should go in as is at this point, for two main
> reasons:
> 
> 1) This is a good midpoint that makes it easier to move to a generic
> structure. Our arch specific stuff is quirky and difficult. This patch
> series does a lot to reduce the complexity, and would make it very easy
> to move these ops into a generic structure at some future point. 
> 
> 2) Trying to go generic at this point risks making the change set so
> complex and wide ranging that it will really struggle to get in. For
> example, Yijing's patch set, despite not changing any of the quirky
> stuff in PowerPC, is already quite long, and will require agreement from
> a lot of people before it can go in.
> 
> Much as I would like to have everything as generic as possible, if we
> were to try to do the whole job in one go, it'd become a big, difficult,
> messy patch set, and would be less likely to happen than if we were to
> do it in two steps.

Ok, fair enough. Let's do this one first then.

	Arnd

Re: [PATCH v2 04/19] powerpc: Create pci_controller_ops.dma_dev_setup and shim

[Michael Ellerman]

On Tue, 2015-04-07 at 09:44 +0200, Arnd Bergmann wrote:
> On Tuesday 07 April 2015 10:31:36 Daniel Axtens wrote:
> > > Please see https://patchwork.ozlabs.org/patch/431333/ for related work.
> > > 
> > I'm familiar with that patch series - I've been helping Yijing get it up
> > to speed on PowerPC.
> > 
> > 
> > > I think it would be better not to introduce another architecture-specific
> > > pci host bridge operations structure, but instead consolidate into
> > > the one that is already there. We are also adding a generic way to set up
> > > PCI DMA, so it would seems reasonable to hook into that place.
> > I see what you're getting at, and I agree that we want to move towards
> > generic operations. 
> > 
> > However, I think this should go in as is at this point, for two main
> > reasons:
> > 
> > 1) This is a good midpoint that makes it easier to move to a generic
> > structure. Our arch specific stuff is quirky and difficult. This patch
> > series does a lot to reduce the complexity, and would make it very easy
> > to move these ops into a generic structure at some future point. 
> > 
> > 2) Trying to go generic at this point risks making the change set so
> > complex and wide ranging that it will really struggle to get in. For
> > example, Yijing's patch set, despite not changing any of the quirky
> > stuff in PowerPC, is already quite long, and will require agreement from
> > a lot of people before it can go in.
> > 
> > Much as I would like to have everything as generic as possible, if we
> > were to try to do the whole job in one go, it'd become a big, difficult,
> > messy patch set, and would be less likely to happen than if we were to
> > do it in two steps.
> 
> Ok, fair enough. Let's do this one first then.

Thanks Arnd.

cheers