Re: [PATCH]: 2.5.38uc1 (MMU-less support)

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Matthew Wilcox]

Thanks for splitting the patch up, makes it easier to see what's going on.
Let's have another go at making this better...

Motorola 5272 ethernet driver:
* In Config.in, let's conditionalise it on CONFIG_PPC or something
* Can you use module_init() so it doesn't need an entry in Space.c?
* You're defining CONFIG_* variables in the .c file.  I don't know whether
  this is something we're still trying to avoid doing ... Greg, you seem
  to be CodingStyle enforcer, what's the word?
* Why do you need to EXPORT_SYMBOL fec_register_ph and fec_unregister_ph?
* There's an awful lot of stuff conditionalised on CONFIG_M5272.  In general,
  having #ifdefs within functions is frowned upon.

Motorola 68328 and ColdFire serial drivers:
* Move to drivers/serial
* Lose this change from the Makefile:
-			selection.o sonypi.o sysrq.o tty_io.o tty_ioctl.o
+			selection.o sonypi.o sysrq.o tty_io.o tty_ioctl.o \
* Drop the custom MIN() definition.
* Port to new serial driver framework.

MTD driver patches for uClinux supported platforms:
I don't see any problems.  Submit to Linus via Dave Woodhouse, I guess.

Motorola 68328 framebuffer:
Don't see any problems here either.

uClinux FLAT file format exe loader:
* Drop the MAX() macro.
* +#include "../lib/inflate2.c".  Er.  You seem to have missed inflate2.c
  from your patch, and this really isn't the right way to do it anyway.
  Can't you share inflate.c these days?
* I'm also a little unsure about your per-arch #defines.  Could you put
  comments by each saying why they're necessary?

I haven't reveiwed the other two patches.

-- 
Revolutions do not require corporate support.

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg Ungerer]

Hi Matthew,

Matthew Wilcox wrote:
> Thanks for splitting the patch up, makes it easier to see what's going on.
> Let's have another go at making this better...

:-)


> Motorola 5272 ethernet driver:
> * In Config.in, let's conditionalise it on CONFIG_PPC or something

The FEC ethernet core has been used by Motorola in both PowerPC parts
(the MPC860) and the ColdFire (5272). But it could be conditional
on either of these.


> * Can you use module_init() so it doesn't need an entry in Space.c?

Easy done :-)


> * You're defining CONFIG_* variables in the .c file.  I don't know whether
>   this is something we're still trying to avoid doing ... Greg, you seem
>   to be CodingStyle enforcer, what's the word?

To be honest I don't know why all of these are not always included.
The extra code size is not that significant (tehse rae just PHY 
definitions).

Some history:
I didn't write this driver. It is Dan Malek's driver for the ethernet
core in the PowerPC 860. I took that code and generalizd it for use
on the ColdFire 5272 which also uses the same ethernet core design.

By and large I have tried to leave the original drivers functionalty
"as it was", conditionalizing the 5272 support as required (there are
a few key differences to be dealt with).

I approached Dan in the past about merging the code, and he was not
interrested.


> * Why do you need to EXPORT_SYMBOL fec_register_ph and fec_unregister_ph?

I can't see any use for this...


> * There's an awful lot of stuff conditionalised on CONFIG_M5272.  In general,
>   having #ifdefs within functions is frowned upon.

Yeah, there is a bit of that. All due to trying to handle the
silicon implementation differences on the 860 and 5272.
I'll see if I can clean it up some.


> Motorola 68328 and ColdFire serial drivers:
> * Move to drivers/serial
> * Lose this change from the Makefile:
> -			selection.o sonypi.o sysrq.o tty_io.o tty_ioctl.o
> +			selection.o sonypi.o sysrq.o tty_io.o tty_ioctl.o \
> * Drop the custom MIN() definition.
> * Port to new serial driver framework.

This is on my todo list :-)


> MTD driver patches for uClinux supported platforms:
> I don't see any problems.  Submit to Linus via Dave Woodhouse, I guess.

Already done. They have just not hit the mailline tree yet.
But David has it in his CVS now.


> Motorola 68328 framebuffer:
> Don't see any problems here either.
> 
> uClinux FLAT file format exe loader:
> * Drop the MAX() macro.
> * +#include "../lib/inflate2.c".  Er.  You seem to have missed inflate2.c
>   from your patch, and this really isn't the right way to do it anyway.
>   Can't you share inflate.c these days?
> * I'm also a little unsure about your per-arch #defines.  Could you put
>   comments by each saying why they're necessary?

I'll look at cleaning this up too. This code has been hacked
on by so many people, I am still cleaning the chewy bits of it :-)


> I haven't reveiwed the other two patches.

mmnommu is probably the most contenious, and effects the most
files. I am still looking at merging the separated mmnommu
directory at the top level back into the mm directory. Just
thinking about the cleanest way to do that.

The m68knommu is purely architecture support, doesn't mess
with anything else in the kernel.

Thanks for the feedback, much appreciated!

Regards
Greg



------------------------------------------------------------------------
Greg Ungerer  --  Chief Software Wizard        EMAIL:  gerg@snapgear.com
Snapgear Pty Ltd                               PHONE:    +61 7 3279 1822
825 Stanley St,                                  FAX:    +61 7 3279 1820
Woolloongabba, QLD, 4102, Australia              WEB:   www.SnapGear.com

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg KH]

On Wed, Sep 25, 2002 at 03:19:43PM +0100, Matthew Wilcox wrote:
> * You're defining CONFIG_* variables in the .c file.  I don't know whether
>   this is something we're still trying to avoid doing ... Greg, you seem
>   to be CodingStyle enforcer, what's the word?

I haven't seen that used very much, but I would not recommend it.  Why
not just move it to the Config.in file?  And if they are always set,
just remove them.

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg Ungerer]

Hi Matthew,

Some more comments on the ethernet driver.

BTW, the original this came from is in the kernel tree
at arch/ppc/8xx_io/fec.c.

Matthew Wilcox wrote:
> Motorola 5272 ethernet driver:
> * In Config.in, let's conditionalise it on CONFIG_PPC or something
> * Can you use module_init() so it doesn't need an entry in Space.c?

I don't think this will work. This is not a device that can be
determined to be present like a PCI device. It is more like an
ISA device, it needs to be probed to figure out if it is really
there. I can't see any way not to use Space.c for non-auto-detectable
type devices... (Offcourse I could be missing something :-)


> * You're defining CONFIG_* variables in the .c file.  I don't know whether
>   this is something we're still trying to avoid doing ... Greg, you seem
>   to be CodingStyle enforcer, what's the word?

I missed this the first time through :-)
I am not sure what you mean, CodingStyle enforcer?
Can you elaborate for me?

Regards
Greg

------------------------------------------------------------------------
Greg Ungerer  --  Chief Software Wizard        EMAIL:  gerg@snapgear.com
SnapGear Pty Ltd                               PHONE:    +61 7 3435 2888
825 Stanley St,                                  FAX:    +61 7 3891 3630
Woolloongabba, QLD, 4102, Australia              WEB:   www.SnapGear.com

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Randy Dunlap]

>> * You're defining CONFIG_* variables in the .c file.  I don't know
>> whether
>>   this is something we're still trying to avoid doing ... Greg, you
>> seem to be CodingStyle enforcer, what's the word?
>
> I missed this the first time through :-)
> I am not sure what you mean, CodingStyle enforcer?
> Can you elaborate for me?


Willy is talking about Greg Kroah-Hartman here, not you.

~Randy

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg Ungerer]

Randy Dunlap wrote:
>>>* You're defining CONFIG_* variables in the .c file.  I don't know
>>>whether
>>>  this is something we're still trying to avoid doing ... Greg, you
>>>seem to be CodingStyle enforcer, what's the word?
>>
>>I missed this the first time through :-)
>>I am not sure what you mean, CodingStyle enforcer?
>>Can you elaborate for me?
> 
> 
> 
> Willy is talking about Greg Kroah-Hartman here, not you.

Oooh, ok, that makes sense then.

------------------------------------------------------------------------
Greg Ungerer  --  Chief Software Wizard        EMAIL:  gerg@snapgear.com
SnapGear Pty Ltd                               PHONE:    +61 7 3435 2888
825 Stanley St,                                  FAX:    +61 7 3891 3630
Woolloongabba, QLD, 4102, Australia              WEB:   www.SnapGear.com

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg Ungerer]

Hi Matthew,

I just generated a new patch, linux-2.5.38uc2.
Which addresses a lot of your comments. I did a
lot of work cleaning the ethernet driver, would
be very interreted in your thoughts on that now...

Regards
Greg




Matthew Wilcox wrote:
> Thanks for splitting the patch up, makes it easier to see what's going on.
> Let's have another go at making this better...
> 
> Motorola 5272 ethernet driver:
> * In Config.in, let's conditionalise it on CONFIG_PPC or something
> * Can you use module_init() so it doesn't need an entry in Space.c?
> * You're defining CONFIG_* variables in the .c file.  I don't know whether
>   this is something we're still trying to avoid doing ... Greg, you seem
>   to be CodingStyle enforcer, what's the word?
> * Why do you need to EXPORT_SYMBOL fec_register_ph and fec_unregister_ph?
> * There's an awful lot of stuff conditionalised on CONFIG_M5272.  In general,
>   having #ifdefs within functions is frowned upon.
> 
> Motorola 68328 and ColdFire serial drivers:
> * Move to drivers/serial
> * Lose this change from the Makefile:
> -			selection.o sonypi.o sysrq.o tty_io.o tty_ioctl.o
> +			selection.o sonypi.o sysrq.o tty_io.o tty_ioctl.o \
> * Drop the custom MIN() definition.
> * Port to new serial driver framework.
> 
> MTD driver patches for uClinux supported platforms:
> I don't see any problems.  Submit to Linus via Dave Woodhouse, I guess.
> 
> Motorola 68328 framebuffer:
> Don't see any problems here either.
> 
> uClinux FLAT file format exe loader:
> * Drop the MAX() macro.
> * +#include "../lib/inflate2.c".  Er.  You seem to have missed inflate2.c
>   from your patch, and this really isn't the right way to do it anyway.
>   Can't you share inflate.c these days?
> * I'm also a little unsure about your per-arch #defines.  Could you put
>   comments by each saying why they're necessary?
> 
> I haven't reveiwed the other two patches.
> 

-- 
------------------------------------------------------------------------
Greg Ungerer  --  Chief Software Wizard        EMAIL:  gerg@snapgear.com
SnapGear Pty Ltd                               PHONE:    +61 7 3435 2888
825 Stanley St,                                  FAX:    +61 7 3891 3630
Woolloongabba, QLD, 4102, Australia              WEB:   www.SnapGear.com

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Matthew Wilcox]

On Thu, Sep 26, 2002 at 12:35:36PM +1000, Greg Ungerer wrote:
> BTW, the original this came from is in the kernel tree
> at arch/ppc/8xx_io/fec.c.

Heh.. looks like that driver should move to drivers/net too.

> I don't think this will work. This is not a device that can be
> determined to be present like a PCI device. It is more like an
> ISA device, it needs to be probed to figure out if it is really
> there. I can't see any way not to use Space.c for non-auto-detectable
> type devices... (Offcourse I could be missing something :-)

Sure you can use module_init for non-pci devices... look at 3c501.c and
3c59x.c for examples.

-- 
Revolutions do not require corporate support.

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg Ungerer]

Hi Matthew,

Matthew Wilcox wrote:
> On Thu, Sep 26, 2002 at 12:35:36PM +1000, Greg Ungerer wrote:
> 
>>BTW, the original this came from is in the kernel tree
>>at arch/ppc/8xx_io/fec.c. 
> 
> Heh.. looks like that driver should move to drivers/net too.

Indeed. Drivers in the arch directories is just wrong.


>>I don't think this will work. This is not a device that can be
>>determined to be present like a PCI device. It is more like an
>>ISA device, it needs to be probed to figure out if it is really
>>there. I can't see any way not to use Space.c for non-auto-detectable
>>type devices... (Offcourse I could be missing something :-) 
> 
> Sure you can use module_init for non-pci devices... look at 3c501.c and
> 3c59x.c for examples.

OK, that is what I needed :-)
The trick is to define locally your net_device structure.
OK, all fixed up now, no Space.c entries required.

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Chief Software Wizard        EMAIL:  gerg@snapgear.com
SnapGear Pty Ltd                               PHONE:    +61 7 3435 2888
825 Stanley St,                                  FAX:    +61 7 3891 3630
Woolloongabba, QLD, 4102, Australia              WEB:   www.SnapGear.com

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg Ungerer]

Hi Greg,

Greg KH wrote:
> The driver patches look good, and I didn't see anything wrong with the
> exception of what Matthew already said.
> 
> But I did have a small question about the font:
> 
> 
>>. Motorola 68328 framebuffer
>>http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-fb.patch.gz
> 
> 
> The % and & characters are the same.  Is this intentional?

No :-)
Yank bug. Fixed now.

Regards
Greg



------------------------------------------------------------------------
Greg Ungerer  --  Chief Software Wizard        EMAIL:  gerg@snapgear.com
SnapGear Pty Ltd                               PHONE:    +61 7 3435 2888
825 Stanley St,                                  FAX:    +61 7 3891 3630
Woolloongabba, QLD, 4102, Australia              WEB:   www.SnapGear.com

Re: [PATCH]: 2.5.38uc1 (MMU-less support)

[Greg KH]

The driver patches look good, and I didn't see anything wrong with the
exception of what Matthew already said.

But I did have a small question about the font:

> . Motorola 68328 framebuffer
> http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-fb.patch.gz

The % and & characters are the same.  Is this intentional?

thanks,

greg k-h

[PATCH]: 2.5.38uc1 (MMU-less support)

[Greg Ungerer]

Hi All,

A new iteration of the uClinux MMU-less support patches.
The all-in-one patch is at:

http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1.patch.gz

And new this time around I have broken this up into a number
of smaller self-contained patches. Each is a nice logical unit
(like a driver, or framebuffer, etc). This should greatly
simplify any merging into the mainline code :-)

So the here they are:

. Motorola 5272 ethernet driver
http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-fec.patch.gz

. Motorola 68328 and ColdFire serial drivers
http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-serial.patch.gz

. MTD driver patches for uClinux supported platforms
http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-mtd.patch.gz

. Motorola 68328 framebuffer
http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-fb.patch.gz

. uClinux FLAT file format exe loader
http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-binflat.patch.gz

. MMU-less support
http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-mmnommu.patch.gz

. Motorola embedded m68k/ColdFire architecture support
   (support for 68328, 68360, 5206, 5206e, 5249, 5272, 5307, 5407)
http://www.uclinux.org/pub/uClinux/uClinux-2.5.x/linux-2.5.38uc1-m68knommu.patch.gz

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Chief Software Wizard        EMAIL:  gerg@snapgear.com
SnapGear Pty Ltd                               PHONE:    +61 7 3435 2888
825 Stanley St,                                  FAX:    +61 7 3891 3630
Woolloongabba, QLD, 4102, Australia              WEB:   www.SnapGear.com