[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[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 <fb@ltec.ch>
---

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(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 766e5ce..7eb3c6f 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -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
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 9c80090..9ff6dbe 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -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) {
-- 
2.7.4

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Lokesh Vutla]

On Tuesday 17 July 2018 12:32 PM, Felix Brack wrote:
> 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 <fb@ltec.ch>
> ---
> 
> 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(-)
> 
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 766e5ce..7eb3c6f 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -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


> +	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
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 9c80090..9ff6dbe 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -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) {
> 

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Alexander Graf]

On 07/17/2018 09:55 AM, Lokesh Vutla wrote:
>
> On Tuesday 17 July 2018 12:32 PM, Felix Brack wrote:
>> 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 <fb@ltec.ch>

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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Lokesh Vutla]

On Tuesday 17 July 2018 02:42 PM, Alexander Graf wrote:
> On 07/17/2018 09:55 AM, Lokesh Vutla wrote:
>>
>> On Tuesday 17 July 2018 12:32 PM, Felix Brack wrote:
>>> 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 <fb@ltec.ch>
> 
> 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 <thomas@wytron.com.tw>
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 <thomas@wytron.com.tw>
    Reviewed-by: Tom Rini <trini@konsulko.com>
    Acked-by: Simon Glass <sjg@chromium.org>


Thanks and regards,
Lokesh

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Alexander Graf]

On 07/17/2018 11:21 AM, Lokesh Vutla wrote:
>
> On Tuesday 17 July 2018 02:42 PM, Alexander Graf wrote:
>> On 07/17/2018 09:55 AM, Lokesh Vutla wrote:
>>> On Tuesday 17 July 2018 12:32 PM, Felix Brack wrote:
>>>> 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 <fb@ltec.ch>
>> 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 <thomas@wytron.com.tw>
> 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 <thomas@wytron.com.tw>
>      Reviewed-by: Tom Rini <trini@konsulko.com>
>      Acked-by: Simon Glass <sjg@chromium.org>

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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Lokesh Vutla]

On Tuesday 17 July 2018 02:57 PM, Alexander Graf wrote:
> On 07/17/2018 11:21 AM, Lokesh Vutla wrote:
>>
>> On Tuesday 17 July 2018 02:42 PM, Alexander Graf wrote:
>>> On 07/17/2018 09:55 AM, Lokesh Vutla wrote:
>>>> On Tuesday 17 July 2018 12:32 PM, Felix Brack wrote:
>>>>> 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 <fb@ltec.ch>
>>> 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 <thomas@wytron.com.tw>
>> 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 <thomas@wytron.com.tw>
>>      Reviewed-by: Tom Rini <trini@konsulko.com>
>>      Acked-by: Simon Glass <sjg@chromium.org>
> 
> 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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Felix Brack]

On 17.07.2018 11:27, Alexander Graf wrote:
> On 07/17/2018 11:21 AM, Lokesh Vutla wrote:
>>
>> On Tuesday 17 July 2018 02:42 PM, Alexander Graf wrote:
>>> On 07/17/2018 09:55 AM, Lokesh Vutla wrote:
>>>> On Tuesday 17 July 2018 12:32 PM, Felix Brack wrote:
>>>>> 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 <fb@ltec.ch>
>>> 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 <thomas@wytron.com.tw>
>> 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 <thomas@wytron.com.tw>
>>      Reviewed-by: Tom Rini <trini@konsulko.com>
>>      Acked-by: Simon Glass <sjg@chromium.org>
> 
> 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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Felix Brack]

Hello Lokesh,

On 17.07.2018 09:55, Lokesh Vutla wrote:
> 
> 
> On Tuesday 17 July 2018 12:32 PM, Felix Brack wrote:
>> 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 <fb@ltec.ch>
>> ---
>>
>> 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(-)
>>
>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>> index 766e5ce..7eb3c6f 100644
>> --- a/drivers/serial/Kconfig
>> +++ b/drivers/serial/Kconfig
>> @@ -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?
>
Yes, I will for v3. I presume you verified this for all addition (none
AM33XX) SoCs of OMAP2+ arch.

regards Felix

> Thanks and regards,
> Lokesh
> 
> 
>> +	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
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 9c80090..9ff6dbe 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -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) {
>>

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Alexey Brodkin]

Hi Felix,

> -----Original Message-----
> From: Felix Brack [mailto:fb at ltec.ch]
> Sent: Tuesday, July 17, 2018 3:13 PM
> To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot at lists.denx.de
> Cc: Wolfgang Denk <wd@denx.de>; Tom Rini <trini@konsulko.com>; Marek Vasut <marek.vasut@gmail.com>; Patrice Chotard
> <patrice.chotard@st.com>; Michal Simek <michal.simek@xilinx.com>; Simon Glass <sjg@chromium.org>; Alexey Brodkin
> <Alexey.Brodkin@synopsys.com>; Bin Meng <bmeng.cn@gmail.com>; Ley Foon Tan <ley.foon.tan@intel.com>; Patrick Delaunay
> <patrick.delaunay@st.com>; Mario Six <mario.six@gdsys.cc>; Stefan Roese <sr@denx.de>; Bernhard Messerklinger
> <bernhard.messerklinger@br-automation.com>
> Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable

[snip]
 
> 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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Tom Rini]

On Tue, Jul 17, 2018 at 12:45:51PM +0000, Alexey Brodkin wrote:
> Hi Felix,
> 
> > -----Original Message-----
> > From: Felix Brack [mailto:fb at ltec.ch]
> > Sent: Tuesday, July 17, 2018 3:13 PM
> > To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot at lists.denx.de
> > Cc: Wolfgang Denk <wd@denx.de>; Tom Rini <trini@konsulko.com>; Marek Vasut <marek.vasut@gmail.com>; Patrice Chotard
> > <patrice.chotard@st.com>; Michal Simek <michal.simek@xilinx.com>; Simon Glass <sjg@chromium.org>; Alexey Brodkin
> > <Alexey.Brodkin@synopsys.com>; Bin Meng <bmeng.cn@gmail.com>; Ley Foon Tan <ley.foon.tan@intel.com>; Patrick Delaunay
> > <patrick.delaunay@st.com>; Mario Six <mario.six@gdsys.cc>; Stefan Roese <sr@denx.de>; Bernhard Messerklinger
> > <bernhard.messerklinger@br-automation.com>
> > Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
> 
> [snip]
>  
> > 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.

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.

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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Felix Brack]

On 17.07.2018 15:21, Tom Rini wrote:
> On Tue, Jul 17, 2018 at 12:45:51PM +0000, Alexey Brodkin wrote:
>> Hi Felix,
>>
>>> -----Original Message-----
>>> From: Felix Brack [mailto:fb at ltec.ch]
>>> Sent: Tuesday, July 17, 2018 3:13 PM
>>> To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot at lists.denx.de
>>> Cc: Wolfgang Denk <wd@denx.de>; Tom Rini <trini@konsulko.com>; Marek Vasut <marek.vasut@gmail.com>; Patrice Chotard
>>> <patrice.chotard@st.com>; Michal Simek <michal.simek@xilinx.com>; Simon Glass <sjg@chromium.org>; Alexey Brodkin
>>> <Alexey.Brodkin@synopsys.com>; Bin Meng <bmeng.cn@gmail.com>; Ley Foon Tan <ley.foon.tan@intel.com>; Patrick Delaunay
>>> <patrick.delaunay@st.com>; Mario Six <mario.six@gdsys.cc>; Stefan Roese <sr@denx.de>; Bernhard Messerklinger
>>> <bernhard.messerklinger@br-automation.com>
>>> Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
>>
>> [snip]
>>  
>>> 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.
> 
> 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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Alexander Graf]

On 07/17/2018 03:34 PM, Felix Brack wrote:
>
> On 17.07.2018 15:21, Tom Rini wrote:
>> On Tue, Jul 17, 2018 at 12:45:51PM +0000, Alexey Brodkin wrote:
>>> Hi Felix,
>>>
>>>> -----Original Message-----
>>>> From: Felix Brack [mailto:fb at ltec.ch]
>>>> Sent: Tuesday, July 17, 2018 3:13 PM
>>>> To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot at lists.denx.de
>>>> Cc: Wolfgang Denk <wd@denx.de>; Tom Rini <trini@konsulko.com>; Marek Vasut <marek.vasut@gmail.com>; Patrice Chotard
>>>> <patrice.chotard@st.com>; Michal Simek <michal.simek@xilinx.com>; Simon Glass <sjg@chromium.org>; Alexey Brodkin
>>>> <Alexey.Brodkin@synopsys.com>; Bin Meng <bmeng.cn@gmail.com>; Ley Foon Tan <ley.foon.tan@intel.com>; Patrick Delaunay
>>>> <patrick.delaunay@st.com>; Mario Six <mario.six@gdsys.cc>; Stefan Roese <sr@denx.de>; Bernhard Messerklinger
>>>> <bernhard.messerklinger@br-automation.com>
>>>> Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
>>> [snip]
>>>   
>>>> 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.
>> 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

[U-Boot] [PATCH v2] serial: ns16550: Add register shift variable

[Tom Rini]

On Tue, Jul 17, 2018 at 03:38:17PM +0200, Alexander Graf wrote:
> On 07/17/2018 03:34 PM, Felix Brack wrote:
> >
> >On 17.07.2018 15:21, Tom Rini wrote:
> >>On Tue, Jul 17, 2018 at 12:45:51PM +0000, Alexey Brodkin wrote:
> >>>Hi Felix,
> >>>
> >>>>-----Original Message-----
> >>>>From: Felix Brack [mailto:fb at ltec.ch]
> >>>>Sent: Tuesday, July 17, 2018 3:13 PM
> >>>>To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot at lists.denx.de
> >>>>Cc: Wolfgang Denk <wd@denx.de>; Tom Rini <trini@konsulko.com>; Marek Vasut <marek.vasut@gmail.com>; Patrice Chotard
> >>>><patrice.chotard@st.com>; Michal Simek <michal.simek@xilinx.com>; Simon Glass <sjg@chromium.org>; Alexey Brodkin
> >>>><Alexey.Brodkin@synopsys.com>; Bin Meng <bmeng.cn@gmail.com>; Ley Foon Tan <ley.foon.tan@intel.com>; Patrick Delaunay
> >>>><patrick.delaunay@st.com>; Mario Six <mario.six@gdsys.cc>; Stefan Roese <sr@denx.de>; Bernhard Messerklinger
> >>>><bernhard.messerklinger@br-automation.com>
> >>>>Subject: Re: [PATCH v2] serial: ns16550: Add register shift variable
> >>>[snip]
> >>>>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.
> >>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.

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.

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