[U-Boot] [PATCH 2/3] x86: BayTrail: Add function to disable the internal legacy UART

[Stefan Roese]

Hi Bin,

On 19.01.2016 11:15, Bin Meng wrote:
> On Tue, Jan 19, 2016 at 5:29 PM, Stefan Roese <sr at denx.de> wrote:
>> Hi Bin,
>>
>> (added Simon again to Cc)
>>
>> On 19.01.2016 09:44, Bin Meng wrote:
>>> Hi Stefan,
>>>
>>> On Tue, Jan 19, 2016 at 4:40 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Stefan,
>>>>
>>>> On Mon, Jan 18, 2016 at 5:56 PM, Stefan Roese <sr at denx.de> wrote:
>>>>> Some BayTrail boards may want to use a different legacy UART than the
>>>>> internal one. E.g. one provided by a Winbond Super IO chip, like the
>>>>> W83627. This patch adds a function to disable this BayTrail internal
>>>>> UART for this purpose.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>> ---
>>>>>    arch/x86/cpu/baytrail/early_uart.c | 9 +++++++++
>>>>>    arch/x86/include/asm/u-boot-x86.h  | 3 +++
>>>>>    2 files changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/cpu/baytrail/early_uart.c b/arch/x86/cpu/baytrail/early_uart.c
>>>>> index b64a3a9..716783c 100644
>>>>> --- a/arch/x86/cpu/baytrail/early_uart.c
>>>>> +++ b/arch/x86/cpu/baytrail/early_uart.c
>>>>> @@ -76,3 +76,12 @@ int setup_early_uart(void)
>>>>>
>>>>>           return 0;
>>>>>    }
>>>>> +
>>>>> +int disable_internal_uart(void)
>>>>> +{
>>>>> +       /* Disable the legacy UART hardware. */
>>>>
>>>> nits: please remove the ending peirod.
>>>>
>>>>> +       x86_pci_write_config32(PCI_DEV_CONFIG(0, LPC_DEV, LPC_FUNC), UART_CONT,
>>>>> +                              0);
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
>>>>> index dbf8e95..0c95796 100644
>>>>> --- a/arch/x86/include/asm/u-boot-x86.h
>>>>> +++ b/arch/x86/include/asm/u-boot-x86.h
>>>>> @@ -47,6 +47,9 @@ int default_print_cpuinfo(void);
>>>>>    /* Set up a UART which can be used with printch(), printhex8(), etc. */
>>>>>    int setup_early_uart(void);
>>>>>
>>>>> +/* Disable the internal legacy UART */
>>>>> +int disable_internal_uart(void);
>>>
>>> If we can call disable_internal_uart() in board-specific codes, then
>>> this declaration can be moved to SoC-specific header instead of x86
>>> generic one.
>>
>> Do you have a preferred header for this in mind?
>>
>
> How about arch/x86/include/asm/arch-baytrail/baytrail.h?
>
>> Another idea would be, to add a parameter to the existing function
>> setup_early_uart() to either enable or disable the internal UART:
>>
>> Like this:
>>
>> int setup_early_uart(int enable)
>> {
>>          /* Enable the legacy UART hardware. */
>>          x86_pci_write_config32(PCI_DEV_CONFIG(0, LPC_DEV, LPC_FUNC), UART_CONT,
>>                                 enable);
>>          if (!enable)
>>                  return 0;
>>          ...
>>
>> What do you think? Should I change it this way?
>>
>
> The issue with setup_early_uart() is that it is only called when
> CONFIG_DEBUG_UART is on.

In fsp_init(), yes. But I can nevertheless call it from the
board specific code in my case to *disable* the internal
legacy UART.

> I think CONFIG_DEBUG_UART is only for debug
> purpose IOW it's still legal to have a U-Boot booting without
> CONFIG_DEBUG_UART.

Correct. But as mentioned above, I can call it from my board
code before the Windond enable function to disable the
internal UART (when changed to provide this functionality
as suggested in the last mail).

Or am I missing something? The function naming would be not
really matching its purpose any more. Perhaps I should also
rename it to setup_internal_uart() instead?

Thanks,
Stefan