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

[Stefan Roese]

Hi Bin,

On 19.01.2016 12:02, Bin Meng wrote:
> 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.

I've just created a new patchset, with 2 patches now and the
suggested change from above implemented.

Thanks,
Stefan