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

[Bin Meng]

Hi Stefan,

On Tue, Jan 19, 2016 at 6:54 PM, Stefan Roese <sr at denx.de> wrote:
> 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.
>

Ah, yes! I thought you wanted to use the existing call.

>> 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).
>

Yes.

> 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?
>

Yep, makes sense. Let's see how Simon thinks.

Regards,
Bin