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

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

[Felix Brack]

This patch adds a new Kconfig variable that allows setting
the register 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>
---

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

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 766e5ce..c80ad4d 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -530,6 +530,18 @@ 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 "Number of bytes to shift register offsets"
+	default 0
+	depends on SYS_NS16550
+	help
+	  Use this to specify the amount of bytes between discrete
+	  device registers. If, for example, the device registers are
+	  located at 0x00, 0x04, 0x08, 0x0C and so forth than set
+	  this to 2. The default value is 0.
+	  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] serial: ns16550: Add register shift variable

[Alexander Graf]

On 07/13/2018 04:43 PM, Felix Brack wrote:
> This patch adds a new Kconfig variable that allows setting
> the register 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

How does Linux determine the shift value then?


Alex

> (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>
> ---
>
>   drivers/serial/Kconfig   | 12 ++++++++++++
>   drivers/serial/ns16550.c |  3 ++-
>   2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 766e5ce..c80ad4d 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -530,6 +530,18 @@ 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 "Number of bytes to shift register offsets"
> +	default 0
> +	depends on SYS_NS16550
> +	help
> +	  Use this to specify the amount of bytes between discrete
> +	  device registers. If, for example, the device registers are
> +	  located at 0x00, 0x04, 0x08, 0x0C and so forth than set
> +	  this to 2. The default value is 0.
> +	  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] serial: ns16550: Add register shift variable

[Wolfgang Denk]

Dear Felix,

In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>
> 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.

Does this mean that U-Boot will not be able to use the same DTB as
Linux?

> +config SYS_NS16550_REG_SHIFT
> +	int "Number of bytes to shift register offsets"
> +	default 0
> +	depends on SYS_NS16550
> +	help
> +	  Use this to specify the amount of bytes between discrete
> +	  device registers. If, for example, the device registers are
> +	  located at 0x00, 0x04, 0x08, 0x0C and so forth than set
> +	  this to 2. The default value is 0.

This description is inconsistent or misleading.  How do you define
"space between registers"?  The unused gaps?  In the example, the
registers are spaced at 4 bytes intervals, so a value of 2 would
only make sense of we have 16 bit registters and you count the gap
bytes.

But this is a very strange and uncommon way to describe such a
situation, especially when you write that you "shift register
offsets".  Here I think about something like a LSL operation, so
shifing by 2 bits would result in a 2^2 = 4 byte spacing.

This needs to be rewritten.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Program maintenance is an entropy-increasing process,  and  even  its
most skilfull execution only delays the subsidence of the system into
unfixable obsolescence.       - Fred Brooks, "The Mythical Man Month"

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

[Tom Rini]

On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
> Dear Felix,
> 
> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
> >
> > 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.
> 
> Does this mean that U-Boot will not be able to use the same DTB as
> Linux?

To be clear, it's the other way around.  We can't use the Linux dtb/dts
files as they've dropped (and in other cases, aren't adding) these
properties as it's handled differently.

> 
> > +config SYS_NS16550_REG_SHIFT
> > +	int "Number of bytes to shift register offsets"
> > +	default 0
> > +	depends on SYS_NS16550
> > +	help
> > +	  Use this to specify the amount of bytes between discrete
> > +	  device registers. If, for example, the device registers are
> > +	  located at 0x00, 0x04, 0x08, 0x0C and so forth than set
> > +	  this to 2. The default value is 0.
> 
> This description is inconsistent or misleading.  How do you define
> "space between registers"?  The unused gaps?  In the example, the
> registers are spaced at 4 bytes intervals, so a value of 2 would
> only make sense of we have 16 bit registters and you count the gap
> bytes.
> 
> But this is a very strange and uncommon way to describe such a
> situation, especially when you write that you "shift register
> offsets".  Here I think about something like a LSL operation, so
> shifing by 2 bits would result in a 2^2 = 4 byte spacing.
> 
> This needs to be rewritten.

To try and help clarify, the property in question means "quantity to
shift the register offsets by."  It should be clear in our Kconfig help
entry as well that this is what we're looking for.

-- 
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/20180714/f1dd1952/attachment.sig>

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

[Alexey Brodkin]

Hi Tom, Felix, all

> -----Original Message-----
> From: Tom Rini [mailto:trini at konsulko.com]
> Sent: Saturday, July 14, 2018 6:50 PM
> To: Wolfgang Denk <wd@denx.de>
> Cc: Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin <Alexey.Brodkin@synopsys.com>;
> Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>
> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
> 
> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
> > Dear Felix,
> >
> > In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
> > >
> > > 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.
> >
> > Does this mean that U-Boot will not be able to use the same DTB as
> > Linux?
> 
> To be clear, it's the other way around.  We can't use the Linux dtb/dts
> files as they've dropped (and in other cases, aren't adding) these
> properties as it's handled differently.

Any chance to get a reference to the commit in Linux kernel that introduces that change?

I still see a bunch of:
----------------------------->8----------------------
reg-shift = <2>;
reg-io-width = <4>;
----------------------------->8----------------------
in .dts files for ARC boards even in linux-next git tree.

-Alexey

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

[Felix Brack]

Hi Alexey,

On 15.07.2018 10:43, Alexey Brodkin wrote:
> Hi Tom, Felix, all
> 
>> -----Original Message-----
>> From: Tom Rini [mailto:trini at konsulko.com]
>> Sent: Saturday, July 14, 2018 6:50 PM
>> To: Wolfgang Denk <wd@denx.de>
>> Cc: Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin <Alexey.Brodkin@synopsys.com>;
>> Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>
>> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
>>
>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>>> Dear Felix,
>>>
>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>>>
>>>> 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.
>>>
>>> Does this mean that U-Boot will not be able to use the same DTB as
>>> Linux?
>>
>> To be clear, it's the other way around.  We can't use the Linux dtb/dts
>> files as they've dropped (and in other cases, aren't adding) these
>> properties as it's handled differently.
> 
> Any chance to get a reference to the commit in Linux kernel that introduces that change?
> 
In fact I believe that the <reg-shift> property never existed in the
am33xx.dtsi file from Linux. U-Boot commit 85cf0e6299 shows that the
property has been added to U-Boot's am33xx.dtsi file. The commit log
clearly states why this happened:

"With the commit 'c7b9686d5d48 ("ns16550: unify serial_omap")' all
TI platforms are broken with DM/DT boot as ns16550 driver expects
reg-shift from DT which is not populated for TI platforms.
Earlier it worked as it was hard coded to 2 in serial-omap
driver. So adding the reg-shift to serial nodes for dra7, am4372
and am33xx dtsi files. Tested this patch on am437x-sk-evm,
am437x-gp-evm, am335x-boneblack, dra74x-evm and dra72x-evm."

> I still see a bunch of:
> ----------------------------->8----------------------
> reg-shift = <2>;
> reg-io-width = <4>;
> ----------------------------->8----------------------
> in .dts files for ARC boards even in linux-next git tree.
> 
> -Alexey
> 

regards Felix

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

[Alexey Brodkin]

Hi Felix,

> -----Original Message-----
> From: Felix Brack [mailto:fb at ltec.ch]
> Sent: Sunday, July 15, 2018 5:02 PM
> To: Alexey Brodkin <Alexey.Brodkin@synopsys.com>; Tom Rini <trini@konsulko.com>
> Cc: u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>;
> Wolfgang Denk <wd@denx.de>
> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
> 
> Hi Alexey,
> 
> On 15.07.2018 10:43, Alexey Brodkin wrote:
> > Hi Tom, Felix, all
> >
> >> -----Original Message-----
> >> From: Tom Rini [mailto:trini at konsulko.com]
> >> Sent: Saturday, July 14, 2018 6:50 PM
> >> To: Wolfgang Denk <wd@denx.de>
> >> Cc: Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin <Alexey.Brodkin@synopsys.com>;
> >> Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>
> >> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
> >>
> >> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
> >>> Dear Felix,
> >>>
> >>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
> >>>>
> >>>> 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.
> >>>
> >>> Does this mean that U-Boot will not be able to use the same DTB as
> >>> Linux?
> >>
> >> To be clear, it's the other way around.  We can't use the Linux dtb/dts
> >> files as they've dropped (and in other cases, aren't adding) these
> >> properties as it's handled differently.
> >
> > Any chance to get a reference to the commit in Linux kernel that introduces that change?
> >
> In fact I believe that the <reg-shift> property never existed in the
> am33xx.dtsi file from Linux. U-Boot commit 85cf0e6299 shows that the
> property has been added to U-Boot's am33xx.dtsi file. The commit log
> clearly states why this happened:
> 
> "With the commit 'c7b9686d5d48 ("ns16550: unify serial_omap")' all
> TI platforms are broken with DM/DT boot as ns16550 driver expects
> reg-shift from DT which is not populated for TI platforms.
> Earlier it worked as it was hard coded to 2 in serial-omap
> driver. So adding the reg-shift to serial nodes for dra7, am4372
> and am33xx dtsi files. Tested this patch on am437x-sk-evm,
> am437x-gp-evm, am335x-boneblack, dra74x-evm and dra72x-evm."

Ok so this has nothing to do with changes in Linux kernel I guess.
Why don't we go the other way around and not just submit
a change to .dts files in Linux kernel that are missing required properties?

-Alexey

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

[Andy Shevchenko]

On Mon, Jul 16, 2018 at 6:04 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:

>> > Any chance to get a reference to the commit in Linux kernel that introduces that change?
>> >
>> In fact I believe that the <reg-shift> property never existed in the
>> am33xx.dtsi file from Linux. U-Boot commit 85cf0e6299 shows that the
>> property has been added to U-Boot's am33xx.dtsi file. The commit log
>> clearly states why this happened:
>>
>> "With the commit 'c7b9686d5d48 ("ns16550: unify serial_omap")' all
>> TI platforms are broken with DM/DT boot as ns16550 driver expects
>> reg-shift from DT which is not populated for TI platforms.
>> Earlier it worked as it was hard coded to 2 in serial-omap
>> driver. So adding the reg-shift to serial nodes for dra7, am4372
>> and am33xx dtsi files. Tested this patch on am437x-sk-evm,
>> am437x-gp-evm, am335x-boneblack, dra74x-evm and dra72x-evm."
>
> Ok so this has nothing to do with changes in Linux kernel I guess.
> Why don't we go the other way around and not just submit
> a change to .dts files in Linux kernel that are missing required properties?

You may change DTSi files in the Linux kernel sources tree, but you
wouldn't be able to fix all DTB:s in the wild.
In this case Linux should have a fallback to the previous value in
case property is not found.

-- 
With Best Regards,
Andy Shevchenko

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

[Tom Rini]

On Mon, Jul 16, 2018 at 06:47:21PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 16, 2018 at 6:04 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
> 
> >> > Any chance to get a reference to the commit in Linux kernel that introduces that change?
> >> >
> >> In fact I believe that the <reg-shift> property never existed in the
> >> am33xx.dtsi file from Linux. U-Boot commit 85cf0e6299 shows that the
> >> property has been added to U-Boot's am33xx.dtsi file. The commit log
> >> clearly states why this happened:
> >>
> >> "With the commit 'c7b9686d5d48 ("ns16550: unify serial_omap")' all
> >> TI platforms are broken with DM/DT boot as ns16550 driver expects
> >> reg-shift from DT which is not populated for TI platforms.
> >> Earlier it worked as it was hard coded to 2 in serial-omap
> >> driver. So adding the reg-shift to serial nodes for dra7, am4372
> >> and am33xx dtsi files. Tested this patch on am437x-sk-evm,
> >> am437x-gp-evm, am335x-boneblack, dra74x-evm and dra72x-evm."
> >
> > Ok so this has nothing to do with changes in Linux kernel I guess.
> > Why don't we go the other way around and not just submit
> > a change to .dts files in Linux kernel that are missing required properties?
> 
> You may change DTSi files in the Linux kernel sources tree, but you
> wouldn't be able to fix all DTB:s in the wild.
> In this case Linux should have a fallback to the previous value in
> case property is not found.

No, we don't allow for U-Boot to be run with an arbitrary "wild" DTB
file nor the same DTB as the kernel (the kernel could run with a subset
of our DTB however).

-- 
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/6a0c5673/attachment.sig>

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

[Felix Brack]

Hello Alexey,

On 16.07.2018 17:04, Alexey Brodkin wrote:
> Hi Felix,
> 
>> -----Original Message-----
>> From: Felix Brack [mailto:fb at ltec.ch]
>> Sent: Sunday, July 15, 2018 5:02 PM
>> To: Alexey Brodkin <Alexey.Brodkin@synopsys.com>; Tom Rini <trini@konsulko.com>
>> Cc: u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>;
>> Wolfgang Denk <wd@denx.de>
>> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
>>
>> Hi Alexey,
>>
>> On 15.07.2018 10:43, Alexey Brodkin wrote:
>>> Hi Tom, Felix, all
>>>
>>>> -----Original Message-----
>>>> From: Tom Rini [mailto:trini at konsulko.com]
>>>> Sent: Saturday, July 14, 2018 6:50 PM
>>>> To: Wolfgang Denk <wd@denx.de>
>>>> Cc: Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin <Alexey.Brodkin@synopsys.com>;
>>>> Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>
>>>> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
>>>>
>>>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>>>>> Dear Felix,
>>>>>
>>>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Does this mean that U-Boot will not be able to use the same DTB as
>>>>> Linux?
>>>>
>>>> To be clear, it's the other way around.  We can't use the Linux dtb/dts
>>>> files as they've dropped (and in other cases, aren't adding) these
>>>> properties as it's handled differently.
>>>
>>> Any chance to get a reference to the commit in Linux kernel that introduces that change?
>>>
>> In fact I believe that the <reg-shift> property never existed in the
>> am33xx.dtsi file from Linux. U-Boot commit 85cf0e6299 shows that the
>> property has been added to U-Boot's am33xx.dtsi file. The commit log
>> clearly states why this happened:
>>
>> "With the commit 'c7b9686d5d48 ("ns16550: unify serial_omap")' all
>> TI platforms are broken with DM/DT boot as ns16550 driver expects
>> reg-shift from DT which is not populated for TI platforms.
>> Earlier it worked as it was hard coded to 2 in serial-omap
>> driver. So adding the reg-shift to serial nodes for dra7, am4372
>> and am33xx dtsi files. Tested this patch on am437x-sk-evm,
>> am437x-gp-evm, am335x-boneblack, dra74x-evm and dra72x-evm."
> 
> Ok so this has nothing to do with changes in Linux kernel I guess.
> Why don't we go the other way around and not just submit
> a change to .dts files in Linux kernel that are missing required properties?
>
I think there is nothing wrong with the kernel's .dtsi files, hence
nothing to be fixed in Linux.
Please read the RFC I posted May 24. It (hopefully) explains the need of
this patch. In short: syncing U-Boot DT files for AM33xx SoC with Linux
DT files will break things. This patch would fix the problem of the
"missing <reg-shift>" property.

regards Felix

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

[Alexander Graf]

On 07/17/2018 08:51 AM, Felix Brack wrote:
> Hello Alexey,
>
> On 16.07.2018 17:04, Alexey Brodkin wrote:
>> Hi Felix,
>>
>>> -----Original Message-----
>>> From: Felix Brack [mailto:fb at ltec.ch]
>>> Sent: Sunday, July 15, 2018 5:02 PM
>>> To: Alexey Brodkin <Alexey.Brodkin@synopsys.com>; Tom Rini <trini@konsulko.com>
>>> Cc: u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>;
>>> Wolfgang Denk <wd@denx.de>
>>> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
>>>
>>> Hi Alexey,
>>>
>>> On 15.07.2018 10:43, Alexey Brodkin wrote:
>>>> Hi Tom, Felix, all
>>>>
>>>>> -----Original Message-----
>>>>> From: Tom Rini [mailto:trini at konsulko.com]
>>>>> Sent: Saturday, July 14, 2018 6:50 PM
>>>>> To: Wolfgang Denk <wd@denx.de>
>>>>> Cc: Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin <Alexey.Brodkin@synopsys.com>;
>>>>> Alexander Graf <agraf@suse.de>; Michal Simek <michal.simek@xilinx.com>
>>>>> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
>>>>>
>>>>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>>>>>> Dear Felix,
>>>>>>
>>>>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>>>>>> 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.
>>>>>> Does this mean that U-Boot will not be able to use the same DTB as
>>>>>> Linux?
>>>>> To be clear, it's the other way around.  We can't use the Linux dtb/dts
>>>>> files as they've dropped (and in other cases, aren't adding) these
>>>>> properties as it's handled differently.
>>>> Any chance to get a reference to the commit in Linux kernel that introduces that change?
>>>>
>>> In fact I believe that the <reg-shift> property never existed in the
>>> am33xx.dtsi file from Linux. U-Boot commit 85cf0e6299 shows that the
>>> property has been added to U-Boot's am33xx.dtsi file. The commit log
>>> clearly states why this happened:
>>>
>>> "With the commit 'c7b9686d5d48 ("ns16550: unify serial_omap")' all
>>> TI platforms are broken with DM/DT boot as ns16550 driver expects
>>> reg-shift from DT which is not populated for TI platforms.
>>> Earlier it worked as it was hard coded to 2 in serial-omap
>>> driver. So adding the reg-shift to serial nodes for dra7, am4372
>>> and am33xx dtsi files. Tested this patch on am437x-sk-evm,
>>> am437x-gp-evm, am335x-boneblack, dra74x-evm and dra72x-evm."
>> Ok so this has nothing to do with changes in Linux kernel I guess.
>> Why don't we go the other way around and not just submit
>> a change to .dts files in Linux kernel that are missing required properties?
>>
> I think there is nothing wrong with the kernel's .dtsi files, hence
> nothing to be fixed in Linux.
> Please read the RFC I posted May 24. It (hopefully) explains the need of
> this patch. In short: syncing U-Boot DT files for AM33xx SoC with Linux
> DT files will break things. This patch would fix the problem of the
> "missing <reg-shift>" property.

It's not missing, we're just simply misimplementing the semantics of the 
"compatible" property.

Really, with a DT described system you should not need any per-device 
Kconfig options. Any of those are a very good indicator you're doing 
something wrong :).


Alex

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

[Felix Brack]

Hi Tom,

On 14.07.2018 17:49, Tom Rini wrote:
> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>> Dear Felix,
>>
>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>>
>>> 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.
>>
>> Does this mean that U-Boot will not be able to use the same DTB as
>> Linux?
> 
> To be clear, it's the other way around.  We can't use the Linux dtb/dts
> files as they've dropped (and in other cases, aren't adding) these
> properties as it's handled differently.
> 
This is exactly the point. These files have diverged quite a lot between
U-Boot and Linux since the last synchronization. I am trying to find a
method to re-synchronize U-Boot generating minimal collateral damage -
this patch is supposed to be part of that effort.

In fact we could even set the default value of SYS_NS16550_REG_SHIFT to
2 for _all_ am33xx SOC boards. That would probably further reduce
possible problems.

>>
>>> +config SYS_NS16550_REG_SHIFT
>>> +	int "Number of bytes to shift register offsets"
>>> +	default 0
>>> +	depends on SYS_NS16550
>>> +	help
>>> +	  Use this to specify the amount of bytes between discrete
>>> +	  device registers. If, for example, the device registers are
>>> +	  located at 0x00, 0x04, 0x08, 0x0C and so forth than set
>>> +	  this to 2. The default value is 0.
>>
>> This description is inconsistent or misleading.  How do you define
>> "space between registers"?  The unused gaps?  In the example, the
>> registers are spaced at 4 bytes intervals, so a value of 2 would
>> only make sense of we have 16 bit registters and you count the gap
>> bytes.
>>
>> But this is a very strange and uncommon way to describe such a
>> situation, especially when you write that you "shift register
>> offsets".  Here I think about something like a LSL operation, so
>> shifing by 2 bits would result in a 2^2 = 4 byte spacing.
>>
>> This needs to be rewritten.
> 
> To try and help clarify, the property in question means "quantity to
> shift the register offsets by."  It should be clear in our Kconfig help
> entry as well that this is what we're looking for.
> 
Thanks for this! The help text will be fixed in v2.

regards Felix

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

[Wolfgang Denk]

Dear Felix,

In message <1b0d33fa-adfb-9825-5ae8-8bded1678c0c@ltec.ch> you wrote:
>
> >> This needs to be rewritten.
> > 
> > To try and help clarify, the property in question means "quantity to
> > shift the register offsets by."  It should be clear in our Kconfig help
> > entry as well that this is what we're looking for.
> > 
> Thanks for this! The help text will be fixed in v2.

None of the places I could find in the Linux
Documentation/devicetree/bindings directory actually explains what
"shift the register offsets" means.   Only the code reveals that
this means a LEFT shift of the register address (offset).

At least one place in Linux interprets <reg-shift> differently; here
it operates on the _data_ (instead there they use <reg-spacing>):

"devicetree/bindings/ipmi/ipmi-smic.txt":

- reg-shift - The amount to shift the registers to the right to get the data
        into bit zero.

(see ipmi_si_mem_io.c


So can we please add an explanation what _exactly_ this means?

Thanks.


Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is easier to write an incorrect program than understand a  correct
one.

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

[Felix Brack]

On 16.07.2018 14:42, Wolfgang Denk wrote:
> Dear Felix,
> 
> In message <1b0d33fa-adfb-9825-5ae8-8bded1678c0c@ltec.ch> you wrote:
>>
>>>> This needs to be rewritten.
>>>
>>> To try and help clarify, the property in question means "quantity to
>>> shift the register offsets by."  It should be clear in our Kconfig help
>>> entry as well that this is what we're looking for.
>>>
>> Thanks for this! The help text will be fixed in v2.
> 
> None of the places I could find in the Linux
> Documentation/devicetree/bindings directory actually explains what
> "shift the register offsets" means.   Only the code reveals that
> this means a LEFT shift of the register address (offset).
> 
> At least one place in Linux interprets <reg-shift> differently; here
> it operates on the _data_ (instead there they use <reg-spacing>):
> 
> "devicetree/bindings/ipmi/ipmi-smic.txt":
> 
> - reg-shift - The amount to shift the registers to the right to get the data
>         into bit zero.
> 
> (see ipmi_si_mem_io.c
> 
> 
> So can we please add an explanation what _exactly_ this means?
>
I will propose one with v2 post.

> Thanks.
> 
> 
> Wolfgang Denk
> 

regards, Felix

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

[Alexander Graf]

On 07/14/2018 05:49 PM, Tom Rini wrote:
> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>> Dear Felix,
>>
>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>> 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.
>> Does this mean that U-Boot will not be able to use the same DTB as
>> Linux?
> To be clear, it's the other way around.  We can't use the Linux dtb/dts
> files as they've dropped (and in other cases, aren't adding) these
> properties as it's handled differently.

What does "differently" mean? Linux tries quite hard to be platform 
agnostic, so a per-build-target #define surely isn't what they're doing.

Are they inferring the shift from the compatible? In that case, we 
should do the same.


Alex

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

[Tom Rini]

On Mon, Jul 16, 2018 at 02:53:26PM +0200, Alexander Graf wrote:
> On 07/14/2018 05:49 PM, Tom Rini wrote:
> >On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
> >>Dear Felix,
> >>
> >>In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
> >>>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.
> >>Does this mean that U-Boot will not be able to use the same DTB as
> >>Linux?
> >To be clear, it's the other way around.  We can't use the Linux dtb/dts
> >files as they've dropped (and in other cases, aren't adding) these
> >properties as it's handled differently.
> 
> What does "differently" mean? Linux tries quite hard to be platform
> agnostic, so a per-build-target #define surely isn't what they're doing.

Yes, what exactly is the Linux kernel doing here?

-- 
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/389d3f26/attachment.sig>

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

[Alexander Graf]

On 07/17/2018 03:25 PM, Tom Rini wrote:
> On Mon, Jul 16, 2018 at 02:53:26PM +0200, Alexander Graf wrote:
>> On 07/14/2018 05:49 PM, Tom Rini wrote:
>>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>>>> Dear Felix,
>>>>
>>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>>>> 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.
>>>> Does this mean that U-Boot will not be able to use the same DTB as
>>>> Linux?
>>> To be clear, it's the other way around.  We can't use the Linux dtb/dts
>>> files as they've dropped (and in other cases, aren't adding) these
>>> properties as it's handled differently.
>> What does "differently" mean? Linux tries quite hard to be platform
>> agnostic, so a per-build-target #define surely isn't what they're doing.
> Yes, what exactly is the Linux kernel doing here?


Linux has a completely separate driver for omap3 (which is wrong too). 
But in a nutshell, it basically determines the shift value by the 
"compatible" string, so we should too.


Alex

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

[Alexey Brodkin]

Hi Alexander, Tom,

> -----Original Message-----
> From: Alexander Graf [mailto:agraf at suse.de]
> Sent: Tuesday, July 17, 2018 4:33 PM
> To: Tom Rini <trini@konsulko.com>
> Cc: Wolfgang Denk <wd@denx.de>; Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin
> <Alexey.Brodkin@synopsys.com>; Michal Simek <michal.simek@xilinx.com>
> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
> 
> On 07/17/2018 03:25 PM, Tom Rini wrote:
> > On Mon, Jul 16, 2018 at 02:53:26PM +0200, Alexander Graf wrote:
> >> On 07/14/2018 05:49 PM, Tom Rini wrote:
> >>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
> >>>> Dear Felix,
> >>>>
> >>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
> >>>>> 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.
> >>>> Does this mean that U-Boot will not be able to use the same DTB as
> >>>> Linux?
> >>> To be clear, it's the other way around.  We can't use the Linux dtb/dts
> >>> files as they've dropped (and in other cases, aren't adding) these
> >>> properties as it's handled differently.
> >> What does "differently" mean? Linux tries quite hard to be platform
> >> agnostic, so a per-build-target #define surely isn't what they're doing.
> > Yes, what exactly is the Linux kernel doing here?
> 
> 
> Linux has a completely separate driver for omap3 (which is wrong too).
> But in a nutshell, it basically determines the shift value by the
> "compatible" string, so we should too.

Here's the driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/omap-serial.c

-Alexey

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

[Tom Rini]

On Tue, Jul 17, 2018 at 01:34:36PM +0000, Alexey Brodkin wrote:
> Hi Alexander, Tom,
> 
> > -----Original Message-----
> > From: Alexander Graf [mailto:agraf at suse.de]
> > Sent: Tuesday, July 17, 2018 4:33 PM
> > To: Tom Rini <trini@konsulko.com>
> > Cc: Wolfgang Denk <wd@denx.de>; Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin
> > <Alexey.Brodkin@synopsys.com>; Michal Simek <michal.simek@xilinx.com>
> > Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
> > 
> > On 07/17/2018 03:25 PM, Tom Rini wrote:
> > > On Mon, Jul 16, 2018 at 02:53:26PM +0200, Alexander Graf wrote:
> > >> On 07/14/2018 05:49 PM, Tom Rini wrote:
> > >>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
> > >>>> Dear Felix,
> > >>>>
> > >>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
> > >>>>> 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.
> > >>>> Does this mean that U-Boot will not be able to use the same DTB as
> > >>>> Linux?
> > >>> To be clear, it's the other way around.  We can't use the Linux dtb/dts
> > >>> files as they've dropped (and in other cases, aren't adding) these
> > >>> properties as it's handled differently.
> > >> What does "differently" mean? Linux tries quite hard to be platform
> > >> agnostic, so a per-build-target #define surely isn't what they're doing.
> > > Yes, what exactly is the Linux kernel doing here?
> > 
> > 
> > Linux has a completely separate driver for omap3 (which is wrong too).
> > But in a nutshell, it basically determines the shift value by the
> > "compatible" string, so we should too.
> 
> Here's the driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/omap-serial.c

But I would swear that's also not required and the generic ns1655x
driver can be used.

-- 
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/31fef243/attachment.sig>

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

[Alexander Graf]

On 07/17/2018 03:41 PM, Tom Rini wrote:
> On Tue, Jul 17, 2018 at 01:34:36PM +0000, Alexey Brodkin wrote:
>> Hi Alexander, Tom,
>>
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf at suse.de]
>>> Sent: Tuesday, July 17, 2018 4:33 PM
>>> To: Tom Rini <trini@konsulko.com>
>>> Cc: Wolfgang Denk <wd@denx.de>; Felix Brack <fb@ltec.ch>; u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin
>>> <Alexey.Brodkin@synopsys.com>; Michal Simek <michal.simek@xilinx.com>
>>> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift variable
>>>
>>> On 07/17/2018 03:25 PM, Tom Rini wrote:
>>>> On Mon, Jul 16, 2018 at 02:53:26PM +0200, Alexander Graf wrote:
>>>>> On 07/14/2018 05:49 PM, Tom Rini wrote:
>>>>>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>>>>>>> Dear Felix,
>>>>>>>
>>>>>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>>>>>>> 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.
>>>>>>> Does this mean that U-Boot will not be able to use the same DTB as
>>>>>>> Linux?
>>>>>> To be clear, it's the other way around.  We can't use the Linux dtb/dts
>>>>>> files as they've dropped (and in other cases, aren't adding) these
>>>>>> properties as it's handled differently.
>>>>> What does "differently" mean? Linux tries quite hard to be platform
>>>>> agnostic, so a per-build-target #define surely isn't what they're doing.
>>>> Yes, what exactly is the Linux kernel doing here?
>>>
>>> Linux has a completely separate driver for omap3 (which is wrong too).
>>> But in a nutshell, it basically determines the shift value by the
>>> "compatible" string, so we should too.
>> Here's the driver:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/omap-serial.c
> But I would swear that's also not required and the generic ns1655x
> driver can be used.


I don't see the generic driver matching on the omap compatible string. 
So you'd have to convert the DT to the generic ns16650 binding (which 
would mean you also had to include the reg-shift property). I can't find 
any omap DT that does refer to its internal ns16650 by the generic 
binding though.

Again, maybe there's some additional functionality in that omap specific 
driver, like power management, that we don't care about at this point.


Alex

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

[Lokesh Vutla]

On Tuesday 17 July 2018 07:13 PM, Alexander Graf wrote:
> On 07/17/2018 03:41 PM, Tom Rini wrote:
>> On Tue, Jul 17, 2018 at 01:34:36PM +0000, Alexey Brodkin wrote:
>>> Hi Alexander, Tom,
>>>
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf at suse.de]
>>>> Sent: Tuesday, July 17, 2018 4:33 PM
>>>> To: Tom Rini <trini@konsulko.com>
>>>> Cc: Wolfgang Denk <wd@denx.de>; Felix Brack <fb@ltec.ch>;
>>>> u-boot at lists.denx.de; Stefan Roese <sr@denx.de>; Alexey Brodkin
>>>> <Alexey.Brodkin@synopsys.com>; Michal Simek <michal.simek@xilinx.com>
>>>> Subject: Re: [U-Boot] [PATCH] serial: ns16550: Add register shift
>>>> variable
>>>>
>>>> On 07/17/2018 03:25 PM, Tom Rini wrote:
>>>>> On Mon, Jul 16, 2018 at 02:53:26PM +0200, Alexander Graf wrote:
>>>>>> On 07/14/2018 05:49 PM, Tom Rini wrote:
>>>>>>> On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
>>>>>>>> Dear Felix,
>>>>>>>>
>>>>>>>> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you
>>>>>>>> wrote:
>>>>>>>>> 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.
>>>>>>>> Does this mean that U-Boot will not be able to use the same DTB as
>>>>>>>> Linux?
>>>>>>> To be clear, it's the other way around.  We can't use the Linux
>>>>>>> dtb/dts
>>>>>>> files as they've dropped (and in other cases, aren't adding) these
>>>>>>> properties as it's handled differently.
>>>>>> What does "differently" mean? Linux tries quite hard to be platform
>>>>>> agnostic, so a per-build-target #define surely isn't what they're
>>>>>> doing.
>>>>> Yes, what exactly is the Linux kernel doing here?
>>>>
>>>> Linux has a completely separate driver for omap3 (which is wrong too).
>>>> But in a nutshell, it basically determines the shift value by the
>>>> "compatible" string, so we should too.
>>> Here's the driver:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/omap-serial.c
>>>
>> But I would swear that's also not required and the generic ns1655x
>> driver can be used.
> 
> 
> I don't see the generic driver matching on the omap compatible string.
> So you'd have to convert the DT to the generic ns16650 binding (which
> would mean you also had to include the reg-shift property). I can't find
> any omap DT that does refer to its internal ns16650 by the generic
> binding though.
> 
> Again, maybe there's some additional functionality in that omap specific
> driver, like power management, that we don't care about at this point.
> 

omap uart do have the differences with ns16650. Affected to u-boot is
the MDR1 register that enables the IP[1]. Right now the difference is
guarded with OMAP2PLUS.

[1]
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/serial/ns16550.c;h=9c80090aa72623a826d25f0601b7c0acd9e589c5;hb=HEAD#l157


Thanks and regards,
Lokesh

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

[Andy Shevchenko]

On Tue, Jul 17, 2018 at 4:34 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:

>> Linux has a completely separate driver for omap3 (which is wrong too).

This is not (fully) correct,

>> But in a nutshell, it basically determines the shift value by the
>> "compatible" string, so we should too.
>
> Here's the driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/omap-serial.c

I dunno why it's still in the tree. The modern one is (check
compatible strings for both):

https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/tty/serial/8250/8250_omap.c




-- 
With Best Regards,
Andy Shevchenko

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

[Adam Ford]

On Tue, Jul 17, 2018 at 4:24 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jul 17, 2018 at 4:34 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
>
> >> Linux has a completely separate driver for omap3 (which is wrong too).
>
> This is not (fully) correct,
>
> >> But in a nutshell, it basically determines the shift value by the
> >> "compatible" string, so we should too.
> >
> > Here's the driver:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/omap-serial.c
>
> I dunno why it's still in the tree. The modern one is (check
> compatible strings for both):
>
> https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/tty/serial/8250/8250_omap.c
>
>
Can we omap2+ users patch the 8250_omap.c code in Linux to make use of
the reg-shift, then do the same from u-boot so they are in sync and
clean out any #defines that could be replaced by the device tree?

(I am not volunteering anyone or myself, it's more of a hypothetical question)

adam

>
>
> --
> With Best Regards,
> Andy Shevchenko
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Andy Shevchenko]

On Wed, Jul 18, 2018 at 12:30 AM, Adam Ford <aford173@gmail.com> wrote:
> On Tue, Jul 17, 2018 at 4:24 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

>> https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/tty/serial/8250/8250_omap.c

> Can we omap2+ users patch the 8250_omap.c code in Linux to make use of
> the reg-shift, then do the same from u-boot so they are in sync and
> clean out any #defines that could be replaced by the device tree?

It's possible, but it doesn't mean you now fully on DT, b/c Linux must
support old DTBs as I already told in this thread.

So, the code in the driver will look like

ret = of_property_read_u32(..., &reg_shift);
if (ret)
 reg_shift = 2;

-- 
With Best Regards,
Andy Shevchenko

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

[Tom Rini]

On Tue, Jul 17, 2018 at 03:33:15PM +0200, Alexander Graf wrote:
> On 07/17/2018 03:25 PM, Tom Rini wrote:
> >On Mon, Jul 16, 2018 at 02:53:26PM +0200, Alexander Graf wrote:
> >>On 07/14/2018 05:49 PM, Tom Rini wrote:
> >>>On Sat, Jul 14, 2018 at 12:47:21PM +0200, Wolfgang Denk wrote:
> >>>>Dear Felix,
> >>>>
> >>>>In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
> >>>>>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.
> >>>>Does this mean that U-Boot will not be able to use the same DTB as
> >>>>Linux?
> >>>To be clear, it's the other way around.  We can't use the Linux dtb/dts
> >>>files as they've dropped (and in other cases, aren't adding) these
> >>>properties as it's handled differently.
> >>What does "differently" mean? Linux tries quite hard to be platform
> >>agnostic, so a per-build-target #define surely isn't what they're doing.
> >Yes, what exactly is the Linux kernel doing here?
> 
> Linux has a completely separate driver for omap3 (which is wrong too). But
> in a nutshell, it basically determines the shift value by the "compatible"
> string, so we should too.

Yes and no.  For a number of years now the omap-only (ttyOx) driver has
been deprecated and the generic ns1665x driver is valid.  What does the
ns1655x driver do here?

-- 
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/8411e0f6/attachment.sig>

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

[Felix Brack]

Hello Wolfgang,

On 14.07.2018 12:47, Wolfgang Denk wrote:
> Dear Felix,
> 
> In message <1531492980-16543-1-git-send-email-fb@ltec.ch> you wrote:
>>
>> 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.
> 
> Does this mean that U-Boot will not be able to use the same DTB as
> Linux?
>
This has already been answered very accurately by Tom.
I have also written an RFC dealing with this: "[RFC] arm: dts: am33xx:
Sync DTS with Linux 4.16.11", posted May 24.

>> +config SYS_NS16550_REG_SHIFT
>> +	int "Number of bytes to shift register offsets"
>> +	default 0
>> +	depends on SYS_NS16550
>> +	help
>> +	  Use this to specify the amount of bytes between discrete
>> +	  device registers. If, for example, the device registers are
>> +	  located at 0x00, 0x04, 0x08, 0x0C and so forth than set
>> +	  this to 2. The default value is 0.
> 
> This description is inconsistent or misleading.  How do you define
> "space between registers"?  The unused gaps?  In the example, the
> registers are spaced at 4 bytes intervals, so a value of 2 would
> only make sense of we have 16 bit registters and you count the gap
> bytes.
> 
> But this is a very strange and uncommon way to describe such a
> situation, especially when you write that you "shift register
> offsets".  Here I think about something like a LSL operation, so
> shifing by 2 bits would result in a 2^2 = 4 byte spacing.
> 
> This needs to be rewritten.
> 
You are right, sorry. It seems I used my brain to mix up two different
explanations and that's the result -  will be fixed.
> 
> Best regards,
> 
> Wolfgang Denk
> 
regards, Felix