[U-Boot] [PATCH v1 0/2] ACPI: Generate SPCR table

[U-Boot] [PATCH v1 0/2] ACPI: Generate SPCR table

[Andy Shevchenko]

This is a WIP series (I have no time to continue with it, so, that's why WIP)
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 PoC 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 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

So, if there is any interest, I would be happy to help, but as I mentioned
above I have no capacity to continue this myself.

The patches would be also available through my U-Boot tree on GH.

Andy Shevchenko (2):
  WIP: serial: Introduce ->getinfo() callback
  WIP: x86: acpi: Generate SPCR table

 arch/x86/include/asm/acpi_table.h | 25 ++++++++++
 arch/x86/lib/acpi_table.c         | 82 +++++++++++++++++++++++++++++++
 drivers/serial/ns16550.c          | 14 ++++++
 drivers/serial/serial-uclass.c    | 21 ++++++++
 include/common.h                  |  3 ++
 include/serial.h                  | 14 ++++++
 6 files changed, 159 insertions(+)

-- 
2.18.0

[U-Boot] [PATCH v1 1/2] WIP: serial: Introduce ->getinfo() callback

[Andy Shevchenko]

TBD

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

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index f9041aa626..a996446936 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 = 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);
@@ -440,6 +453,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)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index ffdcae0931..4778302e8f 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -304,6 +304,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)
 {
 }
@@ -421,6 +440,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 83b3bdc58d..0479f3eac1 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..c73477b079 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,10 @@ struct dm_serial_ops {
 	 * @return 0 if OK, -ve on error
 	 */
 	int (*setconfig)(struct udevice *dev, uint serial_config);
+	/**
+	 * getinfo() - Get serial device information
+	 */
+	int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
 };
 
 /**
-- 
2.18.0

[U-Boot] [PATCH v1 1/2] WIP: serial: Introduce ->getinfo() callback

[Patrice CHOTARD]

Hi Andy

On 09/22/2018 03:05 PM, Andy Shevchenko wrote:
> TBD

You forgot to add a commit message ;-)

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/serial/ns16550.c       | 14 ++++++++++++++
>  drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++
>  include/common.h               |  3 +++
>  include/serial.h               | 14 ++++++++++++++
>  4 files changed, 52 insertions(+)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index f9041aa626..a996446936 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 = 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);
> @@ -440,6 +453,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)
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index ffdcae0931..4778302e8f 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -304,6 +304,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)
>  {
>  }
> @@ -421,6 +440,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 83b3bdc58d..0479f3eac1 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..c73477b079 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,10 @@ struct dm_serial_ops {
>  	 * @return 0 if OK, -ve on error
>  	 */
>  	int (*setconfig)(struct udevice *dev, uint serial_config);
> +	/**
> +	 * getinfo() - Get serial device information
> +	 */
> +	int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
>  };
>  
>  /**
> 

[U-Boot] [PATCH v1 1/2] WIP: serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 10:44 AM Patrice CHOTARD <patrice.chotard@st.com> wrote:
>
> Hi Andy
>
> On 09/22/2018 03:05 PM, Andy Shevchenko wrote:
> > TBD
>
> You forgot to add a commit message ;-)

Yes, cover letter explains why.
It's a halfbaked stuff in case someone is interested to continue.

For your convenience:
patch 1: introduces a hook to retrieve necessary data from UART driver
(in particular ns16550)
patch 2: adds SPCR data structures and uses hook from patch  1 to
generate the table

>
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/serial/ns16550.c       | 14 ++++++++++++++
> >  drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++
> >  include/common.h               |  3 +++
> >  include/serial.h               | 14 ++++++++++++++
> >  4 files changed, 52 insertions(+)
> >
> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> > index f9041aa626..a996446936 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 = 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);
> > @@ -440,6 +453,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)
> > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> > index ffdcae0931..4778302e8f 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -304,6 +304,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)
> >  {
> >  }
> > @@ -421,6 +440,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 83b3bdc58d..0479f3eac1 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..c73477b079 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,10 @@ struct dm_serial_ops {
> >        * @return 0 if OK, -ve on error
> >        */
> >       int (*setconfig)(struct udevice *dev, uint serial_config);
> > +     /**
> > +      * getinfo() - Get serial device information
> > +      */
> > +     int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
> >  };
> >
> >  /**
> >
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



--
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v1 1/2] WIP: serial: Introduce ->getinfo() callback

[Bin Meng]

Hi Andy,

On Sat, Sep 22, 2018 at 9:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> TBD
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/serial/ns16550.c       | 14 ++++++++++++++
>  drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++
>  include/common.h               |  3 +++
>  include/serial.h               | 14 ++++++++++++++
>  4 files changed, 52 insertions(+)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index f9041aa626..a996446936 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;

This one is tricky. Looks current 8250 DT binding does not have a
property to describe the address space to be MMIO or PIO. I am not
sure what's the best option here.

> +       info->reg_width = 8;

For this one, I believe we can use "reg-io-width" property from device tree.

> +       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);
> @@ -440,6 +453,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)
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index ffdcae0931..4778302e8f 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -304,6 +304,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)
>  {
>  }
> @@ -421,6 +440,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 83b3bdc58d..0479f3eac1 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..c73477b079 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,10 @@ struct dm_serial_ops {
>          * @return 0 if OK, -ve on error
>          */
>         int (*setconfig)(struct udevice *dev, uint serial_config);
> +       /**
> +        * getinfo() - Get serial device information
> +        */
> +       int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
>  };

Generally this patch looks OK to me.

BTW: why doesn't the kernel use the 'earlycon' command line?

Regards,
Bin

[U-Boot] [PATCH v1 1/2] WIP: serial: Introduce ->getinfo() callback

[Andy Shevchenko]

On Tue, Sep 25, 2018 at 10:24:42AM +0800, Bin Meng wrote:
> On Sat, Sep 22, 2018 at 9:05 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

I found a bit of time to look at this again.

> > +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;

> This one is tricky. Looks current 8250 DT binding does not have a
> property to describe the address space to be MMIO or PIO. I am not
> sure what's the best option here.

For now I suggest to leave it hard coded to MMIO.

> > +       info->reg_width = 8;

> For this one, I believe we can use "reg-io-width" property from device tree.

I will try this path.

> > +       info->reg_shift = plat->reg_shift;
> > +       info->reg_offset = plat->reg_offset;
> > +       info->addr = plat->base;
> > +       return 0;
> > +}

> Generally this patch looks OK to me.

Thanks! Still I have few amendments for the real submission of the series.

> BTW: why doesn't the kernel use the 'earlycon' command line?

Not all hardware supports it or supports it in a right way.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v1 2/2] WIP: x86: acpi: Generate SPCR table

[Andy Shevchenko]

TBD

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

diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h
index 95fae036f6..27435871b9 100644
--- a/arch/x86/include/asm/acpi_table.h
+++ b/arch/x86/include/asm/acpi_table.h
@@ -303,6 +303,31 @@ struct acpi_mcfg_mmconfig {
 /* ACPI global NVS structure */
 struct acpi_global_nvs;
 
+/* SPCR (Serial Port Console Redirection table) */
+struct __packed acpi_spcr {
+	struct acpi_table_header header;	/* Common ACPI table header */
+	u8 interface_type;	/* 0=full 16550, 1=subset of 16550 */
+	u8 reserved[3];
+	struct acpi_gen_regaddr serial_port;	/* The base address of the Serial Port register set */
+	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);
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index c6b2026613..551a78b11a 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,79 @@ 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)
+		debug("Can't get information of serial device: %d\n", 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;
+
+	/* Hard coded values for now */
+	spcr->parity = 0;
+	spcr->stop_bits = 1;
+
+	/* 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 +426,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 +515,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.18.0

[U-Boot] [PATCH v1 2/2] WIP: x86: acpi: Generate SPCR table

[Patrice CHOTARD]

On 09/22/2018 03:05 PM, Andy Shevchenko wrote:
> TBD

Same remark as Patch 1

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/include/asm/acpi_table.h | 25 ++++++++++
>  arch/x86/lib/acpi_table.c         | 82 +++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+)
> 
> diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h
> index 95fae036f6..27435871b9 100644
> --- a/arch/x86/include/asm/acpi_table.h
> +++ b/arch/x86/include/asm/acpi_table.h
> @@ -303,6 +303,31 @@ struct acpi_mcfg_mmconfig {
>  /* ACPI global NVS structure */
>  struct acpi_global_nvs;
>  
> +/* SPCR (Serial Port Console Redirection table) */
> +struct __packed acpi_spcr {
> +	struct acpi_table_header header;	/* Common ACPI table header */
> +	u8 interface_type;	/* 0=full 16550, 1=subset of 16550 */
> +	u8 reserved[3];
> +	struct acpi_gen_regaddr serial_port;	/* The base address of the Serial Port register set */
> +	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);
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index c6b2026613..551a78b11a 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,79 @@ 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)
> +		debug("Can't get information of serial device: %d\n", 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;
> +
> +	/* Hard coded values for now */
> +	spcr->parity = 0;
> +	spcr->stop_bits = 1;
> +
> +	/* 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 +426,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 +515,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 v1 2/2] WIP: x86: acpi: Generate SPCR table

[Bin Meng]

Hi Andy,

On Sat, Sep 22, 2018 at 9:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> TBD
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/include/asm/acpi_table.h | 25 ++++++++++
>  arch/x86/lib/acpi_table.c         | 82 +++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+)
>
> diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h
> index 95fae036f6..27435871b9 100644
> --- a/arch/x86/include/asm/acpi_table.h
> +++ b/arch/x86/include/asm/acpi_table.h
> @@ -303,6 +303,31 @@ struct acpi_mcfg_mmconfig {
>  /* ACPI global NVS structure */
>  struct acpi_global_nvs;
>
> +/* SPCR (Serial Port Console Redirection table) */
> +struct __packed acpi_spcr {
> +       struct acpi_table_header header;        /* Common ACPI table header */
> +       u8 interface_type;      /* 0=full 16550, 1=subset of 16550 */
> +       u8 reserved[3];
> +       struct acpi_gen_regaddr serial_port;    /* The base address of the Serial Port register set */
> +       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);
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index c6b2026613..551a78b11a 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,79 @@ 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)
> +               debug("Can't get information of serial device: %d\n", 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;
> +
> +       /* Hard coded values for now */
> +       spcr->parity = 0;
> +       spcr->stop_bits = 1;
> +
> +       /* No PCI devices for now */
> +       spcr->pci_device_id = 0xffff;
> +       spcr->pci_vendor_id = 0xffff;
> +

I see not every member of 'struct __packed acpi_spcr' is populated.
Are they not used by kernel? If kernel does not use it, it is used by
Windows?

> +       /* 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 +426,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 +515,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;
> --

Regards,
Bin

[U-Boot] [PATCH v1 2/2] WIP: x86: acpi: Generate SPCR table

[Andy Shevchenko]

On Tue, Sep 25, 2018 at 10:27:33AM +0800, Bin Meng wrote:
> On Sat, Sep 22, 2018 at 9:05 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

> > +/* SPCR (Serial Port Console Redirection table) */
> > +struct __packed acpi_spcr {
> > +       struct acpi_table_header header;        /* Common ACPI table header */
> > +       u8 interface_type;      /* 0=full 16550, 1=subset of 16550 */
> > +       u8 reserved[3];
> > +       struct acpi_gen_regaddr serial_port;    /* The base address of the Serial Port register set */
> > +       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;
> > +};

> I see not every member of 'struct __packed acpi_spcr' is populated.
> Are they not used by kernel? If kernel does not use it, it is used by
> Windows?

No idea about Windows, but there are two statements:
- *not all* should be filled, otherwise it would be contradictory
  (PCI vs. non-PCI device, for example)
- Linux doesn't use all of them, indeed

-- 
With Best Regards,
Andy Shevchenko