[PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Nicolas DET]

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

This 'big' patch adds support for CHRP/MPC52xx based platform. Here, 
this is the bPlan's Efika computer 
(http://www.bplan-gmbh.de/efika_spec_en.html)

We know this patch is not totaly compliant with Documentation/CodingStyle/.

We would like people to comment and review it. This way we would provide 
a new patch with the changes required (if any) for an upcomming merge in 
the kernel.

This patch has been applied on the kernel 2.6.18.1.

Signed-off-by: Nicolas DET <nd@bplan-gmbh.de>
Signed-off-by: Sven Luther <sl@bplan-gmbh.de>



[-- Attachment #2: chrpmpc52x_bigone.patch.bz2 --]
[-- Type: application/x-bzip, Size: 24315 bytes --]

[-- Attachment #3: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Jon Loeliger]

So, like, the other day Nicolas DET mumbled:
> 
> This 'big' patch adds support for CHRP/MPC52xx based platform. Here, 
> this is the bPlan's Efika computer 
> (http://www.bplan-gmbh.de/efika_spec_en.html)

In mail like this with buried patches, could we at least get
the full "git diff --stat HEAD"  or "git apply --stat /path/to/patch"
included in the base mail too please?

Thanks,
jdl

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Linas Vepstas]

On Thu, Oct 19, 2006 at 02:39:01PM +0200, Nicolas DET wrote:
> This 'big' patch adds support for CHRP/MPC52xx based platform. Here, 
> this is the bPlan's Efika computer 
> (http://www.bplan-gmbh.de/efika_spec_en.html)
> 
> We know this patch is not totaly compliant with Documentation/CodingStyle/.
> 
> We would like people to comment and review it. 

Can you send the patch unzipped, and not as an attachement? 
This would make reviewing and commenting much easier.

--linas

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Grant Likely]

On 10/19/06, Linas Vepstas <linas@austin.ibm.com> wrote:
> On Thu, Oct 19, 2006 at 02:39:01PM +0200, Nicolas DET wrote:
> > This 'big' patch adds support for CHRP/MPC52xx based platform. Here,
> > this is the bPlan's Efika computer
> > (http://www.bplan-gmbh.de/efika_spec_en.html)
> >
> > We know this patch is not totaly compliant with Documentation/CodingStyle/.
> >
> > We would like people to comment and review it.
>
> Can you send the patch unzipped, and not as an attachement?
> This would make reviewing and commenting much easier.

Yes, and please break it up into more digestible chunks.

cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Olof Johansson]

On Thu, 19 Oct 2006 14:39:01 +0200 Nicolas DET <nd@bplan-gmbh.de> wrote:

> We know this patch is not totaly compliant with Documentation/CodingStyle/.
> 
> We would like people to comment and review it. This way we would provide 
> a new patch with the changes required (if any) for an upcomming merge in 
> the kernel.

Hi,

Like others have said, a monolithic patch like this is really hard to
review. Please split it up, there are several logical ways it could be
cut, drivers vs base platform support, etc.

First reaction from just browsing through it:

* Lots of SillyCaps, i.e. codingstyle issues, especially in the
  bestcomm code
* No need to have filenames at top of files (they're even wrong in some
  cases)
* You reinvented pr_debug() (everyone loves doing this, myself included)
* Some hardcoded baudrate stuff in the serial driver. Should probably
  be passed in from somewhere -- device tree?
* Network drivers should be posted to netdev, not the arch list


More thorough review would be easier to do with split-up patches.


-Olof

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Benjamin Herrenschmidt]

On Thu, 2006-10-19 at 11:18 -0500, Linas Vepstas wrote:
> On Thu, Oct 19, 2006 at 02:39:01PM +0200, Nicolas DET wrote:
> > This 'big' patch adds support for CHRP/MPC52xx based platform. Here, 
> > this is the bPlan's Efika computer 
> > (http://www.bplan-gmbh.de/efika_spec_en.html)
> > 
> > We know this patch is not totaly compliant with Documentation/CodingStyle/.
> > 
> > We would like people to comment and review it. 
> 
> Can you send the patch unzipped, and not as an attachement? 
> This would make reviewing and commenting much easier.

It's too big for the list.

Ben.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Kumar Gala]

On Oct 19, 2006, at 6:41 PM, Benjamin Herrenschmidt wrote:

> On Thu, 2006-10-19 at 11:18 -0500, Linas Vepstas wrote:
>> On Thu, Oct 19, 2006 at 02:39:01PM +0200, Nicolas DET wrote:
>>> This 'big' patch adds support for CHRP/MPC52xx based platform. Here,
>>> this is the bPlan's Efika computer
>>> (http://www.bplan-gmbh.de/efika_spec_en.html)
>>>
>>> We know this patch is not totaly compliant with Documentation/ 
>>> CodingStyle/.
>>>
>>> We would like people to comment and review it.
>>
>> Can you send the patch unzipped, and not as an attachement?
>> This would make reviewing and commenting much easier.
>
> It's too big for the list.

I think the reasons its to big is it includes patches for platform,  
uart, and enet drivers on 52xx.

- k

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Kumar Gala]

On Oct 19, 2006, at 7:39 AM, Nicolas DET wrote:

> This 'big' patch adds support for CHRP/MPC52xx based platform.  
> Here, this is the bPlan's Efika computer (http://www.bplan-gmbh.de/ 
> efika_spec_en.html)
>
> We know this patch is not totaly compliant with Documentation/ 
> CodingStyle/.
>
> We would like people to comment and review it. This way we would  
> provide a new patch with the changes required (if any) for an  
> upcomming merge in the kernel.
>
> This patch has been applied on the kernel 2.6.18.1.
>
> Signed-off-by: Nicolas DET <nd@bplan-gmbh.de>
> Signed-off-by: Sven Luther <sl@bplan-gmbh.de>

I think it would be extremely useful to get a dump of what the device- 
tree looks like.  We have to be very careful to device nodes for 52xx  
components are competed consistent between this 'CHRP' style and a  
more embedded 52xx boards.

Some high level comments:
1. lets stick with the 52xx naming, instead of 5k2
2. PIC code needs to be updated for new interrupt model (as well as  
remove of pt_regs)
3. use standard kernel debug macros
4. look at replacing sram_allocator w/rheap

if you repost the patch broken up into driver and platform bits it  
will be easier to provide more detailed comments.

- kumar

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Sven Luther]

On Fri, Oct 20, 2006 at 01:06:14AM -0500, Kumar Gala wrote:
> 
> On Oct 19, 2006, at 7:39 AM, Nicolas DET wrote:
> 
> >This 'big' patch adds support for CHRP/MPC52xx based platform.  
> >Here, this is the bPlan's Efika computer (http://www.bplan-gmbh.de/ 
> >efika_spec_en.html)
> >
> >We know this patch is not totaly compliant with Documentation/ 
> >CodingStyle/.
> >
> >We would like people to comment and review it. This way we would  
> >provide a new patch with the changes required (if any) for an  
> >upcomming merge in the kernel.
> >
> >This patch has been applied on the kernel 2.6.18.1.
> >
> >Signed-off-by: Nicolas DET <nd@bplan-gmbh.de>
> >Signed-off-by: Sven Luther <sl@bplan-gmbh.de>
> 
> I think it would be extremely useful to get a dump of what the device- 
> tree looks like.  We have to be very careful to device nodes for 52xx  
> components are competed consistent between this 'CHRP' style and a  
> more embedded 52xx boards.

Well, the efika is a CHRP board, and thus more akin to the pegasos or IBM
boxes than the embedded 52xx boards with regard to the device trees. Nico will
provide you an lsprop output.

> Some high level comments:
> 2. PIC code needs to be updated for new interrupt model (as well as  
> remove of pt_regs)

Notice that the efika does not have the interrupt tree bindings in the
firmware. I implemented those on pegasos last month, but linux refused to
boot, and i am currently waiting for comments from benh on the lsprop output.

Friendly,

Sven Luther

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Benjamin Herrenschmidt]

On Fri, 2006-10-20 at 01:06 -0500, Kumar Gala wrote:
> On Oct 19, 2006, at 7:39 AM, Nicolas DET wrote:
> 
> > This 'big' patch adds support for CHRP/MPC52xx based platform.  
> > Here, this is the bPlan's Efika computer (http://www.bplan-gmbh.de/ 
> > efika_spec_en.html)
> >
> > We know this patch is not totaly compliant with Documentation/ 
> > CodingStyle/.
> >
> > We would like people to comment and review it. This way we would  
> > provide a new patch with the changes required (if any) for an  
> > upcomming merge in the kernel.
> >
> > This patch has been applied on the kernel 2.6.18.1.
> >
> > Signed-off-by: Nicolas DET <nd@bplan-gmbh.de>
> > Signed-off-by: Sven Luther <sl@bplan-gmbh.de>
> 
> I think it would be extremely useful to get a dump of what the device- 
> tree looks like.  We have to be very careful to device nodes for 52xx  
> components are competed consistent between this 'CHRP' style and a  
> more embedded 52xx boards.

They should use either the /soc bus or the of_platform stuff (I'm coming
up with some reworked of_platform that can be useful to them).

Ben.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Nicolas DET]

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

Kumar Gala wrote:
 >
 > On Oct 19, 2006, at 7:39 AM, Nicolas DET wrote:
 >
 >> This 'big' patch adds support for CHRP/MPC52xx based platform. Here, 
this is the bPlan's Efika computer 
(http://www.bplan-gmbh.de/efika_spec_en.html)
 >

 > Some high level comments:
 > 1. lets stick with the 52xx naming, instead of 5k2

Ok. Will be done

 > 2. PIC code needs to be updated for new interrupt model (as well as 
remove of pt_regs)

Ok.

 > 3. use standard kernel debug macros

Loads of changes in the pipe line ;-)

 > 4. look at replacing sram_allocator w/rheap
 >

This SRAM allocator is the exact same from the original Linux one. In 
fact, it is the original one. Would it be possible to accept this code 
as it is and schedule rheap integration later ?

 > if you repost the patch broken up into driver and platform bits it 
will be easier to provide more detailed comments.

Ok. I'm alraedy working on it since I post the first one :-) . Thank you 
very much for your comments. I hope to re submit a much better patch today.

Regads

[-- Attachment #2: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Kumar Gala]

On Oct 20, 2006, at 3:12 AM, Nicolas DET wrote:

> Kumar Gala wrote:
> >
> > On Oct 19, 2006, at 7:39 AM, Nicolas DET wrote:
> >
> >> This 'big' patch adds support for CHRP/MPC52xx based platform.  
> Here, this is the bPlan's Efika computer (http://www.bplan-gmbh.de/ 
> efika_spec_en.html)
> >
>
> > Some high level comments:
> > 1. lets stick with the 52xx naming, instead of 5k2
>
> Ok. Will be done
>
> > 2. PIC code needs to be updated for new interrupt model (as well  
> as remove of pt_regs)
>
> Ok.
>
> > 3. use standard kernel debug macros
>
> Loads of changes in the pipe line ;-)
>
> > 4. look at replacing sram_allocator w/rheap
> >
>
> This SRAM allocator is the exact same from the original Linux one.  
> In fact, it is the original one. Would it be possible to accept  
> this code as it is and schedule rheap integration later ?

I dont follow, what do you mean by 'original Linux one' ?

> > if you repost the patch broken up into driver and platform bits  
> it will be easier to provide more detailed comments.
>
> Ok. I'm alraedy working on it since I post the first one :-) .  
> Thank you very much for your comments. I hope to re submit a much  
> better patch today.
>
> Regads
> <nd.vcf>

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Nicolas DET]

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

Kumar Gala wrote:

 >>
 >> > 4. look at replacing sram_allocator w/rheap
 >> >
 >>
 >> This SRAM allocator is the exact same from the original Linux one. In
 >> fact, it is the original one. Would it be possible to accept this code
 >> as it is and schedule rheap integration later ?
 >
 > I dont follow, what do you mean by 'original Linux one' ?

There are already some kind of Bestcomm DMA engine supoprt for some 
embbeded system (ARCH=ppc). In order to speed up the developement, the 
API is (almost) compatible so we can use the drivers wrote for this API. 
For example, the Ethernet driver enterily rely on this API. However, the 
'soon to be release' ATA driver has been rewritten.

I did not at the time (yet) to look at rheap, but I know the current 
allocator is not really good (it can not free!).

I was just thinking, as this code is alraedy in some Linux tree, we 
could still use it there, and plan to replace it by a much better one 
(keeping the API compatible) later on.

Regards,



[-- Attachment #2: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Kumar Gala]

On Oct 20, 2006, at 9:24 AM, Nicolas DET wrote:

> Kumar Gala wrote:
>
> >>
> >> > 4. look at replacing sram_allocator w/rheap
> >> >
> >>
> >> This SRAM allocator is the exact same from the original Linux  
> one. In
> >> fact, it is the original one. Would it be possible to accept  
> this code
> >> as it is and schedule rheap integration later ?
> >
> > I dont follow, what do you mean by 'original Linux one' ?
>
> There are already some kind of Bestcomm DMA engine supoprt for some  
> embbeded system (ARCH=ppc). In order to speed up the developement,  
> the API is (almost) compatible so we can use the drivers wrote for  
> this API. For example, the Ethernet driver enterily rely on this  
> API. However, the 'soon to be release' ATA driver has been rewritten.
>
> I did not at the time (yet) to look at rheap, but I know the  
> current allocator is not really good (it can not free!).
>
> I was just thinking, as this code is alraedy in some Linux tree, we  
> could still use it there, and plan to replace it by a much better  
> one (keeping the API compatible) later on.

Is this in the kernel tree already, I am not coming across it.

- k

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Benjamin Herrenschmidt]

> > I was just thinking, as this code is alraedy in some Linux tree, we  
> > could still use it there, and plan to replace it by a much better  
> > one (keeping the API compatible) later on.
> 
> Is this in the kernel tree already, I am not coming across it.

Probably only in the WD tree and not even attempted to be merged like
most of the gunk in there...

Ben

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Nicolas DET]

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

Kumar Gala wrote:
 >
 > On Oct 20, 2006, at 3:12 AM, Nicolas DET wrote:
 >
 >> Kumar Gala wrote:
 >> >
 >> > On Oct 19, 2006, at 7:39 AM, Nicolas DET wrote:
 >> >
 >> >> This 'big' patch adds support for CHRP/MPC52xx based platform.
 >> Here, this is the bPlan's Efika computer
 >> (http://www.bplan-gmbh.de/efika_spec_en.html)
 >> >
 >>
 >> > Some high level comments:
 >> > 1. lets stick with the 52xx naming, instead of 5k2
 >>
 >> Ok. Will be done
 >>
 >> > 2. PIC code needs to be updated for new interrupt model (as well as
 >> remove of pt_regs)
 >>
 >> Ok.
 >>
 >> > 3. use standard kernel debug macros
 >>
 >> Loads of changes in the pipe line ;-)
 >>
 >> > 4. look at replacing sram_allocator w/rheap
 >> >
 >>
 >> This SRAM allocator is the exact same from the original Linux one. In
 >> fact, it is the original one. Would it be possible to accept this code
 >> as it is and schedule rheap integration later ?
 >
 > I dont follow, what do you mean by 'original Linux one' ?
 >

There is a driver for Linux (not in the official kernel.org tree) for 
this chipset. This is the one I'm speaking about
You can find it into various embbeded kernel like MontaVista tree, etc...

By the way, I uploaded the current patch on a private URL:
http://powernico.free.fr/chrpmpc52x_bigone.patch.bz2

If people wish to have a look.


[-- Attachment #2: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Benjamin Herrenschmidt]

Ok, here's a batch of comments. They go on top of the comments I sent
about your device-tree, which you might want to make public.

Also note that the size of the patch is partially due to stale ##
and .orig files :)

In general, one big thing is: Don't bother with re-using the arch/ppc
code, API, .h etc... There is not enough re-use there to justify it (FEC
driver is new, serial can easily be changed, as for USB).

* /proc/ppc64/rtas changes: we need to continue that discussion
separately and ask for paulus point of view. I prefer /proc/powerpc
personally with a /proc/ppc64->/proc/powerpc symlink on 64 bits machines
only.

* mpc5k2_bestcomm.c (& helpers):

  - name changes (mpc5200_bestcomm.c would be better)
  - location change -> arch/powerpc/sysdev
  - look into using rheap instead of your current sram bits (and I don't
care about being compatible with arch/ppc code).
  - the whole sdma_io_va/pa/... looks like bullcrap to me. The bestcomm
interface should use the normal dma mapping APIs
  - I don't like having those helpers like fec or ata helpers in that
file. I think we need to rethink the API between the bestcomm and the
drivers. I understand that part of this is related to the fact that the
tasks themselves in the bestcomm are tailored for various devices and
with different descriptor formats. That sucks. I suppose we can live
with that for now (with a bit more cleanup) but we should have a healthy
discussion on how to rework the whole thing properly.

In general, I think there's a lot more code in those files that would be
necessary ;)

* mpc5k2.c :

Well, to start with, I don't think we need that file at all :) A lot of
the stuff in there is completely unnecessary provided that you make
better use of the device-tree. 

The main thing is: use of_platform devices, not platform devices, so
that you have a linkage with the device-tree. In fact, you could
register of platform devices for every direct children of your "buildin"
node (which is to be renamed, I hope, according to my comments about
your device-tree).

You'll even be able to make good use of some work I've been doing lately
for Cell by adding a call to a single function to the main chrp setup
code that will register for you all those devices. You might want to
keep PCI at the root of the tree for now though that won't even be
necessary.

The core of platform matching code will take care of matching devices to
drivers based on standard OF properties (name,device_type,compatible).
Drivers will use standard prom_parse.c functions to obtain resources
like addresses (instead of directly accessing "reg" property from a
useless helper) etc...

But first, let's get your device-tree in shape :)

* mpc5k2_pic.c :

This should go in arch/powerpc/sysdev (and be renamed as for other
files, something like mpc5200_pic.c would be fine).

There are several issues here, though some of them are definitely
related to the chip design being completely alien to common sense:

 - enable/disable/end shouldn't be used that way anymore. use
mask/unmask and set the appropriate IRQ flow handler for your IRQs. That
is, basically, adapt your driver to genirq. That will simplify.

 - You need to add an irq host, probably set it as default, with the
appropriate xlate, map, etc... routines as done by other controllers in
2.6.18.

 - the if/else bits in enable/disable (to become mask/unmask) should
probably be changed into something a bit more sane. You have a large
freedom of chosing what kind of IRQ "hardware" numbers you use. Since
2.6.18 new irq code, there is no direct relationship anymore between a
linux irq number (virtual irq number) and a HW number manipulated by a
PIC. Thus you can do what you want with those HW numbers, like, for
example, using some top bits to represent the "base" (IRQ0...3, SDMA,
PERP, ...) and use bottom bits for the actual bit in the mask. That will
simplify your mask/unmask implementation.

* chrp/pci.c

I dislike the whole "is_bplan" thingy with ugly magic numbers. The
is_pegasos with 3 values was already on the limit .... In fact, you
probably don't even need it. The whole pcibios_fixup change is useless,
let the fixup code run, it will do no harm, and in fact will be
necessary as soon as you have a correct interrupt-tree anyway. Which
means basically that you shoudn't even need to patch this file...

* chrp/setup.c

Did you do any change to chrp_init2() appart from moving it around ?
Please don't move it for no reason or explain why you did it, it makes
the patch bigger than necessary and more difficult to read.

Why did you add back bits that I removed, like:

+       if (_chrp_type == _CHRP_Pegasos)
+               ppc_md.get_irq        = i8259_irq;

This should be handled by the chrp code just fine now (basically 8259
takes over when no mpic was found and sets that callback).

You may have noticed that I changed the code a bit too regaring how irq
controllers are handled in chrp in 2.6.18. You should follow that
instead of bringing back the old scheme. Basically add a
chrp_find_mpc5200_pic or something like that and have it change
ppc_md.get_irq itself if it finds it.

This all init2 vs init_alt business is very ugly. What is your reason
for doing so ? What is chrp_init2 doesn't work for you ? (you should fix
that instead).

* fec driver

(That driver should ultimately be submited to the netdev list & Jeff
Garzik, but here's a fist pass at comments on things that need to be
fixed before you get there).

I'm not sure you have enough files here to justify a subdir. Especially
since you should probably get rid of fec_phy.c and replace it with the
generic PHY code that Andy Fleming wrote in drivers/net/phy/. That old
state machine PHY code from montavista has outlived its usefulness

The driver should be in drivers/net, in fact, it should just be one or
two files: drivers/net/mpc5200_fec.{c,h}

Do not keep #ifdefs around for chrp/non-chrp. I don't want the mpc5200
stuff to have any difference between platforms. The driver wasn't in the
tree before so I don't care about whatevber old API was used by an out
of tree driver in arch/ppc. We are defining an API for use by any
mpc5200 based device under arch/powerpc here, nothing specific to EFIKA
or CHRP.

+       if ( _chrp_type == _CHRP_E5K2 )
+               return (132*1000*1000);
+

Gack ! Make that a  property in the device-tree instead. By converting
the driver to be an of_platform device, you'll get your device-node
reference for free anyway.

kfree_skb() -> dev_kfree_skb() afaik. Also, you might want to make sure
you are getting the right irq/non-irq/any version of it depending on the
context you are calling it in.

You are never calling netif_carrier_on/off... might be useful to do so
to inform the kernel about the state of your link... though if you use
the generic PHY layer, it will do it for you.

+               if (status & 0x370000) {

What about using symbolic constants instead ?

virt_to_phys() is deprecated. See my comments in the bestcomm code too,
use the dma mapping API instead.

Your rx code doesn't seem to do the fairly common optimisation of
copying the data to a new skb when it's small enough rather than
allocating a new one and replacing...

fec_interrupt() has some serious coding style issues.

I'm a bit dubious about the implementation of the re-init code... it
looks racy vs. interrupts, and I don't like at all you calling back into
open(). I'd rather have a separate common routine called by both open
and reinit. You may also want to defer that to a work queue.

ethtool ioctls: You are using a deprecated interface. grep for
ethtool_ops for the right way to do it.

+               while(!priv->sequence_done)
+                       schedule();
+

This is evil. Don't do that. In fact, you should not need to do it, look
at what other drivers do.

* serial stuff

Pretty much all of the same comments as the fec driver regarding
things like:

+static unsigned long get_ipbfreq(void)
+{
+#ifdef CONFIG_PPC_CHRP
+       if ( _chrp_type == _CHRP_E5K2 )
+               return 132*1000*1000;
+
+       return 0;
+#else
+       return __res.bi_ipbfreq
+#endif
+}
+

Just get rid of the bi case and remove the ifdef. Now howevr that I see
at least 2 drivers having the same function to get this ibp freq I'm
starting to wonder why you don't just put the value in the device-tree
possibly in the parent node ?

Same for the default baudrate. Remove the ifdef gunk. Use the baudrate
that's passed in by the system and possibly implement something akin to
check_legacy_serial_console() in arch/powerpc/kernel/legacy_serial.c to
get proper auto-detect. (I would even accept adding a special case to
detect the mpc5200 serial in there)

Make it an of_platform device anyway, that's better. Just ifdef to be
compatible with the stuff in arch/ppc (I didn't even know we ever merged
that... oh well).

* USB

Do an ohci-ppc-of.c that use an OF platform device for probing

Ben.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Matt Sealey]

Curious question.

What are you guys going to do when the PowerPC name is defunct?

Like, last month :)

Power Architecture is where it is at. The trademark is even going to
lapse. It's a bit too late for the ppc->powerpc tree breakout now,
but wouldn't it just confuse people to be using a "Power Architecture"
processor or SoC of some type, using collections of definitions from
the Power ISA 2.03 and have this "powerpc" thing pop up?

It confused me even before, because ppc and ppc64 have also been
used to support real POWER (with a capital P, O, W, E and R) processors,
and now these are lumped in with powerpc which is no better than
ppc64 in these terms?

Just flexing my marketing exec muscles, see if they work, never done
it before. Oh... *crack*.. that wasn't a good noise :]

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Benjamin Herrenschmidt wrote:
> Ok, here's a batch of comments. They go on top of the comments I sent
> about your device-tree, which you might want to make public.
> 
> Also note that the size of the patch is partially due to stale ##
> and .orig files :)
> 
> In general, one big thing is: Don't bother with re-using the arch/ppc
> code, API, .h etc... There is not enough re-use there to justify it (FEC
> driver is new, serial can easily be changed, as for USB).
> 
> * /proc/ppc64/rtas changes: we need to continue that discussion
> separately and ask for paulus point of view. I prefer /proc/powerpc
> personally with a /proc/ppc64->/proc/powerpc symlink on 64 bits machines
> only.
> 
[snip]

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Christoph Hellwig]

On Mon, Oct 23, 2006 at 04:47:49PM +0200, Matt Sealey wrote:
> Curious question.
> 
> What are you guys going to do when the PowerPC name is defunct?
> 
> Like, last month :)
> 
> Power Architecture is where it is at. The trademark is even going to
> lapse. It's a bit too late for the ppc->powerpc tree breakout now,
> but wouldn't it just confuse people to be using a "Power Architecture"
> processor or SoC of some type, using collections of definitions from
> the Power ISA 2.03 and have this "powerpc" thing pop up?

Who the f***k cares.  We've always given a chip what the newspeak party
line of the days is, and we will here aswell.  If the IBM/Mot/PowerYodda
overlords are unhappy with that it's their problem, not ours.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Matt Sealey]

Very eloquently put.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Christoph Hellwig wrote:
> On Mon, Oct 23, 2006 at 04:47:49PM +0200, Matt Sealey wrote:
>> Curious question.
>>
>> What are you guys going to do when the PowerPC name is defunct?
>>
>> Like, last month :)
>>
>> Power Architecture is where it is at. The trademark is even going to
>> lapse. It's a bit too late for the ppc->powerpc tree breakout now,
>> but wouldn't it just confuse people to be using a "Power Architecture"
>> processor or SoC of some type, using collections of definitions from
>> the Power ISA 2.03 and have this "powerpc" thing pop up?
> 
> Who the f***k cares.  We've always given a chip what the newspeak party
> line of the days is, and we will here aswell.  If the IBM/Mot/PowerYodda
> overlords are unhappy with that it's their problem, not ours.
> 

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Segher Boessenkool]

> What are you guys going to do when the PowerPC name is defunct?
>
> Like, last month :)

If some marketeers want to rewrite history, let them try; but we
cannot, simply because we use Git.  Keeps people honest ;-)


Segher

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Benjamin Herrenschmidt]

On Mon, 2006-10-23 at 16:47 +0200, Matt Sealey wrote:
> Curious question.
> 
> What are you guys going to do when the PowerPC name is defunct?

It's not really, is it ? I don't care anyway, it will stay PowerPC in
linux of course :)

> Like, last month :)
> 
> Power Architecture is where it is at. The trademark is even going to
> lapse

Who cares ? Besides, while PAPR says "Power architecture", the processor
architecture specification says PowerPC :)

> . It's a bit too late for the ppc->powerpc tree breakout now,
> but wouldn't it just confuse people to be using a "Power Architecture"
> processor or SoC of some type, using collections of definitions from
> the Power ISA 2.03 and have this "powerpc" thing pop up?

It's PowerPC ISA :)

> It confused me even before, because ppc and ppc64 have also been
> used to support real POWER (with a capital P, O, W, E and R) processors,
> and now these are lumped in with powerpc which is no better than
> ppc64 in these terms?

Historically, POWER means something else ... then with POWER3, POWER
processors became compatible with the PowerPC architecture, then IBM
played name changing game a couple of times and nobody knows what's up
anymore :)
 
> Just flexing my marketing exec muscles, see if they work, never done
> it before. Oh... *crack*.. that wasn't a good noise :]

AFAIK, the processor instruction set architecture is PowerPC an that
will not change.

Ben.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Paul Mackerras]

Matt Sealey writes:

> It confused me even before, because ppc and ppc64 have also been
> used to support real POWER (with a capital P, O, W, E and R) processors,
> and now these are lumped in with powerpc which is no better than
> ppc64 in these terms?

"PowerPC" is the name of the instruction set architecture, "POWER" is
the name of some specific implementations of the PowerPC architecture
made by a certain large multinational computer company.  Unless you're
talking about the really really old ISA that was the predecessor of
PowerPC, that is. :)

Paul.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Matt Sealey]

Paul Mackerras wrote:
> Matt Sealey writes:
>
> "PowerPC" is the name of the instruction set architecture, "POWER" is
> the name of some specific implementations of the PowerPC architecture
> made by a certain large multinational computer company.  Unless you're
> talking about the really really old ISA that was the predecessor of
> PowerPC, that is. :)

http://www.power.org/news/articles/new_brand/#isa

I'm talking about the one they consolidated from all the different specs
Freescale, AMCC and IBM had about what makes a PowerPC processor. Since
POWER is part of it and it's all Power.org now.. they seem to have changed
the name.

I don't think they have actually added anything new, that wasn't there
before, just consolidated it. You won't seen an IBM chip with SPE or VLE
unless they license the gaggle of patents Freescale have on those
units, maybe.. but there is always a possibility. All new chips are
being called Power (not POWER).

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Becky Bruce]

On Oct 24, 2006, at 10:44 AM, Matt Sealey wrote:

>
> Paul Mackerras wrote:
>> Matt Sealey writes:
>>
>> "PowerPC" is the name of the instruction set architecture, "POWER" is
>> the name of some specific implementations of the PowerPC architecture
>> made by a certain large multinational computer company.  Unless  
>> you're
>> talking about the really really old ISA that was the predecessor of
>> PowerPC, that is. :)
>
> http://www.power.org/news/articles/new_brand/#isa
>
> I'm talking about the one they consolidated from all the different  
> specs
> Freescale, AMCC and IBM had about what makes a PowerPC processor.  
> Since
> POWER is part of it and it's all Power.org now.. they seem to have  
> changed
> the name.
>
> I don't think they have actually added anything new, that wasn't there
> before, just consolidated it. You won't seen an IBM chip with SPE  
> or VLE
> unless they license the gaggle of patents Freescale have on those
> units, maybe.. but there is always a possibility. All new chips are
> being called Power (not POWER).
>

The ISA version 2.03 that you are talking about does, in fact, rename  
it to "Power ISA" (big "P" little "ower") from "PowerPC ISA".  This  
spec is mostly a big glom of all of FSL and IBM's architecure  
documentation, although there were some minor changes made and some  
things were added.  If you read the spec carefully (which, btw, I do  
*not* recommend, as it is about 900 pages of spaghetti  
documentation), you will find things in there that do not exist in  
any implemented processor to date.

Cheers,
B

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Paul Mackerras]

Matt Sealey writes:

> http://www.power.org/news/articles/new_brand/#isa
> 
> I'm talking about the one they consolidated from all the different specs
> Freescale, AMCC and IBM had about what makes a PowerPC processor. Since
> POWER is part of it and it's all Power.org now.. they seem to have changed
> the name.

So, are you proposing we drop support for all the PowerPC processors
that don't conform to the recently-published V2.03 ISA?  You don't
want 6xx, 7xx, 7xxx, 52xx, etc. supported any more? :)  (joke :)

Seriously, the v2.03 ISA covers only IBM's latest POWER processors
(i.e. not POWER3, POWER4, or even POWER5), and Book E processors.  We
support a lot more than that.

Paul.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Matt Sealey]

It supports the original processors, G3, G4 etc. as well. Maybe not
the 601? I'm not sure. All the PowerPC ISA Books are in there and
information only seems to have been ADDED (or moved to another
book or been given a new book).

If a processor is not supported it was probably designed and
end-of-lined well before they made the original "Book" specs :D

Have you even looked at it? Even the dumb 'evolution' picture
on Power.org's site makes this quite, quite clear. Most of the
spec is identical, to the letter, over vast amounts of pages.
There are change bars in front of everything that changed and
those that have; it seems they are just lifted and reworked
from Freescale's AltiVec/SPE documentation, reworded in Book E
so you can have a Book E with AltiVec, or has it's own new
book (VLE, which is an IBM-ified copy of the e200 Core
Reference Manual Addendum)

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Paul Mackerras wrote:
> Matt Sealey writes:
> 
>> http://www.power.org/news/articles/new_brand/#isa
>>
>> I'm talking about the one they consolidated from all the different specs
>> Freescale, AMCC and IBM had about what makes a PowerPC processor. Since
>> POWER is part of it and it's all Power.org now.. they seem to have changed
>> the name.
> 
> So, are you proposing we drop support for all the PowerPC processors
> that don't conform to the recently-published V2.03 ISA?  You don't
> want 6xx, 7xx, 7xxx, 52xx, etc. supported any more? :)  (joke :)
> 
> Seriously, the v2.03 ISA covers only IBM's latest POWER processors
> (i.e. not POWER3, POWER4, or even POWER5), and Book E processors.  We
> support a lot more than that.
> 
> Paul.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Kumar Gala]

On Oct 25, 2006, at 9:44 AM, Matt Sealey wrote:

> It supports the original processors, G3, G4 etc. as well. Maybe not
> the 601? I'm not sure. All the PowerPC ISA Books are in there and
> information only seems to have been ADDED (or moved to another
> book or been given a new book).
>
> If a processor is not supported it was probably designed and
> end-of-lined well before they made the original "Book" specs :D
>
> Have you even looked at it? Even the dumb 'evolution' picture
> on Power.org's site makes this quite, quite clear. Most of the
> spec is identical, to the letter, over vast amounts of pages.
> There are change bars in front of everything that changed and
> those that have; it seems they are just lifted and reworked
> from Freescale's AltiVec/SPE documentation, reworded in Book E
> so you can have a Book E with AltiVec, or has it's own new
> book (VLE, which is an IBM-ified copy of the e200 Core
> Reference Manual Addendum)

There is far more new in the 2.03 spec than just merging of various  
existing specs as you are stating.  Please be more careful with  
general statements like this.  The Book III-E has some significant  
changes from Book-E 1.0.

- k

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Matt Sealey]

Kumar Gala wrote:
> 
>> so you can have a Book E with AltiVec, or has it's own new
>> book (VLE, which is an IBM-ified copy of the e200 Core
>> Reference Manual Addendum)
> 
> There is far more new in the 2.03 spec than just merging of various
> existing specs as you are stating.  Please be more careful with general
> statements like this.  The Book III-E has some significant changes from
> Book-E 1.0.

I don't think any of it suddenly makes a billion existing processors
completely incompatible.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Grant Likely]

On 10/25/06, Matt Sealey <matt@genesi-usa.com> wrote:
> It supports the original processors, G3, G4 etc. as well. Maybe not
> the 601? I'm not sure. All the PowerPC ISA Books are in there and
> information only seems to have been ADDED (or moved to another
> book or been given a new book).

Why are we still talking about this?  None of it matters.

Who cares if the new ISA spec incorporates all of the old ones?  The
old specs cannot be unpublished.  powerpc is a sufficient name for
kernel source purposes and it will continue to have meaning in the
minds of developers.  Marketing glossies can (and do) use the name de
jour, but we don't have to.

As a side note; The move from ppc/ppc64->powerpc was a *technical*
decision, not a marketing one.  It was done so there would be only one
code base.  The choice of arch/powerpc was almost arbitrary.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Matt Sealey]

Arbitrary naming is fine.

Thinking up a bunch of excuses like "we refuse to bow to our corporate
marketing overlords" and "but it's definitely called PowerPC ISA, and
anyway, that new one which has a new name, I didn't read anyway, but it
undefines every processor we support" is just.. well I dunno. Making
excuses I guess.

I'm fine with the name I was just curious (see first line of the original
email) about how these things ARE named. It lead to a query I am still
wondering about the answer to; Genesi/bplan could have a new G5 platform
and whatever support code is required submitted to the tree soon. Since
it will have a fairly-PAPR-compliant firmware, this makes it logical to
put it in the CHRP platform for us. But CHRP doesn't include LPAR, this is
a pSeries platform feature. Segher suggested we use the Maple tree (!!)
as this runs SLOF.

What platform do you think a 970MP box with a CHRP-compliant (well, let
us say, Pegasos-compatible) firmware go, if we eventually want to support
LPAR, especially without code duplication and MBs of "pull this out of
pSeries and drop it into CHRP, even though CHRP technically doesn't
support any of this" stuff? Especially since Ben is being quite strict
about what goes into CHRP nowadays.

Is it time for a PAPR platform? Or is that against the Linux
anti-marketing-name lobby we have in here?

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Grant Likely wrote:
> On 10/25/06, Matt Sealey <matt@genesi-usa.com> wrote:
>> It supports the original processors, G3, G4 etc. as well. Maybe not
>> the 601? I'm not sure. All the PowerPC ISA Books are in there and
>> information only seems to have been ADDED (or moved to another
>> book or been given a new book).
> 
> Why are we still talking about this?  None of it matters.
> 
> Who cares if the new ISA spec incorporates all of the old ones?  The
> old specs cannot be unpublished.  powerpc is a sufficient name for
> kernel source purposes and it will continue to have meaning in the
> minds of developers.  Marketing glossies can (and do) use the name de
> jour, but we don't have to.
> 
> As a side note; The move from ppc/ppc64->powerpc was a *technical*
> decision, not a marketing one.  It was done so there would be only one
> code base.  The choice of arch/powerpc was almost arbitrary.
> 
> Cheers,
> g.
> 

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Olof Johansson]

On Wed, 25 Oct 2006 18:18:43 +0200 Matt Sealey <matt@genesi-usa.com> wrote:

> Is it time for a PAPR platform?

There is a PAPR platform in the tree already, it's named pseries.


-Olof

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Paul Mackerras]

Matt Sealey writes:

> It supports the original processors, G3, G4 etc. as well. Maybe not
> the 601? I'm not sure. All the PowerPC ISA Books are in there and
> information only seems to have been ADDED (or moved to another
> book or been given a new book).

The only 32-bit processors it supports are Book E processors.  IBM
removed all the material relating to 32-bit implementations from the
earlier V2.0x architectures, because IBM was no longer interested in
32-bit server processors.

Paul.

Re: [PATCH] General CHRP/MPC5K2 Platform and drivers support - to comment

[Paul Mackerras]

Matt Sealey writes:

> Is it time for a PAPR platform? Or is that against the Linux
> anti-marketing-name lobby we have in here?

:)  All I would say is that if your platform really is PAPR compliant,
then sure, we could have a papr platform, but don't underestimate the
amount of work it will take to be PAPR compliant.  There is a ton of
stuff in there.

Paul.