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