[U-Boot] [PATCH 1/3] misc: Add simple driver to enable the legacy UART on Winbond Super IO chips
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_ */
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);
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
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
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
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
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
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
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
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
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 */
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
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
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.
helpIf you say Y here, you will get support for the WinbondW83627 legacy UART driver. This can be used to enable thelegacy 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
participants (2)
-
Bin Meng -
Stefan Roese