| Message ID | 1492944643-859-5-git-send-email-noltari@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
On 04/23/2017 12:50 PM, Álvaro Fernández Rojas wrote: > From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > > All MIPS boards that support debug uart are calling debug_uart_init right at > the beginning of board_early_init_f. > Instead of doing that, let's provide a generic call to debug_uart_init right > before the call to board_init_f if debug uart is enabled. > > Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > arch/mips/cpu/start.S | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S > index 6740fdf..f7dee81 100644 > --- a/arch/mips/cpu/start.S > +++ b/arch/mips/cpu/start.S > @@ -265,6 +265,12 @@ wr_done: > setup_stack_gd > #endif > > +#ifdef CONFIG_DEBUG_UART > + PTR_LA t9, debug_uart_init This should be called from C code somewhere, in fact, doesn't SPL's common code call debug_uart_init already ? > + jalr t9 > + nop > +#endif > + > move a0, zero # a0 <-- boot_flags = 0 > PTR_LA t9, board_init_f > >
Hi Marek, El 23/04/2017 a las 13:09, Marek Vasut escribió: > On 04/23/2017 12:50 PM, Álvaro Fernández Rojas wrote: >> From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >> >> All MIPS boards that support debug uart are calling debug_uart_init right at >> the beginning of board_early_init_f. >> Instead of doing that, let's provide a generic call to debug_uart_init right >> before the call to board_init_f if debug uart is enabled. >> >> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> --- >> arch/mips/cpu/start.S | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S >> index 6740fdf..f7dee81 100644 >> --- a/arch/mips/cpu/start.S >> +++ b/arch/mips/cpu/start.S >> @@ -265,6 +265,12 @@ wr_done: >> setup_stack_gd >> #endif >> >> +#ifdef CONFIG_DEBUG_UART >> + PTR_LA t9, debug_uart_init > > This should be called from C code somewhere, in fact, doesn't SPL's > common code call debug_uart_init already ? Why should this be called from C code? BTW, I don't think that SPL common code calls debug_uart_init. > >> + jalr t9 >> + nop >> +#endif >> + >> move a0, zero # a0 <-- boot_flags = 0 >> PTR_LA t9, board_init_f >> >> > > Regards, Álvaro.
On 04/23/2017 01:31 PM, Álvaro Fernández Rojas wrote: > Hi Marek, > > El 23/04/2017 a las 13:09, Marek Vasut escribió: >> On 04/23/2017 12:50 PM, Álvaro Fernández Rojas wrote: >>> From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>> >>> All MIPS boards that support debug uart are calling debug_uart_init right at >>> the beginning of board_early_init_f. >>> Instead of doing that, let's provide a generic call to debug_uart_init right >>> before the call to board_init_f if debug uart is enabled. >>> >>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>> --- >>> arch/mips/cpu/start.S | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S >>> index 6740fdf..f7dee81 100644 >>> --- a/arch/mips/cpu/start.S >>> +++ b/arch/mips/cpu/start.S >>> @@ -265,6 +265,12 @@ wr_done: >>> setup_stack_gd >>> #endif >>> >>> +#ifdef CONFIG_DEBUG_UART >>> + PTR_LA t9, debug_uart_init >> >> This should be called from C code somewhere, in fact, doesn't SPL's >> common code call debug_uart_init already ? > > Why should this be called from C code? Because the less stuff we have in assembler the better. > BTW, I don't think that SPL common code calls debug_uart_init. That's where it would make sense, grepping through the source indicates a lot of boards call this from their spl code. Also, you enter board_init_f right below, so you can start there ... >> >>> + jalr t9 >>> + nop >>> +#endif >>> + >>> move a0, zero # a0 <-- boot_flags = 0 >>> PTR_LA t9, board_init_f >>> >>> >> >> > > Regards, > Álvaro. >
Hi Marek, El 23/04/2017 a las 13:44, Marek Vasut escribió: > On 04/23/2017 01:31 PM, Álvaro Fernández Rojas wrote: >> Hi Marek, >> >> El 23/04/2017 a las 13:09, Marek Vasut escribió: >>> On 04/23/2017 12:50 PM, Álvaro Fernández Rojas wrote: >>>> From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>> >>>> All MIPS boards that support debug uart are calling debug_uart_init right at >>>> the beginning of board_early_init_f. >>>> Instead of doing that, let's provide a generic call to debug_uart_init right >>>> before the call to board_init_f if debug uart is enabled. >>>> >>>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>> --- >>>> arch/mips/cpu/start.S | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S >>>> index 6740fdf..f7dee81 100644 >>>> --- a/arch/mips/cpu/start.S >>>> +++ b/arch/mips/cpu/start.S >>>> @@ -265,6 +265,12 @@ wr_done: >>>> setup_stack_gd >>>> #endif >>>> >>>> +#ifdef CONFIG_DEBUG_UART >>>> + PTR_LA t9, debug_uart_init >>> >>> This should be called from C code somewhere, in fact, doesn't SPL's >>> common code call debug_uart_init already ? >> >> Why should this be called from C code? > > Because the less stuff we have in assembler the better. ... or not. > >> BTW, I don't think that SPL common code calls debug_uart_init. > > That's where it would make sense, grepping through the source indicates > a lot of boards call this from their spl code. Also, you enter > board_init_f right below, so you can start there ... Actually I was going to do that and Daniel suggested otherwise :) https://lists.denx.de/pipermail/u-boot/2017-April/287388.html --- I think we should move this to MIPS start.S to support this for all MIPS boards in a generic way. Puttings following code between 'setup_stack_gd' and the jump to 'board_init_f' should work: #ifdef CONFIG_DEBUG_UART PTR_LA t9, debug_uart_init jalr t9 nop #endif --- BTW, like Daniel I also think this is the proper way to go. > >>> >>>> + jalr t9 >>>> + nop >>>> +#endif >>>> + >>>> move a0, zero # a0 <-- boot_flags = 0 >>>> PTR_LA t9, board_init_f >>>> >>>> >>> >>> >> >> Regards, >> Álvaro. >> > > Regards, Álvaro.
On 04/23/2017 02:04 PM, Álvaro Fernández Rojas wrote: > Hi Marek, > > El 23/04/2017 a las 13:44, Marek Vasut escribió: >> On 04/23/2017 01:31 PM, Álvaro Fernández Rojas wrote: >>> Hi Marek, >>> >>> El 23/04/2017 a las 13:09, Marek Vasut escribió: >>>> On 04/23/2017 12:50 PM, Álvaro Fernández Rojas wrote: >>>>> From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>>> >>>>> All MIPS boards that support debug uart are calling debug_uart_init right at >>>>> the beginning of board_early_init_f. >>>>> Instead of doing that, let's provide a generic call to debug_uart_init right >>>>> before the call to board_init_f if debug uart is enabled. >>>>> >>>>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>>> --- >>>>> arch/mips/cpu/start.S | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S >>>>> index 6740fdf..f7dee81 100644 >>>>> --- a/arch/mips/cpu/start.S >>>>> +++ b/arch/mips/cpu/start.S >>>>> @@ -265,6 +265,12 @@ wr_done: >>>>> setup_stack_gd >>>>> #endif >>>>> >>>>> +#ifdef CONFIG_DEBUG_UART >>>>> + PTR_LA t9, debug_uart_init >>>> >>>> This should be called from C code somewhere, in fact, doesn't SPL's >>>> common code call debug_uart_init already ? >>> >>> Why should this be called from C code? >> >> Because the less stuff we have in assembler the better. > ... or not. > >> >>> BTW, I don't think that SPL common code calls debug_uart_init. >> >> That's where it would make sense, grepping through the source indicates >> a lot of boards call this from their spl code. Also, you enter >> board_init_f right below, so you can start there ... > Actually I was going to do that and Daniel suggested otherwise :) > https://lists.denx.de/pipermail/u-boot/2017-April/287388.html > --- > I think we should move this to MIPS start.S to support this for all MIPS > boards in a generic way. Puttings following code between > 'setup_stack_gd' and the jump to 'board_init_f' should work: > > #ifdef CONFIG_DEBUG_UART > PTR_LA t9, debug_uart_init > jalr t9 > nop > #endif > --- > BTW, like Daniel I also think this is the proper way to go. Can you elaborate why is it better to put more stuff into the assembler rather than C? I'd love to hear the rationale for that. I agree this should be called from common code , I disagree this should be called from assembler.
Am 23.04.2017 um 19:08 schrieb Marek Vasut: > On 04/23/2017 02:04 PM, Álvaro Fernández Rojas wrote: >> Hi Marek, >> >> El 23/04/2017 a las 13:44, Marek Vasut escribió: >>> On 04/23/2017 01:31 PM, Álvaro Fernández Rojas wrote: >>>> Hi Marek, >>>> >>>> El 23/04/2017 a las 13:09, Marek Vasut escribió: >>>>> On 04/23/2017 12:50 PM, Álvaro Fernández Rojas wrote: >>>>>> From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>>>> >>>>>> All MIPS boards that support debug uart are calling debug_uart_init right at >>>>>> the beginning of board_early_init_f. >>>>>> Instead of doing that, let's provide a generic call to debug_uart_init right >>>>>> before the call to board_init_f if debug uart is enabled. >>>>>> >>>>>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>>>> --- >>>>>> arch/mips/cpu/start.S | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S >>>>>> index 6740fdf..f7dee81 100644 >>>>>> --- a/arch/mips/cpu/start.S >>>>>> +++ b/arch/mips/cpu/start.S >>>>>> @@ -265,6 +265,12 @@ wr_done: >>>>>> setup_stack_gd >>>>>> #endif >>>>>> >>>>>> +#ifdef CONFIG_DEBUG_UART >>>>>> + PTR_LA t9, debug_uart_init >>>>> >>>>> This should be called from C code somewhere, in fact, doesn't SPL's >>>>> common code call debug_uart_init already ? >>>> >>>> Why should this be called from C code? >>> >>> Because the less stuff we have in assembler the better. >> ... or not. >> >>> >>>> BTW, I don't think that SPL common code calls debug_uart_init. >>> >>> That's where it would make sense, grepping through the source indicates >>> a lot of boards call this from their spl code. Also, you enter >>> board_init_f right below, so you can start there ... >> Actually I was going to do that and Daniel suggested otherwise :) >> https://lists.denx.de/pipermail/u-boot/2017-April/287388.html >> --- >> I think we should move this to MIPS start.S to support this for all MIPS >> boards in a generic way. Puttings following code between >> 'setup_stack_gd' and the jump to 'board_init_f' should work: >> >> #ifdef CONFIG_DEBUG_UART >> PTR_LA t9, debug_uart_init >> jalr t9 >> nop >> #endif >> --- >> BTW, like Daniel I also think this is the proper way to go. > > Can you elaborate why is it better to put more stuff into the assembler > rather than C? I'd love to hear the rationale for that. > > I agree this should be called from common code , I disagree this should > be called from assembler. > The purpose of debug_uart_init is to run as early as possible to provide debug output in the init code until serial_init/console_init_f have been executed. The earliest point for debug_uart_init depends on arch and/or SoC. On MIPS this point is directly after the setup_stack_gd macro. Actually there are two points on MIPS. One is before calling lowlevel_init when the stack can be used from some SRAM (option CONFIG_MIPS_INIT_STACK_IN_SRAM). This allows on some MIPS SoC's (not on all!) to implement lowlevel_init in C and to use debug_uart. This can't be realized with common code called from board_init_f. And no MIPS board currently boots with SPL, so calling debug_uart_init from spl.c is also not an option. Also the earliest board-specific callback from board_init_f is board_early_init_f from where some MIPS boards already call debug_uart_init. Before each new MIPS board copies this style, it's better to solve this in a generic way at least for MIPS. Solving this for all archs would mean to call debug_uart_init as the first function in board_init_f. But this would conflict with some archs (x86, xtensa, nios) which already calling debug_uart_init from start.S.
Am 23.04.2017 um 12:50 schrieb Álvaro Fernández Rojas: > From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > > All MIPS boards that support debug uart are calling debug_uart_init right at > the beginning of board_early_init_f. > Instead of doing that, let's provide a generic call to debug_uart_init right > before the call to board_init_f if debug uart is enabled. > > Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > arch/mips/cpu/start.S | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S > index 6740fdf..f7dee81 100644 > --- a/arch/mips/cpu/start.S > +++ b/arch/mips/cpu/start.S > @@ -265,6 +265,12 @@ wr_done: > setup_stack_gd > #endif > > +#ifdef CONFIG_DEBUG_UART > + PTR_LA t9, debug_uart_init > + jalr t9 > + nop > +#endif > + I think we should move this inside the "#ifndef CONFIG_MIPS_INIT_STACK_IN_SRAM ... #endif" block and also add this in the "#ifdef CONFIG_MIPS_INIT_STACK_IN_SRAM ... #endif" block above. This way a SoC can use debug_uart also in lowlevel_init if it supports the initial stack in SRAM with option CONFIG_MIPS_INIT_STACK_IN_SRAM. > move a0, zero # a0 <-- boot_flags = 0 > PTR_LA t9, board_init_f > >
On 04/23/2017 09:04 PM, Daniel Schwierzeck wrote: > > > Am 23.04.2017 um 19:08 schrieb Marek Vasut: >> On 04/23/2017 02:04 PM, Álvaro Fernández Rojas wrote: >>> Hi Marek, >>> >>> El 23/04/2017 a las 13:44, Marek Vasut escribió: >>>> On 04/23/2017 01:31 PM, Álvaro Fernández Rojas wrote: >>>>> Hi Marek, >>>>> >>>>> El 23/04/2017 a las 13:09, Marek Vasut escribió: >>>>>> On 04/23/2017 12:50 PM, Álvaro Fernández Rojas wrote: >>>>>>> From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>>>>> >>>>>>> All MIPS boards that support debug uart are calling debug_uart_init right at >>>>>>> the beginning of board_early_init_f. >>>>>>> Instead of doing that, let's provide a generic call to debug_uart_init right >>>>>>> before the call to board_init_f if debug uart is enabled. >>>>>>> >>>>>>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>>>>> --- >>>>>>> arch/mips/cpu/start.S | 6 ++++++ >>>>>>> 1 file changed, 6 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S >>>>>>> index 6740fdf..f7dee81 100644 >>>>>>> --- a/arch/mips/cpu/start.S >>>>>>> +++ b/arch/mips/cpu/start.S >>>>>>> @@ -265,6 +265,12 @@ wr_done: >>>>>>> setup_stack_gd >>>>>>> #endif >>>>>>> >>>>>>> +#ifdef CONFIG_DEBUG_UART >>>>>>> + PTR_LA t9, debug_uart_init >>>>>> >>>>>> This should be called from C code somewhere, in fact, doesn't SPL's >>>>>> common code call debug_uart_init already ? >>>>> >>>>> Why should this be called from C code? >>>> >>>> Because the less stuff we have in assembler the better. >>> ... or not. >>> >>>> >>>>> BTW, I don't think that SPL common code calls debug_uart_init. >>>> >>>> That's where it would make sense, grepping through the source indicates >>>> a lot of boards call this from their spl code. Also, you enter >>>> board_init_f right below, so you can start there ... >>> Actually I was going to do that and Daniel suggested otherwise :) >>> https://lists.denx.de/pipermail/u-boot/2017-April/287388.html >>> --- >>> I think we should move this to MIPS start.S to support this for all MIPS >>> boards in a generic way. Puttings following code between >>> 'setup_stack_gd' and the jump to 'board_init_f' should work: >>> >>> #ifdef CONFIG_DEBUG_UART >>> PTR_LA t9, debug_uart_init >>> jalr t9 >>> nop >>> #endif >>> --- >>> BTW, like Daniel I also think this is the proper way to go. >> >> Can you elaborate why is it better to put more stuff into the assembler >> rather than C? I'd love to hear the rationale for that. >> >> I agree this should be called from common code , I disagree this should >> be called from assembler. >> > > The purpose of debug_uart_init is to run as early as possible to provide > debug output in the init code until serial_init/console_init_f have been > executed. The earliest point for debug_uart_init depends on arch and/or > SoC. On MIPS this point is directly after the setup_stack_gd macro. > Actually there are two points on MIPS. One is before calling > lowlevel_init when the stack can be used from some SRAM (option > CONFIG_MIPS_INIT_STACK_IN_SRAM). This allows on some MIPS SoC's (not on > all!) to implement lowlevel_init in C and to use debug_uart. This can't > be realized with common code called from board_init_f. And no MIPS board > currently boots with SPL, so calling debug_uart_init from spl.c is also > not an option. > > Also the earliest board-specific callback from board_init_f is > board_early_init_f from where some MIPS boards already call > debug_uart_init. Before each new MIPS board copies this style, it's > better to solve this in a generic way at least for MIPS. Solving this > for all archs would mean to call debug_uart_init as the first function > in board_init_f. But this would conflict with some archs (x86, xtensa, > nios) which already calling debug_uart_init from start.S. Then it might be time to drain the swamp ? Although that'd likely be way out of scope of this patchset. Isn't arch_cpu_init() called way before board_early_init_f() and possibly the right place for you to put it ? If not, adding another entry into init_sequence_f might be an option (note that this patch adds the call to debug_uart_init() right before jumping into board_init_f , not right after setup_stack_gd ). I cannot say I'm a big fan of growing something comparable to init_sequence_f in mips assembler ... the assembler parts should only be used to bring up the most basic environment to run C code, the rest should be implemented in C code.
diff --git a/arch/mips/cpu/start.S b/arch/mips/cpu/start.S index 6740fdf..f7dee81 100644 --- a/arch/mips/cpu/start.S +++ b/arch/mips/cpu/start.S @@ -265,6 +265,12 @@ wr_done: setup_stack_gd #endif +#ifdef CONFIG_DEBUG_UART + PTR_LA t9, debug_uart_init + jalr t9 + nop +#endif + move a0, zero # a0 <-- boot_flags = 0 PTR_LA t9, board_init_f