| Message ID | 20190107193617.27034-1-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot] arm: socfpga: make debug uart work on socfpga_gen5 | expand |
On 1/7/19 8:36 PM, Simon Goldschmidt wrote: > When debug UART is enabled on socfpga_gen5, the debug uart driver hangs > in an endless loop because 'socfpga_bridges_reset' calls printf before > the debug UART is initialized. > > After the generic fix for this in the UART driver did not work due to > portability issues, let's just drop this printf statement when called > from SPL with debug UART enabled. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> Can we have an un-portable fix which at least works on SoCFPGA ? :) > --- > > This is about my fourth try to get the debug uart usable on socfpga gen5. > Hope this time I'll make it :-) > It's really annoying to have local diffs only for enabling the debug uart! > > arch/arm/mach-socfpga/reset_manager_gen5.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c > index 25baef79bc..39d8fbed94 100644 > --- a/arch/arm/mach-socfpga/reset_manager_gen5.c > +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c > @@ -90,7 +90,10 @@ void socfpga_bridges_reset(int enable) > if (!fpgamgr_test_fpga_ready()) { > /* FPGA not ready, do nothing. We allow system to boot > * without FPGA ready. So, return 0 instead of error. */ > +#if !defined CONFIG_SPL_BUILD || !defined CONFIG_DEBUG_UART > + /* In SPL, this is called before debug-uart init... */ > printf("%s: FPGA not ready, aborting.\n", __func__); > +#endif > return; > } > >
Am 07.01.2019 um 21:19 schrieb Marek Vasut: > On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >> When debug UART is enabled on socfpga_gen5, the debug uart driver hangs >> in an endless loop because 'socfpga_bridges_reset' calls printf before >> the debug UART is initialized. >> >> After the generic fix for this in the UART driver did not work due to >> portability issues, let's just drop this printf statement when called >> from SPL with debug UART enabled. >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > > Can we have an un-portable fix which at least works on SoCFPGA ? :) This one worked on socfpga but broke rockchip: https://patchwork.ozlabs.org/patch/992553/ However, the message below wasn't shown either with that patch applied. The code just runs too early to enable the UART. Do you want to keep the message (although I failed to see in which situation it can be printed) or do you just dislike the #ifdef thing? Regards, Simon > >> --- >> >> This is about my fourth try to get the debug uart usable on socfpga gen5. >> Hope this time I'll make it :-) >> It's really annoying to have local diffs only for enabling the debug uart! >> >> arch/arm/mach-socfpga/reset_manager_gen5.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c >> index 25baef79bc..39d8fbed94 100644 >> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c >> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c >> @@ -90,7 +90,10 @@ void socfpga_bridges_reset(int enable) >> if (!fpgamgr_test_fpga_ready()) { >> /* FPGA not ready, do nothing. We allow system to boot >> * without FPGA ready. So, return 0 instead of error. */ >> +#if !defined CONFIG_SPL_BUILD || !defined CONFIG_DEBUG_UART >> + /* In SPL, this is called before debug-uart init... */ >> printf("%s: FPGA not ready, aborting.\n", __func__); >> +#endif >> return; >> } >> >> > >
On 1/7/19 9:24 PM, Simon Goldschmidt wrote: > Am 07.01.2019 um 21:19 schrieb Marek Vasut: >> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>> When debug UART is enabled on socfpga_gen5, the debug uart driver hangs >>> in an endless loop because 'socfpga_bridges_reset' calls printf before >>> the debug UART is initialized. >>> >>> After the generic fix for this in the UART driver did not work due to >>> portability issues, let's just drop this printf statement when called >>> from SPL with debug UART enabled. >>> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> >> Can we have an un-portable fix which at least works on SoCFPGA ? :) > > This one worked on socfpga but broke rockchip: > > https://patchwork.ozlabs.org/patch/992553/ > > However, the message below wasn't shown either with that patch applied. > The code just runs too early to enable the UART. > > Do you want to keep the message (although I failed to see in which > situation it can be printed) or do you just dislike the #ifdef thing? I'd like to keep the error message if possible. Is it possible ?
Am 07.01.2019 um 21:25 schrieb Marek Vasut: > On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>> When debug UART is enabled on socfpga_gen5, the debug uart driver hangs >>>> in an endless loop because 'socfpga_bridges_reset' calls printf before >>>> the debug UART is initialized. >>>> >>>> After the generic fix for this in the UART driver did not work due to >>>> portability issues, let's just drop this printf statement when called >>>> from SPL with debug UART enabled. >>>> >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>> >>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >> >> This one worked on socfpga but broke rockchip: >> >> https://patchwork.ozlabs.org/patch/992553/ >> >> However, the message below wasn't shown either with that patch applied. >> The code just runs too early to enable the UART. >> >> Do you want to keep the message (although I failed to see in which >> situation it can be printed) or do you just dislike the #ifdef thing? > > I'd like to keep the error message if possible. Is it possible ? I have *never* seen this message yet. I have failed to produce a situation where it is shown. This function ('socfpga_bridges_reset') is called 5 times throughout the code, but only *one* single time with 'reset=0' as argument (only with 0, the code in question is executed). And this is in SPL before initializing the console and even before the debug UART can be initialized. As I could see, the printf *is* executed on every boot (I saw the code hanging when enabling debug UART). However, when not booting from FPGA, it is just normal that the FPGA is not ready when running SPL. Why do you want an error message here anyway? Regards, Simon
On 1/7/19 9:33 PM, Simon Goldschmidt wrote: > Am 07.01.2019 um 21:25 schrieb Marek Vasut: >> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver >>>>> hangs >>>>> in an endless loop because 'socfpga_bridges_reset' calls printf before >>>>> the debug UART is initialized. >>>>> >>>>> After the generic fix for this in the UART driver did not work due to >>>>> portability issues, let's just drop this printf statement when called >>>>> from SPL with debug UART enabled. >>>>> >>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>> >>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >>> >>> This one worked on socfpga but broke rockchip: >>> >>> https://patchwork.ozlabs.org/patch/992553/ >>> >>> However, the message below wasn't shown either with that patch applied. >>> The code just runs too early to enable the UART. >>> >>> Do you want to keep the message (although I failed to see in which >>> situation it can be printed) or do you just dislike the #ifdef thing? >> >> I'd like to keep the error message if possible. Is it possible ? > > I have *never* seen this message yet. I have failed to produce a > situation where it is shown. I believe that. > This function ('socfpga_bridges_reset') is called 5 times throughout the > code, but only *one* single time with 'reset=0' as argument (only with > 0, the code in question is executed). And this is in SPL before > initializing the console and even before the debug UART can be initialized. > > As I could see, the printf *is* executed on every boot (I saw the code > hanging when enabling debug UART). However, when not booting from FPGA, > it is just normal that the FPGA is not ready when running SPL. Why do > you want an error message here anyway? I was under the impression this is an error message, but it might not be so ? Maybe the wording is incorrect ?
Am 07.01.2019 um 21:47 schrieb Marek Vasut: > On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver >>>>>> hangs >>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf before >>>>>> the debug UART is initialized. >>>>>> >>>>>> After the generic fix for this in the UART driver did not work due to >>>>>> portability issues, let's just drop this printf statement when called >>>>>> from SPL with debug UART enabled. >>>>>> >>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>> >>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >>>> >>>> This one worked on socfpga but broke rockchip: >>>> >>>> https://patchwork.ozlabs.org/patch/992553/ >>>> >>>> However, the message below wasn't shown either with that patch applied. >>>> The code just runs too early to enable the UART. >>>> >>>> Do you want to keep the message (although I failed to see in which >>>> situation it can be printed) or do you just dislike the #ifdef thing? >>> >>> I'd like to keep the error message if possible. Is it possible ? >> >> I have *never* seen this message yet. I have failed to produce a >> situation where it is shown. > > I believe that. > >> This function ('socfpga_bridges_reset') is called 5 times throughout the >> code, but only *one* single time with 'reset=0' as argument (only with >> 0, the code in question is executed). And this is in SPL before >> initializing the console and even before the debug UART can be initialized. >> >> As I could see, the printf *is* executed on every boot (I saw the code >> hanging when enabling debug UART). However, when not booting from FPGA, >> it is just normal that the FPGA is not ready when running SPL. Why do >> you want an error message here anyway? > > I was under the impression this is an error message, but it might not be > so ? Maybe the wording is incorrect ? Now that I re-read it, "aborting" is incorrect, yes. So how should we proceed? This is an error message that can never be shown (like the code is now) but breaks debug UART. I'd say we can drop it altogether. It might only be interesint if (in the future) that code would get called from somewhere else (i.e. later, after console initialization). Re-reading spl_gen5.c, there are some 'debug' calls before the debug uart is initialized which probably would need to be removed as well, but that's a different story... Regards, Simon
On 1/7/19 10:01 PM, Simon Goldschmidt wrote: > Am 07.01.2019 um 21:47 schrieb Marek Vasut: >> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver >>>>>>> hangs >>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf >>>>>>> before >>>>>>> the debug UART is initialized. >>>>>>> >>>>>>> After the generic fix for this in the UART driver did not work >>>>>>> due to >>>>>>> portability issues, let's just drop this printf statement when >>>>>>> called >>>>>>> from SPL with debug UART enabled. >>>>>>> >>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>> >>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >>>>> >>>>> This one worked on socfpga but broke rockchip: >>>>> >>>>> https://patchwork.ozlabs.org/patch/992553/ >>>>> >>>>> However, the message below wasn't shown either with that patch >>>>> applied. >>>>> The code just runs too early to enable the UART. >>>>> >>>>> Do you want to keep the message (although I failed to see in which >>>>> situation it can be printed) or do you just dislike the #ifdef thing? >>>> >>>> I'd like to keep the error message if possible. Is it possible ? >>> >>> I have *never* seen this message yet. I have failed to produce a >>> situation where it is shown. >> >> I believe that. >> >>> This function ('socfpga_bridges_reset') is called 5 times throughout the >>> code, but only *one* single time with 'reset=0' as argument (only with >>> 0, the code in question is executed). And this is in SPL before >>> initializing the console and even before the debug UART can be >>> initialized. >>> >>> As I could see, the printf *is* executed on every boot (I saw the code >>> hanging when enabling debug UART). However, when not booting from FPGA, >>> it is just normal that the FPGA is not ready when running SPL. Why do >>> you want an error message here anyway? >> >> I was under the impression this is an error message, but it might not be >> so ? Maybe the wording is incorrect ? > > Now that I re-read it, "aborting" is incorrect, yes. > > So how should we proceed? This is an error message that can never be > shown (like the code is now) but breaks debug UART. > > I'd say we can drop it altogether. It might only be interesint if (in > the future) that code would get called from somewhere else (i.e. later, > after console initialization). > > Re-reading spl_gen5.c, there are some 'debug' calls before the debug > uart is initialized which probably would need to be removed as well, but > that's a different story... How come those don't hang the system then ?
On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: > > On 1/7/19 10:01 PM, Simon Goldschmidt wrote: > > Am 07.01.2019 um 21:47 schrieb Marek Vasut: > >> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: > >>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: > >>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: > >>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: > >>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: > >>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver > >>>>>>> hangs > >>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf > >>>>>>> before > >>>>>>> the debug UART is initialized. > >>>>>>> > >>>>>>> After the generic fix for this in the UART driver did not work > >>>>>>> due to > >>>>>>> portability issues, let's just drop this printf statement when > >>>>>>> called > >>>>>>> from SPL with debug UART enabled. > >>>>>>> > >>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>>>>> > >>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) > >>>>> > >>>>> This one worked on socfpga but broke rockchip: > >>>>> > >>>>> https://patchwork.ozlabs.org/patch/992553/ > >>>>> > >>>>> However, the message below wasn't shown either with that patch > >>>>> applied. > >>>>> The code just runs too early to enable the UART. > >>>>> > >>>>> Do you want to keep the message (although I failed to see in which > >>>>> situation it can be printed) or do you just dislike the #ifdef thing? > >>>> > >>>> I'd like to keep the error message if possible. Is it possible ? > >>> > >>> I have *never* seen this message yet. I have failed to produce a > >>> situation where it is shown. > >> > >> I believe that. > >> > >>> This function ('socfpga_bridges_reset') is called 5 times throughout the > >>> code, but only *one* single time with 'reset=0' as argument (only with > >>> 0, the code in question is executed). And this is in SPL before > >>> initializing the console and even before the debug UART can be > >>> initialized. > >>> > >>> As I could see, the printf *is* executed on every boot (I saw the code > >>> hanging when enabling debug UART). However, when not booting from FPGA, > >>> it is just normal that the FPGA is not ready when running SPL. Why do > >>> you want an error message here anyway? > >> > >> I was under the impression this is an error message, but it might not be > >> so ? Maybe the wording is incorrect ? > > > > Now that I re-read it, "aborting" is incorrect, yes. > > > > So how should we proceed? This is an error message that can never be > > shown (like the code is now) but breaks debug UART. > > > > I'd say we can drop it altogether. It might only be interesint if (in > > the future) that code would get called from somewhere else (i.e. later, > > after console initialization). > > > > Re-reading spl_gen5.c, there are some 'debug' calls before the debug > > uart is initialized which probably would need to be removed as well, but > > that's a different story... > > How come those don't hang the system then ? I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang the system when enabling them. While it would be easy to remove these debug statements, to be future-proof it would of course make sense to make the debug UART robust against this. But given the problems with Rockchip ns16550, we would need a dedicated debug UART for socfpga to solve this. And that would probably mean code duplication. Regards, Simon
On 1/8/19 7:41 AM, Simon Goldschmidt wrote: > On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: >>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: >>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver >>>>>>>>> hangs >>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf >>>>>>>>> before >>>>>>>>> the debug UART is initialized. >>>>>>>>> >>>>>>>>> After the generic fix for this in the UART driver did not work >>>>>>>>> due to >>>>>>>>> portability issues, let's just drop this printf statement when >>>>>>>>> called >>>>>>>>> from SPL with debug UART enabled. >>>>>>>>> >>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>>>> >>>>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >>>>>>> >>>>>>> This one worked on socfpga but broke rockchip: >>>>>>> >>>>>>> https://patchwork.ozlabs.org/patch/992553/ >>>>>>> >>>>>>> However, the message below wasn't shown either with that patch >>>>>>> applied. >>>>>>> The code just runs too early to enable the UART. >>>>>>> >>>>>>> Do you want to keep the message (although I failed to see in which >>>>>>> situation it can be printed) or do you just dislike the #ifdef thing? >>>>>> >>>>>> I'd like to keep the error message if possible. Is it possible ? >>>>> >>>>> I have *never* seen this message yet. I have failed to produce a >>>>> situation where it is shown. >>>> >>>> I believe that. >>>> >>>>> This function ('socfpga_bridges_reset') is called 5 times throughout the >>>>> code, but only *one* single time with 'reset=0' as argument (only with >>>>> 0, the code in question is executed). And this is in SPL before >>>>> initializing the console and even before the debug UART can be >>>>> initialized. >>>>> >>>>> As I could see, the printf *is* executed on every boot (I saw the code >>>>> hanging when enabling debug UART). However, when not booting from FPGA, >>>>> it is just normal that the FPGA is not ready when running SPL. Why do >>>>> you want an error message here anyway? >>>> >>>> I was under the impression this is an error message, but it might not be >>>> so ? Maybe the wording is incorrect ? >>> >>> Now that I re-read it, "aborting" is incorrect, yes. >>> >>> So how should we proceed? This is an error message that can never be >>> shown (like the code is now) but breaks debug UART. >>> >>> I'd say we can drop it altogether. It might only be interesint if (in >>> the future) that code would get called from somewhere else (i.e. later, >>> after console initialization). >>> >>> Re-reading spl_gen5.c, there are some 'debug' calls before the debug >>> uart is initialized which probably would need to be removed as well, but >>> that's a different story... >> >> How come those don't hang the system then ? > > I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang > the system when enabling them. > > While it would be easy to remove these debug statements, to be future-proof > it would of course make sense to make the debug UART robust against this. > > But given the problems with Rockchip ns16550, we would need a dedicated > debug UART for socfpga to solve this. And that would probably mean code > duplication. What is the problem with Rockchip ? I don't want various SoCs blocking others.
On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex@denx.de> wrote: > > On 1/8/19 7:41 AM, Simon Goldschmidt wrote: > > On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: > >>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: > >>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: > >>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: > >>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: > >>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: > >>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: > >>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver > >>>>>>>>> hangs > >>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf > >>>>>>>>> before > >>>>>>>>> the debug UART is initialized. > >>>>>>>>> > >>>>>>>>> After the generic fix for this in the UART driver did not work > >>>>>>>>> due to > >>>>>>>>> portability issues, let's just drop this printf statement when > >>>>>>>>> called > >>>>>>>>> from SPL with debug UART enabled. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>>>>>>> > >>>>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) > >>>>>>> > >>>>>>> This one worked on socfpga but broke rockchip: > >>>>>>> > >>>>>>> https://patchwork.ozlabs.org/patch/992553/ > >>>>>>> > >>>>>>> However, the message below wasn't shown either with that patch > >>>>>>> applied. > >>>>>>> The code just runs too early to enable the UART. > >>>>>>> > >>>>>>> Do you want to keep the message (although I failed to see in which > >>>>>>> situation it can be printed) or do you just dislike the #ifdef thing? > >>>>>> > >>>>>> I'd like to keep the error message if possible. Is it possible ? > >>>>> > >>>>> I have *never* seen this message yet. I have failed to produce a > >>>>> situation where it is shown. > >>>> > >>>> I believe that. > >>>> > >>>>> This function ('socfpga_bridges_reset') is called 5 times throughout the > >>>>> code, but only *one* single time with 'reset=0' as argument (only with > >>>>> 0, the code in question is executed). And this is in SPL before > >>>>> initializing the console and even before the debug UART can be > >>>>> initialized. > >>>>> > >>>>> As I could see, the printf *is* executed on every boot (I saw the code > >>>>> hanging when enabling debug UART). However, when not booting from FPGA, > >>>>> it is just normal that the FPGA is not ready when running SPL. Why do > >>>>> you want an error message here anyway? > >>>> > >>>> I was under the impression this is an error message, but it might not be > >>>> so ? Maybe the wording is incorrect ? > >>> > >>> Now that I re-read it, "aborting" is incorrect, yes. > >>> > >>> So how should we proceed? This is an error message that can never be > >>> shown (like the code is now) but breaks debug UART. > >>> > >>> I'd say we can drop it altogether. It might only be interesint if (in > >>> the future) that code would get called from somewhere else (i.e. later, > >>> after console initialization). > >>> > >>> Re-reading spl_gen5.c, there are some 'debug' calls before the debug > >>> uart is initialized which probably would need to be removed as well, but > >>> that's a different story... > >> > >> How come those don't hang the system then ? > > > > I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang > > the system when enabling them. > > > > While it would be easy to remove these debug statements, to be future-proof > > it would of course make sense to make the debug UART robust against this. > > > > But given the problems with Rockchip ns16550, we would need a dedicated > > debug UART for socfpga to solve this. And that would probably mean code > > duplication. > > What is the problem with Rockchip ? I don't want various SoCs blocking > others. I had sent a patch that does not wait for the TX fifo to hold more bytes if the baudrate prescaler is 0 (according to both the socfgpa and the rockchip docs, the UART is disabled if the prescaler is 0). However, it seems that the prescaler was read back as 0 on a rockchip board which caused chars to be missing from the console output. See this mail: https://lists.denx.de/pipermail/u-boot/2018-December/350355.html I checked with Henri and did not find a solution so I reverted the patch: https://patchwork.ozlabs.org/patch/1007211/ Keeping this patch but only for selected platforms would be my favourite, but it would at least mean we need yet another debug UART selection, plus some changes to make the "prescaler == 0" detection specific to this new debug UART. Would this be better acceptable? Regards, Simon
On 1/8/19 1:06 PM, Simon Goldschmidt wrote: > On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/8/19 7:41 AM, Simon Goldschmidt wrote: >>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: >>>>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: >>>>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >>>>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>>>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver >>>>>>>>>>> hangs >>>>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf >>>>>>>>>>> before >>>>>>>>>>> the debug UART is initialized. >>>>>>>>>>> >>>>>>>>>>> After the generic fix for this in the UART driver did not work >>>>>>>>>>> due to >>>>>>>>>>> portability issues, let's just drop this printf statement when >>>>>>>>>>> called >>>>>>>>>>> from SPL with debug UART enabled. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>>>>>> >>>>>>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >>>>>>>>> >>>>>>>>> This one worked on socfpga but broke rockchip: >>>>>>>>> >>>>>>>>> https://patchwork.ozlabs.org/patch/992553/ >>>>>>>>> >>>>>>>>> However, the message below wasn't shown either with that patch >>>>>>>>> applied. >>>>>>>>> The code just runs too early to enable the UART. >>>>>>>>> >>>>>>>>> Do you want to keep the message (although I failed to see in which >>>>>>>>> situation it can be printed) or do you just dislike the #ifdef thing? >>>>>>>> >>>>>>>> I'd like to keep the error message if possible. Is it possible ? >>>>>>> >>>>>>> I have *never* seen this message yet. I have failed to produce a >>>>>>> situation where it is shown. >>>>>> >>>>>> I believe that. >>>>>> >>>>>>> This function ('socfpga_bridges_reset') is called 5 times throughout the >>>>>>> code, but only *one* single time with 'reset=0' as argument (only with >>>>>>> 0, the code in question is executed). And this is in SPL before >>>>>>> initializing the console and even before the debug UART can be >>>>>>> initialized. >>>>>>> >>>>>>> As I could see, the printf *is* executed on every boot (I saw the code >>>>>>> hanging when enabling debug UART). However, when not booting from FPGA, >>>>>>> it is just normal that the FPGA is not ready when running SPL. Why do >>>>>>> you want an error message here anyway? >>>>>> >>>>>> I was under the impression this is an error message, but it might not be >>>>>> so ? Maybe the wording is incorrect ? >>>>> >>>>> Now that I re-read it, "aborting" is incorrect, yes. >>>>> >>>>> So how should we proceed? This is an error message that can never be >>>>> shown (like the code is now) but breaks debug UART. >>>>> >>>>> I'd say we can drop it altogether. It might only be interesint if (in >>>>> the future) that code would get called from somewhere else (i.e. later, >>>>> after console initialization). >>>>> >>>>> Re-reading spl_gen5.c, there are some 'debug' calls before the debug >>>>> uart is initialized which probably would need to be removed as well, but >>>>> that's a different story... >>>> >>>> How come those don't hang the system then ? >>> >>> I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang >>> the system when enabling them. >>> >>> While it would be easy to remove these debug statements, to be future-proof >>> it would of course make sense to make the debug UART robust against this. >>> >>> But given the problems with Rockchip ns16550, we would need a dedicated >>> debug UART for socfpga to solve this. And that would probably mean code >>> duplication. >> >> What is the problem with Rockchip ? I don't want various SoCs blocking >> others. > > I had sent a patch that does not wait for the TX fifo to hold more bytes if the > baudrate prescaler is 0 (according to both the socfgpa and the rockchip docs, > the UART is disabled if the prescaler is 0). > > However, it seems that the prescaler was read back as 0 on a rockchip board > which caused chars to be missing from the console output. > > See this mail: > https://lists.denx.de/pipermail/u-boot/2018-December/350355.html > > I checked with Henri and did not find a solution so I reverted the patch: > https://patchwork.ozlabs.org/patch/1007211/ > > Keeping this patch but only for selected platforms would be my favourite, but it > would at least mean we need yet another debug UART selection, plus some changes > to make the "prescaler == 0" detection specific to this new debug UART. > Would this be better acceptable? Doesn't the DT compatible tell you the UART type ? It does, so you can match on that and apply the workaround accordingly . Or you can cache the prescaler.
On Tue, Jan 8, 2019 at 1:08 PM Marek Vasut <marex@denx.de> wrote: > > On 1/8/19 1:06 PM, Simon Goldschmidt wrote: > > On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/8/19 7:41 AM, Simon Goldschmidt wrote: > >>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: > >>>>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: > >>>>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: > >>>>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: > >>>>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: > >>>>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: > >>>>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: > >>>>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver > >>>>>>>>>>> hangs > >>>>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf > >>>>>>>>>>> before > >>>>>>>>>>> the debug UART is initialized. > >>>>>>>>>>> > >>>>>>>>>>> After the generic fix for this in the UART driver did not work > >>>>>>>>>>> due to > >>>>>>>>>>> portability issues, let's just drop this printf statement when > >>>>>>>>>>> called > >>>>>>>>>>> from SPL with debug UART enabled. > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>>>>>>>>> > >>>>>>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) > >>>>>>>>> > >>>>>>>>> This one worked on socfpga but broke rockchip: > >>>>>>>>> > >>>>>>>>> https://patchwork.ozlabs.org/patch/992553/ > >>>>>>>>> > >>>>>>>>> However, the message below wasn't shown either with that patch > >>>>>>>>> applied. > >>>>>>>>> The code just runs too early to enable the UART. > >>>>>>>>> > >>>>>>>>> Do you want to keep the message (although I failed to see in which > >>>>>>>>> situation it can be printed) or do you just dislike the #ifdef thing? > >>>>>>>> > >>>>>>>> I'd like to keep the error message if possible. Is it possible ? > >>>>>>> > >>>>>>> I have *never* seen this message yet. I have failed to produce a > >>>>>>> situation where it is shown. > >>>>>> > >>>>>> I believe that. > >>>>>> > >>>>>>> This function ('socfpga_bridges_reset') is called 5 times throughout the > >>>>>>> code, but only *one* single time with 'reset=0' as argument (only with > >>>>>>> 0, the code in question is executed). And this is in SPL before > >>>>>>> initializing the console and even before the debug UART can be > >>>>>>> initialized. > >>>>>>> > >>>>>>> As I could see, the printf *is* executed on every boot (I saw the code > >>>>>>> hanging when enabling debug UART). However, when not booting from FPGA, > >>>>>>> it is just normal that the FPGA is not ready when running SPL. Why do > >>>>>>> you want an error message here anyway? > >>>>>> > >>>>>> I was under the impression this is an error message, but it might not be > >>>>>> so ? Maybe the wording is incorrect ? > >>>>> > >>>>> Now that I re-read it, "aborting" is incorrect, yes. > >>>>> > >>>>> So how should we proceed? This is an error message that can never be > >>>>> shown (like the code is now) but breaks debug UART. > >>>>> > >>>>> I'd say we can drop it altogether. It might only be interesint if (in > >>>>> the future) that code would get called from somewhere else (i.e. later, > >>>>> after console initialization). > >>>>> > >>>>> Re-reading spl_gen5.c, there are some 'debug' calls before the debug > >>>>> uart is initialized which probably would need to be removed as well, but > >>>>> that's a different story... > >>>> > >>>> How come those don't hang the system then ? > >>> > >>> I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang > >>> the system when enabling them. > >>> > >>> While it would be easy to remove these debug statements, to be future-proof > >>> it would of course make sense to make the debug UART robust against this. > >>> > >>> But given the problems with Rockchip ns16550, we would need a dedicated > >>> debug UART for socfpga to solve this. And that would probably mean code > >>> duplication. > >> > >> What is the problem with Rockchip ? I don't want various SoCs blocking > >> others. > > > > I had sent a patch that does not wait for the TX fifo to hold more bytes if the > > baudrate prescaler is 0 (according to both the socfgpa and the rockchip docs, > > the UART is disabled if the prescaler is 0). > > > > However, it seems that the prescaler was read back as 0 on a rockchip board > > which caused chars to be missing from the console output. > > > > See this mail: > > https://lists.denx.de/pipermail/u-boot/2018-December/350355.html > > > > I checked with Henri and did not find a solution so I reverted the patch: > > https://patchwork.ozlabs.org/patch/1007211/ > > > > Keeping this patch but only for selected platforms would be my favourite, but it > > would at least mean we need yet another debug UART selection, plus some changes > > to make the "prescaler == 0" detection specific to this new debug UART. > > Would this be better acceptable? > > Doesn't the DT compatible tell you the UART type ? It does, so you can > match on that and apply the workaround accordingly . We're talking about debug UART here only. No DT compatible involved. > Or you can cache the prescaler. We already had that. Simon mentioned that on some platforms you neither have bss available nor 'gd' at the point where the debug UART is inialized, so there's not portable storage to cache the prescaler, either :-( The best I can think of right now would probably be to extend the list of debug UARTS with an socfpga specific type which compiles the same code as DEBUG_UART_NS16550 but enables this prescaler check. Regards, Simon
On 1/8/19 1:42 PM, Simon Goldschmidt wrote: > On Tue, Jan 8, 2019 at 1:08 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/8/19 1:06 PM, Simon Goldschmidt wrote: >>> On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 1/8/19 7:41 AM, Simon Goldschmidt wrote: >>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: >>>>>>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: >>>>>>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >>>>>>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>>>>>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>>>>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>>>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver >>>>>>>>>>>>> hangs >>>>>>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf >>>>>>>>>>>>> before >>>>>>>>>>>>> the debug UART is initialized. >>>>>>>>>>>>> >>>>>>>>>>>>> After the generic fix for this in the UART driver did not work >>>>>>>>>>>>> due to >>>>>>>>>>>>> portability issues, let's just drop this printf statement when >>>>>>>>>>>>> called >>>>>>>>>>>>> from SPL with debug UART enabled. >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>>>>>>>> >>>>>>>>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >>>>>>>>>>> >>>>>>>>>>> This one worked on socfpga but broke rockchip: >>>>>>>>>>> >>>>>>>>>>> https://patchwork.ozlabs.org/patch/992553/ >>>>>>>>>>> >>>>>>>>>>> However, the message below wasn't shown either with that patch >>>>>>>>>>> applied. >>>>>>>>>>> The code just runs too early to enable the UART. >>>>>>>>>>> >>>>>>>>>>> Do you want to keep the message (although I failed to see in which >>>>>>>>>>> situation it can be printed) or do you just dislike the #ifdef thing? >>>>>>>>>> >>>>>>>>>> I'd like to keep the error message if possible. Is it possible ? >>>>>>>>> >>>>>>>>> I have *never* seen this message yet. I have failed to produce a >>>>>>>>> situation where it is shown. >>>>>>>> >>>>>>>> I believe that. >>>>>>>> >>>>>>>>> This function ('socfpga_bridges_reset') is called 5 times throughout the >>>>>>>>> code, but only *one* single time with 'reset=0' as argument (only with >>>>>>>>> 0, the code in question is executed). And this is in SPL before >>>>>>>>> initializing the console and even before the debug UART can be >>>>>>>>> initialized. >>>>>>>>> >>>>>>>>> As I could see, the printf *is* executed on every boot (I saw the code >>>>>>>>> hanging when enabling debug UART). However, when not booting from FPGA, >>>>>>>>> it is just normal that the FPGA is not ready when running SPL. Why do >>>>>>>>> you want an error message here anyway? >>>>>>>> >>>>>>>> I was under the impression this is an error message, but it might not be >>>>>>>> so ? Maybe the wording is incorrect ? >>>>>>> >>>>>>> Now that I re-read it, "aborting" is incorrect, yes. >>>>>>> >>>>>>> So how should we proceed? This is an error message that can never be >>>>>>> shown (like the code is now) but breaks debug UART. >>>>>>> >>>>>>> I'd say we can drop it altogether. It might only be interesint if (in >>>>>>> the future) that code would get called from somewhere else (i.e. later, >>>>>>> after console initialization). >>>>>>> >>>>>>> Re-reading spl_gen5.c, there are some 'debug' calls before the debug >>>>>>> uart is initialized which probably would need to be removed as well, but >>>>>>> that's a different story... >>>>>> >>>>>> How come those don't hang the system then ? >>>>> >>>>> I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang >>>>> the system when enabling them. >>>>> >>>>> While it would be easy to remove these debug statements, to be future-proof >>>>> it would of course make sense to make the debug UART robust against this. >>>>> >>>>> But given the problems with Rockchip ns16550, we would need a dedicated >>>>> debug UART for socfpga to solve this. And that would probably mean code >>>>> duplication. >>>> >>>> What is the problem with Rockchip ? I don't want various SoCs blocking >>>> others. >>> >>> I had sent a patch that does not wait for the TX fifo to hold more bytes if the >>> baudrate prescaler is 0 (according to both the socfgpa and the rockchip docs, >>> the UART is disabled if the prescaler is 0). >>> >>> However, it seems that the prescaler was read back as 0 on a rockchip board >>> which caused chars to be missing from the console output. >>> >>> See this mail: >>> https://lists.denx.de/pipermail/u-boot/2018-December/350355.html >>> >>> I checked with Henri and did not find a solution so I reverted the patch: >>> https://patchwork.ozlabs.org/patch/1007211/ >>> >>> Keeping this patch but only for selected platforms would be my favourite, but it >>> would at least mean we need yet another debug UART selection, plus some changes >>> to make the "prescaler == 0" detection specific to this new debug UART. >>> Would this be better acceptable? >> >> Doesn't the DT compatible tell you the UART type ? It does, so you can >> match on that and apply the workaround accordingly . > > We're talking about debug UART here only. No DT compatible involved. > >> Or you can cache the prescaler. > > We already had that. Simon mentioned that on some platforms you neither have bss > available nor 'gd' at the point where the debug UART is inialized, so > there's not portable > storage to cache the prescaler, either :-( > > The best I can think of right now would probably be to extend the list > of debug UARTS > with an socfpga specific type which compiles the same code as DEBUG_UART_NS16550 > but enables this prescaler check. Uhhhhhh, unless you find a better option, that'd be fine by me.
Am 08.01.2019 um 13:56 schrieb Marek Vasut: > On 1/8/19 1:42 PM, Simon Goldschmidt wrote: >> On Tue, Jan 8, 2019 at 1:08 PM Marek Vasut <marex@denx.de> wrote: >>> >>> On 1/8/19 1:06 PM, Simon Goldschmidt wrote: >>>> On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>>> >>>>> On 1/8/19 7:41 AM, Simon Goldschmidt wrote: >>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: >>>>>>> >>>>>>> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: >>>>>>>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: >>>>>>>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >>>>>>>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>>>>>>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>>>>>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>>>>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver >>>>>>>>>>>>>> hangs >>>>>>>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf >>>>>>>>>>>>>> before >>>>>>>>>>>>>> the debug UART is initialized. >>>>>>>>>>>>>> >>>>>>>>>>>>>> After the generic fix for this in the UART driver did not work >>>>>>>>>>>>>> due to >>>>>>>>>>>>>> portability issues, let's just drop this printf statement when >>>>>>>>>>>>>> called >>>>>>>>>>>>>> from SPL with debug UART enabled. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>>>>>>>>> >>>>>>>>>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :) >>>>>>>>>>>> >>>>>>>>>>>> This one worked on socfpga but broke rockchip: >>>>>>>>>>>> >>>>>>>>>>>> https://patchwork.ozlabs.org/patch/992553/ >>>>>>>>>>>> >>>>>>>>>>>> However, the message below wasn't shown either with that patch >>>>>>>>>>>> applied. >>>>>>>>>>>> The code just runs too early to enable the UART. >>>>>>>>>>>> >>>>>>>>>>>> Do you want to keep the message (although I failed to see in which >>>>>>>>>>>> situation it can be printed) or do you just dislike the #ifdef thing? >>>>>>>>>>> >>>>>>>>>>> I'd like to keep the error message if possible. Is it possible ? >>>>>>>>>> >>>>>>>>>> I have *never* seen this message yet. I have failed to produce a >>>>>>>>>> situation where it is shown. >>>>>>>>> >>>>>>>>> I believe that. >>>>>>>>> >>>>>>>>>> This function ('socfpga_bridges_reset') is called 5 times throughout the >>>>>>>>>> code, but only *one* single time with 'reset=0' as argument (only with >>>>>>>>>> 0, the code in question is executed). And this is in SPL before >>>>>>>>>> initializing the console and even before the debug UART can be >>>>>>>>>> initialized. >>>>>>>>>> >>>>>>>>>> As I could see, the printf *is* executed on every boot (I saw the code >>>>>>>>>> hanging when enabling debug UART). However, when not booting from FPGA, >>>>>>>>>> it is just normal that the FPGA is not ready when running SPL. Why do >>>>>>>>>> you want an error message here anyway? >>>>>>>>> >>>>>>>>> I was under the impression this is an error message, but it might not be >>>>>>>>> so ? Maybe the wording is incorrect ? >>>>>>>> >>>>>>>> Now that I re-read it, "aborting" is incorrect, yes. >>>>>>>> >>>>>>>> So how should we proceed? This is an error message that can never be >>>>>>>> shown (like the code is now) but breaks debug UART. >>>>>>>> >>>>>>>> I'd say we can drop it altogether. It might only be interesint if (in >>>>>>>> the future) that code would get called from somewhere else (i.e. later, >>>>>>>> after console initialization). >>>>>>>> >>>>>>>> Re-reading spl_gen5.c, there are some 'debug' calls before the debug >>>>>>>> uart is initialized which probably would need to be removed as well, but >>>>>>>> that's a different story... >>>>>>> >>>>>>> How come those don't hang the system then ? >>>>>> >>>>>> I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang >>>>>> the system when enabling them. >>>>>> >>>>>> While it would be easy to remove these debug statements, to be future-proof >>>>>> it would of course make sense to make the debug UART robust against this. >>>>>> >>>>>> But given the problems with Rockchip ns16550, we would need a dedicated >>>>>> debug UART for socfpga to solve this. And that would probably mean code >>>>>> duplication. >>>>> >>>>> What is the problem with Rockchip ? I don't want various SoCs blocking >>>>> others. >>>> >>>> I had sent a patch that does not wait for the TX fifo to hold more bytes if the >>>> baudrate prescaler is 0 (according to both the socfgpa and the rockchip docs, >>>> the UART is disabled if the prescaler is 0). >>>> >>>> However, it seems that the prescaler was read back as 0 on a rockchip board >>>> which caused chars to be missing from the console output. >>>> >>>> See this mail: >>>> https://lists.denx.de/pipermail/u-boot/2018-December/350355.html >>>> >>>> I checked with Henri and did not find a solution so I reverted the patch: >>>> https://patchwork.ozlabs.org/patch/1007211/ >>>> >>>> Keeping this patch but only for selected platforms would be my favourite, but it >>>> would at least mean we need yet another debug UART selection, plus some changes >>>> to make the "prescaler == 0" detection specific to this new debug UART. >>>> Would this be better acceptable? >>> >>> Doesn't the DT compatible tell you the UART type ? It does, so you can >>> match on that and apply the workaround accordingly . >> >> We're talking about debug UART here only. No DT compatible involved. >> >>> Or you can cache the prescaler. >> >> We already had that. Simon mentioned that on some platforms you neither have bss >> available nor 'gd' at the point where the debug UART is inialized, so >> there's not portable >> storage to cache the prescaler, either :-( >> >> The best I can think of right now would probably be to extend the list >> of debug UARTS >> with an socfpga specific type which compiles the same code as DEBUG_UART_NS16550 >> but enables this prescaler check. > > Uhhhhhh, unless you find a better option, that'd be fine by me. This patch should be dropped in favour to another patch taht adds a Kconfig option to check the baudrate prescaler: https://patchwork.ozlabs.org/patch/1022600/ Regards, Simon
On 1/14/19 4:21 PM, Simon Goldschmidt wrote: > Am 08.01.2019 um 13:56 schrieb Marek Vasut: >> On 1/8/19 1:42 PM, Simon Goldschmidt wrote: >>> On Tue, Jan 8, 2019 at 1:08 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 1/8/19 1:06 PM, Simon Goldschmidt wrote: >>>>> On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 1/8/19 7:41 AM, Simon Goldschmidt wrote: >>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: >>>>>>>>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: >>>>>>>>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >>>>>>>>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>>>>>>>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>>>>>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug >>>>>>>>>>>>>>> uart driver >>>>>>>>>>>>>>> hangs >>>>>>>>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls >>>>>>>>>>>>>>> printf >>>>>>>>>>>>>>> before >>>>>>>>>>>>>>> the debug UART is initialized. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> After the generic fix for this in the UART driver did not >>>>>>>>>>>>>>> work >>>>>>>>>>>>>>> due to >>>>>>>>>>>>>>> portability issues, let's just drop this printf statement >>>>>>>>>>>>>>> when >>>>>>>>>>>>>>> called >>>>>>>>>>>>>>> from SPL with debug UART enabled. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Simon Goldschmidt >>>>>>>>>>>>>>> <simon.k.r.goldschmidt@gmail.com> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Can we have an un-portable fix which at least works on >>>>>>>>>>>>>> SoCFPGA ? :) >>>>>>>>>>>>> >>>>>>>>>>>>> This one worked on socfpga but broke rockchip: >>>>>>>>>>>>> >>>>>>>>>>>>> https://patchwork.ozlabs.org/patch/992553/ >>>>>>>>>>>>> >>>>>>>>>>>>> However, the message below wasn't shown either with that patch >>>>>>>>>>>>> applied. >>>>>>>>>>>>> The code just runs too early to enable the UART. >>>>>>>>>>>>> >>>>>>>>>>>>> Do you want to keep the message (although I failed to see >>>>>>>>>>>>> in which >>>>>>>>>>>>> situation it can be printed) or do you just dislike the >>>>>>>>>>>>> #ifdef thing? >>>>>>>>>>>> >>>>>>>>>>>> I'd like to keep the error message if possible. Is it >>>>>>>>>>>> possible ? >>>>>>>>>>> >>>>>>>>>>> I have *never* seen this message yet. I have failed to produce a >>>>>>>>>>> situation where it is shown. >>>>>>>>>> >>>>>>>>>> I believe that. >>>>>>>>>> >>>>>>>>>>> This function ('socfpga_bridges_reset') is called 5 times >>>>>>>>>>> throughout the >>>>>>>>>>> code, but only *one* single time with 'reset=0' as argument >>>>>>>>>>> (only with >>>>>>>>>>> 0, the code in question is executed). And this is in SPL before >>>>>>>>>>> initializing the console and even before the debug UART can be >>>>>>>>>>> initialized. >>>>>>>>>>> >>>>>>>>>>> As I could see, the printf *is* executed on every boot (I saw >>>>>>>>>>> the code >>>>>>>>>>> hanging when enabling debug UART). However, when not booting >>>>>>>>>>> from FPGA, >>>>>>>>>>> it is just normal that the FPGA is not ready when running >>>>>>>>>>> SPL. Why do >>>>>>>>>>> you want an error message here anyway? >>>>>>>>>> >>>>>>>>>> I was under the impression this is an error message, but it >>>>>>>>>> might not be >>>>>>>>>> so ? Maybe the wording is incorrect ? >>>>>>>>> >>>>>>>>> Now that I re-read it, "aborting" is incorrect, yes. >>>>>>>>> >>>>>>>>> So how should we proceed? This is an error message that can >>>>>>>>> never be >>>>>>>>> shown (like the code is now) but breaks debug UART. >>>>>>>>> >>>>>>>>> I'd say we can drop it altogether. It might only be interesint >>>>>>>>> if (in >>>>>>>>> the future) that code would get called from somewhere else >>>>>>>>> (i.e. later, >>>>>>>>> after console initialization). >>>>>>>>> >>>>>>>>> Re-reading spl_gen5.c, there are some 'debug' calls before the >>>>>>>>> debug >>>>>>>>> uart is initialized which probably would need to be removed as >>>>>>>>> well, but >>>>>>>>> that's a different story... >>>>>>>> >>>>>>>> How come those don't hang the system then ? >>>>>>> >>>>>>> I just haven't enabled debug output in spl_gen5.c, yet. I guess >>>>>>> they would hang >>>>>>> the system when enabling them. >>>>>>> >>>>>>> While it would be easy to remove these debug statements, to be >>>>>>> future-proof >>>>>>> it would of course make sense to make the debug UART robust >>>>>>> against this. >>>>>>> >>>>>>> But given the problems with Rockchip ns16550, we would need a >>>>>>> dedicated >>>>>>> debug UART for socfpga to solve this. And that would probably >>>>>>> mean code >>>>>>> duplication. >>>>>> >>>>>> What is the problem with Rockchip ? I don't want various SoCs >>>>>> blocking >>>>>> others. >>>>> >>>>> I had sent a patch that does not wait for the TX fifo to hold more >>>>> bytes if the >>>>> baudrate prescaler is 0 (according to both the socfgpa and the >>>>> rockchip docs, >>>>> the UART is disabled if the prescaler is 0). >>>>> >>>>> However, it seems that the prescaler was read back as 0 on a >>>>> rockchip board >>>>> which caused chars to be missing from the console output. >>>>> >>>>> See this mail: >>>>> https://lists.denx.de/pipermail/u-boot/2018-December/350355.html >>>>> >>>>> I checked with Henri and did not find a solution so I reverted the >>>>> patch: >>>>> https://patchwork.ozlabs.org/patch/1007211/ >>>>> >>>>> Keeping this patch but only for selected platforms would be my >>>>> favourite, but it >>>>> would at least mean we need yet another debug UART selection, plus >>>>> some changes >>>>> to make the "prescaler == 0" detection specific to this new debug >>>>> UART. >>>>> Would this be better acceptable? >>>> >>>> Doesn't the DT compatible tell you the UART type ? It does, so you can >>>> match on that and apply the workaround accordingly . >>> >>> We're talking about debug UART here only. No DT compatible involved. >>> >>>> Or you can cache the prescaler. >>> >>> We already had that. Simon mentioned that on some platforms you >>> neither have bss >>> available nor 'gd' at the point where the debug UART is inialized, so >>> there's not portable >>> storage to cache the prescaler, either :-( >>> >>> The best I can think of right now would probably be to extend the list >>> of debug UARTS >>> with an socfpga specific type which compiles the same code as >>> DEBUG_UART_NS16550 >>> but enables this prescaler check. >> >> Uhhhhhh, unless you find a better option, that'd be fine by me. > > This patch should be dropped in favour to another patch taht adds a > Kconfig option to check the baudrate prescaler: > > https://patchwork.ozlabs.org/patch/1022600/ So that's the real fix here ?
Am 14.01.2019 um 19:26 schrieb Marek Vasut: > On 1/14/19 4:21 PM, Simon Goldschmidt wrote: >> Am 08.01.2019 um 13:56 schrieb Marek Vasut: >>> On 1/8/19 1:42 PM, Simon Goldschmidt wrote: >>>> On Tue, Jan 8, 2019 at 1:08 PM Marek Vasut <marex@denx.de> wrote: >>>>> >>>>> On 1/8/19 1:06 PM, Simon Goldschmidt wrote: >>>>>> On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>>>>> >>>>>>> On 1/8/19 7:41 AM, Simon Goldschmidt wrote: >>>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>>> >>>>>>>>> On 1/7/19 10:01 PM, Simon Goldschmidt wrote: >>>>>>>>>> Am 07.01.2019 um 21:47 schrieb Marek Vasut: >>>>>>>>>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote: >>>>>>>>>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut: >>>>>>>>>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut: >>>>>>>>>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug >>>>>>>>>>>>>>>> uart driver >>>>>>>>>>>>>>>> hangs >>>>>>>>>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls >>>>>>>>>>>>>>>> printf >>>>>>>>>>>>>>>> before >>>>>>>>>>>>>>>> the debug UART is initialized. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> After the generic fix for this in the UART driver did not >>>>>>>>>>>>>>>> work >>>>>>>>>>>>>>>> due to >>>>>>>>>>>>>>>> portability issues, let's just drop this printf statement >>>>>>>>>>>>>>>> when >>>>>>>>>>>>>>>> called >>>>>>>>>>>>>>>> from SPL with debug UART enabled. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Signed-off-by: Simon Goldschmidt >>>>>>>>>>>>>>>> <simon.k.r.goldschmidt@gmail.com> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Can we have an un-portable fix which at least works on >>>>>>>>>>>>>>> SoCFPGA ? :) >>>>>>>>>>>>>> >>>>>>>>>>>>>> This one worked on socfpga but broke rockchip: >>>>>>>>>>>>>> >>>>>>>>>>>>>> https://patchwork.ozlabs.org/patch/992553/ >>>>>>>>>>>>>> >>>>>>>>>>>>>> However, the message below wasn't shown either with that patch >>>>>>>>>>>>>> applied. >>>>>>>>>>>>>> The code just runs too early to enable the UART. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Do you want to keep the message (although I failed to see >>>>>>>>>>>>>> in which >>>>>>>>>>>>>> situation it can be printed) or do you just dislike the >>>>>>>>>>>>>> #ifdef thing? >>>>>>>>>>>>> >>>>>>>>>>>>> I'd like to keep the error message if possible. Is it >>>>>>>>>>>>> possible ? >>>>>>>>>>>> >>>>>>>>>>>> I have *never* seen this message yet. I have failed to produce a >>>>>>>>>>>> situation where it is shown. >>>>>>>>>>> >>>>>>>>>>> I believe that. >>>>>>>>>>> >>>>>>>>>>>> This function ('socfpga_bridges_reset') is called 5 times >>>>>>>>>>>> throughout the >>>>>>>>>>>> code, but only *one* single time with 'reset=0' as argument >>>>>>>>>>>> (only with >>>>>>>>>>>> 0, the code in question is executed). And this is in SPL before >>>>>>>>>>>> initializing the console and even before the debug UART can be >>>>>>>>>>>> initialized. >>>>>>>>>>>> >>>>>>>>>>>> As I could see, the printf *is* executed on every boot (I saw >>>>>>>>>>>> the code >>>>>>>>>>>> hanging when enabling debug UART). However, when not booting >>>>>>>>>>>> from FPGA, >>>>>>>>>>>> it is just normal that the FPGA is not ready when running >>>>>>>>>>>> SPL. Why do >>>>>>>>>>>> you want an error message here anyway? >>>>>>>>>>> >>>>>>>>>>> I was under the impression this is an error message, but it >>>>>>>>>>> might not be >>>>>>>>>>> so ? Maybe the wording is incorrect ? >>>>>>>>>> >>>>>>>>>> Now that I re-read it, "aborting" is incorrect, yes. >>>>>>>>>> >>>>>>>>>> So how should we proceed? This is an error message that can >>>>>>>>>> never be >>>>>>>>>> shown (like the code is now) but breaks debug UART. >>>>>>>>>> >>>>>>>>>> I'd say we can drop it altogether. It might only be interesint >>>>>>>>>> if (in >>>>>>>>>> the future) that code would get called from somewhere else >>>>>>>>>> (i.e. later, >>>>>>>>>> after console initialization). >>>>>>>>>> >>>>>>>>>> Re-reading spl_gen5.c, there are some 'debug' calls before the >>>>>>>>>> debug >>>>>>>>>> uart is initialized which probably would need to be removed as >>>>>>>>>> well, but >>>>>>>>>> that's a different story... >>>>>>>>> >>>>>>>>> How come those don't hang the system then ? >>>>>>>> >>>>>>>> I just haven't enabled debug output in spl_gen5.c, yet. I guess >>>>>>>> they would hang >>>>>>>> the system when enabling them. >>>>>>>> >>>>>>>> While it would be easy to remove these debug statements, to be >>>>>>>> future-proof >>>>>>>> it would of course make sense to make the debug UART robust >>>>>>>> against this. >>>>>>>> >>>>>>>> But given the problems with Rockchip ns16550, we would need a >>>>>>>> dedicated >>>>>>>> debug UART for socfpga to solve this. And that would probably >>>>>>>> mean code >>>>>>>> duplication. >>>>>>> >>>>>>> What is the problem with Rockchip ? I don't want various SoCs >>>>>>> blocking >>>>>>> others. >>>>>> >>>>>> I had sent a patch that does not wait for the TX fifo to hold more >>>>>> bytes if the >>>>>> baudrate prescaler is 0 (according to both the socfgpa and the >>>>>> rockchip docs, >>>>>> the UART is disabled if the prescaler is 0). >>>>>> >>>>>> However, it seems that the prescaler was read back as 0 on a >>>>>> rockchip board >>>>>> which caused chars to be missing from the console output. >>>>>> >>>>>> See this mail: >>>>>> https://lists.denx.de/pipermail/u-boot/2018-December/350355.html >>>>>> >>>>>> I checked with Henri and did not find a solution so I reverted the >>>>>> patch: >>>>>> https://patchwork.ozlabs.org/patch/1007211/ >>>>>> >>>>>> Keeping this patch but only for selected platforms would be my >>>>>> favourite, but it >>>>>> would at least mean we need yet another debug UART selection, plus >>>>>> some changes >>>>>> to make the "prescaler == 0" detection specific to this new debug >>>>>> UART. >>>>>> Would this be better acceptable? >>>>> >>>>> Doesn't the DT compatible tell you the UART type ? It does, so you can >>>>> match on that and apply the workaround accordingly . >>>> >>>> We're talking about debug UART here only. No DT compatible involved. >>>> >>>>> Or you can cache the prescaler. >>>> >>>> We already had that. Simon mentioned that on some platforms you >>>> neither have bss >>>> available nor 'gd' at the point where the debug UART is inialized, so >>>> there's not portable >>>> storage to cache the prescaler, either :-( >>>> >>>> The best I can think of right now would probably be to extend the list >>>> of debug UARTS >>>> with an socfpga specific type which compiles the same code as >>>> DEBUG_UART_NS16550 >>>> but enables this prescaler check. >>> >>> Uhhhhhh, unless you find a better option, that'd be fine by me. >> >> This patch should be dropped in favour to another patch taht adds a >> Kconfig option to check the baudrate prescaler: >> >> https://patchwork.ozlabs.org/patch/1022600/ > > So that's the real fix here ? Yes: The real fix is that the uart driver should not sit in an endless loop (because the tx buffer does not get empty) when its 'putc' function is called but it isn't enabled. The above mentione patch does this, but it only does so when specifically enabled because this patch somehow fails at least on rockchip. And since the driver (ns16550) is pretty generic, it might fail on other chips, too. Regards, Simon
diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c index 25baef79bc..39d8fbed94 100644 --- a/arch/arm/mach-socfpga/reset_manager_gen5.c +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c @@ -90,7 +90,10 @@ void socfpga_bridges_reset(int enable) if (!fpgamgr_test_fpga_ready()) { /* FPGA not ready, do nothing. We allow system to boot * without FPGA ready. So, return 0 instead of error. */ +#if !defined CONFIG_SPL_BUILD || !defined CONFIG_DEBUG_UART + /* In SPL, this is called before debug-uart init... */ printf("%s: FPGA not ready, aborting.\n", __func__); +#endif return; }
When debug UART is enabled on socfpga_gen5, the debug uart driver hangs in an endless loop because 'socfpga_bridges_reset' calls printf before the debug UART is initialized. After the generic fix for this in the UART driver did not work due to portability issues, let's just drop this printf statement when called from SPL with debug UART enabled. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- This is about my fourth try to get the debug uart usable on socfpga gen5. Hope this time I'll make it :-) It's really annoying to have local diffs only for enabling the debug uart! arch/arm/mach-socfpga/reset_manager_gen5.c | 3 +++ 1 file changed, 3 insertions(+)