xilinx-pci driver and pci in general

xilinx-pci driver and pci in general

[Michal Simek]

Hi guys,

We (John and partially I) did initial support for pci on Microblaze.
It is based on powerpc files and almost everything is the same.
There are some small differences which could be easily removed that's 
why I think that will be good to move that file to any generic location.
That's why I would like to know your opinion about this step.
If you don't like or prefer we will add that modified versions to 
arch/microblaze and in future we can merge them and move to any other 
location.

Affected files: xilinx-pci.c, pci_32.c pci-common.c, indirect_pci.c and 
some headers.


The main problems are:
ppc use ppc_md struct which we don't have it on Microblaze.
xilinx-pci driver uses exclude_device function. This function is used in 
indirect_pci.c too. There could be a way to move that function directly 
to pci_controller structure which could be useful for other controllers 
too. What do you think?

Then there are some other ppc_md. calling like pcibios_after_init which
if I see correctly not used for ppc too.

The next thing is that some files contains asm/machdep.h which could be 
added to asm/pci-bridge.h and the same is for asm/ppc-pci.h

Files contains CONFIG_PPC_OF and we would like to use only CONFIG_OF.
I remember any discuss around but not sure what was the conclusion on 
powerpc.

Part of headers are the same that's why there will be a space to move 
them to asm-generic.
Anyway: I look at your dma-mapping.h and you can use 
asm-generic/dma-mapping-common.h as I am using.


Then I have some question about EARLY_PCI_OP in ppc_32.c. Is there any 
reason to use early_##rw##_config_##size fucntions instead of proper 
pci_bus_##rw##_config_##size functions?
There is one comment that these functions are used before PCI scanning 
is done but there are used the same function as are in driver/pci/access.c.
Is there any "secret" reason to do it in this way?


Thanks for this early discuss. I would like to hear your opinion and 
then I will choose solution how to add our pci support to mainline.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663

Re: xilinx-pci driver and pci in general

[Benjamin Herrenschmidt]

On Wed, 2010-01-13 at 16:23 +0100, Michal Simek wrote:

> The main problems are:
> ppc use ppc_md struct which we don't have it on Microblaze.
> xilinx-pci driver uses exclude_device function. This function is used in 
> indirect_pci.c too. There could be a way to move that function directly 
> to pci_controller structure which could be useful for other controllers 
> too. What do you think?
> 
> Then there are some other ppc_md. calling like pcibios_after_init which
> if I see correctly not used for ppc too.

We may not be using after_init() anymore in which case you are welcome
to send a patch to remove it :-)

As for the others, well ... maybe you can do wrappers for these that
call into ppc_md. on powerpc and into some kind of arch_pci_ops. that
the platform provides on microblaze ?

I'm not sure moving them into the pci_controller is the best way to go
there.

> The next thing is that some files contains asm/machdep.h which could be 
> added to asm/pci-bridge.h and the same is for asm/ppc-pci.h

Yeah, moving includes like that is ok.

> Files contains CONFIG_PPC_OF and we would like to use only CONFIG_OF.
> I remember any discuss around but not sure what was the conclusion on 
> powerpc.

I think that should be allright, Grant, any objection there ?

> Part of headers are the same that's why there will be a space to move 
> them to asm-generic.

If you can convince other archs that it makes sense to do so ? :-)

> Anyway: I look at your dma-mapping.h and you can use 
> asm-generic/dma-mapping-common.h as I am using.

Not just quite yet, there's still some stuff we need to cleanup with
the !coherent cases.

> Then I have some question about EARLY_PCI_OP in ppc_32.c. Is there any 
> reason to use early_##rw##_config_##size fucntions instead of proper 
> pci_bus_##rw##_config_##size functions?
> There is one comment that these functions are used before PCI scanning 
> is done but there are used the same function as are in driver/pci/access.c.
> Is there any "secret" reason to do it in this way?

Well, first of all, those aren't ppc32 only anymore, they are in
pci-common.c now. Then, if you look at them you'll notice that
they are just a wrapper on top of pci_bus_* which uses a fake
pci_bus structure. IE. They are meant to be used in very early
arch fixup code at a time when we may not even have the struct
pci_bus at hand. Their use is pretty rare though, maybe we -could-
get rid of them at some stage by moving some of that fixup code.

> Thanks for this early discuss. I would like to hear your opinion and 
> then I will choose solution how to add our pci support to mainline.

Cheers,
Ben.

Re: xilinx-pci driver and pci in general

[Michal Simek]

Benjamin Herrenschmidt wrote:
> On Wed, 2010-01-13 at 16:23 +0100, Michal Simek wrote:
> 
>> The main problems are:
>> ppc use ppc_md struct which we don't have it on Microblaze.
>> xilinx-pci driver uses exclude_device function. This function is used in 
>> indirect_pci.c too. There could be a way to move that function directly 
>> to pci_controller structure which could be useful for other controllers 
>> too. What do you think?
>>
>> Then there are some other ppc_md. calling like pcibios_after_init which
>> if I see correctly not used for ppc too.
> 
> We may not be using after_init() anymore in which case you are welcome
> to send a patch to remove it :-)

hmm. I used older kernel and I see that in the latest version powermac
use it. :-( I will just remove it.

> 
> As for the others, well ... maybe you can do wrappers for these that
> call into ppc_md. on powerpc and into some kind of arch_pci_ops. that
> the platform provides on microblaze ?
> 
> I'm not sure moving them into the pci_controller is the best way to go
> there.

ok. I will remove that part of code for now.

> 
>> The next thing is that some files contains asm/machdep.h which could be 
>> added to asm/pci-bridge.h and the same is for asm/ppc-pci.h
> 
> Yeah, moving includes like that is ok.
> 
>> Files contains CONFIG_PPC_OF and we would like to use only CONFIG_OF.
>> I remember any discuss around but not sure what was the conclusion on 
>> powerpc.
> 
> I think that should be allright, Grant, any objection there ?
> 
>> Part of headers are the same that's why there will be a space to move 
>> them to asm-generic.
> 
> If you can convince other archs that it makes sense to do so ? :-)

will try.

> 
>> Anyway: I look at your dma-mapping.h and you can use 
>> asm-generic/dma-mapping-common.h as I am using.
> 
> Not just quite yet, there's still some stuff we need to cleanup with
> the !coherent cases.
> 
>> Then I have some question about EARLY_PCI_OP in ppc_32.c. Is there any 
>> reason to use early_##rw##_config_##size fucntions instead of proper 
>> pci_bus_##rw##_config_##size functions?
>> There is one comment that these functions are used before PCI scanning 
>> is done but there are used the same function as are in driver/pci/access.c.
>> Is there any "secret" reason to do it in this way?
> 
> Well, first of all, those aren't ppc32 only anymore, they are in
> pci-common.c now. Then, if you look at them you'll notice that
> they are just a wrapper on top of pci_bus_* which uses a fake
> pci_bus structure. IE. They are meant to be used in very early
> arch fixup code at a time when we may not even have the struct
> pci_bus at hand. Their use is pretty rare though, maybe we -could-
> get rid of them at some stage by moving some of that fixup code.

Thanks for that.

> 
>> Thanks for this early discuss. I would like to hear your opinion and 
>> then I will choose solution how to add our pci support to mainline.

I will keep you informed but I see that I will add that part of code to 
mainline and then we look at consolidation work.

Thanks,
Michal

> 
> Cheers,
> Ben.
> 
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: xilinx-pci driver and pci in general

[Grant Likely]

On Fri, Jan 15, 2010 at 2:23 AM, Michal Simek <monstr@monstr.eu> wrote:
> Benjamin Herrenschmidt wrote:
>> On Wed, 2010-01-13 at 16:23 +0100, Michal Simek wrote:
>>> Thanks for this early discuss. I would like to hear your opinion and then
>>> I will choose solution how to add our pci support to mainline.
>
> I will keep you informed but I see that I will add that part of code to
> mainline and then we look at consolidation work.

As mentioned in previous email, please do it the other way around.  It
may seem like more work to do so, but it is less work in the long
term.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: xilinx-pci driver and pci in general

[Grant Likely]

On Wed, Jan 13, 2010 at 7:07 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2010-01-13 at 16:23 +0100, Michal Simek wrote:
>
>> The main problems are:
>> ppc use ppc_md struct which we don't have it on Microblaze.
>> xilinx-pci driver uses exclude_device function. This function is used in
>> indirect_pci.c too. There could be a way to move that function directly
>> to pci_controller structure which could be useful for other controllers
>> too. What do you think?
>>
>> Then there are some other ppc_md. calling like pcibios_after_init which
>> if I see correctly not used for ppc too.
>
> We may not be using after_init() anymore in which case you are welcome
> to send a patch to remove it :-)
>
> As for the others, well ... maybe you can do wrappers for these that
> call into ppc_md. on powerpc and into some kind of arch_pci_ops. that
> the platform provides on microblaze ?

I agree.  Replace the direct ppc_md. references with arch-provided wrappers.

>> Files contains CONFIG_PPC_OF and we would like to use only CONFIG_OF.
>> I remember any discuss around but not sure what was the conclusion on
>> powerpc.
>
> I think that should be allright, Grant, any objection there ?

None whatsoever.

>> Part of headers are the same that's why there will be a space to move
>> them to asm-generic.
>
> If you can convince other archs that it makes sense to do so ? :-)

Arnd can give you good advice here I think.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: xilinx-pci driver and pci in general

[Grant Likely]

On Wed, Jan 13, 2010 at 8:23 AM, Michal Simek
<michal.simek@petalogix.com> wrote:
> Hi guys,
>
> We (John and partially I) did initial support for pci on Microblaze.
> It is based on powerpc files and almost everything is the same.
> There are some small differences which could be easily removed that's why I
> think that will be good to move that file to any generic location.
> That's why I would like to know your opinion about this step.
> If you don't like or prefer we will add that modified versions to
> arch/microblaze and in future we can merge them and move to any other
> location.
>
> Affected files: xilinx-pci.c, pci_32.c pci-common.c, indirect_pci.c and some
> headers.

My preference is to move the common bits to a common location first,
and then modify to make it suitable for Microblaze also.  I've just
finished doing a bunch of fdt merge work where PowerPC and Microblaze
diverged in subtle ways.  It isn't a whole lot of fun.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.