Trilok Soni wrote:
> Hi Eric,
>
> On Mon, Apr 21, 2008 at 2:43 AM, Eric Nelson (Boundary Devices)
> <[EMAIL PROTECTED]> wrote:
>> Signed-off-by: Eric Nelson (Boundary Devices) <[EMAIL PROTECTED]>
>> ---
>> arch/arm/mach-davinci/Kconfig | 25 +++++++++++++++++
>> arch/arm/mach-davinci/clock.c | 18 ++++++++++++-
>> arch/arm/mach-davinci/serial.c | 57
>> +++++++++++++++++++++++++++++++++++-----
>> 3 files changed, 92 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
>> index 0514c85..7b58b84 100644
>> --- a/arch/arm/mach-davinci/Kconfig
>> +++ b/arch/arm/mach-davinci/Kconfig
>> @@ -27,6 +27,31 @@ config DAVINCI_I2C_EXPANDER
>> Configure this option to specify whether the board used
>> has I2C exapnder with ATA, USB, CF.
>>
>> +config DAVINCI_UART0
>> + bool "Enable UART0"
>> + default y if MACH_DAVINCI_EVM
>> + depends on ARCH_DAVINCI644x
>> + help
>> + Choose this to include support for UART0
>> +
>> +config DAVINCI_UART1
>> + bool "Enable UART1"
>> + default n if MACH_DAVINCI_EVM
>> + depends on ARCH_DAVINCI644x
>> + help
>> + Choose this to include support for UART1. Note that the
>> + board-specific startup will need to set up the pin
>> + multiplexing.
>> +
>> +config DAVINCI_UART2
>> + bool "Enable UART2"
>> + default n if MACH_DAVINCI_EVM
>> + depends on ARCH_DAVINCI644x
>> + help
>> + Choose this to include support for UART2. Note that the
>> + board-specific startup will need to set up the pin
>> + multiplexing.
>> +
>> config DAVINCI_MCBSP
>> bool
>> prompt "DaVinci McBSP Driver" if SOUND_DAVINCI=n
>> diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
>> index b3092c1..0446149 100644
>> --- a/arch/arm/mach-davinci/clock.c
>> +++ b/arch/arm/mach-davinci/clock.c
>> @@ -190,11 +190,27 @@ static struct clk davinci_clks[] = {
>> .lpsc = -1,
>> .flags = ALWAYS_ENABLED,
>> },
>> +#ifdef CONFIG_DAVINCI_UART0
>> {
>> - .name = "UART",
>> + .name = "UART0",
>> .rate = &fixedrate,
>> .lpsc = DAVINCI_LPSC_UART0,
>> },
>> +#endif
>> +#ifdef CONFIG_DAVINCI_UART1
>> + {
>> + .name = "UART1",
>> + .rate = &fixedrate,
>> + .lpsc = DAVINCI_LPSC_UART1,
>> + },
>> +#endif
>> +#ifdef CONFIG_DAVINCI_UART2
>> + {
>> + .name = "UART2",
>> + .rate = &fixedrate,
>> + .lpsc = DAVINCI_LPSC_UART2,
>> + },
>> +#endif
>> {
>> .name = "EMACCLK",
>> .rate = &commonrate,
>> diff --git a/arch/arm/mach-davinci/serial.c b/arch/arm/mach-davinci/serial.c
>> index d0e7c52..6684a8f 100644
>> --- a/arch/arm/mach-davinci/serial.c
>> +++ b/arch/arm/mach-davinci/serial.c
>> @@ -50,6 +50,7 @@ static inline void davinci_serial_outp(struct
>> plat_serial8250_port *p,
>> }
>>
>> static struct plat_serial8250_port serial_platform_data[] = {
>> +#ifdef CONFIG_DAVINCI_UART0
>> {
>> .membase = (char *)IO_ADDRESS(DAVINCI_UART0_BASE),
>> .mapbase = (unsigned long)DAVINCI_UART0_BASE,
>> @@ -59,11 +60,48 @@ static struct plat_serial8250_port
>> serial_platform_data[] = {
>> .regshift = 2,
>> .uartclk = 27000000,
>> },
>> +#endif
>> +#ifdef CONFIG_DAVINCI_UART1
>> + {
>> + .membase = (char *)IO_ADDRESS(DAVINCI_UART1_BASE),
>> + .mapbase = (unsigned long)DAVINCI_UART1_BASE,
>> + .irq = IRQ_UARTINT1,
>> + .flags = UPF_BOOT_AUTOCONF | UPF_SKIP_TEST,
>> + .iotype = UPIO_MEM,
>> + .regshift = 2,
>> + .uartclk = 27000000,
>> + },
>> +#endif
>> +#ifdef CONFIG_DAVINCI_UART2
>> + {
>> + .membase = (char *)IO_ADDRESS(DAVINCI_UART2_BASE),
>> + .mapbase = (unsigned long)DAVINCI_UART2_BASE,
>> + .irq = IRQ_UARTINT2,
>> + .flags = UPF_BOOT_AUTOCONF | UPF_SKIP_TEST,
>> + .iotype = UPIO_MEM,
>> + .regshift = 2,
>> + .uartclk = 27000000,
>> + },
>> +#endif
>> {
>> .flags = 0
>> },
>> };
>
>
> I am not happy with this #ifdefers here. Instead it should be done
> like how we are doing on OMAP. We use pass uart info structure from
> board specific file with "enabled uarts" pattern, which can be checked
> in serial.c. As no. of UARTS are going to be constant for DaVinci
> chip, but which UART is going to be enabled/disabled is based on board
> design.
>
Are you referring to mach-omap2/serial.c? If so, it seems that this
adds code unecessarily. As you mention, a given board will either
expose or not expose each of the three UARTs, and by using #ifdef,
the extra code and data will be stripped from the resulting image.
It's my expectation that any given board will have a davinci_xyz_defconfig
which will set the appropriate CONFIG_DAVINCI_UARTx variable.
While reviewing this, I did notice that my patch omitted a necessary
change for the DVEVM (an update to davinci_evm_dm644x_defconfig to
include UART0).
What's more, on boards like our Xenon board, the RS-232 drivers and
connectors are optionally installed on a connector board. Having
these set through CONFIG variables and #ifdefs prevents the need to
edit the sources to configure for a particular machine.
http://boundarydevices.com/xenon.php
This could probably be done in the board-specific davinci_get_config(),
but there's currently no support for that in the davinci tree.
Finally, the main reason I structured this code as I did is that
it was the smallest patch!
Regards,
Eric Nelson
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
