| Message ID | 1531810943-3417-1-git-send-email-fb@ltec.ch |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,v2] serial: ns16550: Add register shift variable | expand |
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) { >
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
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
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
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
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
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) { >>
Hi Felix, > -----Original Message----- > From: Felix Brack [mailto:fb@ltec.ch] > Sent: Tuesday, July 17, 2018 3:13 PM > To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot@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
On Tue, Jul 17, 2018 at 12:45:51PM +0000, Alexey Brodkin wrote: > Hi Felix, > > > -----Original Message----- > > From: Felix Brack [mailto:fb@ltec.ch] > > Sent: Tuesday, July 17, 2018 3:13 PM > > To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot@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.
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@ltec.ch] >>> Sent: Tuesday, July 17, 2018 3:13 PM >>> To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot@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
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@ltec.ch] >>>> Sent: Tuesday, July 17, 2018 3:13 PM >>>> To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot@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
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@ltec.ch] > >>>>Sent: Tuesday, July 17, 2018 3:13 PM > >>>>To: Alexander Graf <agraf@suse.de>; Lokesh Vutla <lokeshvutla@ti.com>; u-boot@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.
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) {
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(-)