| Message ID | 1362738596.5994.48.camel@x61.thuisdomein (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 08, 2013 at 11:29:56AM +0100, Paul Bolle wrote: > When support was added for Nokia N9 (RM-696), with commit > 63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal > support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was > added to the tree. But there's no Kconfig symbol with that name. That > symbol would be superfluous, since support for that machine piggybacks > on MACH_NOKIA_RM680. So drop that select. This is needed because of arch/arm/tools/mach-types. See include/generated/mach-types.h. If you have just CONFIG_MACH_NOKIA_RM680 and run the kernel on RM-696, then machine_is_nokia_rm696() will return false. If I rememeber correctly, this broke at least early printk / uncompressor output at the time. I guess people may still want to use machine_is_... macros e.g. for debugging. > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > 0) Tested with "git grep". git grep won't search generated source files. > 1) Some searching on the web didn't return a "config MACH_NOKIA_RM696". > So apparently there's not even a development tree that uses this symbol. You can see machine_is_nokia_rm696() used in the public kernel source for Nokia N9 product. :-) A.
* Aaro Koskinen <aaro.koskinen@iki.fi> [130308 08:16]: > On Fri, Mar 08, 2013 at 11:29:56AM +0100, Paul Bolle wrote: > > When support was added for Nokia N9 (RM-696), with commit > > 63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal > > support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was > > added to the tree. But there's no Kconfig symbol with that name. That > > symbol would be superfluous, since support for that machine piggybacks > > on MACH_NOKIA_RM680. So drop that select. > > This is needed because of arch/arm/tools/mach-types. See > include/generated/mach-types.h. > > If you have just CONFIG_MACH_NOKIA_RM680 and run the kernel on RM-696, > then machine_is_nokia_rm696() will return false. If I rememeber correctly, > this broke at least early printk / uncompressor output at the time. > > I guess people may still want to use machine_is_... macros e.g. for > debugging. I think the righ fix is to just add config MACH_NOKIA_RM680 bool to the mach-omap2/Kconfig like we have for n8x0 also. Regards, Tony
On Fri, 2013-03-08 at 18:11 +0200, Aaro Koskinen wrote: > On Fri, Mar 08, 2013 at 11:29:56AM +0100, Paul Bolle wrote: > > When support was added for Nokia N9 (RM-696), with commit > > 63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal > > support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was > > added to the tree. But there's no Kconfig symbol with that name. That > > symbol would be superfluous, since support for that machine piggybacks > > on MACH_NOKIA_RM680. So drop that select. > > This is needed because of arch/arm/tools/mach-types. See > include/generated/mach-types.h. How does that file actually need this select statement? > If you have just CONFIG_MACH_NOKIA_RM680 and run the kernel on RM-696, > then machine_is_nokia_rm696() will return false. If I rememeber correctly, > this broke at least early printk / uncompressor output at the time. > > I guess people may still want to use machine_is_... macros e.g. for > debugging. What in the current mainline kernel cares about a machine_is_nokia_rm696() macro? $ git grep -n machine_is_nokia v3.9-rc1 v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:349: if (machine_is_nokia_n800() || slot == 0) v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:395: if (machine_is_nokia_n800()) { v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:428: if (machine_is_nokia_n800()) v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:451: if (machine_is_nokia_n800()) { v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:478: if (machine_is_nokia_n800()) v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:493: if (machine_is_nokia_n810()) { v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:544: if (machine_is_nokia_n810()) { v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:562: if (machine_is_nokia_n810()) { v3.9-rc1:arch/arm/mach-omap2/board-n8x0.c:714: if (machine_is_nokia_n810()) v3.9-rc1:arch/arm/mach-omap2/board-rx51-video.c:71: if (!machine_is_nokia_rx51()) v3.9-rc1:drivers/usb/host/ohci-omap.c:267: } else if (machine_is_nokia770()) { v3.9-rc1:sound/soc/omap/n810.c:309: if (!(machine_is_nokia_n810() || machine_is_nokia_n810_wimax())) v3.9-rc1:sound/soc/omap/rx51.c:400: if (!machine_is_nokia_rx51()) $ git grep -n rm696 v3.9-rc1 v3.9-rc1:arch/arm/tools/mach-types:589:nokia_rm696 MACH_NOKIA_RM696 NOKIA_RM696 3522 I don't find any actual usage of that macro. Are its uses also generated? > > 0) Tested with "git grep". > > git grep won't search generated source files. Of course. That's one of the reasons why I explicitly state that I only tested it that way. > > 1) Some searching on the web didn't return a "config MACH_NOKIA_RM696". > > So apparently there's not even a development tree that uses this symbol. > > You can see machine_is_nokia_rm696() used in the public kernel source > for Nokia N9 product. :-) Where would that be? And does that tree actually have a Kconfig entry MACH_NOKIA_RM696? Paul Bolle
On Fri, 2013-03-08 at 08:35 -0800, Tony Lindgren wrote: > I think the righ fix is to just add > > config MACH_NOKIA_RM680 > bool I guess you meant MACH_NOKIA_RM696. > to the mach-omap2/Kconfig like we have for n8x0 also. Should I draft a patch? Paul Bolle
* Paul Bolle <pebolle@tiscali.nl> [130308 09:24]: > On Fri, 2013-03-08 at 08:35 -0800, Tony Lindgren wrote: > > I think the righ fix is to just add > > > > config MACH_NOKIA_RM680 > > bool > > I guess you meant MACH_NOKIA_RM696. Ooops yes. > > to the mach-omap2/Kconfig like we have for n8x0 also. > > Should I draft a patch? Sure that would be nice. Tony
On Fri, 2013-03-08 at 09:55 -0800, Tony Lindgren wrote: > * Paul Bolle <pebolle@tiscali.nl> [130308 09:24]: > > Should I draft a patch? > > Sure that would be nice. One thing I couldn't determine is how the generated mach-types.h header handles multiple CONFIG_MACH_* macros. If both CONFIG_MACH_FOO and CONFIG_MACH_BAR are defined, and these both have a line in */mach-types, will the machine_is_foo() and machine_is_bar() macros both behave as one would expect? Paul Bolle
* Paul Bolle <pebolle@tiscali.nl> [130308 10:06]: > On Fri, 2013-03-08 at 09:55 -0800, Tony Lindgren wrote: > > * Paul Bolle <pebolle@tiscali.nl> [130308 09:24]: > > > Should I draft a patch? > > > > Sure that would be nice. > > One thing I couldn't determine is how the generated mach-types.h header > handles multiple CONFIG_MACH_* macros. > > If both CONFIG_MACH_FOO and CONFIG_MACH_BAR are defined, and these both > have a line in */mach-types, will the machine_is_foo() and > machine_is_bar() macros both behave as one would expect? Yes they do, for the selected ones the macro becomes machine_arch_type == MACH_TYPE_XYZ instead of 0. Regards, Tony
On Fri, Mar 08, 2013 at 07:02:44PM +0100, Paul Bolle wrote: > On Fri, 2013-03-08 at 09:55 -0800, Tony Lindgren wrote: > > * Paul Bolle <pebolle@tiscali.nl> [130308 09:24]: > > > Should I draft a patch? > > > > Sure that would be nice. > > One thing I couldn't determine is how the generated mach-types.h header > handles multiple CONFIG_MACH_* macros. > > If both CONFIG_MACH_FOO and CONFIG_MACH_BAR are defined, and these both > have a line in */mach-types, will the machine_is_foo() and > machine_is_bar() macros both behave as one would expect? It's actually quite clever. There's two levels to it. The first is that CONFIG_MACH_xxx result in their machine_is_xxx() macros being defined to constant zero if the CONFIG option is not enabled. That allows the compiler to throw away code for disabled platforms because the expression is always false. Otherwise, they end up as (machine_arch_type == MACH_TYPE_xxx). The second is the magic which happens when two CONFIG_MACH_xxx are selected. If only one is selected, then machine_arch_type is defined to the appropriate MACH_TYPE_xxx. This means that the above expression becomes constant-true, and the conditional is eliminated. If more than one is selected, then machine_arch_type is defined to a variable which is appropriately set to one of the MACH_TYPE_xxx values. So, the result is that: - de-selected platforms have their if (machine_is_xxx()) { } optimised out of the kernel. - for a kernel built targetting one platform, all the if (machine_is_xxx()) tests are optimised away, leaving only the relevant code behind. - otherwise, we get the _appropriate_ conditional code for the configuration generated. However, going back to that MACH_NOKIA_RM696. If there exists only a select of this symbol and no "config MACH_NOKIA_RM696" entry, then the symbol will never be generated in the output .config file. I too can find no trace of any use of machine_is_nokia_rm696 in the mainline kernel. So, if there's nothing using the machine_is_() symbol, and nothing using the CONFIG_MACH_NOKIA_RM696 symbol, then any select of that is entirely superfluous. Well, I did this: $ git grep -i nokia_rm696 arch/arm/mach-omap2/Kconfig: select MACH_NOKIA_RM696 arch/arm/mach-omap2/board-rm680.c:MACHINE_START(NOKIA_RM696, "Nokia RM-696 board") arch/arm/tools/mach-types:nokia_rm696 MACH_NOKIA_RM696 NOKIA_RM696 3522 So, there exists platform support for this device, provided by the RM680 support, but there's no use of the machine_is_xxx() symbol - and if there was, it would always be false. My conclusion is... it's a mess.
On Sat, 2013-03-09 at 00:01 +0000, Russell King - ARM Linux wrote: > It's actually quite clever. There's two levels to it. > > The first is that CONFIG_MACH_xxx result in their machine_is_xxx() macros > being defined to constant zero if the CONFIG option is not enabled. That > allows the compiler to throw away code for disabled platforms because > the expression is always false. > > Otherwise, they end up as (machine_arch_type == MACH_TYPE_xxx). > > The second is the magic which happens when two CONFIG_MACH_xxx are > selected. If only one is selected, then machine_arch_type is defined > to the appropriate MACH_TYPE_xxx. This means that the above expression > becomes constant-true, and the conditional is eliminated. > > If more than one is selected, then machine_arch_type is defined to a > variable which is appropriately set to one of the MACH_TYPE_xxx values. At boot? > So, the result is that: > - de-selected platforms have their if (machine_is_xxx()) { } optimised > out of the kernel. > - for a kernel built targetting one platform, all the > if (machine_is_xxx()) tests are optimised away, leaving only the > relevant code behind. > - otherwise, we get the _appropriate_ conditional code for the > configuration generated. Thanks for clarifying this. Quite clever indeed. > However, going back to that MACH_NOKIA_RM696. If there exists only a > select of this symbol and no "config MACH_NOKIA_RM696" entry, then the > symbol will never be generated in the output .config file. > >[...] > > My conclusion is... it's a mess. That mess can only be fully cleaned up if the code for the RM-696 that now is maintained in some unknown to me repository gets merged into mainline, can't it? In the meantime, how do you prefer I solve the (trivial) issue of an useless select for MACH_NOKIA_RM696? Drop that select or add an (equally useless) config entry for MACH_NOKIA_RM696? Or should I try to ignore it for the time being? Paul Bolle
* Paul Bolle <pebolle@tiscali.nl> [130309 11:52]: > On Sat, 2013-03-09 at 00:01 +0000, Russell King - ARM Linux wrote: > > > However, going back to that MACH_NOKIA_RM696. If there exists only a > > select of this symbol and no "config MACH_NOKIA_RM696" entry, then the > > symbol will never be generated in the output .config file. > > > >[...] > > > > My conclusion is... it's a mess. > > That mess can only be fully cleaned up if the code for the RM-696 that > now is maintained in some unknown to me repository gets merged into > mainline, can't it? > > In the meantime, how do you prefer I solve the (trivial) issue of an > useless select for MACH_NOKIA_RM696? Drop that select or add an (equally > useless) config entry for MACH_NOKIA_RM696? Or should I try to ignore it > for the time being? Just adding the config MACH_NOKIA_RM696 to Kconfig as bool should fix this unless I'm missing something here. Regards, Tony
On Mon, 2013-03-11 at 09:33 -0700, Tony Lindgren wrote: > * Paul Bolle <pebolle@tiscali.nl> [130309 11:52]: > > In the meantime, how do you prefer I solve the (trivial) issue of an > > useless select for MACH_NOKIA_RM696? Drop that select or add an (equally > > useless) config entry for MACH_NOKIA_RM696? Or should I try to ignore it > > for the time being? > > Just adding the config MACH_NOKIA_RM696 to Kconfig as bool should fix > this unless I'm missing something here. Yes, that should do. I already promised to draft that small patch last week. I hope to actually do that shortly. Paul Bolle
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig index 49ac3df..5cafa77 100644 --- a/arch/arm/mach-omap2/Kconfig +++ b/arch/arm/mach-omap2/Kconfig @@ -296,7 +296,6 @@ config MACH_NOKIA_RM680 bool "Nokia N950 (RM-680) / N9 (RM-696) phones" depends on ARCH_OMAP3 default y - select MACH_NOKIA_RM696 select OMAP_PACKAGE_CBB config MACH_NOKIA_RX51
When support was added for Nokia N9 (RM-696), with commit 63fc5f3bb3d0ca9ab4767a801b518aa6335f87ad ("ARM: OMAP: add minimal support for Nokia RM-696"), a select statement for MACH_NOKIA_RM696 was added to the tree. But there's no Kconfig symbol with that name. That symbol would be superfluous, since support for that machine piggybacks on MACH_NOKIA_RM680. So drop that select. Signed-off-by: Paul Bolle <pebolle@tiscali.nl> --- 0) Tested with "git grep". 1) Some searching on the web didn't return a "config MACH_NOKIA_RM696". So apparently there's not even a development tree that uses this symbol. arch/arm/mach-omap2/Kconfig | 1 - 1 file changed, 1 deletion(-)