diff mbox

[U-Boot,U-Boot,v3,09/13] ns16550: unify serial_tegra

Message ID 5660C870.9030607@wwwdotorg.org
State RFC
Headers show

Commit Message

Stephen Warren Dec. 3, 2015, 10:55 p.m. UTC 
On 12/03/2015 02:43 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 3 December 2015 at 14:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 11/22/2015 08:53 AM, Tom Rini wrote:
>>>
>>> On Thu, Nov 19, 2015 at 09:48:11PM +0800, Thomas Chou wrote:
>>>
>>>> Unify serial_tegra, 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>
>>>
>>>
>>> Applied to u-boot/master, thanks!
>>
>>
>> FYI, this patch causes at least Jetson TK1 to immediately reset at boot
>> time. I imagine this affects all Tegra boards, at least those that use SPL
>> or those that are ARMv7; my p2371-2180 Jetson TX1 ARMv8 board which doesn't
>> use SPL seems to still work.
>>
>> I guess I'll see if I can pin-point the problem, unless you can spot an
>> obvious typo or something like that.
>>
>> Reported-by: Kevin Hilman <khilman@linaro.org>
>> (Bisected by me)
>
> Unfortunately I didn't test this patch.
>
> Things to check:
> - for SPL, that the platform data is available somewhere
> - clock-frequency and reg-shift are in the device tree

The patch below appears to solve the problem. Any ideas how to fix this 
cleanly? I don't think we want to introduce a CONFIG_SPL_xxx for every 
CONFIG_xxx, yet CONFIG_IS_ENABLED() (e.g. used around 
arch/arm/mach-tegra/board.c's U_BOOT_DEVICE(ns16550_com1) definition) 
seems to require this.

Unfortunately, the patch doesn't revert cleanly, or at least would 
require other more recent patches to be reverted first.

Also, while debugging this I found that U-Boot force-probes any device 
referenced by /chosen/stdout-path even if the DT node is disabled; yet 
more fundamental incorrect DT processing:-( That's why the patch below 
only affects SPL. More fixes to actually enable the serial port in DT 
should be required.

Comments

Thomas Chou Dec. 4, 2015, 1:59 p.m. UTC | #1 
Hi Stephen,

On 2015年12月04日 06:55, Stephen Warren wrote:
> The patch below appears to solve the problem. Any ideas how to fix this
> cleanly? I don't think we want to introduce a CONFIG_SPL_xxx for every
> CONFIG_xxx, yet CONFIG_IS_ENABLED() (e.g. used around
> arch/arm/mach-tegra/board.c's U_BOOT_DEVICE(ns16550_com1) definition)
> seems to require this.
>
> Unfortunately, the patch doesn't revert cleanly, or at least would
> require other more recent patches to be reverted first.
>
> Also, while debugging this I found that U-Boot force-probes any device
> referenced by /chosen/stdout-path even if the DT node is disabled; yet
> more fundamental incorrect DT processing:-( That's why the patch below
> only affects SPL. More fixes to actually enable the serial port in DT
> should be required.
>
> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> index fbfb204e6ec8..15609e7dc773 100644
> --- a/arch/arm/mach-tegra/Kconfig
> +++ b/arch/arm/mach-tegra/Kconfig
> @@ -1,5 +1,8 @@
>   if TEGRA
>
> +config SPL_DM_SERIAL
> +    bool "Fix build"
> +
>   config TEGRA_COMMON
>       bool "Tegra common options"
>       select DM
> @@ -12,6 +15,7 @@ config TEGRA_COMMON
>       select DM_SPI
>       select DM_SPI_FLASH
>       select OF_CONTROL
> +    select SPL_DM_SERIAL
>

But all serial drivers should be converted to driver model by the end of 
Jan, 2016. The DM_SERIAL should be removed then.

As SPL might not use OF_CONTROL. We might need better logic in tegra 
board.c like,
   SPL_BUILD ? SPL_OF_CONTROL : OF_CONTROL


>   config TEGRA_ARMV7_COMMON
>       bool "Tegra 32-bit common options"
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 166deabcd436..256c7eafd76e 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -451,5 +451,6 @@ U_BOOT_DRIVER(ns16550_serial) = {
>       .priv_auto_alloc_size = sizeof(struct NS16550),
>       .probe = ns16550_serial_probe,
>       .ops    = &ns16550_serial_ops,
> +    .flags    = DM_FLAG_PRE_RELOC,

Yes, this pre-reloc flag should be added.

>   };
>   #endif /* CONFIG_DM_SERIAL */
>

Best regards,
Thomas
Yegor Yefremov Dec. 4, 2015, 2:10 p.m. UTC | #2 
On Fri, Dec 4, 2015 at 2:59 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> Hi Stephen,
>
>
> On 2015年12月04日 06:55, Stephen Warren wrote:
>>
>> The patch below appears to solve the problem. Any ideas how to fix this
>> cleanly? I don't think we want to introduce a CONFIG_SPL_xxx for every
>> CONFIG_xxx, yet CONFIG_IS_ENABLED() (e.g. used around
>> arch/arm/mach-tegra/board.c's U_BOOT_DEVICE(ns16550_com1) definition)
>> seems to require this.
>>
>> Unfortunately, the patch doesn't revert cleanly, or at least would
>> require other more recent patches to be reverted first.
>>
>> Also, while debugging this I found that U-Boot force-probes any device
>> referenced by /chosen/stdout-path even if the DT node is disabled; yet
>> more fundamental incorrect DT processing:-( That's why the patch below
>> only affects SPL. More fixes to actually enable the serial port in DT
>> should be required.
>>
>> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
>> index fbfb204e6ec8..15609e7dc773 100644
>> --- a/arch/arm/mach-tegra/Kconfig
>> +++ b/arch/arm/mach-tegra/Kconfig
>> @@ -1,5 +1,8 @@
>>   if TEGRA
>>
>> +config SPL_DM_SERIAL
>> +    bool "Fix build"
>> +
>>   config TEGRA_COMMON
>>       bool "Tegra common options"
>>       select DM
>> @@ -12,6 +15,7 @@ config TEGRA_COMMON
>>       select DM_SPI
>>       select DM_SPI_FLASH
>>       select OF_CONTROL
>> +    select SPL_DM_SERIAL
>>
>
> But all serial drivers should be converted to driver model by the end of
> Jan, 2016. The DM_SERIAL should be removed then.
>
> As SPL might not use OF_CONTROL. We might need better logic in tegra board.c
> like,
>   SPL_BUILD ? SPL_OF_CONTROL : OF_CONTROL
>
>
>>   config TEGRA_ARMV7_COMMON
>>       bool "Tegra 32-bit common options"
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 166deabcd436..256c7eafd76e 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -451,5 +451,6 @@ U_BOOT_DRIVER(ns16550_serial) = {
>>       .priv_auto_alloc_size = sizeof(struct NS16550),
>>       .probe = ns16550_serial_probe,
>>       .ops    = &ns16550_serial_ops,
>> +    .flags    = DM_FLAG_PRE_RELOC,
>
>
> Yes, this pre-reloc flag should be added.
>
>>   };
>>   #endif /* CONFIG_DM_SERIAL */

Have the same issue with am335x boards
(http://lists.denx.de/pipermail/u-boot/2015-December/236615.html).

Yegor
Tom Rini Dec. 4, 2015, 3:09 p.m. UTC | #3 
On Fri, Dec 04, 2015 at 03:10:14PM +0100, Yegor Yefremov wrote:
> On Fri, Dec 4, 2015 at 2:59 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> > Hi Stephen,
> >
> >
> > On 2015年12月04日 06:55, Stephen Warren wrote:
> >>
> >> The patch below appears to solve the problem. Any ideas how to fix this
> >> cleanly? I don't think we want to introduce a CONFIG_SPL_xxx for every
> >> CONFIG_xxx, yet CONFIG_IS_ENABLED() (e.g. used around
> >> arch/arm/mach-tegra/board.c's U_BOOT_DEVICE(ns16550_com1) definition)
> >> seems to require this.
> >>
> >> Unfortunately, the patch doesn't revert cleanly, or at least would
> >> require other more recent patches to be reverted first.
> >>
> >> Also, while debugging this I found that U-Boot force-probes any device
> >> referenced by /chosen/stdout-path even if the DT node is disabled; yet
> >> more fundamental incorrect DT processing:-( That's why the patch below
> >> only affects SPL. More fixes to actually enable the serial port in DT
> >> should be required.
> >>
> >> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> >> index fbfb204e6ec8..15609e7dc773 100644
> >> --- a/arch/arm/mach-tegra/Kconfig
> >> +++ b/arch/arm/mach-tegra/Kconfig
> >> @@ -1,5 +1,8 @@
> >>   if TEGRA
> >>
> >> +config SPL_DM_SERIAL
> >> +    bool "Fix build"
> >> +
> >>   config TEGRA_COMMON
> >>       bool "Tegra common options"
> >>       select DM
> >> @@ -12,6 +15,7 @@ config TEGRA_COMMON
> >>       select DM_SPI
> >>       select DM_SPI_FLASH
> >>       select OF_CONTROL
> >> +    select SPL_DM_SERIAL
> >>
> >
> > But all serial drivers should be converted to driver model by the end of
> > Jan, 2016. The DM_SERIAL should be removed then.
> >
> > As SPL might not use OF_CONTROL. We might need better logic in tegra board.c
> > like,
> >   SPL_BUILD ? SPL_OF_CONTROL : OF_CONTROL
> >
> >
> >>   config TEGRA_ARMV7_COMMON
> >>       bool "Tegra 32-bit common options"
> >> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> >> index 166deabcd436..256c7eafd76e 100644
> >> --- a/drivers/serial/ns16550.c
> >> +++ b/drivers/serial/ns16550.c
> >> @@ -451,5 +451,6 @@ U_BOOT_DRIVER(ns16550_serial) = {
> >>       .priv_auto_alloc_size = sizeof(struct NS16550),
> >>       .probe = ns16550_serial_probe,
> >>       .ops    = &ns16550_serial_ops,
> >> +    .flags    = DM_FLAG_PRE_RELOC,
> >
> >
> > Yes, this pre-reloc flag should be added.
> >
> >>   };
> >>   #endif /* CONFIG_DM_SERIAL */
> 
> Have the same issue with am335x boards
> (http://lists.denx.de/pipermail/u-boot/2015-December/236615.html).

Bah.  I tested this on omap4_panda where I didn't see a problem..
Simon Glass Dec. 4, 2015, 3:52 p.m. UTC | #4 
Hi,

On 4 December 2015 at 07:09, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Dec 04, 2015 at 03:10:14PM +0100, Yegor Yefremov wrote:
>> On Fri, Dec 4, 2015 at 2:59 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
>> > Hi Stephen,
>> >
>> >
>> > On 2015年12月04日 06:55, Stephen Warren wrote:
>> >>
>> >> The patch below appears to solve the problem. Any ideas how to fix this
>> >> cleanly? I don't think we want to introduce a CONFIG_SPL_xxx for every
>> >> CONFIG_xxx, yet CONFIG_IS_ENABLED() (e.g. used around
>> >> arch/arm/mach-tegra/board.c's U_BOOT_DEVICE(ns16550_com1) definition)
>> >> seems to require this.
>> >>
>> >> Unfortunately, the patch doesn't revert cleanly, or at least would
>> >> require other more recent patches to be reverted first.
>> >>
>> >> Also, while debugging this I found that U-Boot force-probes any device
>> >> referenced by /chosen/stdout-path even if the DT node is disabled; yet
>> >> more fundamental incorrect DT processing:-( That's why the patch below
>> >> only affects SPL. More fixes to actually enable the serial port in DT
>> >> should be required.
>> >>
>> >> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
>> >> index fbfb204e6ec8..15609e7dc773 100644
>> >> --- a/arch/arm/mach-tegra/Kconfig
>> >> +++ b/arch/arm/mach-tegra/Kconfig
>> >> @@ -1,5 +1,8 @@
>> >>   if TEGRA
>> >>
>> >> +config SPL_DM_SERIAL
>> >> +    bool "Fix build"
>> >> +
>> >>   config TEGRA_COMMON
>> >>       bool "Tegra common options"
>> >>       select DM
>> >> @@ -12,6 +15,7 @@ config TEGRA_COMMON
>> >>       select DM_SPI
>> >>       select DM_SPI_FLASH
>> >>       select OF_CONTROL
>> >> +    select SPL_DM_SERIAL
>> >>
>> >
>> > But all serial drivers should be converted to driver model by the end of
>> > Jan, 2016. The DM_SERIAL should be removed then.
>> >
>> > As SPL might not use OF_CONTROL. We might need better logic in tegra board.c
>> > like,
>> >   SPL_BUILD ? SPL_OF_CONTROL : OF_CONTROL
>> >
>> >
>> >>   config TEGRA_ARMV7_COMMON
>> >>       bool "Tegra 32-bit common options"
>> >> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> >> index 166deabcd436..256c7eafd76e 100644
>> >> --- a/drivers/serial/ns16550.c
>> >> +++ b/drivers/serial/ns16550.c
>> >> @@ -451,5 +451,6 @@ U_BOOT_DRIVER(ns16550_serial) = {
>> >>       .priv_auto_alloc_size = sizeof(struct NS16550),
>> >>       .probe = ns16550_serial_probe,
>> >>       .ops    = &ns16550_serial_ops,
>> >> +    .flags    = DM_FLAG_PRE_RELOC,
>> >
>> >
>> > Yes, this pre-reloc flag should be added.
>> >
>> >>   };
>> >>   #endif /* CONFIG_DM_SERIAL */
>>
>> Have the same issue with am335x boards
>> (http://lists.denx.de/pipermail/u-boot/2015-December/236615.html).
>
> Bah.  I tested this on omap4_panda where I didn't see a problem..

I found someone on beaglebone black so will send a patch.

Regards,
Simon
diff mbox

Patch

diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index fbfb204e6ec8..15609e7dc773 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -1,5 +1,8 @@ 
  if TEGRA

+config SPL_DM_SERIAL
+	bool "Fix build"
+
  config TEGRA_COMMON
  	bool "Tegra common options"
  	select DM
@@ -12,6 +15,7 @@  config TEGRA_COMMON
  	select DM_SPI
  	select DM_SPI_FLASH
  	select OF_CONTROL
+	select SPL_DM_SERIAL

  config TEGRA_ARMV7_COMMON
  	bool "Tegra 32-bit common options"
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 166deabcd436..256c7eafd76e 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -451,5 +451,6 @@  U_BOOT_DRIVER(ns16550_serial) = {
  	.priv_auto_alloc_size = sizeof(struct NS16550),
  	.probe = ns16550_serial_probe,
  	.ops	= &ns16550_serial_ops,
+	.flags	= DM_FLAG_PRE_RELOC,
  };
  #endif /* CONFIG_DM_SERIAL */