[U-Boot] [PATCH 1/3] misc: Add simple driver to enable the legacy UART on Winbond Super IO chips

[U-Boot] [PATCH 1/3] misc: Add simple driver to enable the legacy UART on Winbond Super IO chips

[Stefan Roese]

On most x86 boards, the legacy serial ports (io address 0x3f8/0x2f8)
are provided by a superio chip connected to the LPC bus. We must
program the superio chip so that serial ports are available for us.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/misc/Kconfig          |  7 +++++++
 drivers/misc/Makefile         |  1 +
 drivers/misc/winbond_w83627.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/winbond_w83627.h      | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+)
 create mode 100644 drivers/misc/winbond_w83627.c
 create mode 100644 include/winbond_w83627.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b92da4e..e024fa7 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -112,4 +112,11 @@ config RESET
 	  effect a reset. The uclass will try all available drivers when
 	  reset_walk() is called.
 
+config WINBOND_W83627
+	bool "Enable Winbond legacy UART driver"
+	help
+	  If you say Y here, you will get support for the Winbond
+	  W83627 legacy UART driver. This can be used to enable the
+	  legacy UART in the Winbond Super IO chips on X86 platforms.
+
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index aa137f5..15505bf 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_FSL_SEC_MON) += fsl_sec_mon.o
 obj-$(CONFIG_PCA9551_LED) += pca9551_led.o
 obj-$(CONFIG_RESET) += reset-uclass.o
 obj-$(CONFIG_FSL_DEVICE_DISABLE) += fsl_devdis.o
+obj-$(CONFIG_WINBOND_W83627) += winbond_w83627.o
diff --git a/drivers/misc/winbond_w83627.c b/drivers/misc/winbond_w83627.c
new file mode 100644
index 0000000..7c6a033
--- /dev/null
+++ b/drivers/misc/winbond_w83627.c
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2016 Stefan Roese <sr@denx.de>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/pnp_def.h>
+
+#define WINBOND_ENTRY_KEY	0x87
+#define WINBOND_EXIT_KEY	0xAA
+
+/* Enable configuration: pass entry key '0x87' into index port dev. */
+static void pnp_enter_conf_state(u16 dev)
+{
+	u16 port = dev >> 8;
+
+	outb(WINBOND_ENTRY_KEY, port);
+	outb(WINBOND_ENTRY_KEY, port);
+}
+
+/* Disable configuration: pass exit key '0xAA' into index port dev. */
+static void pnp_exit_conf_state(u16 dev)
+{
+	u16 port = dev >> 8;
+
+	outb(WINBOND_EXIT_KEY, port);
+}
+
+/* Bring up early serial debugging output before the RAM is initialized. */
+void winbond_enable_serial(uint dev, uint iobase, uint irq)
+{
+	pnp_enter_conf_state(dev);
+	pnp_set_logical_device(dev);
+	pnp_set_enable(dev, 0);
+	pnp_set_iobase(dev, PNP_IDX_IO0, iobase);
+	pnp_set_irq(dev, PNP_IDX_IRQ0, irq);
+	pnp_set_enable(dev, 1);
+	pnp_exit_conf_state(dev);
+}
diff --git a/include/winbond_w83627.h b/include/winbond_w83627.h
new file mode 100644
index 0000000..ac3bec6
--- /dev/null
+++ b/include/winbond_w83627.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2016 Stefan Roese <sr@denx.de>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef _WINBOND_W83627_H_
+#define _WINBOND_W83627_H_
+
+/* I/O address of Winbond Super IO chip */
+#define WINBOND_IO_PORT		0x2e
+
+/* Logical device number */
+#define W83627DHG_FDC		0	/* Floppy */
+#define W83627DHG_PP		1	/* Parallel port */
+#define W83627DHG_SP1		2	/* Com1 */
+#define W83627DHG_SP2		3	/* Com2 */
+#define W83627DHG_KBC		5	/* PS/2 keyboard & mouse */
+#define W83627DHG_SPI		6	/* Serial peripheral interface */
+#define W83627DHG_WDTO_PLED	8	/* WDTO#, PLED */
+#define W83627DHG_ACPI		10	/* ACPI */
+#define W83627DHG_HWM		11	/* Hardware monitor */
+#define W83627DHG_PECI_SST	12	/* PECI, SST */
+
+/**
+ * Configure the base I/O port of the specified serial device and enable the
+ * serial device.
+ *
+ * @dev: high 8 bits = super I/O port, low 8 bits = logical device number
+ * @iobase: processor I/O port address to assign to this serial device
+ * @irq: processor IRQ number to assign to this serial device
+ */
+void winbond_enable_serial(uint dev, uint iobase, uint irq);
+
+#endif /* _WINBOND_W83627_H_ */
-- 
2.6.5

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

[Stefan Roese]

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. */
+	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);
+
 void setup_pcat_compatibility(void);
 
 void isa_unmap_rom(u32 addr);
-- 
2.6.5

[U-Boot] [PATCH 3/3] x86: fsp: Disable legacy internal UART if necessary

[Stefan Roese]

The FSP enables the BayTrail internal UART (again). Boards that don't use
this UART but an external one instead (e.g. provided by a Super IO chip)
need to disable this internal UART. So that the one from the Super IO
chip can be used. This patch adds the necessary code, to disable the
internal legacy UART if the Winbond Super IO chip is enabled.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
---
 arch/x86/lib/fsp/fsp_support.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
index 875c96a..8114b81 100644
--- a/arch/x86/lib/fsp/fsp_support.c
+++ b/arch/x86/lib/fsp/fsp_support.c
@@ -89,6 +89,13 @@ struct fsp_header *__attribute__((optimize("O0"))) find_fsp_header(void)
 
 void fsp_continue(u32 status, void *hob_list)
 {
+	/*
+	 * The FSP enables the BayTrail internal legacy UART (again).
+	 * Disable it again, so that the Winbond one can be used.
+	 */
+	if (IS_ENABLED(CONFIG_WINBOND_W83627))
+		disable_internal_uart();
+
 	post_code(POST_MRC);
 
 	assert(status == 0);
@@ -114,6 +121,13 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
 	setup_early_uart();
 #endif
 
+	/*
+	 * To use the Winbond legacy UART (COM1), the BayTrail internal
+	 * legacy UART needs to get disabled first.
+	 */
+	if (IS_ENABLED(CONFIG_WINBOND_W83627))
+		disable_internal_uart();
+
 	fsp_hdr = find_fsp_header();
 	if (fsp_hdr == NULL) {
 		/* No valid FSP info header was found */
-- 
2.6.5

[U-Boot] [PATCH 1/3] misc: Add simple driver to enable the legacy UART on Winbond Super IO chips

[Bin Meng]

Hi Stefan,

On Mon, Jan 18, 2016 at 5:56 PM, Stefan Roese <sr@denx.de> wrote:
> On most x86 boards, the legacy serial ports (io address 0x3f8/0x2f8)
> are provided by a superio chip connected to the LPC bus. We must
> program the superio chip so that serial ports are available for us.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  drivers/misc/Kconfig          |  7 +++++++
>  drivers/misc/Makefile         |  1 +
>  drivers/misc/winbond_w83627.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/winbond_w83627.h      | 35 +++++++++++++++++++++++++++++++++++
>  4 files changed, 84 insertions(+)
>  create mode 100644 drivers/misc/winbond_w83627.c
>  create mode 100644 include/winbond_w83627.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b92da4e..e024fa7 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -112,4 +112,11 @@ config RESET
>           effect a reset. The uclass will try all available drivers when
>           reset_walk() is called.
>
> +config WINBOND_W83627
> +       bool "Enable Winbond legacy UART driver"

Winbond 83627 Super I/O driver? As we may extend its support in the
future so that it is not only for legacy UART.

> +       help
> +         If you say Y here, you will get support for the Winbond
> +         W83627 legacy UART driver. This can be used to enable the
> +         legacy UART in the Winbond Super IO chips on X86 platforms.
> +
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index aa137f5..15505bf 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_FSL_SEC_MON) += fsl_sec_mon.o
>  obj-$(CONFIG_PCA9551_LED) += pca9551_led.o
>  obj-$(CONFIG_RESET) += reset-uclass.o
>  obj-$(CONFIG_FSL_DEVICE_DISABLE) += fsl_devdis.o
> +obj-$(CONFIG_WINBOND_W83627) += winbond_w83627.o
> diff --git a/drivers/misc/winbond_w83627.c b/drivers/misc/winbond_w83627.c
> new file mode 100644
> index 0000000..7c6a033
> --- /dev/null
> +++ b/drivers/misc/winbond_w83627.c
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (C) 2016 Stefan Roese <sr@denx.de>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/pnp_def.h>
> +
> +#define WINBOND_ENTRY_KEY      0x87
> +#define WINBOND_EXIT_KEY       0xAA

nits: use lower case 0xaa

> +
> +/* Enable configuration: pass entry key '0x87' into index port dev. */

nits: please remove the ending period. please fix this globally.

And based on the codes below, probably it's better to say: pass entry
key '0x87' into index port dev _twice_?

> +static void pnp_enter_conf_state(u16 dev)
> +{
> +       u16 port = dev >> 8;
> +
> +       outb(WINBOND_ENTRY_KEY, port);
> +       outb(WINBOND_ENTRY_KEY, port);
> +}
> +
> +/* Disable configuration: pass exit key '0xAA' into index port dev. */
> +static void pnp_exit_conf_state(u16 dev)
> +{
> +       u16 port = dev >> 8;
> +
> +       outb(WINBOND_EXIT_KEY, port);
> +}
> +
> +/* Bring up early serial debugging output before the RAM is initialized. */
> +void winbond_enable_serial(uint dev, uint iobase, uint irq)
> +{
> +       pnp_enter_conf_state(dev);
> +       pnp_set_logical_device(dev);
> +       pnp_set_enable(dev, 0);
> +       pnp_set_iobase(dev, PNP_IDX_IO0, iobase);
> +       pnp_set_irq(dev, PNP_IDX_IRQ0, irq);
> +       pnp_set_enable(dev, 1);
> +       pnp_exit_conf_state(dev);
> +}
> diff --git a/include/winbond_w83627.h b/include/winbond_w83627.h
> new file mode 100644
> index 0000000..ac3bec6
> --- /dev/null
> +++ b/include/winbond_w83627.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2016 Stefan Roese <sr@denx.de>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef _WINBOND_W83627_H_
> +#define _WINBOND_W83627_H_
> +
> +/* I/O address of Winbond Super IO chip */
> +#define WINBOND_IO_PORT                0x2e
> +
> +/* Logical device number */
> +#define W83627DHG_FDC          0       /* Floppy */
> +#define W83627DHG_PP           1       /* Parallel port */
> +#define W83627DHG_SP1          2       /* Com1 */
> +#define W83627DHG_SP2          3       /* Com2 */
> +#define W83627DHG_KBC          5       /* PS/2 keyboard & mouse */
> +#define W83627DHG_SPI          6       /* Serial peripheral interface */
> +#define W83627DHG_WDTO_PLED    8       /* WDTO#, PLED */
> +#define W83627DHG_ACPI         10      /* ACPI */
> +#define W83627DHG_HWM          11      /* Hardware monitor */
> +#define W83627DHG_PECI_SST     12      /* PECI, SST */
> +
> +/**
> + * Configure the base I/O port of the specified serial device and enable the
> + * serial device.
> + *
> + * @dev: high 8 bits = super I/O port, low 8 bits = logical device number
> + * @iobase: processor I/O port address to assign to this serial device
> + * @irq: processor IRQ number to assign to this serial device
> + */
> +void winbond_enable_serial(uint dev, uint iobase, uint irq);
> +
> +#endif /* _WINBOND_W83627_H_ */
> --

Regards,
Bin

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

[Bin Meng]

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);
> +
>  void setup_pcat_compatibility(void);
>
>  void isa_unmap_rom(u32 addr);
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 3/3] x86: fsp: Disable legacy internal UART if necessary

[Bin Meng]

Hi Stefan,

On Mon, Jan 18, 2016 at 5:56 PM, Stefan Roese <sr@denx.de> wrote:
> The FSP enables the BayTrail internal UART (again). Boards that don't use
> this UART but an external one instead (e.g. provided by a Super IO chip)
> need to disable this internal UART. So that the one from the Super IO
> chip can be used. This patch adds the necessary code, to disable the
> internal legacy UART if the Winbond Super IO chip is enabled.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  arch/x86/lib/fsp/fsp_support.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
> index 875c96a..8114b81 100644
> --- a/arch/x86/lib/fsp/fsp_support.c
> +++ b/arch/x86/lib/fsp/fsp_support.c
> @@ -89,6 +89,13 @@ struct fsp_header *__attribute__((optimize("O0"))) find_fsp_header(void)
>
>  void fsp_continue(u32 status, void *hob_list)
>  {
> +       /*
> +        * The FSP enables the BayTrail internal legacy UART (again).
> +        * Disable it again, so that the Winbond one can be used.
> +        */
> +       if (IS_ENABLED(CONFIG_WINBOND_W83627))
> +               disable_internal_uart();
> +

I would put this into the board_init_f(), right before enabling super
I/O legacy UART. This way we avoid changing the generic FSP codes.

>         post_code(POST_MRC);
>
>         assert(status == 0);
> @@ -114,6 +121,13 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>         setup_early_uart();
>  #endif
>
> +       /*
> +        * To use the Winbond legacy UART (COM1), the BayTrail internal
> +        * legacy UART needs to get disabled first.
> +        */
> +       if (IS_ENABLED(CONFIG_WINBOND_W83627))
> +               disable_internal_uart();
> +

I don't think this change is needed as fsp_init() will enable legacy
UART anyway.

>         fsp_hdr = find_fsp_header();
>         if (fsp_hdr == NULL) {
>                 /* No valid FSP info header was found */
> --

Regards,
Bin

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

[Bin Meng]

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.

>> +
>>  void setup_pcat_compatibility(void);
>>
>>  void isa_unmap_rom(u32 addr);
>> --
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

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

[Stefan Roese]

Hi Bin,

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

Let me check if your suggestion to patch 3/3 works and I'll re-spin
the patch series.

Thanks for the review,
Stefan

[U-Boot] [PATCH 3/3] x86: fsp: Disable legacy internal UART if necessary

[Stefan Roese]

Hi Bin,

On 19.01.2016 09:40, Bin Meng wrote:
> Hi Stefan,
>
> On Mon, Jan 18, 2016 at 5:56 PM, Stefan Roese <sr@denx.de> wrote:
>> The FSP enables the BayTrail internal UART (again). Boards that don't use
>> this UART but an external one instead (e.g. provided by a Super IO chip)
>> need to disable this internal UART. So that the one from the Super IO
>> chip can be used. This patch adds the necessary code, to disable the
>> internal legacy UART if the Winbond Super IO chip is enabled.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>   arch/x86/lib/fsp/fsp_support.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
>> index 875c96a..8114b81 100644
>> --- a/arch/x86/lib/fsp/fsp_support.c
>> +++ b/arch/x86/lib/fsp/fsp_support.c
>> @@ -89,6 +89,13 @@ struct fsp_header *__attribute__((optimize("O0"))) find_fsp_header(void)
>>
>>   void fsp_continue(u32 status, void *hob_list)
>>   {
>> +       /*
>> +        * The FSP enables the BayTrail internal legacy UART (again).
>> +        * Disable it again, so that the Winbond one can be used.
>> +        */
>> +       if (IS_ENABLED(CONFIG_WINBOND_W83627))
>> +               disable_internal_uart();
>> +
>
> I would put this into the board_init_f(), right before enabling super
> I/O legacy UART. This way we avoid changing the generic FSP codes.
>
>>          post_code(POST_MRC);
>>
>>          assert(status == 0);
>> @@ -114,6 +121,13 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>>          setup_early_uart();
>>   #endif
>>
>> +       /*
>> +        * To use the Winbond legacy UART (COM1), the BayTrail internal
>> +        * legacy UART needs to get disabled first.
>> +        */
>> +       if (IS_ENABLED(CONFIG_WINBOND_W83627))
>> +               disable_internal_uart();
>> +
>
> I don't think this change is needed as fsp_init() will enable legacy
> UART anyway.

You are correct. This patch is not needed at all, when I move the
call to disable_internal_uart() to the board specific code.

So I withdraw this patch and will send updated versions of the
other 2 patches.

Thanks,
Stefan

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

[Stefan Roese]

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

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

[Bin Meng]

Hi Stefan,

On Tue, Jan 19, 2016 at 5:29 PM, Stefan Roese <sr@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@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?
>

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. 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.

Regards,
Bin

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

[Stefan Roese]

Hi Bin,

On 19.01.2016 11:15, Bin Meng wrote:
> On Tue, Jan 19, 2016 at 5:29 PM, Stefan Roese <sr@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@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?
>>
>
> 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

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

[Bin Meng]

Hi Stefan,

On Tue, Jan 19, 2016 at 6:54 PM, Stefan Roese <sr@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@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@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?
>>>
>>
>> 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

[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@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@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@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?
>>>>
>>>
>>> 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