[PATCH u-boot 0/3] Revert broken Bootmenu commits
Stefan Roese
sr at denx.de
Thu Jul 13 17:12:14 CEST 2023
Hi,
(added Jaehoon Chung to Cc)
On 7/10/23 18:03, Stefan Roese wrote:
> On 7/10/23 17:17, Tom Rini wrote:
>> On Mon, Jul 10, 2023 at 04:10:39PM +0200, Pali Rohár wrote:
>>> On Sunday 25 June 2023 21:08:19 Tom Rini wrote:
>>>> On Sun, Jun 25, 2023 at 05:15:34PM +0200, Pali Rohár wrote:
>>>>> On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
>>>>>> On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
>>>>>>> On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
>>>>>>>> On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
>>>>>>>>> On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
>>>>>>>>>> Hi Tom, Pali,
>>>>>>>>>>
>>>>>>>>>> On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini at konsulko.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
>>>>>>>>>>>
>>>>>>>>>>>> These 3 commits broke support for U-Boot Bootmenu on Nokia
>>>>>>>>>>>> N900.
>>>>>>>>>>>> Issues were reported more than month ago, but nobody reacted
>>>>>>>>>>>> to them.
>>>>>>>>>>>> So revert these broken commits for now, to fix U-Boot
>>>>>>>>>>>> Bootmenu support.
>>>>>>>>>>>>
>>>>>>>>>>>> With these revered commits, U-Boot Bootmenu from master
>>>>>>>>>>>> branch is
>>>>>>>>>>>> working fine again on Nokia N900.
>>>>>>>>>>>>
>>>>>>>>>>>> Pali Rohár (3):
>>>>>>>>>>>> Revert "video: Enable VIDEO_ANSI by default only with EFI"
>>>>>>>>>>>> Revert "menu: Factor out menu-keypress decoding"
>>>>>>>>>>>> Revert "menu: Make use of CLI character processing"
>>>>>>>>>>>>
>>>>>>>>>>>> cmd/bootmenu.c | 9 ++--
>>>>>>>>>>>> cmd/eficonfig.c | 12 ++---
>>>>>>>>>>>> common/cli_getch.c | 12 ++---
>>>>>>>>>>>> common/menu.c | 122
>>>>>>>>>>>> ++++++++++++++++++++++++++----------------
>>>>>>>>>>>> drivers/video/Kconfig | 7 +--
>>>>>>>>>>>> include/cli.h | 4 +-
>>>>>>>>>>>> include/menu.h | 17 +-----
>>>>>>>>>>>> 7 files changed, 91 insertions(+), 92 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> Following up over here, while
>>>>>>>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
>>>>>>>>>>> addresses some of the issues, but not all (as it clearly
>>>>>>>>>>> isn't working
>>>>>>>>>>> in all of the cases Pali has explained), looking in to the
>>>>>>>>>>> VIDEO_ANSI
>>>>>>>>>>> part of this too, all three of these reverts are related
>>>>>>>>>>> seemingly to
>>>>>>>>>>> something escape-character related. I'm not taking any of
>>>>>>>>>>> the revert
>>>>>>>>>>> commits in just yet, but will by the next -rc if we don't
>>>>>>>>>>> have something
>>>>>>>>>>> that fixes all of the issues.
>>>>>>>>>>
>>>>>>>>>> I did send a series [1] with a fix for the nokia_rx51 keyboard
>>>>>>>>>> driver,
>>>>>>>>>> including the previous ansi-code patch. Given that:
>>>>>>>>>>
>>>>>>>>>> - this problem doesn't seem to manifest on other boards
>>>>>>>>>> - it does not cause any CI test to fail
>>>>>>>>>> - there seem to be bugs in the nokia_rx51 implementation which
>>>>>>>>>> are a
>>>>>>>>>> possible/likely cause
>>>>>>>>>> - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
>>>>>>>>>> - the problem goes away if debugging is added, suggesting it is
>>>>>>>>>> related to timing
>>>>>>>>>>
>>>>>>>>>> ...I don't think a revert is appropriate.
>>>>>>>>>
>>>>>>>>> Unfortunately it does not fix this issue :-( New patch series
>>>>>>>>> from [1]
>>>>>>>>> applied on top of the master branch has still issue with DOWN
>>>>>>>>> key on
>>>>>>>>> emulated UART terminal.
>>>>>>>>>
>>>>>>>>>> Pali, can you please take a look and see if you can debug what is
>>>>>>>>>> actually going on? Here is my guess:
>>>>>>>>>>
>>>>>>>>>> 1. When an arrow key is pressed, cli_ch_process() handles it
>>>>>>>>>> by being
>>>>>>>>>> passed the codes in sequence: \e [ B
>>>>>>>>>> 2. Calling cli_ch_process() with ichar = 0 causes it to think the
>>>>>>>>>> sequence is finished: \e [ \0 B (this doesn't work since the
>>>>>>>>>> \0 ends
>>>>>>>>>> the sequence)
>>>>>>>>>> 3. nokia_rx51 keyboard driver is returning '\0' even when key
>>>>>>>>>> codes
>>>>>>>>>> are pending, causing cli_ch_process() to be told that the
>>>>>>>>>> sequence is
>>>>>>>>>> done
>>>>>>>>>
>>>>>>>>> I can look at it later, but I'm loosing motivation to do whole
>>>>>>>>> debugging
>>>>>>>>> for another issue again, because for the last year my fixes and
>>>>>>>>> other
>>>>>>>>> patches were stalled and u-boot devs show me that they are not
>>>>>>>>> interested even in commenting them. I do not want to work again on
>>>>>>>>> something which would be ignored.
>>>>>>>>
>>>>>>>> Well, unless your changes here break something else, I don't
>>>>>>>> think a fix
>>>>>>>> for your problem will be ignored.
>>>>>>>
>>>>>>> This is something which I read here more times in the past... and
>>>>>>> reality was different.
>>>>>>>
>>>>>>> Should I remind you that there are waiting eMMC fixes for mvebu and
>>>>>>> again discussion about them stalled? Or rather should I say that
>>>>>>> it is
>>>>>>> again ignored?
>>>>>>
>>>>>> No, you should just say they're still waiting for review. Since
>>>>>> they're
>>>>>> waiting for review. The MMC custodian has been asked to review
>>>>>> them, and
>>>>>> hasn't yet. And they don't appear to be super critical changes,
>>>>>> and they
>>>>>> conflict with other series, so yes, they'll get picked up when the
>>>>>> custodian has time to review everything.
>>>>>
>>>>> And what is "critical change" if it is not fixing booting (from eMMC)?
>>>>
>>>> The when and where, and since when. Since re-reading everything in
>>>> https://patchwork.ozlabs.org/project/uboot/list/?submitter=78810 that's
>>>> not this revert series doesn't say "fix booting on $platform that was
>>>> broken by ...". It says clean up. Clean up can wait.
>>>
>>> "Fix eMMC boot" - This is not clear that it is a "fix"???
>>>
>>> Or are you again going to play a game "I do not see your patches"?
>>> Ok, then here is direct link:
>>>
>>> https://lore.kernel.org/u-boot/20230413205750.10641-1-pali@kernel.org/
>>
>> Oh, oh, I think I get it now. You're wondering why I guess:
>> https://patchwork.ozlabs.org/project/uboot/patch/20230505193710.n35h2ofq6fogk4bq@pali/
>> https://patchwork.ozlabs.org/project/uboot/patch/20230516184943.7206-1-pali@kernel.org/
>> haven't been picked up? They're assigned to Stefan.
>
> Hmmm, I might have missed something here. I'm traveling right now, so
> my time is limited this week. Perhaps by end of this week. Or the
> patches should be delegated to some other custodian if this makes
> sense.
So I've worked though a few of the pending patches and also the 2 eMMC
related ones mentioned above. They depend on this series AFAICT:
mmc: Explain and cleanup partition selection
Which is currently being reviewed by Jaehoon Chung AFAIU. Once this is
applied I'll take care of these other 2 patches above (if possible).
Thanks,
Stefan
More information about the U-Boot
mailing list