[PATCH] Make ATNGW100 serial ports configurable

[PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell]

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



[-- Attachment #2: atngw100_serial.patch --]
[-- Type: text/plain, Size: 2610 bytes --]

Make configuration of ATNGW100 serial ports selectable in kernel configuration.

Signed-off-by: Anders Blomdell <anders.blomdell@control.lth.se>



diff -urb orig/arch/avr32/boards/atngw100/Kconfig linux-2.6.27-rc6/arch/avr32/boards/atngw100/Kconfig
--- orig/arch/avr32/boards/atngw100/Kconfig	2008-10-09 17:56:14.000000000 +0200
+++ linux-2.6.27-rc6/arch/avr32/boards/atngw100/Kconfig	2008-10-09 16:26:08.000000000 +0200
@@ -0,0 +1,18 @@
+config SERIAL_ATMEL_USART0
+	bool "Enable USART0"
+	depends on SERIAL_ATMEL
+	help
+	  Enable USART0 (on expansion header J5, pins 12, 11)
+
+config SERIAL_ATMEL_USART2
+	bool "Enable USART2"
+	depends on SERIAL_ATMEL
+	help
+	  Enable USART2 (on expansion header J6, pins 27, 26)
+
+config SERIAL_ATMEL_USART3
+	bool "Enable USART3"
+	depends on SERIAL_ATMEL
+	help
+	  Enable USART3 (on expansion header J6, pins 17, 18)
+
diff -urb orig/arch/avr32/boards/atngw100/setup.c linux-2.6.27-rc6/arch/avr32/boards/atngw100/setup.c
--- orig/arch/avr32/boards/atngw100/setup.c	2008-10-09 17:54:55.000000000 +0200
+++ linux-2.6.27-rc6/arch/avr32/boards/atngw100/setup.c	2008-10-09 16:33:50.000000000 +0200
@@ -114,8 +114,20 @@
 
 void __init setup_board(void)
 {
-	at32_map_usart(1, 0);	/* USART 1: /dev/ttyS0, DB9 */
+	int i = 0;
+
+	at32_map_usart(1, i++);	/* USART 1: /dev/ttyS0, DB9 */
 	at32_setup_serial_console(0);
+
+#ifdef CONFIG_SERIAL_ATMEL_USART0
+	at32_map_usart(0, i++);	/* USART 0: /dev/ttySX, J5, pins 12, 11 */
+#endif
+#ifdef CONFIG_SERIAL_ATMEL_USART2
+	at32_map_usart(2, i++);	/* USART 2: /dev/ttySX, J6, pins 27, 26 */
+#endif
+#ifdef CONFIG_SERIAL_ATMEL_USART3
+	at32_map_usart(3, i++);	/* USART 3: /dev/ttySX, J6, pins 17, 18 */
+#endif
 }
 
 static const struct gpio_led ngw_leds[] = {
@@ -170,7 +182,17 @@
 
 	at32_add_system_devices();
 
-	at32_add_device_usart(0);
+	i = 0;
+	at32_add_device_usart(i++);
+#ifdef CONFIG_SERIAL_ATMEL_USART0
+	at32_add_device_usart(i++);
+#endif
+#ifdef CONFIG_SERIAL_ATMEL_USART2
+	at32_add_device_usart(i++);
+#endif
+#ifdef CONFIG_SERIAL_ATMEL_USART3
+	at32_add_device_usart(i++);
+#endif
 
 	set_hw_addr(at32_add_device_eth(0, &eth_data[0]));
 	set_hw_addr(at32_add_device_eth(1, &eth_data[1]));
diff -urb orig/arch/avr32/Kconfig linux-2.6.27-rc6/arch/avr32/Kconfig
--- orig/arch/avr32/Kconfig	2008-10-09 17:54:55.000000000 +0200
+++ linux-2.6.27-rc6/arch/avr32/Kconfig	2008-10-09 16:21:49.000000000 +0200
@@ -125,6 +125,10 @@
 source "arch/avr32/boards/atstk1000/Kconfig"
 endif
 
+if BOARD_ATNGW100
+source "arch/avr32/boards/atngw100/Kconfig"
+endif
+
 choice
 	prompt "Boot loader type"
 	default LOADER_U_BOOT

Re: [PATCH] Make ATNGW100 serial ports configurable

[Haavard Skinnemoen]

Anders Blomdell <anders.blomdell@control.lth.se> wrote:
> Make configuration of ATNGW100 serial ports selectable in kernel configuration.
> 
> Signed-off-by: Anders Blomdell <anders.blomdell@control.lth.se>

Gak. If we're going down this path, why not add #ifdefs for every other
conceivable hardware mod as well and turn the ATNGW100 board code into
an even worse mess than the ATSTK1000 board code?

The ATNGW100 has one serial port on board, so IMO the standard board
code should only initialize that one port.

However, it might be sensible to add some sort of interface for
expansion board code. For example something like this:

#ifdef CONFIG_ATNGW100_EXPANSION
	atngw100_setup_expansion_board();
#endif

This will allow people with hardware mods to add all the extra devices
they need, including serial ports, by simply adding another file with
expansion board code. People who aren't afraid to solder stuff on their
boards shouldn't be afraid of writing some board code too, right?

What do you think?

Haavard

Re: [PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell]

Haavard Skinnemoen wrote:
> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>> Make configuration of ATNGW100 serial ports selectable in kernel configuration.
>>
>> Signed-off-by: Anders Blomdell <anders.blomdell@control.lth.se>
> 
> Gak. If we're going down this path, why not add #ifdefs for every other
> conceivable hardware mod as well and turn the ATNGW100 board code into
> an even worse mess than the ATSTK1000 board code?
OK, good point. The reason I sent in the patch is that people on AVRFreaks had
asked how to do it, and putting it in the configuration would (theoretically)
make it possible to catch pin conflicts.


> The ATNGW100 has one serial port on board, so IMO the standard board
> code should only initialize that one port.
Which was my intention, unless SERIAL_ATMEL_USARTn was selected.

> However, it might be sensible to add some sort of interface for
> expansion board code. For example something like this:
> 
> #ifdef CONFIG_ATNGW100_EXPANSION
> 	atngw100_setup_expansion_board();
> #endif
Since that file has to reside inside the kernel tree (?), one could almost as
easily replace the entire setup.c.

Shouldn't this be something like:

void __init setup_board(void)
{
...
#ifdef CONFIG_ATNGW100_EXPANSION
 	atngw100_setup_expansion_board();
#endif
}

static int __init atngw100_init(void)
{
...
#ifdef CONFIG_ATNGW100_EXPANSION
 	atngw100_init_expansion_board();
#endif
}

> This will allow people with hardware mods to add all the extra devices
> they need, including serial ports, by simply adding another file with
> expansion board code. People who aren't afraid to solder stuff on their
> boards shouldn't be afraid of writing some board code too, right?
> 
> What do you think?
An even nicer way of handling it (provided that initialization does not need to
take place during boot), might be to do EXPORT_SYMBOL() on
at32_add_device_usart, at32_map_usart, etc and then write a loadable module that
handles the initialization.

Regards

Anders Blomdell


-- 
Anders Blomdell                  Email: anders.blomdell@control.lth.se
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden

Re: [PATCH] Make ATNGW100 serial ports configurable

[Haavard Skinnemoen]

Anders Blomdell <anders.blomdell@control.lth.se> wrote:
> Haavard Skinnemoen wrote:
> > Anders Blomdell <anders.blomdell@control.lth.se> wrote:
> >> Make configuration of ATNGW100 serial ports selectable in kernel configuration.
> >>
> >> Signed-off-by: Anders Blomdell <anders.blomdell@control.lth.se>
> > 
> > Gak. If we're going down this path, why not add #ifdefs for every other
> > conceivable hardware mod as well and turn the ATNGW100 board code into
> > an even worse mess than the ATSTK1000 board code?
> OK, good point. The reason I sent in the patch is that people on AVRFreaks had
> asked how to do it, and putting it in the configuration would (theoretically)
> make it possible to catch pin conflicts.

Yes, I do realize people want this kind of thing. But I also think
there's more than one way to do it, and while a Kconfig option might
make things easier while trying it out, I think it will cause
maintenance pains in the long run.

> > The ATNGW100 has one serial port on board, so IMO the standard board
> > code should only initialize that one port.
> Which was my intention, unless SERIAL_ATMEL_USARTn was selected.

Yes, but it's not sensible to even offer the option of other ports when
the board just doesn't have them.

> > However, it might be sensible to add some sort of interface for
> > expansion board code. For example something like this:
> > 
> > #ifdef CONFIG_ATNGW100_EXPANSION
> > 	atngw100_setup_expansion_board();
> > #endif
> Since that file has to reside inside the kernel tree (?), one could almost as
> easily replace the entire setup.c.

True...but it would take quite a bit of duplication, and you'll have to
sync up whenever the "main" setup.c changes.

> Shouldn't this be something like:
> 
> void __init setup_board(void)
> {
> ...
> #ifdef CONFIG_ATNGW100_EXPANSION
>  	atngw100_setup_expansion_board();
> #endif
> }
> 
> static int __init atngw100_init(void)
> {
> ...
> #ifdef CONFIG_ATNGW100_EXPANSION
>  	atngw100_init_expansion_board();
> #endif
> }

Well, you could do it like that, but you could also just add another
postcore_initcall. The only hook which is really needed is the "setup"
part, and only if you have additional serial ports.

In fact, it's probably better to just add a weak definition for it
since not all expansion boards will need to override it.

> > This will allow people with hardware mods to add all the extra devices
> > they need, including serial ports, by simply adding another file with
> > expansion board code. People who aren't afraid to solder stuff on their
> > boards shouldn't be afraid of writing some board code too, right?
> > 
> > What do you think?
> An even nicer way of handling it (provided that initialization does not need to
> take place during boot), might be to do EXPORT_SYMBOL() on
> at32_add_device_usart, at32_map_usart, etc and then write a loadable module that
> handles the initialization.

Hmm...why would that be nicer exactly?

What _I_ think would be a nicer way to do it is to implement support
for flattened device trees and get rid of the board code entirely. Or
almost entirely; it depends on how complete we can make the device tree.

Haavard

Re: [PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell]

Haavard Skinnemoen wrote:
> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>> Haavard Skinnemoen wrote:
>>> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>>>> Make configuration of ATNGW100 serial ports selectable in kernel configuration.
>>>>
>>>> Signed-off-by: Anders Blomdell <anders.blomdell@control.lth.se>
>>> Gak. If we're going down this path, why not add #ifdefs for every other
>>> conceivable hardware mod as well and turn the ATNGW100 board code into
>>> an even worse mess than the ATSTK1000 board code?
>> OK, good point. The reason I sent in the patch is that people on AVRFreaks had
>> asked how to do it, and putting it in the configuration would (theoretically)
>> make it possible to catch pin conflicts.
> 
> Yes, I do realize people want this kind of thing. But I also think
> there's more than one way to do it, and while a Kconfig option might
> make things easier while trying it out, I think it will cause
> maintenance pains in the long run.
> 
>>> The ATNGW100 has one serial port on board, so IMO the standard board
>>> code should only initialize that one port.
>> Which was my intention, unless SERIAL_ATMEL_USARTn was selected.
> 
> Yes, but it's not sensible to even offer the option of other ports when
> the board just doesn't have them.
> 
>>> However, it might be sensible to add some sort of interface for
>>> expansion board code. For example something like this:
>>>
>>> #ifdef CONFIG_ATNGW100_EXPANSION
>>> 	atngw100_setup_expansion_board();
>>> #endif
>> Since that file has to reside inside the kernel tree (?), one could almost as
>> easily replace the entire setup.c.
> 
> True...but it would take quite a bit of duplication, and you'll have to
> sync up whenever the "main" setup.c changes.
> 
>> Shouldn't this be something like:
>>
>> void __init setup_board(void)
>> {
>> ...
>> #ifdef CONFIG_ATNGW100_EXPANSION
>>  	atngw100_setup_expansion_board();
>> #endif
>> }
>>
>> static int __init atngw100_init(void)
>> {
>> ...
>> #ifdef CONFIG_ATNGW100_EXPANSION
>>  	atngw100_init_expansion_board();
>> #endif
>> }
> 
> Well, you could do it like that, but you could also just add another
> postcore_initcall. The only hook which is really needed is the "setup"
> part, and only if you have additional serial ports.
Does that mean that 'at32_add_device_usart(...);' is not needed?

> In fact, it's probably better to just add a weak definition for it
> since not all expansion boards will need to override it.
> 
>>> This will allow people with hardware mods to add all the extra devices
>>> they need, including serial ports, by simply adding another file with
>>> expansion board code. People who aren't afraid to solder stuff on their
>>> boards shouldn't be afraid of writing some board code too, right?
>>>
>>> What do you think?
>> An even nicer way of handling it (provided that initialization does not need to
>> take place during boot), might be to do EXPORT_SYMBOL() on
>> at32_add_device_usart, at32_map_usart, etc and then write a loadable module that
>> handles the initialization.
> 
> Hmm...why would that be nicer exactly?
You could compile your board specific inits outside the kernel tree, making it
much easier to follow kernel versions (not everyone gets their board specific
code into the kernel tree :-).

> What _I_ think would be a nicer way to do it is to implement support
> for flattened device trees and get rid of the board code entirely. Or
> almost entirely; it depends on how complete we can make the device tree.
I don't understand the above paragraph, could you please elaborate?


-- 
Anders Blomdell                  Email: anders.blomdell@control.lth.se
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden

Re: [PATCH] Make ATNGW100 serial ports configurable

[Haavard Skinnemoen]

Anders Blomdell <anders.blomdell@control.lth.se> wrote:
> Haavard Skinnemoen wrote:
> > Well, you could do it like that, but you could also just add another
> > postcore_initcall. The only hook which is really needed is the "setup"
> > part, and only if you have additional serial ports.
> Does that mean that 'at32_add_device_usart(...);' is not needed?

It's needed, but you don't need a hook in the main board code to do it.
You can simply do

static int __init my_expansion_board_init(void)
{
	at32_add_device_usart(...);
	/* more stuff */
}
postcore_initcall(my_expansion_board_init);

> >> An even nicer way of handling it (provided that initialization does not need to
> >> take place during boot), might be to do EXPORT_SYMBOL() on
> >> at32_add_device_usart, at32_map_usart, etc and then write a loadable module that
> >> handles the initialization.
> > 
> > Hmm...why would that be nicer exactly?
> You could compile your board specific inits outside the kernel tree, making it
> much easier to follow kernel versions (not everyone gets their board specific
> code into the kernel tree :-).

Adding EXPORT_SYMBOLs purely for use by out-of-tree modules is
generally frowned upon. And patching in an additional file to the build
is just about the easiest thing you can do.

> > What _I_ think would be a nicer way to do it is to implement support
> > for flattened device trees and get rid of the board code entirely. Or
> > almost entirely; it depends on how complete we can make the device tree.
> I don't understand the above paragraph, could you please elaborate?

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/powerpc/booting-without-of.txt;h=de4063cb4fdc0ad6abea29d766cae78616837311;hb=HEAD

Haavard

Re: [PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell]

Haavard Skinnemoen wrote:
> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>> Haavard Skinnemoen wrote:
>>> Well, you could do it like that, but you could also just add another
>>> postcore_initcall. The only hook which is really needed is the "setup"
>>> part, and only if you have additional serial ports.
>> Does that mean that 'at32_add_device_usart(...);' is not needed?
> 
> It's needed, but you don't need a hook in the main board code to do it.
> You can simply do
> 
> static int __init my_expansion_board_init(void)
> {
> 	at32_add_device_usart(...);
> 	/* more stuff */
> }
> postcore_initcall(my_expansion_board_init);
> 
>>>> An even nicer way of handling it (provided that initialization does not need to
>>>> take place during boot), might be to do EXPORT_SYMBOL() on
>>>> at32_add_device_usart, at32_map_usart, etc and then write a loadable module that
>>>> handles the initialization.
>>> Hmm...why would that be nicer exactly?
>> You could compile your board specific inits outside the kernel tree, making it
>> much easier to follow kernel versions (not everyone gets their board specific
>> code into the kernel tree :-).
> 
> Adding EXPORT_SYMBOLs purely for use by out-of-tree modules is
> generally frowned upon. 
If I'm not mistaken, you need EXPORT_SYMBOL for any loadable module, be it in-
or out-of-tree.

>And patching in an additional file to the build is just about the easiest
>thing you can do.
Next to config options and loadable modules out-of-tree :-)


>>> What _I_ think would be a nicer way to do it is to implement support
>>> for flattened device trees and get rid of the board code entirely. Or
>>> almost entirely; it depends on how complete we can make the device tree.
>> I don't understand the above paragraph, could you please elaborate?
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/powerpc/booting-without-of.txt;h=de4063cb4fdc0ad6abea29d766cae78616837311;hb=HEAD
OK, essentially compile all drivers into the kernel and let the boot-loader tell
us what to activate. Gives a small kernel bloat (which I can live with), but
what if two conflicting drivers (e.g. same I/O pin) are selected by the
flattened tree?

But I take your replies that you are adamantly against kernel options for the
atngw100, so I'll drop the subject for now (letting everyone figure out their
favorite way to configure modified boards).

Regards

Anders

-- 
Anders Blomdell                  Email: anders.blomdell@control.lth.se
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden

Re: [PATCH] Make ATNGW100 serial ports configurable

[Haavard Skinnemoen]

Anders Blomdell <anders.blomdell@control.lth.se> wrote:
> Haavard Skinnemoen wrote:
> > Adding EXPORT_SYMBOLs purely for use by out-of-tree modules is
> > generally frowned upon. 
> If I'm not mistaken, you need EXPORT_SYMBOL for any loadable module, be it in-
> or out-of-tree.

Yes, but no in-kernel modules need those symbols exported.

> >And patching in an additional file to the build is just about the easiest
> >thing you can do.
> Next to config options and loadable modules out-of-tree :-)

Really? I personally think out-of-tree modules is a real hassle causing
extra work each time I need to update the kernel, whether it's on my
embedded target or my PC.

On the other hand, a trivial patch adding support for an expansion
board can just stay in your tree while you do a 'git pull' to grab the
most up-to-date kernel.

Even better, if you send your trivial patch upstream, it will just be
there the next time you update your kernel. No need to do anything
extra at all.

A Kconfig option is admittedly just as easy, but with the added
downside that it makes the board code less readable, and there will
never be enough of them to support every board mod that people make.

> >>> What _I_ think would be a nicer way to do it is to implement support
> >>> for flattened device trees and get rid of the board code entirely. Or
> >>> almost entirely; it depends on how complete we can make the device tree.
> >> I don't understand the above paragraph, could you please elaborate?
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/powerpc/booting-without-of.txt;h=de4063cb4fdc0ad6abea29d766cae78616837311;hb=HEAD
> OK, essentially compile all drivers into the kernel and let the boot-loader tell
> us what to activate. Gives a small kernel bloat (which I can live with), but
> what if two conflicting drivers (e.g. same I/O pin) are selected by the
> flattened tree?

That's how it works today, except that it's the board code which
specifies what to activate instead of the boot loader (or a device tree
blob in flash.) So there won't be any additional bloat unless you
go and select lots of drivers you don't need.

On the other hand, these functions are currently __init, so if we
export them for use by modules, it will definitely bloat the kernel.

> But I take your replies that you are adamantly against kernel options for the
> atngw100, so I'll drop the subject for now (letting everyone figure out their
> favorite way to configure modified boards).

I'm not fundamentally against kernel options for the atngw100. I just
don't like cluttering up the board code.

If you add support for an expansion board, you'll definitely need a
config option to select it as well. But that option is only used to
select the implementation file for that board; it won't add any
additional mess to the core NGW100 code.

Have a look at the EVKLCD10x code that was just applied for an example
of how to add support for an expansion board:

http://git.kernel.org/?p=linux/kernel/git/hskinnemoen/avr32-2.6.git;a=commitdiff;h=a3bee42f058c2f9fe281df942eff397924630a12

Most of it is LCD-related, so if you only need to add a USART, you'll
only need a fraction of the code.

Also note that it shows a huge problem with the Kconfig approach: Most
devices need additional board-specific data. And even though the USART
doesn't currently need any of that, it may in the future when we add
support for RS485, hardware flow control, etc. How will you deal with
that through Kconfig?

Haavard

Re: [PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell]

Haavard Skinnemoen wrote:
> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>> Haavard Skinnemoen wrote:
>>> Adding EXPORT_SYMBOLs purely for use by out-of-tree modules is
>>> generally frowned upon. 
>> If I'm not mistaken, you need EXPORT_SYMBOL for any loadable module, be it in-
>> or out-of-tree.
> 
> Yes, but no in-kernel modules need those symbols exported.
No, that's right.

>>> And patching in an additional file to the build is just about the easiest
>>> thing you can do.
>> Next to config options and loadable modules out-of-tree :-)
> 
> Really? I personally think out-of-tree modules is a real hassle causing
> extra work each time I need to update the kernel, whether it's on my
> embedded target or my PC.
Depends on if it's you or somebody else (who doesn't care about your code) that
rebuilds the kernel. Since I plan to use some tens of atngw100's eventually,
with at least 3 different expansions boards, a solution that doesn't need 3
different kernels (i.e. loadable modules) is much preferred to a Kconfig (I know
I started out with this, but one does change opinion :-) occasionally) or
in-tree init code.

> On the other hand, a trivial patch adding support for an expansion
> board can just stay in your tree while you do a 'git pull' to grab the
> most up-to-date kernel.
> 
> Even better, if you send your trivial patch upstream, it will just be
> there the next time you update your kernel. No need to do anything
> extra at all.
> 
> A Kconfig option is admittedly just as easy, but with the added
> downside that it makes the board code less readable, and there will
> never be enough of them to support every board mod that people make.
> 
>>>>> What _I_ think would be a nicer way to do it is to implement support
>>>>> for flattened device trees and get rid of the board code entirely. Or
>>>>> almost entirely; it depends on how complete we can make the device tree.
>>>> I don't understand the above paragraph, could you please elaborate?
>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/powerpc/booting-without-of.txt;h=de4063cb4fdc0ad6abea29d766cae78616837311;hb=HEAD
>> OK, essentially compile all drivers into the kernel and let the boot-loader tell
>> us what to activate. Gives a small kernel bloat (which I can live with), but
>> what if two conflicting drivers (e.g. same I/O pin) are selected by the
>> flattened tree?
> 
> That's how it works today, except that it's the board code which
> specifies what to activate instead of the boot loader (or a device tree
> blob in flash.) So there won't be any additional bloat unless you
> go and select lots of drivers you don't need.
> 
> On the other hand, these functions are currently __init, so if we
> export them for use by modules, it will definitely bloat the kernel.
> 
>> But I take your replies that you are adamantly against kernel options for the
>> atngw100, so I'll drop the subject for now (letting everyone figure out their
>> favorite way to configure modified boards).
> 
> I'm not fundamentally against kernel options for the atngw100. I just
> don't like cluttering up the board code.
> 
> If you add support for an expansion board, you'll definitely need a
> config option to select it as well. But that option is only used to
> select the implementation file for that board; it won't add any
> additional mess to the core NGW100 code.
> 
> Have a look at the EVKLCD10x code that was just applied for an example
> of how to add support for an expansion board:
> 
> http://git.kernel.org/?p=linux/kernel/git/hskinnemoen/avr32-2.6.git;a=commitdiff;h=a3bee42f058c2f9fe281df942eff397924630a12
> 
> Most of it is LCD-related, so if you only need to add a USART, you'll
> only need a fraction of the code.
I see a distinct clutter in setup.c (#ifndef CONFIG_BOARD_ATNGW100_EVKLCD10X)
:-) (it clearly shows the need for conflict resolution, though)

> Also note that it shows a huge problem with the Kconfig approach: Most
> devices need additional board-specific data. And even though the USART
> doesn't currently need any of that, it may in the future when we add
> support for RS485, hardware flow control, etc. How will you deal with
> that through Kconfig?
Honestly, I don't know :-(, but it would be nice to be able to select all these
options wihout having to write the code myself :-)

Question is if there is any way to come up with a solution that makes it
possible to select software modules for what is present on a specific expansion
board, my naive thought was that it should be selected by Kconfig (and in that
vein, my 5 cents is that video for ngw100/stk100x should be selected as
TCG057QVLAD/PH320240T/LTV350QV, and not as a by-product of selecting a specific
expansion board, this way it would be easier to add video to a custom expansion
board withou dragging in unrelated functions like USARTs). Perhaps it is
possible to solve my problems as well as the stk100x clutter if it is done right?

Regards

Anders

-- 
Anders Blomdell                  Email: anders.blomdell@control.lth.se
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden

Re: [PATCH] Make ATNGW100 serial ports configurable

[Haavard Skinnemoen]

Anders Blomdell <anders.blomdell@control.lth.se> wrote:
> Haavard Skinnemoen wrote:
> > Really? I personally think out-of-tree modules is a real hassle causing
> > extra work each time I need to update the kernel, whether it's on my
> > embedded target or my PC.
> Depends on if it's you or somebody else (who doesn't care about your code) that
> rebuilds the kernel. Since I plan to use some tens of atngw100's eventually,
> with at least 3 different expansions boards, a solution that doesn't need 3
> different kernels (i.e. loadable modules) is much preferred to a Kconfig (I know
> I started out with this, but one does change opinion :-) occasionally) or
> in-tree init code.

So what you really need is device tree support.

I suppose modules would do the trick in your case, but it sounds like a
very specialized setup, and I don't feel too good about bloating the
kernel for everyone else just to support this kind of thing.

Device tree support would probably make the kernel bigger too, but I
think that would be mostly __init code, so it doesn't matter as much.

> > Have a look at the EVKLCD10x code that was just applied for an example
> > of how to add support for an expansion board:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/hskinnemoen/avr32-2.6.git;a=commitdiff;h=a3bee42f058c2f9fe281df942eff397924630a12
> > 
> > Most of it is LCD-related, so if you only need to add a USART, you'll
> > only need a fraction of the code.
> I see a distinct clutter in setup.c (#ifndef CONFIG_BOARD_ATNGW100_EVKLCD10X)
> :-) (it clearly shows the need for conflict resolution, though)

Yes, but it's hard to avoid that, it's confined to the support code
for that particular expansion board, and the clutter is there to
support actual variations of the board. It could have been moved into
separate files, but that would have led to more duplication.

It's all about finding the right balance, really.

I didn't quite understand what you mean by "conflict resolution".

> > Also note that it shows a huge problem with the Kconfig approach: Most
> > devices need additional board-specific data. And even though the USART
> > doesn't currently need any of that, it may in the future when we add
> > support for RS485, hardware flow control, etc. How will you deal with
> > that through Kconfig?
> Honestly, I don't know :-(, but it would be nice to be able to select all these
> options wihout having to write the code myself :-)

But is wading through several dozen config options really that much
easier than writing ten lines of code?

> Question is if there is any way to come up with a solution that makes it
> possible to select software modules for what is present on a specific expansion
> board, my naive thought was that it should be selected by Kconfig (and in that
> vein, my 5 cents is that video for ngw100/stk100x should be selected as
> TCG057QVLAD/PH320240T/LTV350QV, and not as a by-product of selecting a specific
> expansion board, this way it would be easier to add video to a custom expansion
> board withou dragging in unrelated functions like USARTs). Perhaps it is
> possible to solve my problems as well as the stk100x clutter if it is done right?

Possibly. But adding options for the USARTs is only solving a tiny part
of the problem, and I'm worried that solving the rest in the same
manner is going to end up as a spectacular mess. If you can prove me
wrong, I'm all for it.

Haavard

Re: [PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell]

Dropped the kernel list CC on previous post, sorry.

Haavard Skinnemoen wrote:
> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>>>> I see a distinct clutter in setup.c (#ifndef CONFIG_BOARD_ATNGW100_EVKLCD10X)
>>>> :-) (it clearly shows the need for conflict resolution, though)  
>>> Yes, but it's hard to avoid that, it's confined to the support code
>>> for that particular expansion board, and the clutter is there to
>>> support actual variations of the board. It could have been moved into
>>> separate files, but that would have led to more duplication.  
>> Nope, it's right in the setup.c file.
>>
>>> It's all about finding the right balance, really.
>>>
>>> I didn't quite understand what you mean by "conflict resolution".  
>> That .detect_pin and .wp_pin conflicts with the LCD (so the #ifdef disables them)
> 
> Oh, that stuff. Yeah, when an expansion board needs to disable features
> on the motherboard, it gets a bit messy.
How should this be handled with device tree?

>>> But is wading through several dozen config options really that much
>>> easier than writing ten lines of code?  
>> Nope, but if there are changes in some kernel code, local changes can easily get
>> out of sync.
> 
> That's why you should send it upstream so that it can be kept in sync
> whenever something changes. Also note that the same applies to your
> out-of-tree module.
Which is what I'm trying to do, but the maintainer is complaining :-)
But we probably don't want to add everyones prototype boards to the kernel tree!

>>>> Question is if there is any way to come up with a solution that makes it
>>>> possible to select software modules for what is present on a specific expansion
>>>> board, my naive thought was that it should be selected by Kconfig (and in that
>>>> vein, my 5 cents is that video for ngw100/stk100x should be selected as
>>>> TCG057QVLAD/PH320240T/LTV350QV, and not as a by-product of selecting a specific
>>>> expansion board, this way it would be easier to add video to a custom expansion
>>>> board withou dragging in unrelated functions like USARTs). Perhaps it is
>>>> possible to solve my problems as well as the stk100x clutter if it is done right?  
>>> Possibly. But adding options for the USARTs is only solving a tiny part
>>> of the problem, and I'm worried that solving the rest in the same
>>> manner is going to end up as a spectacular mess. If you can prove me
>>> wrong, I'm all for it.  
>> It's not about proving either me or you wrong, it's about making this easy to do
>> for everybody (not only me, I can live with modifying the kernel tree), if it is
>> not possible, so be it.
> 
> I'm asserting that doing this kind of board customization through
> Kconfig is going to get messy once we support enough features to make
> everyone happy. If you can show that it can be done in a clean way,
> i.e. prove me wrong, nothing would make me happier.
> 
> But I do think device trees along with a nice graphical editor, or a
> few u-boot commands, would go a long way towards this goal. If we
> manage to get something like that working, you won't even have to
> recompile anything.
As far as I can see, the device trees will push the conflict resolution to
run-time instead of compile-time, which I belive is bad for both memory
footprint as well as performance (as well as predictability, this kernel worked
yesterday; who added which device which makes it crash today...)

>> But personally I would be happy with a generic ap7000
>> board, where I could pick all the options I like and the ngw100 and stk100x
>> would just be an instance of this board with all/most options preselected. That
>> #ifdefs are messy to read is something we agree about...
> 
> Right, I also want more generic board support. But I don't think
> Kconfig is the way to go. There are just too many variables.
Wouldn't there be as many variables with a device tree?

A graphical board-configurator against the Kconfig should certainly be possible?

I'm also not (yet) convinced that your approach makes the configuration any simpler.

How about putting each needed extension in a separate file (with a specified
format), and use some kind of preprocessor to generate Kconfig's, Makefiles,
setup.c, etc from those files (and generating the appropriate at32_reserve_*,
at32_select* as well). I.e. something like:

USART2.def:

%PINS {
  PA08, PA09
}

%GLOBAL {
  platform device *usart2;
}

%INIT {
  usart2 = at32_add_device_usart(2);
}

%SETUP {
  new_at32_map_usart(usart2, at_32_last_mapped_usart++);
}

/Anders

-- 
Anders Blomdell                  Email: anders.blomdell@control.lth.se
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden

Re: [PATCH] Make ATNGW100 serial ports configurable

[Haavard Skinnemoen]

On Mon, 13 Oct 2008 20:08:21 +0200
Anders Blomdell <anders.blomdell@control.lth.se> wrote:

> Dropped the kernel list CC on previous post, sorry.
> 
> Haavard Skinnemoen wrote:
> > Oh, that stuff. Yeah, when an expansion board needs to disable features
> > on the motherboard, it gets a bit messy.
> How should this be handled with device tree?

By simply leaving those pins out. Or perhaps there's some mechanism for
disabling them.

> > That's why you should send it upstream so that it can be kept in sync
> > whenever something changes. Also note that the same applies to your
> > out-of-tree module.
> Which is what I'm trying to do, but the maintainer is complaining :-)

No, you're trying to push a mechanism which will allow you to avoid
doing that, at the expense of less readable board code for everyone
else.

> But we probably don't want to add everyones prototype boards to the kernel tree!

Well, why not? It's not like it takes up much space. And it can always
be removed later when the prototype gets reassigned to collecting dust
at the top of some shelf.

It might even be a nice experience for later when you want support for
your production board upstream...

> > But I do think device trees along with a nice graphical editor, or a
> > few u-boot commands, would go a long way towards this goal. If we
> > manage to get something like that working, you won't even have to
> > recompile anything.
> As far as I can see, the device trees will push the conflict resolution to
> run-time instead of compile-time, which I belive is bad for both memory
> footprint as well as performance (as well as predictability, this kernel worked
> yesterday; who added which device which makes it crash today...)

Any conflict resolution _today_ happens at run time in the form of a
friendly error message and a stack dump if you try to assign the same
pin to two different devices.

And with your patch, some device might suddenly stop working because
you enabled a USART which conflicts with some other device that gets
initialized later (the USARTs are initialized very early). How is that
any better?

As for performance and memory footprint, I think the impact will be
minimal. This is init code that we're talking about -- it gets run once
and then discarded.

Oh, and the device tree stuff comes with some nice infrastructure which
would make it quite easy to write a static validator for pin assignments
and such. That would actually move the conflict resolution from
run-time to compile-time.

> >> But personally I would be happy with a generic ap7000
> >> board, where I could pick all the options I like and the ngw100 and stk100x
> >> would just be an instance of this board with all/most options preselected. That
> >> #ifdefs are messy to read is something we agree about...
> > 
> > Right, I also want more generic board support. But I don't think
> > Kconfig is the way to go. There are just too many variables.
> Wouldn't there be as many variables with a device tree?

Yes, but you're exposing them to the right audience: The board vendors,
not the users. In the case of hobbyists creating their own boards,
they're of course the same people, but they are definitely power users.

Also, I believe that, if the device tree layout is carefully designed,
it might be possible to turn bits of it on and off at run-time. That
would be great for configurable boards like the STK1000 -- with a bit
of extra support from u-boot, you should be able to do something like

	switch 5 on

to select between two different mux settings (e.g. second ethernet
controller vs. LCD).

> A graphical board-configurator against the Kconfig should certainly be possible?

It will probably be just as easy to have the board configurator
generate C code, or a device tree.

> I'm also not (yet) convinced that your approach makes the configuration any simpler.

I guess we'll just have to implement it and find out. Until then,
there's really no way to tell -- you may of course be right.

> How about putting each needed extension in a separate file (with a specified
> format), and use some kind of preprocessor to generate Kconfig's, Makefiles,
> setup.c, etc from those files (and generating the appropriate at32_reserve_*,
> at32_select* as well). I.e. something like:

If you're generating code, why bother with Kconfig at all?

> USART2.def:
> 
> %PINS {
>   PA08, PA09
> }
> 
> %GLOBAL {
>   platform device *usart2;
> }
> 
> %INIT {
>   usart2 = at32_add_device_usart(2);
> }
> 
> %SETUP {
>   new_at32_map_usart(usart2, at_32_last_mapped_usart++);
> }

You've already written most of the board code, and added quite a bit of
additional noise. Why not write it in C instead of some
yet-another-weird-configuration-language?

Hey, why not use XML? I'm sure lots of LKML subscribers would be
absolutely thrilled! ;-)

Haavard

Re: [PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell]

Haavard Skinnemoen wrote:
> On Mon, 13 Oct 2008 20:08:21 +0200
> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
> 
>> Dropped the kernel list CC on previous post, sorry.
>>
>> Haavard Skinnemoen wrote:
>>> Oh, that stuff. Yeah, when an expansion board needs to disable features
>>> on the motherboard, it gets a bit messy.
>> How should this be handled with device tree?
> 
> By simply leaving those pins out. Or perhaps there's some mechanism for
> disabling them.
> 
>>> That's why you should send it upstream so that it can be kept in sync
>>> whenever something changes. Also note that the same applies to your
>>> out-of-tree module.
>> Which is what I'm trying to do, but the maintainer is complaining :-)
> 
> No, you're trying to push a mechanism which will allow you to avoid
> doing that, at the expense of less readable board code for everyone
> else.
> 
>> But we probably don't want to add everyones prototype boards to the kernel tree!
> 
> Well, why not? It's not like it takes up much space. And it can always
> be removed later when the prototype gets reassigned to collecting dust
> at the top of some shelf.
> 
> It might even be a nice experience for later when you want support for
> your production board upstream...
> 
>>> But I do think device trees along with a nice graphical editor, or a
>>> few u-boot commands, would go a long way towards this goal. If we
>>> manage to get something like that working, you won't even have to
>>> recompile anything.
>> As far as I can see, the device trees will push the conflict resolution to
>> run-time instead of compile-time, which I belive is bad for both memory
>> footprint as well as performance (as well as predictability, this kernel worked
>> yesterday; who added which device which makes it crash today...)
> 
> Any conflict resolution _today_ happens at run time in the form of a
> friendly error message and a stack dump if you try to assign the same
> pin to two different devices.
> 
> And with your patch, some device might suddenly stop working because
> you enabled a USART which conflicts with some other device that gets
> initialized later (the USARTs are initialized very early). How is that
> any better?
> 
> As for performance and memory footprint, I think the impact will be
> minimal. This is init code that we're talking about -- it gets run once
> and then discarded.
> 
> Oh, and the device tree stuff comes with some nice infrastructure which
> would make it quite easy to write a static validator for pin assignments
> and such. That would actually move the conflict resolution from
> run-time to compile-time.
> 
>>>> But personally I would be happy with a generic ap7000
>>>> board, where I could pick all the options I like and the ngw100 and stk100x
>>>> would just be an instance of this board with all/most options preselected. That
>>>> #ifdefs are messy to read is something we agree about...
>>> Right, I also want more generic board support. But I don't think
>>> Kconfig is the way to go. There are just too many variables.
>> Wouldn't there be as many variables with a device tree?
> 
> Yes, but you're exposing them to the right audience: The board vendors,
> not the users. In the case of hobbyists creating their own boards,
> they're of course the same people, but they are definitely power users.
> 
> Also, I believe that, if the device tree layout is carefully designed,
> it might be possible to turn bits of it on and off at run-time. That
> would be great for configurable boards like the STK1000 -- with a bit
> of extra support from u-boot, you should be able to do something like
> 
> 	switch 5 on
> 
> to select between two different mux settings (e.g. second ethernet
> controller vs. LCD).
> 
>> A graphical board-configurator against the Kconfig should certainly be possible?
> 
> It will probably be just as easy to have the board configurator
> generate C code, or a device tree.
> 
>> I'm also not (yet) convinced that your approach makes the configuration any simpler.
> 
> I guess we'll just have to implement it and find out. Until then,
> there's really no way to tell -- you may of course be right.
OK, I'll wait for the device tree to appear...

>> How about putting each needed extension in a separate file (with a specified
>> format), and use some kind of preprocessor to generate Kconfig's, Makefiles,
>> setup.c, etc from those files (and generating the appropriate at32_reserve_*,
>> at32_select* as well). I.e. something like:
> 
> If you're generating code, why bother with Kconfig at all?
Because that is the way I'm used to select drivers when I configure a kernel
(call me old fashioned if you will :-).

>> USART2.def:
>>
>> %PINS {
>>   PA08, PA09
>> }
>>
>> %GLOBAL {
>>   platform device *usart2;
>> }
>>
>> %INIT {
>>   usart2 = at32_add_device_usart(2);
>> }
>>
>> %SETUP {
>>   new_at32_map_usart(usart2, at_32_last_mapped_usart++);
>> }
> 
> You've already written most of the board code, and added quite a bit of
> additional noise. Why not write it in C instead of some
> yet-another-weird-configuration-language?
Oh, that leads to lots of noise^H^H^H^H^H^H #ifdefs :-)

> Hey, why not use XML? I'm sure lots of LKML subscribers would be
> absolutely thrilled! ;-)
XML is OK with me...



-- 
Anders Blomdell                  Email: anders.blomdell@control.lth.se
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden