From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Tue, 19 Jan 2016 14:06:50 +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> <569E15D9.7060402@denx.de> Message-ID: <569E34EA.1070402@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 12:02, Bin Meng wrote: > 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. I've just created a new patchset, with 2 patches now and the suggested change from above implemented. Thanks, Stefan