From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Tue, 19 Jan 2016 11:54:17 +0100 Subject: [U-Boot] [PATCH 2/3] x86: BayTrail: Add function to disable the internal legacy UART In-Reply-To: References: <1453111018-27744-1-git-send-email-sr@denx.de> <1453111018-27744-2-git-send-email-sr@denx.de> <569E01EC.9060903@denx.de> Message-ID: <569E15D9.7060402@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Bin, On 19.01.2016 11:15, Bin Meng wrote: > On Tue, Jan 19, 2016 at 5:29 PM, Stefan Roese 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 wrote: >>>> Hi Stefan, >>>> >>>> On Mon, Jan 18, 2016 at 5:56 PM, Stefan Roese 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 >>>>> Cc: Bin Meng >>>>> Cc: Simon Glass >>>>> --- >>>>> 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