[U-Boot] [PATCH v2 0/5] ACPI: Generate SPCR table

[U-Boot] [PATCH v2 0/5] ACPI: Generate SPCR table

[Andy Shevchenko]

This is a series to enable SPCR table generation in U-Boot.

This table is useful to get early console in Linux for debugging purposes.
The benefit of using it is not only for x86, but also for arm64 community
(actually they introduced support in Linux kernel).

This implementation has been tested on Intel Edison with enabled ACPI support.

Known issues:
 - there is no ->getconfig() callback, thus some data is hard coded
 - if the serial parameters are changed at run time, SPCR is not regenerated,
   thus, might not produce what user excepts
   (this is more likely design issue of U-Boot itself)
 - implementation is not final and might not utilize best practices in U-Boot

Changelog v2:
- drop WIP state
- test on most recent U-Boot and Linux kernel

Andy Shevchenko (5):
  serial: Introduce ->getinfo() callback
  serial: ns16550: Read reg-io-width from device tree
  serial: ns16550: Provide ->getinfo() implementation
  x86: acpi: Add SPCR table description
  x86: acpi: Generate SPCR table

 arch/x86/include/asm/acpi_table.h | 51 +++++++++++++++++++
 arch/x86/lib/acpi_table.c         | 85 +++++++++++++++++++++++++++++++
 drivers/serial/ns16550.c          | 15 ++++++
 drivers/serial/serial-uclass.c    | 21 ++++++++
 include/common.h                  |  3 ++
 include/ns16550.h                 |  4 +-
 include/serial.h                  | 17 +++++++
 7 files changed, 195 insertions(+), 1 deletion(-)

--
2.19.1

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Andy Shevchenko]

New callback will give a necessary information to fill up ACPI SPCR table,
for example. Maybe used later for other purposes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++
 include/common.h               |  3 +++
 include/serial.h               | 17 +++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 665cca85cb..274734d059 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -308,6 +308,25 @@ int serial_setconfig(uint config)
 	return 0;
 }
 
+int serial_getinfo(struct serial_device_info *info)
+{
+	struct dm_serial_ops *ops;
+
+	if (!gd->cur_serial_dev)
+		return -ENODEV;
+
+	if (!info)
+		return -EINVAL;
+
+	info->baudrate = gd->baudrate;
+
+	ops = serial_get_ops(gd->cur_serial_dev);
+	if (ops->getinfo)
+		return ops->getinfo(gd->cur_serial_dev, info);
+
+	return -EINVAL;
+}
+
 void serial_stdio_init(void)
 {
 }
@@ -425,6 +444,8 @@ static int serial_post_probe(struct udevice *dev)
 	if (ops->loop)
 		ops->loop += gd->reloc_off;
 #endif
+	if (ops->getinfo)
+		ops->getinfo += gd->reloc_off;
 #endif
 	/* Set the baud rate */
 	if (ops->setbrg) {
diff --git a/include/common.h b/include/common.h
index 8b9f859c07..1f9c98e735 100644
--- a/include/common.h
+++ b/include/common.h
@@ -349,6 +349,8 @@ void smp_set_core_boot_addr(unsigned long addr, int corenr);
 void smp_kick_all_cpus(void);
 
 /* $(CPU)/serial.c */
+struct serial_device_info;
+
 int	serial_init   (void);
 void	serial_setbrg (void);
 void	serial_putc   (const char);
@@ -357,6 +359,7 @@ void	serial_puts   (const char *);
 int	serial_getc   (void);
 int	serial_tstc   (void);
 int	serial_setconfig(uint config);
+int	serial_getinfo(struct serial_device_info *info);
 
 /* $(CPU)/speed.c */
 int	get_clocks (void);
diff --git a/include/serial.h b/include/serial.h
index 020cd392e8..33531b7791 100644
--- a/include/serial.h
+++ b/include/serial.h
@@ -111,6 +111,16 @@ enum serial_stop {
 				SERIAL_8_BITS << SERIAL_BITS_SHIFT | \
 				SERIAL_ONE_STOP << SERIAL_STOP_SHIFT
 
+/* REVISIT: ACPI GAS specification implied */
+struct serial_device_info {
+	unsigned int baudrate;
+	u8	addr_space;	/* 0 - MMIO, 1 - IO */
+	u8	reg_width;
+	u8	reg_offset;
+	u8	reg_shift;
+	u64	addr;
+};
+
 /**
  * struct struct dm_serial_ops - Driver model serial operations
  *
@@ -201,6 +211,13 @@ struct dm_serial_ops {
 	 * @return 0 if OK, -ve on error
 	 */
 	int (*setconfig)(struct udevice *dev, uint serial_config);
+	/**
+	 * getinfo() - Get serial device information
+	 *
+	 * @dev: Device pointer
+	 * @info: struct serial_device_info to fill
+	 */
+	int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
 };
 
 /**
-- 
2.19.1

[U-Boot] [PATCH v2 2/5] serial: ns16550: Read reg-io-width from device tree

[Andy Shevchenko]

Cache the value of the reg-io-width property for the future use.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/serial/ns16550.c | 1 +
 include/ns16550.h        | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index f9041aa626..b51b56de9f 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -408,6 +408,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 
 	plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0);
 	plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0);
+	plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
 
 	err = clk_get_by_index(dev, 0, &clk);
 	if (!err) {
diff --git a/include/ns16550.h b/include/ns16550.h
index 5fcbcd2e74..22b89e4d6d 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -49,14 +49,16 @@
  * struct ns16550_platdata - information about a NS16550 port
  *
  * @base:		Base register address
+ * @reg_width:		IO accesses size of registers (in bytes)
  * @reg_shift:		Shift size of registers (0=byte, 1=16bit, 2=32bit...)
  * @clock:		UART base clock speed in Hz
  */
 struct ns16550_platdata {
 	unsigned long base;
+	int reg_width;
 	int reg_shift;
-	int clock;
 	int reg_offset;
+	int clock;
 	u32 fcr;
 };
 
-- 
2.19.1

[U-Boot] [PATCH v2 3/5] serial: ns16550: Provide ->getinfo() implementation

[Andy Shevchenko]

New callback will supply necessary information, for example,
to ACPI SPCR table.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/serial/ns16550.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index b51b56de9f..698acbfb51 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -334,6 +334,19 @@ static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
 	return 0;
 }
 
+static int ns16550_serial_getinfo(struct udevice *dev, struct serial_device_info *info)
+{
+	struct NS16550 *const com_port = dev_get_priv(dev);
+	struct ns16550_platdata *plat = com_port->plat;
+
+	info->addr_space = 0;
+	info->reg_width = plat->reg_width * 8;
+	info->reg_shift = plat->reg_shift;
+	info->reg_offset = plat->reg_offset;
+	info->addr = plat->base;
+	return 0;
+}
+
 int ns16550_serial_probe(struct udevice *dev)
 {
 	struct NS16550 *const com_port = dev_get_priv(dev);
@@ -441,6 +454,7 @@ const struct dm_serial_ops ns16550_serial_ops = {
 	.pending = ns16550_serial_pending,
 	.getc = ns16550_serial_getc,
 	.setbrg = ns16550_serial_setbrg,
+	.getinfo = ns16550_serial_getinfo,
 };
 
 #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
-- 
2.19.1

[U-Boot] [PATCH v2 4/5] x86: acpi: Add SPCR table description

[Andy Shevchenko]

Add SPCR table description as it provided in Linux kernel.

Port subtype for ACPI_DBG2_SERIAL_PORT is used as an interface type in SPCR.
Thus, provide a set of definitions to be utilized later.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/include/asm/acpi_table.h | 49 +++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h
index 95fae036f6..51cc806673 100644
--- a/arch/x86/include/asm/acpi_table.h
+++ b/arch/x86/include/asm/acpi_table.h
@@ -303,6 +303,55 @@ struct acpi_mcfg_mmconfig {
 /* ACPI global NVS structure */
 struct acpi_global_nvs;
 
+/* DBG2 definitions are partially used for SPCR interface_type */
+
+/* Types for port_type field */
+
+#define ACPI_DBG2_SERIAL_PORT		0x8000
+#define ACPI_DBG2_1394_PORT		0x8001
+#define ACPI_DBG2_USB_PORT		0x8002
+#define ACPI_DBG2_NET_PORT		0x8003
+
+/* Subtypes for port_subtype field */
+
+#define ACPI_DBG2_16550_COMPATIBLE	0x0000
+#define ACPI_DBG2_16550_SUBSET		0x0001
+#define ACPI_DBG2_ARM_PL011		0x0003
+#define ACPI_DBG2_ARM_SBSA_32BIT	0x000D
+#define ACPI_DBG2_ARM_SBSA_GENERIC	0x000E
+#define ACPI_DBG2_ARM_DCC		0x000F
+#define ACPI_DBG2_BCM2835		0x0010
+
+#define ACPI_DBG2_1394_STANDARD		0x0000
+
+#define ACPI_DBG2_USB_XHCI		0x0000
+#define ACPI_DBG2_USB_EHCI		0x0001
+
+/* SPCR (Serial Port Console Redirection table) */
+struct __packed acpi_spcr {
+	struct acpi_table_header header;
+	u8 interface_type;
+	u8 reserved[3];
+	struct acpi_gen_regaddr serial_port;
+	u8 interrupt_type;
+	u8 pc_interrupt;
+	u32 interrupt;		/* Global system interrupt */
+	u8 baud_rate;
+	u8 parity;
+	u8 stop_bits;
+	u8 flow_control;
+	u8 terminal_type;
+	u8 reserved1;
+	u16 pci_device_id;	/* Must be 0xffff if not PCI device */
+	u16 pci_vendor_id;	/* Must be 0xffff if not PCI device */
+	u8 pci_bus;
+	u8 pci_device;
+	u8 pci_function;
+	u32 pci_flags;
+	u8 pci_segment;
+	u32 reserved2;
+};
+
 /* These can be used by the target port */
 
 void acpi_fill_header(struct acpi_table_header *header, char *signature);
-- 
2.19.1

[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Andy Shevchenko]

Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
Let's provide it in U-Boot.

[1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/include/asm/acpi_table.h |  2 +
 arch/x86/lib/acpi_table.c         | 85 +++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h
index 51cc806673..e3b65cff66 100644
--- a/arch/x86/include/asm/acpi_table.h
+++ b/arch/x86/include/asm/acpi_table.h
@@ -327,6 +327,8 @@ struct acpi_global_nvs;
 #define ACPI_DBG2_USB_XHCI		0x0000
 #define ACPI_DBG2_USB_EHCI		0x0001
 
+#define ACPI_DBG2_UNKNOWN		0x00FF
+
 /* SPCR (Serial Port Console Redirection table) */
 struct __packed acpi_spcr {
 	struct acpi_table_header header;
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index c6b2026613..d8b44aea02 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -12,6 +12,7 @@
 #include <cpu.h>
 #include <dm.h>
 #include <dm/uclass-internal.h>
+#include <serial.h>
 #include <version.h>
 #include <asm/acpi/global_nvs.h>
 #include <asm/acpi_table.h>
@@ -338,6 +339,82 @@ static void acpi_create_mcfg(struct acpi_mcfg *mcfg)
 	header->checksum = table_compute_checksum((void *)mcfg, header->length);
 }
 
+static void acpi_create_spcr(struct acpi_spcr *spcr)
+{
+	struct acpi_table_header *header = &(spcr->header);
+	struct serial_device_info info = {0};
+	int access_size;
+	int ret;
+
+	/* Fill out header fields */
+	acpi_fill_header(header, "SPCR");
+	header->length = sizeof(struct acpi_spcr);
+	header->revision = 2;
+
+	ret = serial_getinfo(&info);
+	if (ret)
+		spcr->interface_type = ACPI_DBG2_UNKNOWN;
+	else
+		spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE;
+	debug("UART type %u (serial_getinfo() rc=%d)\n", spcr->interface_type, ret);
+
+	/* Encode baud rate */
+	switch (info.baudrate) {
+	case 9600:
+		spcr->baud_rate = 3;
+		break;
+	case 19200:
+		spcr->baud_rate = 4;
+		break;
+	case 57600:
+		spcr->baud_rate = 6;
+		break;
+	case 115200:
+		spcr->baud_rate = 7;
+		break;
+	default:
+		spcr->baud_rate = 0;
+		break;
+	}
+
+	/* Encode register access size */
+	switch (info.reg_shift) {
+	case 0:
+		access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS;
+		break;
+	case 1:
+		access_size = ACPI_ACCESS_SIZE_WORD_ACCESS;
+		break;
+	case 2:
+		access_size = ACPI_ACCESS_SIZE_DWORD_ACCESS;
+		break;
+	case 3:
+		access_size = ACPI_ACCESS_SIZE_QWORD_ACCESS;
+		break;
+	default:
+		access_size = ACPI_ACCESS_SIZE_UNDEFINED;
+		break;
+	}
+
+	spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY;
+	spcr->serial_port.bit_width = info.reg_width;
+	spcr->serial_port.bit_offset = info.reg_offset;
+	spcr->serial_port.access_size = access_size;
+	spcr->serial_port.addrl = info.addr >> 0;
+	spcr->serial_port.addrh = info.addr >> 32;
+
+	/* REVISIT: Hard coded values for now */
+	spcr->parity = 0;
+	spcr->stop_bits = 1;
+
+	/* REVISIT: No PCI devices for now */
+	spcr->pci_device_id = 0xffff;
+	spcr->pci_vendor_id = 0xffff;
+
+	/* Fix checksum */
+	header->checksum = table_compute_checksum((void *)spcr, header->length);
+}
+
 /*
  * QEMU's version of write_acpi_tables is defined in drivers/misc/qfw.c
  */
@@ -352,6 +429,7 @@ ulong write_acpi_tables(ulong start)
 	struct acpi_fadt *fadt;
 	struct acpi_mcfg *mcfg;
 	struct acpi_madt *madt;
+	struct acpi_spcr *spcr;
 	int i;
 
 	current = start;
@@ -440,6 +518,13 @@ ulong write_acpi_tables(ulong start)
 	acpi_add_table(rsdp, mcfg);
 	current = ALIGN(current, 16);
 
+	debug("ACPI:    * SPCR\n");
+	spcr = (struct acpi_spcr *)current;
+	acpi_create_spcr(spcr);
+	current += spcr->header.length;
+	acpi_add_table(rsdp, spcr);
+	current = ALIGN(current, 16);
+
 	debug("current = %x\n", current);
 
 	acpi_rsdp_addr = (unsigned long)rsdp;
-- 
2.19.1

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Alexander Graf]

On 15.11.18 18:58, Andy Shevchenko wrote:
> New callback will give a necessary information to fill up ACPI SPCR table,
> for example. Maybe used later for other purposes.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++
>  include/common.h               |  3 +++
>  include/serial.h               | 17 +++++++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 665cca85cb..274734d059 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -308,6 +308,25 @@ int serial_setconfig(uint config)
>  	return 0;
>  }
>  
> +int serial_getinfo(struct serial_device_info *info)
> +{
> +	struct dm_serial_ops *ops;
> +
> +	if (!gd->cur_serial_dev)
> +		return -ENODEV;
> +
> +	if (!info)
> +		return -EINVAL;
> +
> +	info->baudrate = gd->baudrate;
> +
> +	ops = serial_get_ops(gd->cur_serial_dev);
> +	if (ops->getinfo)
> +		return ops->getinfo(gd->cur_serial_dev, info);
> +
> +	return -EINVAL;
> +}
> +
>  void serial_stdio_init(void)
>  {
>  }
> @@ -425,6 +444,8 @@ static int serial_post_probe(struct udevice *dev)
>  	if (ops->loop)
>  		ops->loop += gd->reloc_off;
>  #endif
> +	if (ops->getinfo)
> +		ops->getinfo += gd->reloc_off;
>  #endif
>  	/* Set the baud rate */
>  	if (ops->setbrg) {
> diff --git a/include/common.h b/include/common.h
> index 8b9f859c07..1f9c98e735 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -349,6 +349,8 @@ void smp_set_core_boot_addr(unsigned long addr, int corenr);
>  void smp_kick_all_cpus(void);
>  
>  /* $(CPU)/serial.c */
> +struct serial_device_info;
> +
>  int	serial_init   (void);
>  void	serial_setbrg (void);
>  void	serial_putc   (const char);
> @@ -357,6 +359,7 @@ void	serial_puts   (const char *);
>  int	serial_getc   (void);
>  int	serial_tstc   (void);
>  int	serial_setconfig(uint config);
> +int	serial_getinfo(struct serial_device_info *info);
>  
>  /* $(CPU)/speed.c */
>  int	get_clocks (void);
> diff --git a/include/serial.h b/include/serial.h
> index 020cd392e8..33531b7791 100644
> --- a/include/serial.h
> +++ b/include/serial.h
> @@ -111,6 +111,16 @@ enum serial_stop {
>  				SERIAL_8_BITS << SERIAL_BITS_SHIFT | \
>  				SERIAL_ONE_STOP << SERIAL_STOP_SHIFT
>  
> +/* REVISIT: ACPI GAS specification implied */

What does this REVISIT tag mean?


Alex

> +struct serial_device_info {
> +	unsigned int baudrate;
> +	u8	addr_space;	/* 0 - MMIO, 1 - IO */
> +	u8	reg_width;
> +	u8	reg_offset;
> +	u8	reg_shift;
> +	u64	addr;
> +};
> +
>  /**
>   * struct struct dm_serial_ops - Driver model serial operations
>   *
> @@ -201,6 +211,13 @@ struct dm_serial_ops {
>  	 * @return 0 if OK, -ve on error
>  	 */
>  	int (*setconfig)(struct udevice *dev, uint serial_config);
> +	/**
> +	 * getinfo() - Get serial device information
> +	 *
> +	 * @dev: Device pointer
> +	 * @info: struct serial_device_info to fill
> +	 */
> +	int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
>  };
>  
>  /**
> 

[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Alexander Graf]

On 15.11.18 18:58, Andy Shevchenko wrote:
> Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
> Let's provide it in U-Boot.
> 
> [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/include/asm/acpi_table.h |  2 +
>  arch/x86/lib/acpi_table.c         | 85 +++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h
> index 51cc806673..e3b65cff66 100644
> --- a/arch/x86/include/asm/acpi_table.h
> +++ b/arch/x86/include/asm/acpi_table.h
> @@ -327,6 +327,8 @@ struct acpi_global_nvs;
>  #define ACPI_DBG2_USB_XHCI		0x0000
>  #define ACPI_DBG2_USB_EHCI		0x0001
>  
> +#define ACPI_DBG2_UNKNOWN		0x00FF
> +
>  /* SPCR (Serial Port Console Redirection table) */
>  struct __packed acpi_spcr {
>  	struct acpi_table_header header;
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index c6b2026613..d8b44aea02 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -12,6 +12,7 @@
>  #include <cpu.h>
>  #include <dm.h>
>  #include <dm/uclass-internal.h>
> +#include <serial.h>
>  #include <version.h>
>  #include <asm/acpi/global_nvs.h>
>  #include <asm/acpi_table.h>
> @@ -338,6 +339,82 @@ static void acpi_create_mcfg(struct acpi_mcfg *mcfg)
>  	header->checksum = table_compute_checksum((void *)mcfg, header->length);
>  }
>  
> +static void acpi_create_spcr(struct acpi_spcr *spcr)
> +{
> +	struct acpi_table_header *header = &(spcr->header);
> +	struct serial_device_info info = {0};
> +	int access_size;
> +	int ret;
> +
> +	/* Fill out header fields */
> +	acpi_fill_header(header, "SPCR");
> +	header->length = sizeof(struct acpi_spcr);
> +	header->revision = 2;
> +
> +	ret = serial_getinfo(&info);
> +	if (ret)
> +		spcr->interface_type = ACPI_DBG2_UNKNOWN;
> +	else
> +		spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE;

This sounds like pretty subtle semantics. Could you just include the
interface type in &info?

The main problem I'm seeing is that your UART might be a PL011
compatible which could still implement getinfo() but then wouldn't be
16660 compatible.

> +	debug("UART type %u (serial_getinfo() rc=%d)\n", spcr->interface_type, ret);
> +
> +	/* Encode baud rate */
> +	switch (info.baudrate) {
> +	case 9600:
> +		spcr->baud_rate = 3;
> +		break;
> +	case 19200:
> +		spcr->baud_rate = 4;
> +		break;
> +	case 57600:
> +		spcr->baud_rate = 6;
> +		break;
> +	case 115200:
> +		spcr->baud_rate = 7;
> +		break;

Are any higher (and lower) ones specified too? If so, you may want to
add them as well.

> +	default:
> +		spcr->baud_rate = 0;
> +		break;
> +	}
> +
> +	/* Encode register access size */
> +	switch (info.reg_shift) {
> +	case 0:
> +		access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS;
> +		break;
> +	case 1:
> +		access_size = ACPI_ACCESS_SIZE_WORD_ACCESS;
> +		break;
> +	case 2:
> +		access_size = ACPI_ACCESS_SIZE_DWORD_ACCESS;
> +		break;
> +	case 3:
> +		access_size = ACPI_ACCESS_SIZE_QWORD_ACCESS;
> +		break;
> +	default:
> +		access_size = ACPI_ACCESS_SIZE_UNDEFINED;
> +		break;
> +	}
> +
> +	spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY;

Can you guarantee that? Should probably be part of &info as well if addr is.

> +	spcr->serial_port.bit_width = info.reg_width;
> +	spcr->serial_port.bit_offset = info.reg_offset;
> +	spcr->serial_port.access_size = access_size;
> +	spcr->serial_port.addrl = info.addr >> 0;
> +	spcr->serial_port.addrh = info.addr >> 32;
> +
> +	/* REVISIT: Hard coded values for now */
> +	spcr->parity = 0;
> +	spcr->stop_bits = 1;

IMHO those should be part of &info as well. If you have to hard code
them, better hard code them in the driver rather than the generic ACPI
generation code.


Alex

> +
> +	/* REVISIT: No PCI devices for now */
> +	spcr->pci_device_id = 0xffff;
> +	spcr->pci_vendor_id = 0xffff;
> +
> +	/* Fix checksum */
> +	header->checksum = table_compute_checksum((void *)spcr, header->length);
> +}
> +
>  /*
>   * QEMU's version of write_acpi_tables is defined in drivers/misc/qfw.c
>   */
> @@ -352,6 +429,7 @@ ulong write_acpi_tables(ulong start)
>  	struct acpi_fadt *fadt;
>  	struct acpi_mcfg *mcfg;
>  	struct acpi_madt *madt;
> +	struct acpi_spcr *spcr;
>  	int i;
>  
>  	current = start;
> @@ -440,6 +518,13 @@ ulong write_acpi_tables(ulong start)
>  	acpi_add_table(rsdp, mcfg);
>  	current = ALIGN(current, 16);
>  
> +	debug("ACPI:    * SPCR\n");
> +	spcr = (struct acpi_spcr *)current;
> +	acpi_create_spcr(spcr);
> +	current += spcr->header.length;
> +	acpi_add_table(rsdp, spcr);
> +	current = ALIGN(current, 16);
> +
>  	debug("current = %x\n", current);
>  
>  	acpi_rsdp_addr = (unsigned long)rsdp;
> 

[U-Boot] [PATCH v2 2/5] serial: ns16550: Read reg-io-width from device tree

[Alexander Graf]

On 15.11.18 18:58, Andy Shevchenko wrote:
> Cache the value of the reg-io-width property for the future use.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/serial/ns16550.c | 1 +
>  include/ns16550.h        | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index f9041aa626..b51b56de9f 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -408,6 +408,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
>  
>  	plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0);
>  	plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0);
> +	plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
>  
>  	err = clk_get_by_index(dev, 0, &clk);
>  	if (!err) {
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 5fcbcd2e74..22b89e4d6d 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -49,14 +49,16 @@
>   * struct ns16550_platdata - information about a NS16550 port
>   *
>   * @base:		Base register address
> + * @reg_width:		IO accesses size of registers (in bytes)
>   * @reg_shift:		Shift size of registers (0=byte, 1=16bit, 2=32bit...)
>   * @clock:		UART base clock speed in Hz
>   */
>  struct ns16550_platdata {
>  	unsigned long base;
> +	int reg_width;
>  	int reg_shift;
> -	int clock;
>  	int reg_offset;
> +	int clock;

Why move clock?

Alex

>  	u32 fcr;
>  };
>  
> 

[U-Boot] [PATCH v2 3/5] serial: ns16550: Provide ->getinfo() implementation

[Alexander Graf]

On 15.11.18 18:58, Andy Shevchenko wrote:
> New callback will supply necessary information, for example,
> to ACPI SPCR table.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/serial/ns16550.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index b51b56de9f..698acbfb51 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -334,6 +334,19 @@ static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
>  	return 0;
>  }
>  
> +static int ns16550_serial_getinfo(struct udevice *dev, struct serial_device_info *info)

This line is above 80 characters. Please run checkpatch.pl on your patches.


Alex

> +{
> +	struct NS16550 *const com_port = dev_get_priv(dev);
> +	struct ns16550_platdata *plat = com_port->plat;
> +
> +	info->addr_space = 0;
> +	info->reg_width = plat->reg_width * 8;
> +	info->reg_shift = plat->reg_shift;
> +	info->reg_offset = plat->reg_offset;
> +	info->addr = plat->base;
> +	return 0;
> +}
> +
>  int ns16550_serial_probe(struct udevice *dev)
>  {
>  	struct NS16550 *const com_port = dev_get_priv(dev);
> @@ -441,6 +454,7 @@ const struct dm_serial_ops ns16550_serial_ops = {
>  	.pending = ns16550_serial_pending,
>  	.getc = ns16550_serial_getc,
>  	.setbrg = ns16550_serial_setbrg,
> +	.getinfo = ns16550_serial_getinfo,
>  };
>  
>  #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
> 

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 8:22 PM Alexander Graf <agraf@suse.de> wrote:
> On 15.11.18 18:58, Andy Shevchenko wrote:
> > New callback will give a necessary information to fill up ACPI SPCR table,
> > for example. Maybe used later for other purposes.

> > +/* REVISIT: ACPI GAS specification implied */
>
> What does this REVISIT tag mean?

Had you chance to read my cover letter?
There is a section called "Known issues", item 3 there might answer to
your question.

> > +struct serial_device_info {
> > +     unsigned int baudrate;
> > +     u8      addr_space;     /* 0 - MMIO, 1 - IO */
> > +     u8      reg_width;
> > +     u8      reg_offset;
> > +     u8      reg_shift;
> > +     u64     addr;
> > +};

--
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 2/5] serial: ns16550: Read reg-io-width from device tree

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 8:28 PM Alexander Graf <agraf@suse.de> wrote:
> On 15.11.18 18:58, Andy Shevchenko wrote:
> > Cache the value of the reg-io-width property for the future use.

> >  struct ns16550_platdata {
> >       unsigned long base;
> > +     int reg_width;
> >       int reg_shift;
> > -     int clock;
> >       int reg_offset;
> > +     int clock;
>
> Why move clock?

To group reg_* parameters together as it might make sense in the
future to have some other struct which would be used inside
serial_device_info and here.
If it's a big deal, don't pay attention to it, I would not touch it by request.

P.S. Why to overquoute?

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 3/5] serial: ns16550: Provide ->getinfo() implementation

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 8:30 PM Alexander Graf <agraf@suse.de> wrote:
> On 15.11.18 18:58, Andy Shevchenko wrote:
> > New callback will supply necessary information, for example,
> > to ACPI SPCR table.

> > +static int ns16550_serial_getinfo(struct udevice *dev, struct serial_device_info *info)
>
> This line is above 80 characters. Please run checkpatch.pl on your patches.

When you saw last time the terminal for 80 character limit?
I really don't give a crap about 80 characters limit when it uglifies the code.

Okay, here is 87 characters, so, I tend to split line as you suggested.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 8:27 PM Alexander Graf <agraf@suse.de> wrote:
> On 15.11.18 18:58, Andy Shevchenko wrote:
> > Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
> > Let's provide it in U-Boot.
> >
> > [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx

> > +     ret = serial_getinfo(&info);
> > +     if (ret)
> > +             spcr->interface_type = ACPI_DBG2_UNKNOWN;
> > +     else
> > +             spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE;
>
> This sounds like pretty subtle semantics. Could you just include the
> interface type in &info?

It might be another step if you provide a cool way to do so. Otherwise
we need first to de-x86 acpi stuff. I won't do it right now since it
so-o out of the scope of this series.

> The main problem I'm seeing is that your UART might be a PL011
> compatible which could still implement getinfo() but then wouldn't be
> 16660 compatible.

Yes, I know. Perhaps I forgot to mark it in "Known issues" section.

Have you seen, btw, PL011 on x86 environment?

> Are any higher (and lower) ones specified too? If so, you may want to
> add them as well.

There is a link to a specification. Care to read?

> > +     default:
> > +             spcr->baud_rate = 0;
> > +             break;

Going ahead.
This part actually something to develop through specifications, but I
need to ping Microsoft and our internal ACPI people for that.

> > +             break;
> > +     default:
> > +             access_size = ACPI_ACCESS_SIZE_UNDEFINED;
> > +             break;
> > +     }
> > +
> > +     spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY;
>
> Can you guarantee that? Should probably be part of &info as well if addr is.

Not now.
As per moving to info, see my comment about de-x86 of ACPI in U-Boot.

> > +     /* REVISIT: Hard coded values for now */
> > +     spcr->parity = 0;
> > +     spcr->stop_bits = 1;
>
> IMHO those should be part of &info as well. If you have to hard code
> them, better hard code them in the driver rather than the generic ACPI
> generation code.

Care to read cover letter carefully?

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 3/5] serial: ns16550: Provide ->getinfo() implementation

[Simon Glass]

HI Andy,

On 15 November 2018 at 11:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Nov 15, 2018 at 8:30 PM Alexander Graf <agraf@suse.de> wrote:
>> On 15.11.18 18:58, Andy Shevchenko wrote:
>> > New callback will supply necessary information, for example,
>> > to ACPI SPCR table.
>
>> > +static int ns16550_serial_getinfo(struct udevice *dev, struct serial_device_info *info)
>>
>> This line is above 80 characters. Please run checkpatch.pl on your patches.
>
> When you saw last time the terminal for 80 character limit?
> I really don't give a crap about 80 characters limit when it uglifies the code.
>
> Okay, here is 87 characters, so, I tend to split line as you suggested.

If you use patman it will check this for you.

I'm going to hold off on reviewing this since I'm not sure about the
struct purpose.

Regards,
Simon

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Simon Glass]

Hi Andy,

On 15 November 2018 at 09:58, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> New callback will give a necessary information to fill up ACPI SPCR table,
> for example. Maybe used later for other purposes.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++
>  include/common.h               |  3 +++
>  include/serial.h               | 17 +++++++++++++++++
>  3 files changed, 41 insertions(+)
>

Seems useful to me.

Please add a test to test/dm/serial.c

> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 665cca85cb..274734d059 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -308,6 +308,25 @@ int serial_setconfig(uint config)
>         return 0;
>  }
>
> +int serial_getinfo(struct serial_device_info *info)

This should use driver model, so:

int serial_getinfo(struct udevice *dev, struct serial_device_info *info)

> +{
> +       struct dm_serial_ops *ops;
> +
> +       if (!gd->cur_serial_dev)
> +               return -ENODEV;
> +
> +       if (!info)
> +               return -EINVAL;
> +
> +       info->baudrate = gd->baudrate;
> +
> +       ops = serial_get_ops(gd->cur_serial_dev);
> +       if (ops->getinfo)
> +               return ops->getinfo(gd->cur_serial_dev, info);
> +
> +       return -EINVAL;
> +}
> +
>  void serial_stdio_init(void)
>  {
>  }
> @@ -425,6 +444,8 @@ static int serial_post_probe(struct udevice *dev)
>         if (ops->loop)
>                 ops->loop += gd->reloc_off;
>  #endif
> +       if (ops->getinfo)
> +               ops->getinfo += gd->reloc_off;
>  #endif
>         /* Set the baud rate */
>         if (ops->setbrg) {
> diff --git a/include/common.h b/include/common.h
> index 8b9f859c07..1f9c98e735 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -349,6 +349,8 @@ void smp_set_core_boot_addr(unsigned long addr, int corenr);
>  void smp_kick_all_cpus(void);
>
>  /* $(CPU)/serial.c */
> +struct serial_device_info;
> +
>  int    serial_init   (void);
>  void   serial_setbrg (void);
>  void   serial_putc   (const char);
> @@ -357,6 +359,7 @@ void        serial_puts   (const char *);
>  int    serial_getc   (void);
>  int    serial_tstc   (void);
>  int    serial_setconfig(uint config);
> +int    serial_getinfo(struct serial_device_info *info);

See above - please don't add any more non-DM functions.

>
>  /* $(CPU)/speed.c */
>  int    get_clocks (void);
> diff --git a/include/serial.h b/include/serial.h
> index 020cd392e8..33531b7791 100644
> --- a/include/serial.h
> +++ b/include/serial.h
> @@ -111,6 +111,16 @@ enum serial_stop {
>                                 SERIAL_8_BITS << SERIAL_BITS_SHIFT | \
>                                 SERIAL_ONE_STOP << SERIAL_STOP_SHIFT
>
> +/* REVISIT: ACPI GAS specification implied */
> +struct serial_device_info {
> +       unsigned int baudrate;
> +       u8      addr_space;     /* 0 - MMIO, 1 - IO */

Please make this an enum

> +       u8      reg_width;
> +       u8      reg_offset;
> +       u8      reg_shift;
> +       u64     addr;

ulong

Needs a struct comment as I don't know what most of these do.

What about parity, number of bits, etc?

> +};
> +
>  /**
>   * struct struct dm_serial_ops - Driver model serial operations
>   *
> @@ -201,6 +211,13 @@ struct dm_serial_ops {
>          * @return 0 if OK, -ve on error
>          */
>         int (*setconfig)(struct udevice *dev, uint serial_config);
> +       /**
> +        * getinfo() - Get serial device information
> +        *
> +        * @dev: Device pointer
> +        * @info: struct serial_device_info to fill
> +        */
> +       int (*getinfo)(struct udevice *dev, struct serial_device_info *info);

This bit looks good

>  };
>
>  /**
> --
> 2.19.1
>

Regards,
SImon

[U-Boot] [PATCH v2 2/5] serial: ns16550: Read reg-io-width from device tree

[Simon Glass]

Hi Andy,

On 15 November 2018 at 09:58, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Cache the value of the reg-io-width property for the future use.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/serial/ns16550.c | 1 +
>  include/ns16550.h        | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index f9041aa626..b51b56de9f 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -408,6 +408,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
>
>         plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0);
>         plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0);
> +       plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
>

Is your intent to actually use this for something? It sounds good, as
at present we have all the horrible #ifdefs.

>         err = clk_get_by_index(dev, 0, &clk);
>         if (!err) {
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 5fcbcd2e74..22b89e4d6d 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -49,14 +49,16 @@
>   * struct ns16550_platdata - information about a NS16550 port
>   *
>   * @base:              Base register address
> + * @reg_width:         IO accesses size of registers (in bytes)
>   * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
>   * @clock:             UART base clock speed in Hz
>   */
>  struct ns16550_platdata {
>         unsigned long base;
> +       int reg_width;
>         int reg_shift;
> -       int clock;
>         int reg_offset;
> +       int clock;

Should be in a separate patch.

>         u32 fcr;
>  };
>
> --
> 2.19.1
>

Regards,
Simon

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 9:46 PM Simon Glass <sjg@chromium.org> wrote:
> On 15 November 2018 at 09:58, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > New callback will give a necessary information to fill up ACPI SPCR table,
> > for example. Maybe used later for other purposes.

> Seems useful to me.

Thanks. What do you think about introducing ->getconfig() at some point?

> Please add a test to test/dm/serial.c

Will do.

> > +int serial_getinfo(struct serial_device_info *info)
>
> This should use driver model, so:
>
> int serial_getinfo(struct udevice *dev, struct serial_device_info *info)

Oh, sure!

> > +/* REVISIT: ACPI GAS specification implied */
> > +struct serial_device_info {
> > +       unsigned int baudrate;
> > +       u8      addr_space;     /* 0 - MMIO, 1 - IO */
>
> Please make this an enum
>
> > +       u8      reg_width;
> > +       u8      reg_offset;
> > +       u8      reg_shift;
> > +       u64     addr;
>
> ulong
>
> Needs a struct comment as I don't know what most of these do.
>
> What about parity, number of bits, etc?

What about splitting up some parameters structure with U-Boot defined semantics?
In the acpi_create_spcr() we can convert it to what ACPI expects w/o
polluting U-Boot generic code.

That's why it has "REVISIT" tag, I would like to hear proposals how
these data structures should look like. Also I have no clue about
non-16550 drivers.

> > +};

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 2/5] serial: ns16550: Read reg-io-width from device tree

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 9:47 PM Simon Glass <sjg@chromium.org> wrote:
> On 15 November 2018 at 09:58, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Cache the value of the reg-io-width property for the future use.

> >         plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0);
> >         plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0);
> > +       plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
> >
>
> Is your intent to actually use this for something? It sounds good, as
> at present we have all the horrible #ifdefs.

Not in this series.
Here is just an implementation (enough for SPCR case) of Bin's proposal.

> > -       int clock;
> >         int reg_offset;
> > +       int clock;
>
> Should be in a separate patch.

Yes, if we conclude something about date structures. Otherwise don't
mind I won't touch it.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Alexander Graf]

On 15.11.18 20:31, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 8:22 PM Alexander Graf <agraf@suse.de> wrote:
>> On 15.11.18 18:58, Andy Shevchenko wrote:
>>> New callback will give a necessary information to fill up ACPI SPCR table,
>>> for example. Maybe used later for other purposes.
> 
>>> +/* REVISIT: ACPI GAS specification implied */
>>
>> What does this REVISIT tag mean?
> 
> Had you chance to read my cover letter?
> There is a section called "Known issues", item 3 there might answer to
> your question.

The usual tag for "not finalized, please don't apply yet" in upstream
patch submissions is "RFC". If you don't think your code is ready to be
applied, just tag the patches with RFC, like this:

  [RFC] serial: Introduce ->getinfo() callback

Then everyone knows that you don't think the patch is good enough yet.
Otherwise, every submission really implies that you think it should get
applied.

As for the data structure itself, I think I gave you a bit of feedback.
I wouldn't worry too much about getting everything right from the start,
as this is a monolithic open source project. So changing the structure
later on is easy.


Alex

[U-Boot] [PATCH v2 2/5] serial: ns16550: Read reg-io-width from device tree

[Alexander Graf]

On 15.11.18 20:33, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 8:28 PM Alexander Graf <agraf@suse.de> wrote:
>> On 15.11.18 18:58, Andy Shevchenko wrote:
>>> Cache the value of the reg-io-width property for the future use.
> 
>>>  struct ns16550_platdata {
>>>       unsigned long base;
>>> +     int reg_width;
>>>       int reg_shift;
>>> -     int clock;
>>>       int reg_offset;
>>> +     int clock;
>>
>> Why move clock?
> 
> To group reg_* parameters together as it might make sense in the
> future to have some other struct which would be used inside
> serial_device_info and here.
> If it's a big deal, don't pay attention to it, I would not touch it by request.

Well, there are a few users of "patch" semantics:

  * mainlining process
  * patch review
  * backports (to stable / distro versions)
  * reverts if something broke

While I agree that you could ignore it for the normal mainlining
process, all other users will get hurt by having multiple "things" done
in a single patch. Review gets harder, backports get more confusing and
a revert ends up changing more code, which in turn makes it harder.

So please keep changes isolated to the thing they do :).

> P.S. Why to overquoute?

What do you mean? Why I don't remove useless parts of the quote?
Lazyness I suppose.


Alex

[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Alexander Graf]

On 15.11.18 20:43, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 8:27 PM Alexander Graf <agraf@suse.de> wrote:
>> On 15.11.18 18:58, Andy Shevchenko wrote:
>>> Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
>>> Let's provide it in U-Boot.
>>>
>>> [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
> 
>>> +     ret = serial_getinfo(&info);
>>> +     if (ret)
>>> +             spcr->interface_type = ACPI_DBG2_UNKNOWN;
>>> +     else
>>> +             spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE;
>>
>> This sounds like pretty subtle semantics. Could you just include the
>> interface type in &info?
> 
> It might be another step if you provide a cool way to do so. Otherwise

Define an enum with serial_type where 0 is unknown and 1 is
16550_compatible. Provide that enum in getinfo().

Bonus points for keeping the enum and ACPI_DBG2 IDs in sync, so that we
don't need too much code to copy from one to the other.

> we need first to de-x86 acpi stuff. I won't do it right now since it
> so-o out of the scope of this series.

Well, we will probably want to be able to do that in the future. And
anything that hard codes more x86 assumptions is a step in the wrong
direction.

> 
>> The main problem I'm seeing is that your UART might be a PL011
>> compatible which could still implement getinfo() but then wouldn't be
>> 16660 compatible.
> 
> Yes, I know. Perhaps I forgot to mark it in "Known issues" section.
> 
> Have you seen, btw, PL011 on x86 environment?

I can create one for you in QEMU if you like?

> 
>> Are any higher (and lower) ones specified too? If so, you may want to
>> add them as well.
> 
> There is a link to a specification. Care to read?

A simple "no, the spec only defines the ones above" would've been a
better answer and saved both of us a few seconds of our lives ;).

> 
>>> +     default:
>>> +             spcr->baud_rate = 0;
>>> +             break;
> 
> Going ahead.
> This part actually something to develop through specifications, but I
> need to ping Microsoft and our internal ACPI people for that.
> 
>>> +             break;
>>> +     default:
>>> +             access_size = ACPI_ACCESS_SIZE_UNDEFINED;
>>> +             break;
>>> +     }
>>> +
>>> +     spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY;
>>
>> Can you guarantee that? Should probably be part of &info as well if addr is.
> 
> Not now.
> As per moving to info, see my comment about de-x86 of ACPI in U-Boot.

I don't understand how describing your address space correctly has
anything to do with de-x86'ing anything? x86 is the only (living) user
of non-MMIO address space out there.

> 
>>> +     /* REVISIT: Hard coded values for now */
>>> +     spcr->parity = 0;
>>> +     spcr->stop_bits = 1;
>>
>> IMHO those should be part of &info as well. If you have to hard code
>> them, better hard code them in the driver rather than the generic ACPI
>> generation code.
> 
> Care to read cover letter carefully?

Again, if you think your patch is terrible and shouldn't be applied
anyway, please mark it as RFC. I'll be happy to wait with review until
you've made up your mind then :).


Alex

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Simon Glass]

Hi Andy,

On 15 November 2018 at 11:51, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Nov 15, 2018 at 9:46 PM Simon Glass <sjg@chromium.org> wrote:
>> On 15 November 2018 at 09:58, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> >
>> > New callback will give a necessary information to fill up ACPI SPCR table,
>> > for example. Maybe used later for other purposes.
>
>> Seems useful to me.
>
> Thanks. What do you think about introducing ->getconfig() at some point?

Let's do it.

>
>> Please add a test to test/dm/serial.c
>
> Will do.
>
>> > +int serial_getinfo(struct serial_device_info *info)
>>
>> This should use driver model, so:
>>
>> int serial_getinfo(struct udevice *dev, struct serial_device_info *info)
>
> Oh, sure!
>
>> > +/* REVISIT: ACPI GAS specification implied */
>> > +struct serial_device_info {
>> > +       unsigned int baudrate;
>> > +       u8      addr_space;     /* 0 - MMIO, 1 - IO */
>>
>> Please make this an enum
>>
>> > +       u8      reg_width;
>> > +       u8      reg_offset;
>> > +       u8      reg_shift;
>> > +       u64     addr;
>>
>> ulong
>>
>> Needs a struct comment as I don't know what most of these do.
>>
>> What about parity, number of bits, etc?
>
> What about splitting up some parameters structure with U-Boot defined semantics?
> In the acpi_create_spcr() we can convert it to what ACPI expects w/o
> polluting U-Boot generic code.
>
> That's why it has "REVISIT" tag, I would like to hear proposals how
> these data structures should look like. Also I have no clue about
> non-16550 drivers.

Well so long as you document the struct members I think this is fine.
But I think you should add serial-format info (the 8n1 / 7e1 business)
to this.

Regards,
Simon

[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Bin Meng]

Hi Andy,

On Fri, Nov 16, 2018 at 1:59 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
> Let's provide it in U-Boot.
>
> [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
>

This URL redirects to
https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table.
It looks that the redirected one has a better name that deserves to be
mentioned in the commit message :)

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/include/asm/acpi_table.h |  2 +
>  arch/x86/lib/acpi_table.c         | 85 +++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
>

[snip]

Regards,
Bin

[U-Boot] [PATCH v2 4/5] x86: acpi: Add SPCR table description

[Bin Meng]

On Fri, Nov 16, 2018 at 1:59 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Add SPCR table description as it provided in Linux kernel.
>
> Port subtype for ACPI_DBG2_SERIAL_PORT is used as an interface type in SPCR.
> Thus, provide a set of definitions to be utilized later.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/include/asm/acpi_table.h | 49 +++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>

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

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 12:22:31PM -0800, Simon Glass wrote:
> On 15 November 2018 at 11:51, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Nov 15, 2018 at 9:46 PM Simon Glass <sjg@chromium.org> wrote:
> >> On 15 November 2018 at 09:58, Andy Shevchenko
> >> <andriy.shevchenko@linux.intel.com> wrote:

> >> > +int serial_getinfo(struct serial_device_info *info)
> >>
> >> This should use driver model, so:
> >>
> >> int serial_getinfo(struct udevice *dev, struct serial_device_info *info)
> >
> > Oh, sure!

I have to withdraw this comment based on two points:
- the rest of API is using it in this way
- it is all about current console IIUC, so, anyway we would need the same code to retrieve it

Are you still thinking that serial_getinfo(dev, info) would be preferable?

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 09:09:55PM +0100, Alexander Graf wrote:
> 
> 
> On 15.11.18 20:31, Andy Shevchenko wrote:
> > On Thu, Nov 15, 2018 at 8:22 PM Alexander Graf <agraf@suse.de> wrote:
> >> On 15.11.18 18:58, Andy Shevchenko wrote:
> >>> New callback will give a necessary information to fill up ACPI SPCR table,
> >>> for example. Maybe used later for other purposes.
> > 
> >>> +/* REVISIT: ACPI GAS specification implied */
> >>
> >> What does this REVISIT tag mean?
> > 
> > Had you chance to read my cover letter?
> > There is a section called "Known issues", item 3 there might answer to
> > your question.
> 
> The usual tag for "not finalized, please don't apply yet" in upstream
> patch submissions is "RFC". If you don't think your code is ready to be
> applied, just tag the patches with RFC, like this:
> 
>   [RFC] serial: Introduce ->getinfo() callback
> 
> Then everyone knows that you don't think the patch is good enough yet.
> Otherwise, every submission really implies that you think it should get
> applied.
> 
> As for the data structure itself, I think I gave you a bit of feedback.
> I wouldn't worry too much about getting everything right from the start,
> as this is a monolithic open source project. So changing the structure
> later on is easy.

Thanks, noted.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 11:45:32AM -0800, Simon Glass wrote:
> On 15 November 2018 at 09:58, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

> > +/* REVISIT: ACPI GAS specification implied */
> > +struct serial_device_info {
> > +       unsigned int baudrate;
> > +       u8      addr_space;     /* 0 - MMIO, 1 - IO */
> 
> Please make this an enum

OK.

> 
> > +       u8      reg_width;
> > +       u8      reg_offset;
> > +       u8      reg_shift;
> > +       u64     addr;
> 
> ulong

This, unfortunately, will not work. ACPI takes the address as 32-bit halves,
and shift to 32 on 32-bit platform is UB.

> Needs a struct comment as I don't know what most of these do.

OK.

> What about parity, number of bits, etc?

As discussed before, it will be filled thru getconfig().
Though, I would add necessary members explicitly.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Andy Shevchenko]

On Sun, Nov 18, 2018 at 08:37:20PM +0800, Bin Meng wrote:
> Hi Andy,
> 
> On Fri, Nov 16, 2018 at 1:59 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
> > Let's provide it in U-Boot.
> >
> > [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
> >
> 
> This URL redirects to
> https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table.
> It looks that the redirected one has a better name that deserves to be
> mentioned in the commit message :)

Sure, thanks!

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 09:19:38PM +0100, Alexander Graf wrote:
> On 15.11.18 20:43, Andy Shevchenko wrote:
> > On Thu, Nov 15, 2018 at 8:27 PM Alexander Graf <agraf@suse.de> wrote:
> >> On 15.11.18 18:58, Andy Shevchenko wrote:
> >>> Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
> >>> Let's provide it in U-Boot.
> >>>
> >>> [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx

Thanks for review.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Alexander Graf]

On 20.11.18 19:32, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 11:45:32AM -0800, Simon Glass wrote:
>> On 15 November 2018 at 09:58, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
> 
>>> +/* REVISIT: ACPI GAS specification implied */
>>> +struct serial_device_info {
>>> +       unsigned int baudrate;
>>> +       u8      addr_space;     /* 0 - MMIO, 1 - IO */
>>
>> Please make this an enum
> 
> OK.
> 
>>
>>> +       u8      reg_width;
>>> +       u8      reg_offset;
>>> +       u8      reg_shift;
>>> +       u64     addr;
>>
>> ulong
> 
> This, unfortunately, will not work. ACPI takes the address as 32-bit halves,
> and shift to 32 on 32-bit platform is UB.

I guess what Simon wanted to get to is more something like "phys_addr_t"
which actually tells you what the variable is supposed to transfer.

Keep in mind that this is only for interim data. You can put this into a
u64 before putting it into the ACPI table inside your ACPI specific code
just fine.


Alex

[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Wed, Nov 21, 2018 at 12:04 AM Alexander Graf <agraf@suse.de> wrote:
> On 20.11.18 19:32, Andy Shevchenko wrote:
> > On Thu, Nov 15, 2018 at 11:45:32AM -0800, Simon Glass wrote:
> >> On 15 November 2018 at 09:58, Andy Shevchenko
> >> <andriy.shevchenko@linux.intel.com> wrote:

> >>> +       u64     addr;
> >>
> >> ulong
> >
> > This, unfortunately, will not work. ACPI takes the address as 32-bit halves,
> > and shift to 32 on 32-bit platform is UB.
>
> I guess what Simon wanted to get to is more something like "phys_addr_t"
> which actually tells you what the variable is supposed to transfer.
>
> Keep in mind that this is only for interim data. You can put this into a
> u64 before putting it into the ACPI table inside your ACPI specific code
> just fine.

I have learned today that U-Boot inherited lower_32_bits() and upper_32_bits()
macros, so, I use ulong.

-- 
With Best Regards,
Andy Shevchenko