ACPI: Generate SPCR table
v2 cover 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
PATCH v2 1/5 serial: Introduce ->getinfo() callback
New callback will give a necessary information to fill up ACPI SPCR table, for example. Maybe used later for other purposes.
--- drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++ include/common.h | 3 +++ include/serial.h | 17 +++++++++++++++++ 3 files changed, 41 insertions(+)
drivers/serial/serial-uclass.c +21 -0
@@ -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) {
include/common.h +3 -0
@@ -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 );
include/serial.h +17 -0
@@ -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); };/**
Signature · 1 line
2.19.1
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 75 lines · 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 <[email protected]>--- drivers/serial/serial-uclass.c | 21 +++++++++++++++++++++ include/common.h | 3 +++ include/serial.h | 17 +++++++++++++++++ 3 files changed, 41 insertions(+)drivers/serial/serial-uclass.c +21
-0@@ -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) {include/common.h +3
-0@@ -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 );include/serial.h +0
-0
include/serial.h enum serial_stop { · line 114
Quoted 4 lines
include/serial.h +1
-0@@ -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
Quoted 27 lines
include/serial.h +16
-0+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); };/**
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 7 lines · Alexander Graf
Quoted 4 lines · Andy Shevchenko
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.
Quoted 8 lines
include/serial.h enum serial_stop { · line 122
Quoted 8 lines
include/serial.h +8
-0+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
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · Alexander Graf
Quoted 2 lines · Andy Shevchenko
New callback will give a necessary information to fill up ACPI SPCR table, for example. Maybe used later for other purposes.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+/* 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
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 22 lines · Alexander Graf
Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · Alexander Graf
Quoted 2 lines · Andy Shevchenko
New callback will give a necessary information to fill up ACPI SPCR table, for example. Maybe used later for other purposes.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+/* 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
Quoted 8 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 1/5 serial: Introduce ->getinfo() callback
In reply to Andy ShevchenkoHi Andy,
Quoted 11 lines · 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 <[email protected]>--- 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
drivers/serial/serial-uclass.c int serial_setconfig(uint config) · line 311
Quoted 9 lines
drivers/serial/serial-uclass.c +1
-0@@ -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)
Quoted 44 lines
drivers/serial/serial-uclass.c +20
-0+{ +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) {include/common.h +2
-0@@ -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 );
include/common.h void serial_puts (const char *); · line 362
Quoted 4 lines
include/common.h +1
-0@@ -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.
Quoted 8 lines
include/common.h +0
-0/* $(CPU)/speed.c */ int get_clocks (void );include/serial.h +0
-0
include/serial.h enum serial_stop { · line 117
Quoted 7 lines
include/serial.h +4
-0@@ -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
include/serial.h enum serial_stop { · line 121
Quoted 4 lines
include/serial.h +4
-0+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?
Quoted 6 lines
include/serial.h +2
-0+}; +/** * struct struct dm_serial_ops - Driver model serial operations *
include/serial.h struct dm_serial_ops { · line 220
Quoted 10 lines
include/serial.h +7
-0@@ -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
Quoted 6 lines
include/serial.h +0
-1};/** -- 2.19.1
Regards,
SImon
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 7 lines · Simon Glass
Quoted 3 lines · Andy Shevchenko
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?
Quoted 1 line
Please add a test to
test/dm/serial.c
Will do.
Quoted 5 lines
Quoted 1 line
+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!
Quoted 12 lines
include/serial.h enum serial_stop { · line 117
Quoted 4 lines
include/serial.h +4
-0+/* REVISIT: ACPI GAS specification implied */ +struct serial_device_info { +unsigned int baudrate; +u8 addr_space;/* 0 - MMIO, 1 - IO */ Please make this an enum
include/serial.h enum serial_stop { · line 121
Quoted 4 lines
include/serial.h +4
-0+u8 reg_width; +u8 reg_offset; +u8 reg_shift; +u64 addr;
Quoted 5 lines
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.
Quoted 1 line
Quoted 1 line
+};
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Hi Andy,
Quoted 10 lines · Andy Shevchenko
Quoted 7 lines · Simon Glass
Quoted 3 lines · Andy Shevchenko
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.
Quoted 31 lines
Quoted 2 lines
Please add a test to
test/dm/serial.cWill do.
Quoted 5 lines
Quoted 1 line
+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!
Quoted 12 lines
include/serial.h enum serial_stop { · line 117
Quoted 4 lines
include/serial.h +4
-0+/* REVISIT: ACPI GAS specification implied */ +struct serial_device_info { +unsigned int baudrate; +u8 addr_space;/* 0 - MMIO, 1 - IO */ Please make this an enum
include/serial.h enum serial_stop { · line 121
Quoted 4 lines
include/serial.h +4
-0+u8 reg_width; +u8 reg_offset; +u8 reg_shift; +u64 addr;Quoted 5 lines
ulong
Needs a struct comment as I don't know what most of these do.
What about parity, number of bits, etc?
Quoted 7 lines
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
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 5 lines · Simon Glass
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Simon Glass
On 15 November 2018 at 09:58, Andy Shevchenko
<[email protected]> wrote:
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
+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?
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 1/5 serial: Introduce ->getinfo() callback
In reply to Simon GlassQuoted 9 lines · Simon Glass
include/serial.h enum serial_stop { · line 117
Quoted 4 lines · Andy Shevchenko
include/serial.h +4
-0+/* 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.
Quoted 7 lines
include/serial.h enum serial_stop { · line 121
Quoted 5 lines
include/serial.h +4
-0+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.
Quoted 1 line
Needs a struct comment as I don't know what most of these do.
OK.
Quoted 1 line
What about parity, number of bits, etc?
As discussed before, it will be filled thru getconfig(). Though, I would add necessary members explicitly.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 19 lines · Andy Shevchenko
Quoted 9 lines · Simon Glass
include/serial.h enum serial_stop { · line 117
Quoted 4 lines · Andy Shevchenko
include/serial.h +4
-0+/* 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.
Quoted 6 lines
Quoted 5 lines
include/serial.h +4
-0+u8 reg_width; +u8 reg_offset; +u8 reg_shift; +u64 addr;
Quoted 4 lines
Quoted 1 line
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
PATCH v2 1/5 serial: Introduce ->getinfo() callback
Quoted 12 lines · Alexander Graf
Quoted 10 lines · Andy Shevchenko
Quoted 6 lines · Simon Glass
Quoted 1 line · Andy Shevchenko
+ 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.
Quoted 6 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 2/5 serial: ns16550: Read reg-io-width from device tree
In reply to Andy ShevchenkoCache the value of the reg-io-width property for the future use.
--- drivers/serial/ns16550.c | 1 + include/ns16550.h | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)
drivers/serial/ns16550.c +1 -0
@@ -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) {
include/ns16550.h +3 -1
@@ -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; };
Signature · 1 line
2.19.1
PATCH v2 2/5 serial: ns16550: Read reg-io-width from device tree
Quoted 29 lines · Andy Shevchenko
Cache the value of the reg-io-width property for the future use.
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/serial/ns16550.c | 1 + include/ns16550.h | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)drivers/serial/ns16550.c +1
-0@@ -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) {include/ns16550.h +1
-0@@ -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)
include/ns16550.h line 61
Quoted 10 lines
include/ns16550.h +2
-1* @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
Quoted 4 lines
include/ns16550.h +0
-0u32 fcr; };
PATCH v2 2/5 serial: ns16550: Read reg-io-width from device tree
Quoted 11 lines · Alexander Graf
include/ns16550.h line 61
Quoted 9 lines · Andy Shevchenko
Cache the value of the reg-io-width property for the future use.
struct ns16550_platdata { unsigned long base;include/ns16550.h +2
-1+int reg_width;int reg_shift; -int clock;int reg_offset; +int clock;
Quoted 1 line
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?
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 2/5 serial: ns16550: Read reg-io-width from device tree
Quoted 12 lines · Andy Shevchenko
Quoted 11 lines · Alexander Graf
Quoted 9 lines · Andy Shevchenko
Cache the value of the reg-io-width property for the future use.
struct ns16550_platdata { unsigned long base;include/ns16550.h +2
-1+int reg_width;int reg_shift; -int clock;int reg_offset; +int clock;
Quoted 6 lines
Quoted 1 line
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_infoand 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 :).
Quoted 1 line
P.S. Why to overquoute?
What do you mean? Why I don't remove useless parts of the quote? Lazyness I suppose.
Alex
PATCH v2 2/5 serial: ns16550: Read reg-io-width from device tree
In reply to Andy ShevchenkoHi Andy,
Quoted 13 lines · Andy Shevchenko
Cache the value of the reg-io-width property for the future use.
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/serial/ns16550.c | 1 + include/ns16550.h | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)drivers/serial/ns16550.c +0
-0
drivers/serial/ns16550.c int ns16550_serial_ofdata_to_platdata(struct udevice *dev) · line 412
Quoted 5 lines
drivers/serial/ns16550.c +1
-0@@ -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.
Quoted 11 lines
drivers/serial/ns16550.c +0
-0err =clk_get_by_index (dev,0 , &clk);if (!err) {include/ns16550.h +1
-0@@ -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)
include/ns16550.h line 61
Quoted 10 lines
include/ns16550.h +2
-1* @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.
Quoted 6 lines
include/ns16550.h +0
-1u32 fcr; }; --2.19.1
Regards,
Simon
PATCH v2 2/5 serial: ns16550: Read reg-io-width from device tree
Quoted 11 lines · Simon Glass
Quoted 6 lines · Andy Shevchenko
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.
Quoted 5 lines
include/ns16550.h line 61
Quoted 3 lines
include/ns16550.h +1
-1-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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 3/5 serial: ns16550: Provide ->getinfo() implementation
In reply to Andy ShevchenkoNew callback will supply necessary information, for example, to ACPI SPCR table.
--- drivers/serial/ns16550.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/serial/ns16550.c +14 -0
@@ -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 )
Signature · 1 line
2.19.1
PATCH v2 3/5 serial: ns16550: Provide ->getinfo() implementation
Quoted 13 lines · Andy Shevchenko
New callback will supply necessary information, for example, to ACPI SPCR table.
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/serial/ns16550.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)drivers/serial/ns16550.c +0
-0
drivers/serial/ns16550.c static int ns16550_serial_setbrg(struct udevice *dev, int baudrate) · line 337
Quoted 4 lines
drivers/serial/ns16550.c +1
-0@@ -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
Quoted 24 lines
drivers/serial/ns16550.c +13
-0+{ +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 )
PATCH v2 3/5 serial: ns16550: Provide ->getinfo() implementation
Quoted 7 lines · Alexander Graf
Quoted 4 lines · Andy Shevchenko
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 3/5 serial: ns16550: Provide ->getinfo() implementation
HI Andy,
Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · Alexander Graf
Quoted 2 lines · Andy Shevchenko
New callback will supply necessary information, for example, to ACPI SPCR table.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+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
PATCH v2 4/5 x86: acpi: Add SPCR table description
In reply to Andy ShevchenkoAdd 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.
--- arch/x86/include/asm/acpi_table.h | 49 +++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
arch/x86/include/asm/acpi_table.h +49 -0
@@ -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);
Signature · 1 line
2.19.1
PATCH v2 4/5 x86: acpi: Add SPCR table description
Quoted 11 lines · Andy Shevchenko
Add SPCR table description as it provided in Linux kernel.
Port subtype for
ACPI_DBG2_SERIAL_PORTis used as an interface type in SPCR. Thus, provide a set of definitions to be utilized later.Signed-off-by Andy Shevchenko <[email protected]>--- arch/x86/include/asm/acpi_table.h | 49 +++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
PATCH v2 5/5 x86: acpi: Generate SPCR table
In reply to Andy ShevchenkoMicrosoft 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
--- arch/x86/include/asm/acpi_table.h | 2 + arch/x86/lib/acpi_table.c | 85 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)
arch/x86/include/asm/acpi_table.h +2 -0
@@ -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;
arch/x86/lib/acpi_table.c +85 -0
109 lines not on this page · raw message
Signature · 1 line
2.19.1
PATCH v2 5/5 x86: acpi: Generate SPCR table
Quoted 47 lines · 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 <[email protected]>--- arch/x86/include/asm/acpi_table.h | 2 + arch/x86/lib/acpi_table.c | 85 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)arch/x86/include/asm/acpi_table.h +2
-0@@ -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;arch/x86/lib/acpi_table.c +8
-0@@ -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; +
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 358
Quoted 10 lines
arch/x86/lib/acpi_table.c +10
-0+/* 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.
Quoted 6 lines
arch/x86/lib/acpi_table.c +6
-0+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 ;
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 374
Quoted 10 lines
arch/x86/lib/acpi_table.c +10
-0+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.
Quoted 15 lines
arch/x86/lib/acpi_table.c +15
-0+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 ;
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 399
Quoted 10 lines
arch/x86/lib/acpi_table.c +10
-0+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.
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 408
Quoted 9 lines
arch/x86/lib/acpi_table.c +9
-0+ 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
Quoted 35 lines
arch/x86/lib/acpi_table.c +17
-0+ +/* 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;
PATCH v2 5/5 x86: acpi: Generate SPCR table
Quoted 6 lines · Alexander Graf
Quoted 4 lines · 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
Quoted 8 lines
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 358
Quoted 5 lines
arch/x86/lib/acpi_table.c +5
-0+ 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.
Quoted 3 lines
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?
Quoted 2 lines
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?
Quoted 3 lines
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 377
Quoted 3 lines
arch/x86/lib/acpi_table.c +3
-0+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.
Quoted 9 lines
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 399
Quoted 7 lines
arch/x86/lib/acpi_table.c +7
-0+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.
Quoted 7 lines
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 408
Quoted 3 lines
arch/x86/lib/acpi_table.c +3
-0+/* 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?
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 5/5 x86: acpi: Generate SPCR table
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Alexander Graf
Quoted 10 lines · 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
arch/x86/lib/acpi_table.c +5
-0+ ret =serial_getinfo (&info); +if (ret) + spcr->interface_type =ACPI_DBG2_UNKNOWN ; +else + spcr->interface_type =ACPI_DBG2_16550_COMPATIBLE ;
Quoted 4 lines
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 5 lines
Quoted 3 lines
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 ;).
Quoted 17 lines
Quoted 4 lines
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 377
Quoted 4 lines
arch/x86/lib/acpi_table.c +3
-0+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.Quoted 8 lines
Quoted 7 lines
arch/x86/lib/acpi_table.c +7
-0+break ; +default : + access_size =ACPI_ACCESS_SIZE_UNDEFINED ; +break ; + } + + spcr->serial_port.space_id =ACPI_ADDRESS_SPACE_MEMORY ;
Quoted 4 lines
Quoted 1 line
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.
Quoted 10 lines
Quoted 8 lines
arch/x86/lib/acpi_table.c static void acpi_create_mcfg(struct acpi_mcfg *mcfg) · line 408
Quoted 4 lines
arch/x86/lib/acpi_table.c +3
-0+/* 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
PATCH v2 5/5 x86: acpi: Generate SPCR table
Quoted 7 lines · Alexander Graf
Quoted 6 lines · Andy Shevchenko
Quoted 5 lines · Alexander Graf
Quoted 4 lines · 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
Thanks for review.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 5/5 x86: acpi: Generate SPCR table
In reply to Andy ShevchenkoHi Andy,
Quoted 6 lines · 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
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 :)
Quoted 6 lines
Signed-off-by Andy Shevchenko <[email protected]>--- arch/x86/include/asm/acpi_table.h | 2 + arch/x86/lib/acpi_table.c | 85 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)
[snip]
Regards,
Bin
PATCH v2 5/5 x86: acpi: Generate SPCR table
Quoted 8 lines · Bin Meng
Hi Andy,
Quoted 4 lines · Andy Shevchenko
Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. Let's provide it in U-Boot.
Quoted 7 lines
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!
Signature · 2 lines
With Best Regards, Andy Shevchenko