diff mbox

[U-Boot] am33xx, spl, siemens: enable debug uart output again

Message ID 54F817FA.7070506@denx.de
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Heiko Schocher March 5, 2015, 8:46 a.m. UTC
Hello Tom,

Am 05.03.2015 07:22, schrieb Heiko Schocher:
> Hello Tom,
>
> Am 04.03.2015 17:40, schrieb Tom Rini:
>> On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
>>> Hello Tom,
>>>
>>> Am 02.03.2015 14:59, schrieb Tom Rini:
>>>> On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
>>>>> Hello Simon,
>>>>>
>>>>> Am 24.02.2015 14:31, schrieb Simon Glass:
>>>>>> Hi Heiko,
>>>>>>
>>>>>> On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
>>>>>>> a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
>>>>>>>
>>>>>>> moves the init of the debug uart at the very end of SPL code.
>>>>>>> Enable it for the siemens board earlier, as they print
>>>>>>> ddr settings ... all debug output before board_init_r()
>>>>>>> is here currently useless. Maybe we must rework this
>>>>>>> globally?
>>>>>>
>>>>>> Assuming we are talking about U-Boot proper, the DDR init should
>>>>>> happen in board_init_f(), specifically dram_init(). so I think this
>>>>>> code should be updated.
>>>>>>
>>>>>> If it is SPL, then DDR init should happen in SPL's board_init_f().
>>>>>
>>>>> It is in SPL...
>>>>>
>>>>> sdram_init() is called from:
>>>>>
>>>>> ./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
>>>>>
>>>>>> I sent a series a few weeks ago (available at u-boot-dm branch
>>>>>> spl-working) related to this topic:
>>>>>>
>>>>>> http://patchwork.ozlabs.org/patch/438581/
>>>>>
>>>>> Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
>>>>> a rework, right?
>>>>>
>>>>> Is a simple rename s_init() -> board_init_f() correct?
>>>>
>>>> Right so, no, we can't just rename s_init to board_init_f.  This is what
>>>> I was talking about in the thread about the function Hans wants to add
>>>> to enable some bits in CP15 on sunxi, iirc.
>>>>
>>>> In short, armv7 has a different set of abstraction hooks than the
>>>> previous ARM cores (armv8 followed what we have for v7) and I'm not
>>>> convinced in the end that it really won us anything.  See
>>>> http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
>>>>
>>>> For today you need to rework the Siemens code to print out the DDR
>>>> values (when desired) in spl_board_init() as we do not, or will not
>>>> shortly, have gd prior to board_init_f running.
>>>
>>> Hmm... first I thought, ok, no problem, move the output from the RAM
>>> parameters to spl_board_init() ... but thats only the half of the
>>> story ... They read the RAM parameters from an i2c eeprom, and if
>>> there are errors, they print this errors ... currently this does
>>> not work, and thats I think the more important case ... and I could
>>> not move this error printfs to somewhere, because if RAM is not
>>> working ... there is no later ...
>>>
>>> So I have to enable the console early ... maybe I missed something,
>>> but this worked fine in the past (and I think we should not break
>>> this, as this is an essential feature).
>>
>> OK, I missed something too.  I think this gets better now once I merge
>> Simon's SPL series as we do all of this from board_init_f() and the
>> siemens code should just work again.
>
> Yes, just saw your patch ;-)
>
> If they are in mainline (or do you have them somewhere in a git repo?),
> I test it again on the dxr2 board, thanks!

If I am correct, all needed patches from you are in mainline, just
tried this on the dxr2 board ... but I still need:


to see the console output ...

bye,
Heiko

Comments

Tom Rini March 5, 2015, 2:50 p.m. UTC | #1
On Thu, Mar 05, 2015 at 09:46:50AM +0100, Heiko Schocher wrote:
> Hello Tom,
> 
> Am 05.03.2015 07:22, schrieb Heiko Schocher:
> >Hello Tom,
> >
> >Am 04.03.2015 17:40, schrieb Tom Rini:
> >>On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
> >>>Hello Tom,
> >>>
> >>>Am 02.03.2015 14:59, schrieb Tom Rini:
> >>>>On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
> >>>>>Hello Simon,
> >>>>>
> >>>>>Am 24.02.2015 14:31, schrieb Simon Glass:
> >>>>>>Hi Heiko,
> >>>>>>
> >>>>>>On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
> >>>>>>>a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
> >>>>>>>
> >>>>>>>moves the init of the debug uart at the very end of SPL code.
> >>>>>>>Enable it for the siemens board earlier, as they print
> >>>>>>>ddr settings ... all debug output before board_init_r()
> >>>>>>>is here currently useless. Maybe we must rework this
> >>>>>>>globally?
> >>>>>>
> >>>>>>Assuming we are talking about U-Boot proper, the DDR init should
> >>>>>>happen in board_init_f(), specifically dram_init(). so I think this
> >>>>>>code should be updated.
> >>>>>>
> >>>>>>If it is SPL, then DDR init should happen in SPL's board_init_f().
> >>>>>
> >>>>>It is in SPL...
> >>>>>
> >>>>>sdram_init() is called from:
> >>>>>
> >>>>>./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
> >>>>>
> >>>>>>I sent a series a few weeks ago (available at u-boot-dm branch
> >>>>>>spl-working) related to this topic:
> >>>>>>
> >>>>>>http://patchwork.ozlabs.org/patch/438581/
> >>>>>
> >>>>>Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
> >>>>>a rework, right?
> >>>>>
> >>>>>Is a simple rename s_init() -> board_init_f() correct?
> >>>>
> >>>>Right so, no, we can't just rename s_init to board_init_f.  This is what
> >>>>I was talking about in the thread about the function Hans wants to add
> >>>>to enable some bits in CP15 on sunxi, iirc.
> >>>>
> >>>>In short, armv7 has a different set of abstraction hooks than the
> >>>>previous ARM cores (armv8 followed what we have for v7) and I'm not
> >>>>convinced in the end that it really won us anything.  See
> >>>>http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
> >>>>
> >>>>For today you need to rework the Siemens code to print out the DDR
> >>>>values (when desired) in spl_board_init() as we do not, or will not
> >>>>shortly, have gd prior to board_init_f running.
> >>>
> >>>Hmm... first I thought, ok, no problem, move the output from the RAM
> >>>parameters to spl_board_init() ... but thats only the half of the
> >>>story ... They read the RAM parameters from an i2c eeprom, and if
> >>>there are errors, they print this errors ... currently this does
> >>>not work, and thats I think the more important case ... and I could
> >>>not move this error printfs to somewhere, because if RAM is not
> >>>working ... there is no later ...
> >>>
> >>>So I have to enable the console early ... maybe I missed something,
> >>>but this worked fine in the past (and I think we should not break
> >>>this, as this is an essential feature).
> >>
> >>OK, I missed something too.  I think this gets better now once I merge
> >>Simon's SPL series as we do all of this from board_init_f() and the
> >>siemens code should just work again.
> >
> >Yes, just saw your patch ;-)
> >
> >If they are in mainline (or do you have them somewhere in a git repo?),
> >I test it again on the dxr2 board, thanks!
> 
> If I am correct, all needed patches from you are in mainline, just
> tried this on the dxr2 board ... but I still need:
> 
> diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
> index a39cbd0..8724604 100644
> --- a/board/siemens/common/board.c
> +++ b/board/siemens/common/board.c
> @@ -40,6 +40,11 @@ void set_uart_mux_conf(void)
> 
>  void set_mux_conf_regs(void)
>  {
> +       /* enable early the console */
> +       gd->baudrate = CONFIG_BAUDRATE;
> +       serial_init();
> +       gd->have_console = 1;
> +
>         /* Initalize the board header */
>         enable_i2c0_pin_mux();
>         i2c_set_bus_num(0);
> 
> to see the console output ...

Yes, if you want console prior to spl_board_init() then you need to do
that where you are.
Heiko Schocher March 6, 2015, 7:24 a.m. UTC | #2
Hello Tom,

Am 05.03.2015 15:50, schrieb Tom Rini:
> On Thu, Mar 05, 2015 at 09:46:50AM +0100, Heiko Schocher wrote:
>> Hello Tom,
>>
>> Am 05.03.2015 07:22, schrieb Heiko Schocher:
>>> Hello Tom,
>>>
>>> Am 04.03.2015 17:40, schrieb Tom Rini:
>>>> On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
>>>>> Hello Tom,
>>>>>
>>>>> Am 02.03.2015 14:59, schrieb Tom Rini:
>>>>>> On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
>>>>>>> Hello Simon,
>>>>>>>
>>>>>>> Am 24.02.2015 14:31, schrieb Simon Glass:
>>>>>>>> Hi Heiko,
>>>>>>>>
>>>>>>>> On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
>>>>>>>>> a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
>>>>>>>>>
>>>>>>>>> moves the init of the debug uart at the very end of SPL code.
>>>>>>>>> Enable it for the siemens board earlier, as they print
>>>>>>>>> ddr settings ... all debug output before board_init_r()
>>>>>>>>> is here currently useless. Maybe we must rework this
>>>>>>>>> globally?
>>>>>>>>
>>>>>>>> Assuming we are talking about U-Boot proper, the DDR init should
>>>>>>>> happen in board_init_f(), specifically dram_init(). so I think this
>>>>>>>> code should be updated.
>>>>>>>>
>>>>>>>> If it is SPL, then DDR init should happen in SPL's board_init_f().
>>>>>>>
>>>>>>> It is in SPL...
>>>>>>>
>>>>>>> sdram_init() is called from:
>>>>>>>
>>>>>>> ./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
>>>>>>>
>>>>>>>> I sent a series a few weeks ago (available at u-boot-dm branch
>>>>>>>> spl-working) related to this topic:
>>>>>>>>
>>>>>>>> http://patchwork.ozlabs.org/patch/438581/
>>>>>>>
>>>>>>> Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
>>>>>>> a rework, right?
>>>>>>>
>>>>>>> Is a simple rename s_init() -> board_init_f() correct?
>>>>>>
>>>>>> Right so, no, we can't just rename s_init to board_init_f.  This is what
>>>>>> I was talking about in the thread about the function Hans wants to add
>>>>>> to enable some bits in CP15 on sunxi, iirc.
>>>>>>
>>>>>> In short, armv7 has a different set of abstraction hooks than the
>>>>>> previous ARM cores (armv8 followed what we have for v7) and I'm not
>>>>>> convinced in the end that it really won us anything.  See
>>>>>> http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
>>>>>>
>>>>>> For today you need to rework the Siemens code to print out the DDR
>>>>>> values (when desired) in spl_board_init() as we do not, or will not
>>>>>> shortly, have gd prior to board_init_f running.
>>>>>
>>>>> Hmm... first I thought, ok, no problem, move the output from the RAM
>>>>> parameters to spl_board_init() ... but thats only the half of the
>>>>> story ... They read the RAM parameters from an i2c eeprom, and if
>>>>> there are errors, they print this errors ... currently this does
>>>>> not work, and thats I think the more important case ... and I could
>>>>> not move this error printfs to somewhere, because if RAM is not
>>>>> working ... there is no later ...
>>>>>
>>>>> So I have to enable the console early ... maybe I missed something,
>>>>> but this worked fine in the past (and I think we should not break
>>>>> this, as this is an essential feature).
>>>>
>>>> OK, I missed something too.  I think this gets better now once I merge
>>>> Simon's SPL series as we do all of this from board_init_f() and the
>>>> siemens code should just work again.
>>>
>>> Yes, just saw your patch ;-)
>>>
>>> If they are in mainline (or do you have them somewhere in a git repo?),
>>> I test it again on the dxr2 board, thanks!
>>
>> If I am correct, all needed patches from you are in mainline, just
>> tried this on the dxr2 board ... but I still need:
>>
>> diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
>> index a39cbd0..8724604 100644
>> --- a/board/siemens/common/board.c
>> +++ b/board/siemens/common/board.c
>> @@ -40,6 +40,11 @@ void set_uart_mux_conf(void)
>>
>>   void set_mux_conf_regs(void)
>>   {
>> +       /* enable early the console */
>> +       gd->baudrate = CONFIG_BAUDRATE;
>> +       serial_init();
>> +       gd->have_console = 1;
>> +
>>          /* Initalize the board header */
>>          enable_i2c0_pin_mux();
>>          i2c_set_bus_num(0);
>>
>> to see the console output ...
>
> Yes, if you want console prior to spl_board_init() then you need to do
> that where you are.
>

Ah, ok... so this patch is OK?

bye,
Heiko
Tom Rini March 6, 2015, 12:15 p.m. UTC | #3
On Fri, Mar 06, 2015 at 08:24:47AM +0100, Heiko Schocher wrote:
> Hello Tom,
> 
> Am 05.03.2015 15:50, schrieb Tom Rini:
> >On Thu, Mar 05, 2015 at 09:46:50AM +0100, Heiko Schocher wrote:
> >>Hello Tom,
> >>
> >>Am 05.03.2015 07:22, schrieb Heiko Schocher:
> >>>Hello Tom,
> >>>
> >>>Am 04.03.2015 17:40, schrieb Tom Rini:
> >>>>On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
> >>>>>Hello Tom,
> >>>>>
> >>>>>Am 02.03.2015 14:59, schrieb Tom Rini:
> >>>>>>On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
> >>>>>>>Hello Simon,
> >>>>>>>
> >>>>>>>Am 24.02.2015 14:31, schrieb Simon Glass:
> >>>>>>>>Hi Heiko,
> >>>>>>>>
> >>>>>>>>On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
> >>>>>>>>>a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
> >>>>>>>>>
> >>>>>>>>>moves the init of the debug uart at the very end of SPL code.
> >>>>>>>>>Enable it for the siemens board earlier, as they print
> >>>>>>>>>ddr settings ... all debug output before board_init_r()
> >>>>>>>>>is here currently useless. Maybe we must rework this
> >>>>>>>>>globally?
> >>>>>>>>
> >>>>>>>>Assuming we are talking about U-Boot proper, the DDR init should
> >>>>>>>>happen in board_init_f(), specifically dram_init(). so I think this
> >>>>>>>>code should be updated.
> >>>>>>>>
> >>>>>>>>If it is SPL, then DDR init should happen in SPL's board_init_f().
> >>>>>>>
> >>>>>>>It is in SPL...
> >>>>>>>
> >>>>>>>sdram_init() is called from:
> >>>>>>>
> >>>>>>>./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
> >>>>>>>
> >>>>>>>>I sent a series a few weeks ago (available at u-boot-dm branch
> >>>>>>>>spl-working) related to this topic:
> >>>>>>>>
> >>>>>>>>http://patchwork.ozlabs.org/patch/438581/
> >>>>>>>
> >>>>>>>Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
> >>>>>>>a rework, right?
> >>>>>>>
> >>>>>>>Is a simple rename s_init() -> board_init_f() correct?
> >>>>>>
> >>>>>>Right so, no, we can't just rename s_init to board_init_f.  This is what
> >>>>>>I was talking about in the thread about the function Hans wants to add
> >>>>>>to enable some bits in CP15 on sunxi, iirc.
> >>>>>>
> >>>>>>In short, armv7 has a different set of abstraction hooks than the
> >>>>>>previous ARM cores (armv8 followed what we have for v7) and I'm not
> >>>>>>convinced in the end that it really won us anything.  See
> >>>>>>http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
> >>>>>>
> >>>>>>For today you need to rework the Siemens code to print out the DDR
> >>>>>>values (when desired) in spl_board_init() as we do not, or will not
> >>>>>>shortly, have gd prior to board_init_f running.
> >>>>>
> >>>>>Hmm... first I thought, ok, no problem, move the output from the RAM
> >>>>>parameters to spl_board_init() ... but thats only the half of the
> >>>>>story ... They read the RAM parameters from an i2c eeprom, and if
> >>>>>there are errors, they print this errors ... currently this does
> >>>>>not work, and thats I think the more important case ... and I could
> >>>>>not move this error printfs to somewhere, because if RAM is not
> >>>>>working ... there is no later ...
> >>>>>
> >>>>>So I have to enable the console early ... maybe I missed something,
> >>>>>but this worked fine in the past (and I think we should not break
> >>>>>this, as this is an essential feature).
> >>>>
> >>>>OK, I missed something too.  I think this gets better now once I merge
> >>>>Simon's SPL series as we do all of this from board_init_f() and the
> >>>>siemens code should just work again.
> >>>
> >>>Yes, just saw your patch ;-)
> >>>
> >>>If they are in mainline (or do you have them somewhere in a git repo?),
> >>>I test it again on the dxr2 board, thanks!
> >>
> >>If I am correct, all needed patches from you are in mainline, just
> >>tried this on the dxr2 board ... but I still need:
> >>
> >>diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
> >>index a39cbd0..8724604 100644
> >>--- a/board/siemens/common/board.c
> >>+++ b/board/siemens/common/board.c
> >>@@ -40,6 +40,11 @@ void set_uart_mux_conf(void)
> >>
> >>  void set_mux_conf_regs(void)
> >>  {
> >>+       /* enable early the console */
> >>+       gd->baudrate = CONFIG_BAUDRATE;
> >>+       serial_init();
> >>+       gd->have_console = 1;
> >>+
> >>         /* Initalize the board header */
> >>         enable_i2c0_pin_mux();
> >>         i2c_set_bus_num(0);
> >>
> >>to see the console output ...
> >
> >Yes, if you want console prior to spl_board_init() then you need to do
> >that where you are.
> >
> 
> Ah, ok... so this patch is OK?

If you must print out the DDR timings, yes, this is normal.
Heiko Schocher May 14, 2015, 10:37 a.m. UTC | #4
Hello Tom,

Am 06.03.2015 08:24, schrieb Heiko Schocher:
> Hello Tom,
>
> Am 05.03.2015 15:50, schrieb Tom Rini:
>> On Thu, Mar 05, 2015 at 09:46:50AM +0100, Heiko Schocher wrote:
>>> Hello Tom,
>>>
>>> Am 05.03.2015 07:22, schrieb Heiko Schocher:
>>>> Hello Tom,
>>>>
>>>> Am 04.03.2015 17:40, schrieb Tom Rini:
>>>>> On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
>>>>>> Hello Tom,
>>>>>>
>>>>>> Am 02.03.2015 14:59, schrieb Tom Rini:
>>>>>>> On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
>>>>>>>> Hello Simon,
>>>>>>>>
>>>>>>>> Am 24.02.2015 14:31, schrieb Simon Glass:
>>>>>>>>> Hi Heiko,
>>>>>>>>>
>>>>>>>>> On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
>>>>>>>>>> a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
>>>>>>>>>>
>>>>>>>>>> moves the init of the debug uart at the very end of SPL code.
>>>>>>>>>> Enable it for the siemens board earlier, as they print
>>>>>>>>>> ddr settings ... all debug output before board_init_r()
>>>>>>>>>> is here currently useless. Maybe we must rework this
>>>>>>>>>> globally?
>>>>>>>>>
>>>>>>>>> Assuming we are talking about U-Boot proper, the DDR init should
>>>>>>>>> happen in board_init_f(), specifically dram_init(). so I think this
>>>>>>>>> code should be updated.
>>>>>>>>>
>>>>>>>>> If it is SPL, then DDR init should happen in SPL's board_init_f().
>>>>>>>>
>>>>>>>> It is in SPL...
>>>>>>>>
>>>>>>>> sdram_init() is called from:
>>>>>>>>
>>>>>>>> ./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
>>>>>>>>
>>>>>>>>> I sent a series a few weeks ago (available at u-boot-dm branch
>>>>>>>>> spl-working) related to this topic:
>>>>>>>>>
>>>>>>>>> http://patchwork.ozlabs.org/patch/438581/
>>>>>>>>
>>>>>>>> Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
>>>>>>>> a rework, right?
>>>>>>>>
>>>>>>>> Is a simple rename s_init() -> board_init_f() correct?
>>>>>>>
>>>>>>> Right so, no, we can't just rename s_init to board_init_f.  This is what
>>>>>>> I was talking about in the thread about the function Hans wants to add
>>>>>>> to enable some bits in CP15 on sunxi, iirc.
>>>>>>>
>>>>>>> In short, armv7 has a different set of abstraction hooks than the
>>>>>>> previous ARM cores (armv8 followed what we have for v7) and I'm not
>>>>>>> convinced in the end that it really won us anything.  See
>>>>>>> http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
>>>>>>>
>>>>>>> For today you need to rework the Siemens code to print out the DDR
>>>>>>> values (when desired) in spl_board_init() as we do not, or will not
>>>>>>> shortly, have gd prior to board_init_f running.
>>>>>>
>>>>>> Hmm... first I thought, ok, no problem, move the output from the RAM
>>>>>> parameters to spl_board_init() ... but thats only the half of the
>>>>>> story ... They read the RAM parameters from an i2c eeprom, and if
>>>>>> there are errors, they print this errors ... currently this does
>>>>>> not work, and thats I think the more important case ... and I could
>>>>>> not move this error printfs to somewhere, because if RAM is not
>>>>>> working ... there is no later ...
>>>>>>
>>>>>> So I have to enable the console early ... maybe I missed something,
>>>>>> but this worked fine in the past (and I think we should not break
>>>>>> this, as this is an essential feature).
>>>>>
>>>>> OK, I missed something too.  I think this gets better now once I merge
>>>>> Simon's SPL series as we do all of this from board_init_f() and the
>>>>> siemens code should just work again.
>>>>
>>>> Yes, just saw your patch ;-)
>>>>
>>>> If they are in mainline (or do you have them somewhere in a git repo?),
>>>> I test it again on the dxr2 board, thanks!
>>>
>>> If I am correct, all needed patches from you are in mainline, just
>>> tried this on the dxr2 board ... but I still need:
>>>
>>> diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
>>> index a39cbd0..8724604 100644
>>> --- a/board/siemens/common/board.c
>>> +++ b/board/siemens/common/board.c
>>> @@ -40,6 +40,11 @@ void set_uart_mux_conf(void)
>>>
>>>   void set_mux_conf_regs(void)
>>>   {
>>> +       /* enable early the console */
>>> +       gd->baudrate = CONFIG_BAUDRATE;
>>> +       serial_init();
>>> +       gd->have_console = 1;
>>> +
>>>          /* Initalize the board header */
>>>          enable_i2c0_pin_mux();
>>>          i2c_set_bus_num(0);
>>>
>>> to see the console output ...
>>
>> Yes, if you want console prior to spl_board_init() then you need to do
>> that where you are.
>>
>
> Ah, ok... so this patch is OK?

Just found this thread again, as I want to post updates for the am335x
based siemens boards ... Is this patch [1] Ok, and can get applied?

Thanks!

bye,
Heiko
[1] Patchwork [U-Boot] am33xx, spl, siemens: enable debug uart output again
https://patchwork.ozlabs.org/patch/446644/

Hups... found this patch in patchwork marked as RFC and Delegated
to me ... ?
Tom Rini May 14, 2015, 10:51 a.m. UTC | #5
On Thu, May 14, 2015 at 12:37:40PM +0200, Heiko Schocher wrote:
> Hello Tom,
> 
> Am 06.03.2015 08:24, schrieb Heiko Schocher:
> >Hello Tom,
> >
> >Am 05.03.2015 15:50, schrieb Tom Rini:
> >>On Thu, Mar 05, 2015 at 09:46:50AM +0100, Heiko Schocher wrote:
> >>>Hello Tom,
> >>>
> >>>Am 05.03.2015 07:22, schrieb Heiko Schocher:
> >>>>Hello Tom,
> >>>>
> >>>>Am 04.03.2015 17:40, schrieb Tom Rini:
> >>>>>On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
> >>>>>>Hello Tom,
> >>>>>>
> >>>>>>Am 02.03.2015 14:59, schrieb Tom Rini:
> >>>>>>>On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
> >>>>>>>>Hello Simon,
> >>>>>>>>
> >>>>>>>>Am 24.02.2015 14:31, schrieb Simon Glass:
> >>>>>>>>>Hi Heiko,
> >>>>>>>>>
> >>>>>>>>>On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
> >>>>>>>>>>a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
> >>>>>>>>>>
> >>>>>>>>>>moves the init of the debug uart at the very end of SPL code.
> >>>>>>>>>>Enable it for the siemens board earlier, as they print
> >>>>>>>>>>ddr settings ... all debug output before board_init_r()
> >>>>>>>>>>is here currently useless. Maybe we must rework this
> >>>>>>>>>>globally?
> >>>>>>>>>
> >>>>>>>>>Assuming we are talking about U-Boot proper, the DDR init should
> >>>>>>>>>happen in board_init_f(), specifically dram_init(). so I think this
> >>>>>>>>>code should be updated.
> >>>>>>>>>
> >>>>>>>>>If it is SPL, then DDR init should happen in SPL's board_init_f().
> >>>>>>>>
> >>>>>>>>It is in SPL...
> >>>>>>>>
> >>>>>>>>sdram_init() is called from:
> >>>>>>>>
> >>>>>>>>./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
> >>>>>>>>
> >>>>>>>>>I sent a series a few weeks ago (available at u-boot-dm branch
> >>>>>>>>>spl-working) related to this topic:
> >>>>>>>>>
> >>>>>>>>>http://patchwork.ozlabs.org/patch/438581/
> >>>>>>>>
> >>>>>>>>Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
> >>>>>>>>a rework, right?
> >>>>>>>>
> >>>>>>>>Is a simple rename s_init() -> board_init_f() correct?
> >>>>>>>
> >>>>>>>Right so, no, we can't just rename s_init to board_init_f.  This is what
> >>>>>>>I was talking about in the thread about the function Hans wants to add
> >>>>>>>to enable some bits in CP15 on sunxi, iirc.
> >>>>>>>
> >>>>>>>In short, armv7 has a different set of abstraction hooks than the
> >>>>>>>previous ARM cores (armv8 followed what we have for v7) and I'm not
> >>>>>>>convinced in the end that it really won us anything.  See
> >>>>>>>http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
> >>>>>>>
> >>>>>>>For today you need to rework the Siemens code to print out the DDR
> >>>>>>>values (when desired) in spl_board_init() as we do not, or will not
> >>>>>>>shortly, have gd prior to board_init_f running.
> >>>>>>
> >>>>>>Hmm... first I thought, ok, no problem, move the output from the RAM
> >>>>>>parameters to spl_board_init() ... but thats only the half of the
> >>>>>>story ... They read the RAM parameters from an i2c eeprom, and if
> >>>>>>there are errors, they print this errors ... currently this does
> >>>>>>not work, and thats I think the more important case ... and I could
> >>>>>>not move this error printfs to somewhere, because if RAM is not
> >>>>>>working ... there is no later ...
> >>>>>>
> >>>>>>So I have to enable the console early ... maybe I missed something,
> >>>>>>but this worked fine in the past (and I think we should not break
> >>>>>>this, as this is an essential feature).
> >>>>>
> >>>>>OK, I missed something too.  I think this gets better now once I merge
> >>>>>Simon's SPL series as we do all of this from board_init_f() and the
> >>>>>siemens code should just work again.
> >>>>
> >>>>Yes, just saw your patch ;-)
> >>>>
> >>>>If they are in mainline (or do you have them somewhere in a git repo?),
> >>>>I test it again on the dxr2 board, thanks!
> >>>
> >>>If I am correct, all needed patches from you are in mainline, just
> >>>tried this on the dxr2 board ... but I still need:
> >>>
> >>>diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
> >>>index a39cbd0..8724604 100644
> >>>--- a/board/siemens/common/board.c
> >>>+++ b/board/siemens/common/board.c
> >>>@@ -40,6 +40,11 @@ void set_uart_mux_conf(void)
> >>>
> >>>  void set_mux_conf_regs(void)
> >>>  {
> >>>+       /* enable early the console */
> >>>+       gd->baudrate = CONFIG_BAUDRATE;
> >>>+       serial_init();
> >>>+       gd->have_console = 1;
> >>>+
> >>>         /* Initalize the board header */
> >>>         enable_i2c0_pin_mux();
> >>>         i2c_set_bus_num(0);
> >>>
> >>>to see the console output ...
> >>
> >>Yes, if you want console prior to spl_board_init() then you need to do
> >>that where you are.
> >>
> >
> >Ah, ok... so this patch is OK?
> 
> Just found this thread again, as I want to post updates for the am335x
> based siemens boards ... Is this patch [1] Ok, and can get applied?
> 
> Thanks!
> 
> bye,
> Heiko
> [1] Patchwork [U-Boot] am33xx, spl, siemens: enable debug uart output again
> https://patchwork.ozlabs.org/patch/446644/
> 
> Hups... found this patch in patchwork marked as RFC and Delegated
> to me ... ?

I'll look it over again then, thanks.
Simon Glass May 14, 2015, 12:46 p.m. UTC | #6
Hi Heiko,

On 14 May 2015 at 04:51, Tom Rini <trini@konsulko.com> wrote:
> On Thu, May 14, 2015 at 12:37:40PM +0200, Heiko Schocher wrote:
>> Hello Tom,
>>
>> Am 06.03.2015 08:24, schrieb Heiko Schocher:
>> >Hello Tom,
>> >
>> >Am 05.03.2015 15:50, schrieb Tom Rini:
>> >>On Thu, Mar 05, 2015 at 09:46:50AM +0100, Heiko Schocher wrote:
>> >>>Hello Tom,
>> >>>
>> >>>Am 05.03.2015 07:22, schrieb Heiko Schocher:
>> >>>>Hello Tom,
>> >>>>
>> >>>>Am 04.03.2015 17:40, schrieb Tom Rini:
>> >>>>>On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
>> >>>>>>Hello Tom,
>> >>>>>>
>> >>>>>>Am 02.03.2015 14:59, schrieb Tom Rini:
>> >>>>>>>On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
>> >>>>>>>>Hello Simon,
>> >>>>>>>>
>> >>>>>>>>Am 24.02.2015 14:31, schrieb Simon Glass:
>> >>>>>>>>>Hi Heiko,
>> >>>>>>>>>
>> >>>>>>>>>On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
>> >>>>>>>>>>a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
>> >>>>>>>>>>
>> >>>>>>>>>>moves the init of the debug uart at the very end of SPL code.
>> >>>>>>>>>>Enable it for the siemens board earlier, as they print
>> >>>>>>>>>>ddr settings ... all debug output before board_init_r()
>> >>>>>>>>>>is here currently useless. Maybe we must rework this
>> >>>>>>>>>>globally?
>> >>>>>>>>>
>> >>>>>>>>>Assuming we are talking about U-Boot proper, the DDR init should
>> >>>>>>>>>happen in board_init_f(), specifically dram_init(). so I think this
>> >>>>>>>>>code should be updated.
>> >>>>>>>>>
>> >>>>>>>>>If it is SPL, then DDR init should happen in SPL's board_init_f().
>> >>>>>>>>
>> >>>>>>>>It is in SPL...
>> >>>>>>>>
>> >>>>>>>>sdram_init() is called from:
>> >>>>>>>>
>> >>>>>>>>./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
>> >>>>>>>>
>> >>>>>>>>>I sent a series a few weeks ago (available at u-boot-dm branch
>> >>>>>>>>>spl-working) related to this topic:
>> >>>>>>>>>
>> >>>>>>>>>http://patchwork.ozlabs.org/patch/438581/
>> >>>>>>>>
>> >>>>>>>>Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
>> >>>>>>>>a rework, right?
>> >>>>>>>>
>> >>>>>>>>Is a simple rename s_init() -> board_init_f() correct?
>> >>>>>>>
>> >>>>>>>Right so, no, we can't just rename s_init to board_init_f.  This is what
>> >>>>>>>I was talking about in the thread about the function Hans wants to add
>> >>>>>>>to enable some bits in CP15 on sunxi, iirc.
>> >>>>>>>
>> >>>>>>>In short, armv7 has a different set of abstraction hooks than the
>> >>>>>>>previous ARM cores (armv8 followed what we have for v7) and I'm not
>> >>>>>>>convinced in the end that it really won us anything.  See
>> >>>>>>>http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
>> >>>>>>>
>> >>>>>>>For today you need to rework the Siemens code to print out the DDR
>> >>>>>>>values (when desired) in spl_board_init() as we do not, or will not
>> >>>>>>>shortly, have gd prior to board_init_f running.
>> >>>>>>
>> >>>>>>Hmm... first I thought, ok, no problem, move the output from the RAM
>> >>>>>>parameters to spl_board_init() ... but thats only the half of the
>> >>>>>>story ... They read the RAM parameters from an i2c eeprom, and if
>> >>>>>>there are errors, they print this errors ... currently this does
>> >>>>>>not work, and thats I think the more important case ... and I could
>> >>>>>>not move this error printfs to somewhere, because if RAM is not
>> >>>>>>working ... there is no later ...
>> >>>>>>
>> >>>>>>So I have to enable the console early ... maybe I missed something,
>> >>>>>>but this worked fine in the past (and I think we should not break
>> >>>>>>this, as this is an essential feature).
>> >>>>>
>> >>>>>OK, I missed something too.  I think this gets better now once I merge
>> >>>>>Simon's SPL series as we do all of this from board_init_f() and the
>> >>>>>siemens code should just work again.
>> >>>>
>> >>>>Yes, just saw your patch ;-)
>> >>>>
>> >>>>If they are in mainline (or do you have them somewhere in a git repo?),
>> >>>>I test it again on the dxr2 board, thanks!
>> >>>
>> >>>If I am correct, all needed patches from you are in mainline, just
>> >>>tried this on the dxr2 board ... but I still need:
>> >>>
>> >>>diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
>> >>>index a39cbd0..8724604 100644
>> >>>--- a/board/siemens/common/board.c
>> >>>+++ b/board/siemens/common/board.c
>> >>>@@ -40,6 +40,11 @@ void set_uart_mux_conf(void)
>> >>>
>> >>>  void set_mux_conf_regs(void)
>> >>>  {
>> >>>+       /* enable early the console */
>> >>>+       gd->baudrate = CONFIG_BAUDRATE;
>> >>>+       serial_init();
>> >>>+       gd->have_console = 1;
>> >>>+
>> >>>         /* Initalize the board header */
>> >>>         enable_i2c0_pin_mux();
>> >>>         i2c_set_bus_num(0);
>> >>>
>> >>>to see the console output ...
>> >>
>> >>Yes, if you want console prior to spl_board_init() then you need to do
>> >>that where you are.
>> >>
>> >
>> >Ah, ok... so this patch is OK?
>>
>> Just found this thread again, as I want to post updates for the am335x
>> based siemens boards ... Is this patch [1] Ok, and can get applied?
>>
>> Thanks!
>>
>> bye,
>> Heiko
>> [1] Patchwork [U-Boot] am33xx, spl, siemens: enable debug uart output again
>> https://patchwork.ozlabs.org/patch/446644/
>>
>> Hups... found this patch in patchwork marked as RFC and Delegated
>> to me ... ?
>
> I'll look it over again then, thanks.

It's a bit confusing - am I right in thinking that this is called from
board_early_init_f() in arch/arm/cpu/armv7/am33xx/board.c? That in
turn is called from board_init_f() in the same file?

If so it seems OK, although I wonder if the SPL console setup could
happen in board_early_init_f() or even board_init_f() rather than
being buried so deep? It seems odd that setting up the mux registers
should enable the console, and I doubt people will find it easily.

Regards,
Simon
Heiko Schocher May 15, 2015, 5:23 a.m. UTC | #7
Hello Simon,

Am 14.05.2015 14:46, schrieb Simon Glass:
> Hi Heiko,
>
> On 14 May 2015 at 04:51, Tom Rini <trini@konsulko.com> wrote:
>> On Thu, May 14, 2015 at 12:37:40PM +0200, Heiko Schocher wrote:
>>> Hello Tom,
>>>
>>> Am 06.03.2015 08:24, schrieb Heiko Schocher:
>>>> Hello Tom,
>>>>
>>>> Am 05.03.2015 15:50, schrieb Tom Rini:
>>>>> On Thu, Mar 05, 2015 at 09:46:50AM +0100, Heiko Schocher wrote:
>>>>>> Hello Tom,
>>>>>>
>>>>>> Am 05.03.2015 07:22, schrieb Heiko Schocher:
>>>>>>> Hello Tom,
>>>>>>>
>>>>>>> Am 04.03.2015 17:40, schrieb Tom Rini:
>>>>>>>> On Wed, Mar 04, 2015 at 08:42:58AM +0100, Heiko Schocher wrote:
>>>>>>>>> Hello Tom,
>>>>>>>>>
>>>>>>>>> Am 02.03.2015 14:59, schrieb Tom Rini:
>>>>>>>>>> On Mon, Mar 02, 2015 at 07:56:41AM +0100, Heiko Schocher wrote:
>>>>>>>>>>> Hello Simon,
>>>>>>>>>>>
>>>>>>>>>>> Am 24.02.2015 14:31, schrieb Simon Glass:
>>>>>>>>>>>> Hi Heiko,
>>>>>>>>>>>>
>>>>>>>>>>>> On 23 February 2015 at 23:18, Heiko Schocher <hs@denx.de> wrote:
>>>>>>>>>>>>> a6b541b090: TI ARMv7: Don't use GD before crt0.S has set it
>>>>>>>>>>>>>
>>>>>>>>>>>>> moves the init of the debug uart at the very end of SPL code.
>>>>>>>>>>>>> Enable it for the siemens board earlier, as they print
>>>>>>>>>>>>> ddr settings ... all debug output before board_init_r()
>>>>>>>>>>>>> is here currently useless. Maybe we must rework this
>>>>>>>>>>>>> globally?
>>>>>>>>>>>>
>>>>>>>>>>>> Assuming we are talking about U-Boot proper, the DDR init should
>>>>>>>>>>>> happen in board_init_f(), specifically dram_init(). so I think this
>>>>>>>>>>>> code should be updated.
>>>>>>>>>>>>
>>>>>>>>>>>> If it is SPL, then DDR init should happen in SPL's board_init_f().
>>>>>>>>>>>
>>>>>>>>>>> It is in SPL...
>>>>>>>>>>>
>>>>>>>>>>> sdram_init() is called from:
>>>>>>>>>>>
>>>>>>>>>>> ./arch/arm/cpu/armv7/am33xx/board.c from s_init() ...
>>>>>>>>>>>
>>>>>>>>>>>> I sent a series a few weeks ago (available at u-boot-dm branch
>>>>>>>>>>>> spl-working) related to this topic:
>>>>>>>>>>>>
>>>>>>>>>>>> http://patchwork.ozlabs.org/patch/438581/
>>>>>>>>>>>
>>>>>>>>>>> Ah ... Hmm... so "./arch/arm/cpu/armv7/am33xx/board.c" needs
>>>>>>>>>>> a rework, right?
>>>>>>>>>>>
>>>>>>>>>>> Is a simple rename s_init() -> board_init_f() correct?
>>>>>>>>>>
>>>>>>>>>> Right so, no, we can't just rename s_init to board_init_f.  This is what
>>>>>>>>>> I was talking about in the thread about the function Hans wants to add
>>>>>>>>>> to enable some bits in CP15 on sunxi, iirc.
>>>>>>>>>>
>>>>>>>>>> In short, armv7 has a different set of abstraction hooks than the
>>>>>>>>>> previous ARM cores (armv8 followed what we have for v7) and I'm not
>>>>>>>>>> convinced in the end that it really won us anything.  See
>>>>>>>>>> http://lists.denx.de/pipermail/u-boot/2015-January/202350.html
>>>>>>>>>>
>>>>>>>>>> For today you need to rework the Siemens code to print out the DDR
>>>>>>>>>> values (when desired) in spl_board_init() as we do not, or will not
>>>>>>>>>> shortly, have gd prior to board_init_f running.
>>>>>>>>>
>>>>>>>>> Hmm... first I thought, ok, no problem, move the output from the RAM
>>>>>>>>> parameters to spl_board_init() ... but thats only the half of the
>>>>>>>>> story ... They read the RAM parameters from an i2c eeprom, and if
>>>>>>>>> there are errors, they print this errors ... currently this does
>>>>>>>>> not work, and thats I think the more important case ... and I could
>>>>>>>>> not move this error printfs to somewhere, because if RAM is not
>>>>>>>>> working ... there is no later ...
>>>>>>>>>
>>>>>>>>> So I have to enable the console early ... maybe I missed something,
>>>>>>>>> but this worked fine in the past (and I think we should not break
>>>>>>>>> this, as this is an essential feature).
>>>>>>>>
>>>>>>>> OK, I missed something too.  I think this gets better now once I merge
>>>>>>>> Simon's SPL series as we do all of this from board_init_f() and the
>>>>>>>> siemens code should just work again.
>>>>>>>
>>>>>>> Yes, just saw your patch ;-)
>>>>>>>
>>>>>>> If they are in mainline (or do you have them somewhere in a git repo?),
>>>>>>> I test it again on the dxr2 board, thanks!
>>>>>>
>>>>>> If I am correct, all needed patches from you are in mainline, just
>>>>>> tried this on the dxr2 board ... but I still need:
>>>>>>
>>>>>> diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
>>>>>> index a39cbd0..8724604 100644
>>>>>> --- a/board/siemens/common/board.c
>>>>>> +++ b/board/siemens/common/board.c
>>>>>> @@ -40,6 +40,11 @@ void set_uart_mux_conf(void)
>>>>>>
>>>>>>   void set_mux_conf_regs(void)
>>>>>>   {
>>>>>> +       /* enable early the console */
>>>>>> +       gd->baudrate = CONFIG_BAUDRATE;
>>>>>> +       serial_init();
>>>>>> +       gd->have_console = 1;
>>>>>> +
>>>>>>          /* Initalize the board header */
>>>>>>          enable_i2c0_pin_mux();
>>>>>>          i2c_set_bus_num(0);
>>>>>>
>>>>>> to see the console output ...
>>>>>
>>>>> Yes, if you want console prior to spl_board_init() then you need to do
>>>>> that where you are.
>>>>>
>>>>
>>>> Ah, ok... so this patch is OK?
>>>
>>> Just found this thread again, as I want to post updates for the am335x
>>> based siemens boards ... Is this patch [1] Ok, and can get applied?
>>>
>>> Thanks!
>>>
>>> bye,
>>> Heiko
>>> [1] Patchwork [U-Boot] am33xx, spl, siemens: enable debug uart output again
>>> https://patchwork.ozlabs.org/patch/446644/
>>>
>>> Hups... found this patch in patchwork marked as RFC and Delegated
>>> to me ... ?
>>
>> I'll look it over again then, thanks.
>
> It's a bit confusing - am I right in thinking that this is called from
> board_early_init_f() in arch/arm/cpu/armv7/am33xx/board.c? That in
> turn is called from board_init_f() in the same file?

I think so, yes (Could not currently power on the board, so I look
in it, if I have it again accessable)

> If so it seems OK, although I wonder if the SPL console setup could
> happen in board_early_init_f() or even board_init_f() rather than
> being buried so deep? It seems odd that setting up the mux registers
> should enable the console, and I doubt people will find it easily.

Yes, board_early_init_f() seems a better place for me too, but this
affects then all am335x boards (or we add a weak function, which
boards can define)?

since commit:

a6b541b09022: TI ARMv7: Don't use GD before crt0.S has set it

early debug output is not longer shown on the siemens am335x boards,
and this patch fixes this issue ... I cannot say, if we need this
for all am335x boards ...

bye,
Heiko
diff mbox

Patch

diff --git a/board/siemens/common/board.c b/board/siemens/common/board.c
index a39cbd0..8724604 100644
--- a/board/siemens/common/board.c
+++ b/board/siemens/common/board.c
@@ -40,6 +40,11 @@  void set_uart_mux_conf(void)

  void set_mux_conf_regs(void)
  {
+       /* enable early the console */
+       gd->baudrate = CONFIG_BAUDRATE;
+       serial_init();
+       gd->have_console = 1;
+
         /* Initalize the board header */
         enable_i2c0_pin_mux();
         i2c_set_bus_num(0);