[PATCH 05/13] ARM: LPC32XX: arch Kconfig, plat Kconfig, and makefiles

Uwe Kleine-König u.kleine-koenig at pengutronix.de
Wed Feb 3 13:56:44 EST 2010 

Hey Kevin,

> > > +config ARCH_LPC32XX_HSUART7_ENABLE
> > > +	bool "Enable high speed UART7"
> > > +	help
> > > +	 Also enable LPC32xx high speed serial support in drivers/serial
> > > +
> > > +endmenu
> > IMHO "enable" is a bit misleading here.  Depending on the selection here
> > the devices are defined or not.
> 
> I'm not sure what what you mean here. I'll look at the wording and try to
> fix.
For me enable is something like

	clk_enable(...)

but not

	platform_device_register(...)

.  Does this make it clearer?

> > > +choice
> > > +	prompt "Kernel uncompress status output UART selection"
> > > +	default ARCH_LPC32XX_UNCOMP_U5
> > > +
> > > +	config ARCH_LPC32XX_UNCOMP_HSU1
> > > +		bool "High speed UART 1"
> > > +		help
> > > +		 Kernel uncompress output is on high speed UART 1
> > > +
> > > +	config ARCH_LPC32XX_UNCOMP_HSU2
> > > +		bool "High speed UART 2"
> > > +		help
> > > +		 Kernel uncompress output is on high speed UART 2
> > > +
> > > +	config ARCH_LPC32XX_UNCOMP_U3
> > > +		bool "Standard UART 3"
> > > +		help
> > > +		 Kernel uncompress output is on standard UART 3
> > > +
> > > +	config ARCH_LPC32XX_UNCOMP_U4
> > > +		bool "Standard UART 4"
> > > +		help
> > > +		 Kernel uncompress output is on standard UART 4
> > > +
> > > +	config ARCH_LPC32XX_UNCOMP_U5
> > > +		bool "Standard UART 5"
> > > +		help
> > > +		 Kernel uncompress output is on standard UART 5
> > > +
> > > +	config ARCH_LPC32XX_UNCOMP_U6
> > > +		bool "Standard UART 6"
> > > +		help
> > > +		 Kernel uncompress output is on standard UART 6
> > > +
> > > +	config ARCH_LPC32XX_UNCOMP_HSU7
> > > +		bool "High speed UART 7"
> > > +		help
> > > +		 Kernel uncompress output is on high speed UART 7
> > > +
> > > +endchoice
> > Can you autodetect this?
> 
> This is the uncompress output prior to kernel startup. There are
> 7 UARTs on the device and different board manufacturers don't
> always use the same UART. For the currently supported boards
> (PHY3250, EA3250, and FDI3250 mach ids), this was the only way
> I could get the UART selection into the config without changing
> code. I suppose it's possible to put specific board checks in
> the uncompress macros, but it would require changes to that file
> for new boards. This seemed the best way to go.
IMHO the best way is to do something like
arch/arm/plat-mxc/include/mach/uncompress.h or
arch/arm/mach-ns9xxx/include/mach/uncompress.h.  That is either (mxc)
make it depend on mach_id or (ns9xxx) check all available ports and use
the first that is enabled.
 
> > > +choice
> > > +	prompt "debug output (printascii) UART selection"
> > > +	default ARCH_LPC32XX_DEBUGO_U5
> > > +
> > > +	config ARCH_LPC32XX_DEBUGO_U3
> > > +		bool "Standard UART 3"
> > > +		help
> > > +		 printascii messages are output on standard UART 3
> > > +
> > > +	config ARCH_LPC32XX_DEBUGO_U4
> > > +		bool "Standard UART 4"
> > > +		help
> > > +		 printascii messages are output on standard UART 4
> > > +
> > > +	config ARCH_LPC32XX_DEBUGO_U5
> > > +		bool "Standard UART 5"
> > > +		help
> > > +		 printascii messages are output on standard UART 5
> > > +
> > > +	config ARCH_LPC32XX_DEBUGO_U6
> > > +		bool "Standard UART 6"
> > > +		help
> > > +		 printascii messages are output on standard UART 6
> > > +
> > > +endchoice
> > IMHO this isn't something that needs configuration via Kconfig.  It's
> > enough to have this via #defines in debug-macro.S.
> 
> See comment about uncompress output above. Some developers prefer
> printascii messages on a specific UART output (seperated from main
> serial or console output). This handles that.
No, printascii shouldn't be used but for debugging.  (So IMHO the
printascii that I saw somewhere in the series should go away.)
So if I want to debug something in early boot code then I check that
the cpp symbols are set up correctly for the board I'm currently using
and then I start adding printasciis and printhex8s.  When I found my
problem all these are removed again.  So don't expose it to your users.

> > > --- /dev/null
> > > +++ b/arch/arm/mach-lpc32xx/Kconfig.plat
> > > @@ -0,0 +1,98 @@
> > > +menu "LPC32XX platform choices"
> > > +
> > > +choice
> > > +    prompt "Choose your board"
> > > +    default MACH_PHY3250
> > > +    help
> > > +        This menu selects the LPC3250 board to support for this build
> > > +
> > > +    config MACH_PHY3250
> > > +        bool "Phytec 3250 development board"
> > > +	help
> > > +	    Support for the Phytec 3250 development board
> > > +
> > > +endchoice
> > Again, support more than one mach per kernel?
> 
> There are a minimum of 3 supported machs for this arch. To keep thing
> simple, I wanted to release initially with just 1 mach. What is a good
> approach for this? Should I break up the arch and plat areas into different
> subdirectories under arch/arm similar to other platforms and remove
> Kconfig.plat?
My comment was more about the "choice" than on the existance of only a
single board.  For now it doesn't matter, but later it would be nice if
you could have a kernel with

	[X] PHY3250
	[X] EA3250
	[X] FDI3250

and therefor you must not use

	choice
	    prompt ...

but e.g.

	comment "LPC32XX platforms:"
	config MACH_PHY3250
		bool ...
		...

	config MACH_EA3250
		bool ...
		...
	
> > > +choice
> > > +	prompt "Phytec Carrier board revisions"
> > > +	depends on MACH_PHY3250
> > > +	default PHY3250_CARRIER_1305_3
> > > +	help
> > > +	  Select one of the supported carrier board revisions
> > > +
> > > +config PHY3250_CARRIER_1305_01
> > > +	bool "1305.0 or 1305.1 carrier board"
> > > +	help
> > > +	  Use carrier board version 1305.0 or 1305.1
> > > +
> > > +config PHY3250_CARRIER_1305_2
> > > +	bool "1305.2 carrier board"
> > > +	help
> > > +	  Use carrier board version 1305.2
> > > +
> > > +config PHY3250_CARRIER_1305_3
> > > +	bool "1305.3 carrier board"
> > > +	help
> > > +	  Use carrier board version 1305.3
> > > +
> > > +endchoice
> > ditto
> 
> I can make these 3 kernel parameters, but not all of them are
> autodetectable. Is this approach wrong?
It's all about having one image for more than one machine.  Then the
autobuilder needs only build a single image for you and you don't have
to care about the exact revisions of all your hardware (assuming you can
autodetect it).
 
> > 
> > > +choice
> > > +	prompt "Internal IRAM use"
> > > +	default MACH_LPC32XX_IRAM_RESERVED
> > > +	depends on MACH_PHY3250
> > > +
> > > +config MACH_LPC32XX_IRAM_RESERVED
> > > +	bool "IRAM is not used (reserved)"
> > > +	help
> > > +	  IRAM is not used for video or networking and can be used for
> > > +	  other purposes or drivers
> > > +
> > > +config MACH_LPC32XX_IRAM_FOR_CLCD
> > > +	bool "Use IRAM as a video frame buffer"
> > > +	help
> > > +	  IRAM will be used for the LCD frame buffer. If the required
> > buffer
> > > +	  size is larger than the size of IRAM, then SDRAM will be used
> > > +	  instead.
> > > +
> > > +endchoice
> > A request API would be nice here!?
> 
> I'm not sure what you mean here by a request API?. The IRAM also can
> be used for network buffer storage (the option was purposely removed
> for initial release). Can you please post an example?
Something like request_mem_region.  This way the first driver who wants
to can use the IRAM, no need to fix it at compile time.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

More information about the linux-arm-kernel mailing list