From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Tue, 19 Jan 2016 10:29:16 +0100
Subject: [U-Boot] [PATCH 2/3] x86: BayTrail: Add function to disable the
 internal legacy UART
In-Reply-To: <CAEUhbmXZhkd6_OP_qzxtqT=ScMiVOPLKvop+oPXETzp+guuJ_g@mail.gmail.com>
References: <1453111018-27744-1-git-send-email-sr@denx.de>
 <1453111018-27744-2-git-send-email-sr@denx.de>
 <CAEUhbmWfsNb0niiAAphbAKa2+3vty3TFv=PMtVucKcZm5B+f9w@mail.gmail.com>
 <CAEUhbmXZhkd6_OP_qzxtqT=ScMiVOPLKvop+oPXETzp+guuJ_g@mail.gmail.com>
Message-ID: <569E01EC.9060903@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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@gmail.com> wrote:
>> Hi Stefan,
>>
>> On Mon, Jan 18, 2016 at 5:56 PM, Stefan Roese <sr@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@denx.de>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Simon Glass <sjg@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?

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?

Thanks,
Stefan