serial: ns16550: Add register shift variable

PATCH Superseded Felix Brack 5 participants 13 messages Jul 17, 2018

Share & tools

Felix Brack

author

2018-07-17 07:02 UTC #0 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.

The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.

Signed-off-by Felix Brack <[email protected]>

---

Changes in v2:
- clarify variable usage
- set default value to 2 for AM33XX SoC

 drivers/serial/Kconfig   | 15 +++++++++++++++
 drivers/serial/ns16550.c |  3 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)

drivers/serial/Kconfig +15 -0

index 766e5ce..7eb3c6f 100644
@@ -530,6 +530,21 @@ config SYS_NS16550
 	  be used. It can be a constant or a function to get clock, eg,
 	  get_serial_clock().
 
+config SYS_NS16550_REG_SHIFT
+	int "Amount of bits to shift register offsets left"
+	default 2 if AM33XX
+	default 0
+	depends on SYS_NS16550
+	help
+	  Use this to specify the amount of bits to shift device register
+	  offsets to the left. The resulting register offset is calculate as
+	  follows: "reg offset" << SYS_NS16550_REG_SHIFT. If, for example,
+	  the device register offsets are 0x00, 0x04, 0x08, 0x0C and so forth
+	  than set this to 2.
+	  In case of AM33XX SoC the default value is 2, 0 otherwise. Note
+	  that a <reg-shift> property defined in a UART node of the device
+	  tree will always take precedence.
+
 config INTEL_MID_SERIAL
 	bool "Intel MID platform UART support"
 	depends on DM_SERIAL && OF_CONTROL

drivers/serial/ns16550.c +2 -1

index 9c80090..9ff6dbe 100644
@@ -442,7 +442,8 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 #endif
 
 	plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0);
-	plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0);
+	plat->reg_shift = dev_read_u32_default(dev, "reg-shift",
+					       CONFIG_SYS_NS16550_REG_SHIFT);
 
 	err = clk_get_by_index(dev, 0, &clk);
 	if (!err) {

Signature · 1 line

2.7.4

Lokesh Vutla

2018-07-17 07:55 UTC #1 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 32 lines · Felix Brack

This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.
The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.
Signed-off-by Felix Brack <[email protected]>
---
Changes in v2:
- clarify variable usage
- set default value to 2 for AM33XX SoC
 drivers/serial/Kconfig   | 15 +++++++++++++++
 drivers/serial/ns16550.c |  3 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)
drivers/serial/Kconfig +0 -0
index 766e5ce..7eb3c6f 100644

drivers/serial/Kconfig config SYS_NS16550 · line 535

Quoted 6 lines

drivers/serial/Kconfig +3 -0

@@ -530,6 +530,21 @@ config SYS_NS16550
 	  be used. It can be a constant or a function to get clock, eg,
 	  get_serial_clock().
 
+config SYS_NS16550_REG_SHIFT
+	int "Amount of bits to shift register offsets left"
+	default 2 if AM33XX

Can you make it default for ARCH_OMAP2PLUS?

Thanks and regards,
Lokesh

Quoted 30 lines

drivers/serial/Kconfig +12 -0

+	default 0
+	depends on SYS_NS16550
+	help
+	  Use this to specify the amount of bits to shift device register
+	  offsets to the left. The resulting register offset is calculate as
+	  follows: "reg offset" << SYS_NS16550_REG_SHIFT. If, for example,
+	  the device register offsets are 0x00, 0x04, 0x08, 0x0C and so forth
+	  than set this to 2.
+	  In case of AM33XX SoC the default value is 2, 0 otherwise. Note
+	  that a <reg-shift> property defined in a UART node of the device
+	  tree will always take precedence.
+
 config INTEL_MID_SERIAL
 	bool "Intel MID platform UART support"
 	depends on DM_SERIAL && OF_CONTROL

drivers/serial/ns16550.c +2 -1

index 9c80090..9ff6dbe 100644
@@ -442,7 +442,8 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 #endif
 
 	plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0);
-	plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0);
+	plat->reg_shift = dev_read_u32_default(dev, "reg-shift",
+					       CONFIG_SYS_NS16550_REG_SHIFT);
 
 	err = clk_get_by_index(dev, 0, &clk);
 	if (!err) {

Alexander Graf

2018-07-17 09:12 UTC #2 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 18 lines · Lokesh Vutla

Quoted 16 lines · Felix Brack
This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.
The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.

Quoted 1 line

Quoted 1 line
Signed-off-by Felix Brack <[email protected]>

NAK. Please determine the shift value from the compatible instead. Ideally you would create a subclass of the ns16550 device class and set reg_shift in there.

Take a look at drivers/serial/serial_rockchip.c for an example.

Alex

Lokesh Vutla

2018-07-17 09:21 UTC #3 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 19 lines · Alexander Graf

Quoted 18 lines · Lokesh Vutla
Quoted 16 lines · Felix Brack
This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.
The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.

Quoted 5 lines

Quoted 1 line
Quoted 1 line
Signed-off-by Felix Brack <[email protected]>
NAK. Please determine the shift value from the compatible instead. Ideally you would create a subclass of the ns16550 device class and set reg_shift in there.

There was a separate driver for omap_serial initially but was deleted by the below commit:

commit c7b9686d5d482c8e952598841ea467e6ec0ec0de

Author: Thomas Chou <[email protected]>
Date:   Thu Nov 19 21:48:12 2015 +0800

ns16550: unify serial_omap

Unify serial_omap, and use the generic binding.

Signed-off-by Thomas Chou <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Acked-by Simon Glass <[email protected]>

Thanks and regards,
Lokesh

Alexander Graf

2018-07-17 09:27 UTC #4 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 31 lines · Lokesh Vutla

Quoted 18 lines · Alexander Graf
Quoted 17 lines · Lokesh Vutla
Quoted 16 lines · Felix Brack
This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.
The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Signed-off-by Felix Brack <[email protected]>
NAK. Please determine the shift value from the compatible instead. Ideally you would create a subclass of the ns16550 device class and set reg_shift in there.
There was a separate driver for omap_serial initially but was deleted by the below commit:
commit c7b9686d5d482c8e952598841ea467e6ec0ec0de
Author: Thomas Chou <[email protected]>
Date:   Thu Nov 19 21:48:12 2015 +0800

Quoted 7 lines

ns16550: unify serial_omap
Unify serial_omap, and use the generic binding.
Signed-off-by Thomas Chou <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Acked-by Simon Glass <[email protected]>

Sounds like that wasn't a terribly smart move :).

If you really don't want a separate driver (and I'm not sure why you wouldn't), declare a separate PORT for ti,omap3-uart in ns16550_serial_ids and set the shift value based on that in ns16550_serial_ofdata_to_platdata() if DT doesn't describe one.

Alex

Lokesh Vutla

2018-07-17 09:31 UTC #5 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 40 lines · Alexander Graf

Quoted 31 lines · Lokesh Vutla
Quoted 18 lines · Alexander Graf
Quoted 17 lines · Lokesh Vutla
Quoted 16 lines · Felix Brack
This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.
The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Signed-off-by Felix Brack <[email protected]>
NAK. Please determine the shift value from the compatible instead. Ideally you would create a subclass of the ns16550 device class and set reg_shift in there.
There was a separate driver for omap_serial initially but was deleted by the below commit:
commit c7b9686d5d482c8e952598841ea467e6ec0ec0de
Author: Thomas Chou <[email protected]>
Date:   Thu Nov 19 21:48:12 2015 +0800
Quoted 7 lines
     ns16550: unify serial_omap
     Unify serial_omap, and use the generic binding.
Signed-off-by Thomas Chou <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Acked-by Simon Glass <[email protected]>

Quoted 6 lines

Sounds like that wasn't a terribly smart move :).
If you really don't want a separate driver (and I'm not sure why you wouldn't), declare a separate PORT for ti,omap3-uart in ns16550_serial_ids and set the shift value based on that in ns16550_serial_ofdata_to_platdata() if DT doesn't describe one.

I do prefer a separate driver. But I don't want to see a patch again reverting the driver :P.

Tom, Simon,
What do you prefer?

Thanks and regards,
Lokesh

Felix Brack

author

2018-07-17 12:12 UTC #6 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

In reply to Alexander Graf

Quoted 40 lines · Alexander Graf

Quoted 31 lines · Lokesh Vutla
Quoted 18 lines · Alexander Graf
Quoted 17 lines · Lokesh Vutla
Quoted 16 lines · Felix Brack
This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.
The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Signed-off-by Felix Brack <[email protected]>
NAK. Please determine the shift value from the compatible instead. Ideally you would create a subclass of the ns16550 device class and set reg_shift in there.
There was a separate driver for omap_serial initially but was deleted by the below commit:
commit c7b9686d5d482c8e952598841ea467e6ec0ec0de
Author: Thomas Chou <[email protected]>
Date:   Thu Nov 19 21:48:12 2015 +0800
Quoted 7 lines
     ns16550: unify serial_omap
     Unify serial_omap, and use the generic binding.
Signed-off-by Thomas Chou <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Acked-by Simon Glass <[email protected]>

Quoted 7 lines

Sounds like that wasn't a terribly smart move :).
If you really don't want a separate driver (and I'm not sure why you wouldn't), declare a separate PORT for ti,omap3-uart in ns16550_serial_ids and set the shift value based on that in ns16550_serial_ofdata_to_platdata() if DT doesn't describe one.

Adding a separate driver when you can use a generic one is of no benefit, it just bloats the source code.

Adding a separate PORT in ns16550_serial_ids for a particular architecture, platform or SoC would be an option. However the patch I posted is much more generic as it offers to set the reg-shift property for no matter what architecture, platform or SoC. It can also easily be extended by adding more conditional defaults to the Kconfig file.

regards Felix

Alexey Brodkin

2018-07-17 12:45 UTC #7 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Hi Felix,

Quoted 10 lines

-----Original Message-----
From: Felix Brack [mailto:fb at ltec.ch]
Sent: Tuesday, July 17, 2018 3:13 PM
To: Alexander Graf <[email protected]>; Lokesh Vutla <[email protected]>; u-boot at lists.denx.de
Cc: Wolfgang Denk <[email protected]>; Tom Rini <[email protected]>; Marek Vasut <[email protected]>; Patrice Chotard <[email protected]>; Michal Simek <[email protected]>; Simon Glass <[email protected]>; Alexey Brodkin <[email protected]>; Bin Meng <[email protected]>; Ley Foon Tan <[email protected]>; Patrick Delaunay <[email protected]>; Mario Six <[email protected]>; Stefan Roese <[email protected]>; Bernhard Messerklinger <[email protected]>
Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable

[snip]

Quoted 5 lines

Adding a separate PORT in ns16550_serial_ids for a particular architecture, platform or SoC would be an option. However the patch I posted is much more generic as it offers to set the reg-shift property for no matter what architecture, platform or SoC. It can also easily be extended by adding more conditional defaults to the Kconfig file.

I'd say we're dealing with just one corner-case here.
If I understand a concept of Device Tree it is supposed to describe your hardware. Thus if reg shift exists in your HW it should be explicitly mentioned in your .dts. If for some [historical] reason you have to deal with "incorrect" .dts then I'd prefer to have mentioned quirk with a separate PORT in ns16550_serial_ids instead of adding yet another Kconfig option.

Again if OMAP UART is just another flavor of standard 16550 serial port maybe it's a good idea to convert Linux's "drivers/tty/serial/omap-serial.c" to something like "drivers/tty/serial/8250/8250_omap.c" with simultaneous fix of .dtsi?

-Alexey

Tom Rini

2018-07-17 13:21 UTC #8 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 21 lines · Alexey Brodkin

Hi Felix,
Quoted 10 lines
-----Original Message-----
From: Felix Brack [mailto:fb at ltec.ch]
Sent: Tuesday, July 17, 2018 3:13 PM
To: Alexander Graf <[email protected]>; Lokesh Vutla <[email protected]>; u-boot at lists.denx.de
Cc: Wolfgang Denk <[email protected]>; Tom Rini <[email protected]>; Marek Vasut <[email protected]>; Patrice Chotard <[email protected]>; Michal Simek <[email protected]>; Simon Glass <[email protected]>; Alexey Brodkin <[email protected]>; Bin Meng <[email protected]>; Ley Foon Tan <[email protected]>; Patrick Delaunay <[email protected]>; Mario Six <[email protected]>; Stefan Roese <[email protected]>; Bernhard Messerklinger <[email protected]>
Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
[snip]
Quoted 5 lines
Adding a separate PORT in ns16550_serial_ids for a particular architecture, platform or SoC would be an option. However the patch I posted is much more generic as it offers to set the reg-shift property for no matter what architecture, platform or SoC. It can also easily be extended by adding more conditional defaults to the Kconfig file.

Quoted 6 lines

I'd say we're dealing with just one corner-case here.
If I understand a concept of Device Tree it is supposed to describe your hardware. Thus if reg shift exists in your HW it should be explicitly mentioned in your .dts. If for some [historical] reason you have to deal with "incorrect" .dts then I'd prefer to have mentioned quirk with a separate PORT in ns16550_serial_ids instead of adding yet another Kconfig option.

So, this is part of the problem I suppose. I don't know _why_ we can't just add the correct and valid reg-shift property to the dtsi file in Linux and be done with it. Then the U-Boot driver would work because we parse that property.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180717/086be57d/attachment.sig>

Felix Brack

author

2018-07-17 13:34 UTC #9 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 29 lines · Tom Rini

Quoted 21 lines · Alexey Brodkin
Hi Felix,
Quoted 10 lines
-----Original Message-----
From: Felix Brack [mailto:fb at ltec.ch]
Sent: Tuesday, July 17, 2018 3:13 PM
To: Alexander Graf <[email protected]>; Lokesh Vutla <[email protected]>; u-boot at lists.denx.de
Cc: Wolfgang Denk <[email protected]>; Tom Rini <[email protected]>; Marek Vasut <[email protected]>; Patrice Chotard <[email protected]>; Michal Simek <[email protected]>; Simon Glass <[email protected]>; Alexey Brodkin <[email protected]>; Bin Meng <[email protected]>; Ley Foon Tan <[email protected]>; Patrick Delaunay <[email protected]>; Mario Six <[email protected]>; Stefan Roese <[email protected]>; Bernhard Messerklinger <[email protected]>
Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
[snip]
Quoted 5 lines
Adding a separate PORT in ns16550_serial_ids for a particular architecture, platform or SoC would be an option. However the patch I posted is much more generic as it offers to set the reg-shift property for no matter what architecture, platform or SoC. It can also easily be extended by adding more conditional defaults to the Kconfig file.
Quoted 6 lines
I'd say we're dealing with just one corner-case here.
If I understand a concept of Device Tree it is supposed to describe your hardware. Thus if reg shift exists in your HW it should be explicitly mentioned in your .dts. If for some [historical] reason you have to deal with "incorrect" .dts then I'd prefer to have mentioned quirk with a separate PORT in ns16550_serial_ids instead of adding yet another Kconfig option.

Quoted 5 lines

So, this is part of the problem I suppose. I don't know _why_ we can't just add the correct and valid reg-shift property to the dtsi file in Linux and be done with it. Then the U-Boot driver would work because we parse that property.

The only reason I can see why the <reg-shift> property "can't be added" to the Linux .dtsi file is that there is nothing broken in Linux. Hence we would actually ask Linux to add a property required by U-Boot.

This is exactly the reason for my RFC suggesting other solutions as the U-Boot build system is able to use a SoC and even a board specific .dtsi file.

regards Felix

Alexander Graf

2018-07-17 13:38 UTC #10 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 28 lines · Felix Brack

Quoted 26 lines · Tom Rini
Quoted 25 lines · Alexey Brodkin
Hi Felix,
Quoted 10 lines
-----Original Message-----
From: Felix Brack [mailto:fb at ltec.ch]
Sent: Tuesday, July 17, 2018 3:13 PM
To: Alexander Graf <[email protected]>; Lokesh Vutla <[email protected]>; u-boot at lists.denx.de
Cc: Wolfgang Denk <[email protected]>; Tom Rini <[email protected]>; Marek Vasut <[email protected]>; Patrice Chotard <[email protected]>; Michal Simek <[email protected]>; Simon Glass <[email protected]>; Alexey Brodkin <[email protected]>; Bin Meng <[email protected]>; Ley Foon Tan <[email protected]>; Patrick Delaunay <[email protected]>; Mario Six <[email protected]>; Stefan Roese <[email protected]>; Bernhard Messerklinger <[email protected]>
Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
[snip]
Quoted 5 lines
Adding a separate PORT in ns16550_serial_ids for a particular architecture, platform or SoC would be an option. However the patch I posted is much more generic as it offers to set the reg-shift property for no matter what architecture, platform or SoC. It can also easily be extended by adding more conditional defaults to the Kconfig file.
I'd say we're dealing with just one corner-case here.
If I understand a concept of Device Tree it is supposed to describe your hardware. Thus if reg shift exists in your HW it should be explicitly mentioned in your .dts. If for some [historical] reason you have to deal with "incorrect" .dts then I'd prefer to have mentioned quirk with a separate PORT in ns16550_serial_ids instead of adding yet another Kconfig option.

Quoted 8 lines

Quoted 5 lines
So, this is part of the problem I suppose. I don't know _why_ we can't just add the correct and valid reg-shift property to the dtsi file in Linux and be done with it. Then the U-Boot driver would work because we parse that property.
The only reason I can see why the <reg-shift> property "can't be added" to the Linux .dtsi file is that there is nothing broken in Linux. Hence we would actually ask Linux to add a property required by U-Boot.

In the DT you can describe hardware specifics by either a different compatible string or by additional properties on a generic compatible. So

compatible = "ti,omap3-uart";

is correct, as is

compatible = "ns16550";
reg-shift = 2;

There might be more that gets implied by the omap3-uart compatible that I'm not aware of, but in a nutshell it's all about whether you use a generic compatible string or a device specific one. Linux went for the specific one, so it didn't need reg-shift. U-Boot (incorrectly) treats the device specific compatible string as generic which is why you see the failure.

Alex

Tom Rini

2018-07-17 13:45 UTC #11 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

Quoted 44 lines · Alexander Graf

Quoted 27 lines · Felix Brack
Quoted 25 lines · Tom Rini
Quoted 24 lines · Alexey Brodkin
Hi Felix,
Quoted 10 lines
-----Original Message-----
From: Felix Brack [mailto:fb at ltec.ch]
Sent: Tuesday, July 17, 2018 3:13 PM
To: Alexander Graf <[email protected]>; Lokesh Vutla <[email protected]>; u-boot at lists.denx.de
Cc: Wolfgang Denk <[email protected]>; Tom Rini <[email protected]>; Marek Vasut <[email protected]>; Patrice Chotard <[email protected]>; Michal Simek <[email protected]>; Simon Glass <[email protected]>; Alexey Brodkin <[email protected]>; Bin Meng <[email protected]>; Ley Foon Tan <[email protected]>; Patrick Delaunay <[email protected]>; Mario Six <[email protected]>; Stefan Roese <[email protected]>; Bernhard Messerklinger <[email protected]>
Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
[snip]
Quoted 5 lines
Adding a separate PORT in ns16550_serial_ids for a particular architecture, platform or SoC would be an option. However the patch I posted is much more generic as it offers to set the reg-shift property for no matter what architecture, platform or SoC. It can also easily be extended by adding more conditional defaults to the Kconfig file.
I'd say we're dealing with just one corner-case here.
If I understand a concept of Device Tree it is supposed to describe your hardware. Thus if reg shift exists in your HW it should be explicitly mentioned in your .dts. If for some [historical] reason you have to deal with "incorrect" .dts then I'd prefer to have mentioned quirk with a separate PORT in ns16550_serial_ids instead of adding yet another Kconfig option.
Quoted 8 lines
Quoted 5 lines
So, this is part of the problem I suppose. I don't know _why_ we can't just add the correct and valid reg-shift property to the dtsi file in Linux and be done with it. Then the U-Boot driver would work because we parse that property.
The only reason I can see why the <reg-shift> property "can't be added" to the Linux .dtsi file is that there is nothing broken in Linux. Hence we would actually ask Linux to add a property required by U-Boot.
In the DT you can describe hardware specifics by either a different compatible string or by additional properties on a generic compatible. So
compatible = "ti,omap3-uart";
is correct, as is

Quoted 8 lines

compatible = "ns16550";
reg-shift = 2;
There might be more that gets implied by the omap3-uart compatible that I'm not aware of, but in a nutshell it's all about whether you use a generic compatible string or a device specific one. Linux went for the specific one, so it didn't need reg-shift. U-Boot (incorrectly) treats the device specific compatible string as generic which is why you see the failure.

So, to answer my own questions, drivers/tty/serial/8250/8250_omap.c takes the compatible and forces the reg-shift. Honestly I assume they do this because there's other things being handled in those SoC-specific files in there. I guess we need to follow suit, sigh.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180717/1bc01fda/attachment.sig>

Felix Brack

author

2018-07-17 12:22 UTC #12 lore raw

PATCH v2 1/1 serial: ns16550: Add register shift variable

In reply to Lokesh Vutla

Hello Lokesh,

Quoted 38 lines · Lokesh Vutla

Quoted 35 lines · Felix Brack
This patch adds a new Kconfig variable that allows setting the register offset shift value for the ns16550 driver to some other value then 0 if not defined by the DT. All credit for this patch goes to Lokesh Vutla as it was his idea.
The motivation for writing this patch originates in the effort of synchronizing U-Boot DT to Linux DT for am33xx SOCs. The current am33xx.dtsi file from U-Boot defines the <reg-shift> property for all UART nodes. The actual (4.18+) am33xx.dtsi file from Linux does not define <reg-shift> anymore. To prevent (probably difficult) changes in many .dts and .dtsi files once the synchronization is done, one can use this new variable. For the pdu001 board, for example, SYS_NS16550_REG_SHIFT is set to 2; no need to clutter U-Boot and board specific dts files with <reg-shift> properties.
Signed-off-by Felix Brack <[email protected]>
---
Changes in v2:
- clarify variable usage
- set default value to 2 for AM33XX SoC
 drivers/serial/Kconfig   | 15 +++++++++++++++
 drivers/serial/ns16550.c |  3 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)
drivers/serial/Kconfig +0 -0
index 766e5ce..7eb3c6f 100644
@@ -530,6 +530,21 @@ config SYS_NS16550
 	  be used. It can be a constant or a function to get clock, eg,
 	  get_serial_clock().
 

Quoted 6 lines

drivers/serial/Kconfig config SYS_NS16550 · line 535
Quoted 3 lines
drivers/serial/Kconfig +3 -0
+config SYS_NS16550_REG_SHIFT
+	int "Amount of bits to shift register offsets left"
+	default 2 if AM33XX
Can you make it default for ARCH_OMAP2PLUS?

Yes, I will for v3. I presume you verified this for all addition (none AM33XX) SoCs of OMAP2+ arch.

regards Felix

Quoted 34 lines

Thanks and regards,
Lokesh

Quoted 30 lines

drivers/serial/Kconfig +12 -0

+	default 0
+	depends on SYS_NS16550
+	help
+	  Use this to specify the amount of bits to shift device register
+	  offsets to the left. The resulting register offset is calculate as
+	  follows: "reg offset" << SYS_NS16550_REG_SHIFT. If, for example,
+	  the device register offsets are 0x00, 0x04, 0x08, 0x0C and so forth
+	  than set this to 2.
+	  In case of AM33XX SoC the default value is 2, 0 otherwise. Note
+	  that a <reg-shift> property defined in a UART node of the device
+	  tree will always take precedence.
+
 config INTEL_MID_SERIAL
 	bool "Intel MID platform UART support"
 	depends on DM_SERIAL && OF_CONTROL

drivers/serial/ns16550.c +2 -1

index 9c80090..9ff6dbe 100644
@@ -442,7 +442,8 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 #endif
 
 	plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0);
-	plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0);
+	plat->reg_shift = dev_read_u32_default(dev, "reg-shift",
+					       CONFIG_SYS_NS16550_REG_SHIFT);
 
 	err = clk_get_by_index(dev, 0, &clk);
 	if (!err) {