[U-Boot] [PATCH v2 0/5] ACPI: Generate SPCR table
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
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); };
/**
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);
};
/**
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
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
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.
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
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.
+};
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
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?
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.
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
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.
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; };
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; };
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?
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
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
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.
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)
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)
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.
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
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);
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
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)....
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;
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)....
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;
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)....
ret = serial_getinfo(&info);if (ret)spcr->interface_type = ACPI_DBG2_UNKNOWN;elsespcr->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?
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)....
ret = serial_getinfo(&info);if (ret)spcr->interface_type = ACPI_DBG2_UNKNOWN;elsespcr->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
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)....
Thanks for review.
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)....
This URL redirects to https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-po.... 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
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)....
This URL redirects to https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-po.... It looks that the redirected one has a better name that deserves to be mentioned in the commit message :)
Sure, thanks!
participants (5)
-
Alexander Graf -
Andy Shevchenko -
Andy Shevchenko -
Bin Meng -
Simon Glass