Re: [PATCH v3 1/4] serial: ns16550: Support run-time configuration
Hi Simon, On Mon, Feb 3, 2020 at 10:34 AM Bin Meng wrote: > > On Mon, Feb 3, 2020 at 10:29 AM Bin Meng wrote: > > > > On Fri, Dec 20, 2019 at 8:58 AM Simon Glass wrote: > > > > > > At present this driver uses an assortment of CONFIG options to control > > > how it accesses the hardware. This is painful for platforms that are > > > supposed to be controlled by a device tree or a previous-stage bootloader. > > > > > > Add a new CONFIG option to enable fully dynamic configuration. This > > > controls register spacing, size, offset and endianness. > > > > > > Signed-off-by: Simon Glass > > > --- > > > > > > Changes in v3: > > > - Rewrite the conditional logic to a fix a bug and match serial_xx_shift() > > > - Add a separate flag for whether to use endian-aware out() functions > > > > > > Changes in v2: > > > - runtime -> run-time > > > - Enable run-time config for slimbootloader too > > > - Improve Kconfig help based on Bin's comments > > > - Use ns16550 in patch subject > > > > > > drivers/serial/Kconfig | 21 ++ > > > drivers/serial/ns16550.c | 59 ++-- > > > include/ns16550.h| 16 ++- > > > 3 files changed, 87 insertions(+), 9 deletions(-) > > > > > > > Reviewed-by: Bin Meng > > applied to u-boot-x86, thanks! This patch unfortunately breaks lots of platforms. See build log here: https://dev.azure.com/bmeng/GitHub/_build/results?buildId=152&view=results Could you please post a fix patch that could be squashed into this patch? This patch is already applied in u-boot-x86/master. Regards, Bin
Re: [PATCH v3 1/4] serial: ns16550: Support run-time configuration
On Mon, Feb 3, 2020 at 10:29 AM Bin Meng wrote: > > On Fri, Dec 20, 2019 at 8:58 AM Simon Glass wrote: > > > > At present this driver uses an assortment of CONFIG options to control > > how it accesses the hardware. This is painful for platforms that are > > supposed to be controlled by a device tree or a previous-stage bootloader. > > > > Add a new CONFIG option to enable fully dynamic configuration. This > > controls register spacing, size, offset and endianness. > > > > Signed-off-by: Simon Glass > > --- > > > > Changes in v3: > > - Rewrite the conditional logic to a fix a bug and match serial_xx_shift() > > - Add a separate flag for whether to use endian-aware out() functions > > > > Changes in v2: > > - runtime -> run-time > > - Enable run-time config for slimbootloader too > > - Improve Kconfig help based on Bin's comments > > - Use ns16550 in patch subject > > > > drivers/serial/Kconfig | 21 ++ > > drivers/serial/ns16550.c | 59 ++-- > > include/ns16550.h| 16 ++- > > 3 files changed, 87 insertions(+), 9 deletions(-) > > > > Reviewed-by: Bin Meng applied to u-boot-x86, thanks!
Re: [PATCH v3 1/4] serial: ns16550: Support run-time configuration
On Fri, Dec 20, 2019 at 8:58 AM Simon Glass wrote: > > At present this driver uses an assortment of CONFIG options to control > how it accesses the hardware. This is painful for platforms that are > supposed to be controlled by a device tree or a previous-stage bootloader. > > Add a new CONFIG option to enable fully dynamic configuration. This > controls register spacing, size, offset and endianness. > > Signed-off-by: Simon Glass > --- > > Changes in v3: > - Rewrite the conditional logic to a fix a bug and match serial_xx_shift() > - Add a separate flag for whether to use endian-aware out() functions > > Changes in v2: > - runtime -> run-time > - Enable run-time config for slimbootloader too > - Improve Kconfig help based on Bin's comments > - Use ns16550 in patch subject > > drivers/serial/Kconfig | 21 ++ > drivers/serial/ns16550.c | 59 ++-- > include/ns16550.h| 16 ++- > 3 files changed, 87 insertions(+), 9 deletions(-) > Reviewed-by: Bin Meng
RE: [PATCH v3 1/4] serial: ns16550: Support run-time configuration
Hi Simon,
Thanks for enabling this dynamic serial config. I have verified this with Slim
Bootloader
change(https://patchwork.ozlabs.org/project/uboot/list/?series=149214) on QEMU
and APL platform, and it works well as expected.
Reviewed-by: Aiden Park
Tested-by: Aiden Park
> -Original Message-
> From: U-Boot On Behalf Of Simon Glass
> Sent: Thursday, December 19, 2019 4:58 PM
> To: U-Boot Mailing List
> Cc: Stefan Roese ; Angelo Dureghello
> Subject: [PATCH v3 1/4] serial: ns16550: Support run-time configuration
>
> At present this driver uses an assortment of CONFIG options to control how
> it accesses the hardware. This is painful for platforms that are supposed to
> be
> controlled by a device tree or a previous-stage bootloader.
>
> Add a new CONFIG option to enable fully dynamic configuration. This
> controls register spacing, size, offset and endianness.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v3:
> - Rewrite the conditional logic to a fix a bug and match serial_xx_shift()
> - Add a separate flag for whether to use endian-aware out() functions
>
> Changes in v2:
> - runtime -> run-time
> - Enable run-time config for slimbootloader too
> - Improve Kconfig help based on Bin's comments
> - Use ns16550 in patch subject
>
> drivers/serial/Kconfig | 21 ++
> drivers/serial/ns16550.c | 59 ++
> --
> include/ns16550.h| 16 ++-
> 3 files changed, 87 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index
> ece7d87d4c..472a9f0929 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -598,6 +598,27 @@ config SYS_NS16550
> be used. It can be a constant or a function to get clock, eg,
> get_serial_clock().
>
> +config NS16550_DYNAMIC
> + bool "Allow NS16550 to be configured at runtime"
> + default y if SYS_COREBOOT || SYS_SLIMBOOTLOADER
> + help
> + Enable this option to allow device-tree control of the driver.
> +
> + Normally this driver is controlled by the following options:
> +
> + CONFIG_SYS_NS16550_PORT_MAPPED - indicates that port I/O is
> used for
> + access. If not enabled, then the UART is memory-mapped.
> + CONFIG_SYS_NS16550_MEM32 - if memory-mapped, indicates that
> 32-bit
> + access should be used (instead of 8-bit)
> + CONFIG_SYS_NS16550_REG_SIZE - indicates register width and also
> + endianness. If positive, big-endian access is used. If negative,
> + little-endian is used.
> +
> + It is not a good practice for a driver to be statically configured,
> + since it prevents the same driver being used for different types of
> + UARTs in a system. This option avoids this problem at the cost of a
> + slightly increased code size.
> +
> config INTEL_MID_SERIAL
> bool "Intel MID platform UART support"
> depends on DM_SERIAL && OF_CONTROL
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index
> 754b6e9921..eacca5b703 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -92,19 +92,59 @@ static inline int serial_in_shift(void *addr, int shift)
> #define CONFIG_SYS_NS16550_CLK 0 #endif
>
> +static void serial_out_dynamic(struct ns16550_platdata *plat, u8 *addr,
> +int value)
> +{
> + if (plat->flags & NS16550_FLAG_IO) {
> + outb(value, addr);
> + } else if (plat->reg_width == 4) {
> + if (plat->flags & NS16550_FLAG_ENDIAN) {
> + if (plat->flags & NS16550_FLAG_BE)
> + out_be32(addr, value);
> + else
> + out_le32(addr, value);
> + } else {
> + writel(value, addr);
> + }
> + } else if (plat->flags & NS16550_FLAG_BE) {
> + writeb(value, addr + (1 << plat->reg_shift) - 1);
> + } else {
> + writeb(value, addr);
> + }
> +}
> +
> +static int serial_in_dynamic(struct ns16550_platdata *plat, u8 *addr) {
> + if (plat->flags & NS16550_FLAG_IO) {
> + return inb(addr);
> + } else if (plat->reg_width == 4) {
> + if (plat->flags & NS16550_FLAG_ENDIAN) {
> + if (plat->flags & NS16550_FLAG_BE)
> + return in_be32(addr);
> + else
> + return in_le32(addr);
> + } else {
> +
[PATCH v3 1/4] serial: ns16550: Support run-time configuration
At present this driver uses an assortment of CONFIG options to control
how it accesses the hardware. This is painful for platforms that are
supposed to be controlled by a device tree or a previous-stage bootloader.
Add a new CONFIG option to enable fully dynamic configuration. This
controls register spacing, size, offset and endianness.
Signed-off-by: Simon Glass
---
Changes in v3:
- Rewrite the conditional logic to a fix a bug and match serial_xx_shift()
- Add a separate flag for whether to use endian-aware out() functions
Changes in v2:
- runtime -> run-time
- Enable run-time config for slimbootloader too
- Improve Kconfig help based on Bin's comments
- Use ns16550 in patch subject
drivers/serial/Kconfig | 21 ++
drivers/serial/ns16550.c | 59 ++--
include/ns16550.h| 16 ++-
3 files changed, 87 insertions(+), 9 deletions(-)
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index ece7d87d4c..472a9f0929 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -598,6 +598,27 @@ config SYS_NS16550
be used. It can be a constant or a function to get clock, eg,
get_serial_clock().
+config NS16550_DYNAMIC
+ bool "Allow NS16550 to be configured at runtime"
+ default y if SYS_COREBOOT || SYS_SLIMBOOTLOADER
+ help
+ Enable this option to allow device-tree control of the driver.
+
+ Normally this driver is controlled by the following options:
+
+ CONFIG_SYS_NS16550_PORT_MAPPED - indicates that port I/O is used for
+access. If not enabled, then the UART is memory-mapped.
+ CONFIG_SYS_NS16550_MEM32 - if memory-mapped, indicates that 32-bit
+access should be used (instead of 8-bit)
+ CONFIG_SYS_NS16550_REG_SIZE - indicates register width and also
+endianness. If positive, big-endian access is used. If negative,
+little-endian is used.
+
+ It is not a good practice for a driver to be statically configured,
+ since it prevents the same driver being used for different types of
+ UARTs in a system. This option avoids this problem at the cost of a
+ slightly increased code size.
+
config INTEL_MID_SERIAL
bool "Intel MID platform UART support"
depends on DM_SERIAL && OF_CONTROL
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 754b6e9921..eacca5b703 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -92,19 +92,59 @@ static inline int serial_in_shift(void *addr, int shift)
#define CONFIG_SYS_NS16550_CLK 0
#endif
+static void serial_out_dynamic(struct ns16550_platdata *plat, u8 *addr,
+ int value)
+{
+ if (plat->flags & NS16550_FLAG_IO) {
+ outb(value, addr);
+ } else if (plat->reg_width == 4) {
+ if (plat->flags & NS16550_FLAG_ENDIAN) {
+ if (plat->flags & NS16550_FLAG_BE)
+ out_be32(addr, value);
+ else
+ out_le32(addr, value);
+ } else {
+ writel(value, addr);
+ }
+ } else if (plat->flags & NS16550_FLAG_BE) {
+ writeb(value, addr + (1 << plat->reg_shift) - 1);
+ } else {
+ writeb(value, addr);
+ }
+}
+
+static int serial_in_dynamic(struct ns16550_platdata *plat, u8 *addr)
+{
+ if (plat->flags & NS16550_FLAG_IO) {
+ return inb(addr);
+ } else if (plat->reg_width == 4) {
+ if (plat->flags & NS16550_FLAG_ENDIAN) {
+ if (plat->flags & NS16550_FLAG_BE)
+ return in_be32(addr);
+ else
+ return in_le32(addr);
+ } else {
+ return readl(addr);
+ }
+ } else if (plat->flags & NS16550_FLAG_BE) {
+ return readb(addr + (1 << plat->reg_shift) - 1);
+ } else {
+ return readb(addr);
+ }
+}
+
static void ns16550_writeb(NS16550_t port, int offset, int value)
{
struct ns16550_platdata *plat = port->plat;
unsigned char *addr;
offset *= 1 << plat->reg_shift;
- addr = (unsigned char *)plat->base + offset;
+ addr = (unsigned char *)plat->base + offset + plat->reg_offset;
- /*
-* As far as we know it doesn't make sense to support selection of
-* these options at run-time, so use the existing CONFIG options.
-*/
- serial_out_shift(addr + plat->reg_offset, plat->reg_shift, value);
+ if (IS_ENABLED(CONFIG_NS16550_DYNAMIC))
+ serial_out_dynamic(plat, addr, value);
+ else
+ serial_out_shift(addr, plat->reg_shift, value);
}
static int ns16550_readb(NS16550_t port, int offset)
@@ -113,9 +153,12 @@ static int ns16550_readb(N
