commit 787f04bb6a - imx: add USB2_BOOT type

commit 787f04bb6a - imx: add USB2_BOOT type

[Rasmus Villemoes]

Hi Peng

It seems that commit 787f04bb6a (imx: add USB2_BOOT type) broke our
board logic which relies on whether get_boot_device() returns USB_BOOT
or not. Instrumenting get_boot_device() shows that on our imx8mp board,
boot_instance is 3 when we're booting over USB, so boot_dev becomes 20
instead of 17 aka USB_BOOT.

I assume that for the boards/socs you mentioned in the commit message,
boot_instance could be 0 or 1. However, I don't see anywhere that uses
that USB2_BOOT define.

Short-term, I'll probably just revert 787f04bb6a locally. A bit longer
term, I'm thinking I should be doing rom_api_query_boot_infor() and
"decoding" the return value myself.

Is there any chance you could make some information on that ROM API
public so it's possible for outsiders to understand what's going on?

Rasmus

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Peng Fan]

Hi Rasmus

On 10/7/2022 4:08 PM, Rasmus Villemoes wrote:
> Hi Peng
> 
> It seems that commit 787f04bb6a (imx: add USB2_BOOT type) broke our
> board logic which relies on whether get_boot_device() returns USB_BOOT
> or not. Instrumenting get_boot_device() shows that on our imx8mp board,
> boot_instance is 3 when we're booting over USB, so boot_dev becomes 20
> instead of 17 aka USB_BOOT.
> 
> I assume that for the boards/socs you mentioned in the commit message,
> boot_instance could be 0 or 1. However, I don't see anywhere that uses
> that USB2_BOOT define.
> 
> Short-term, I'll probably just revert 787f04bb6a locally. A bit longer
> term, I'm thinking I should be doing rom_api_query_boot_infor() and
> "decoding" the return value myself.
> 
> Is there any chance you could make some information on that ROM API
> public so it's possible for outsiders to understand what's going on?

Could you please try below changes to check whether it fixes your issue?

diff --git a/arch/arm/mach-imx/romapi.c b/arch/arm/mach-imx/romapi.c
index c8accdb04db..ad7f8640401 100644
--- a/arch/arm/mach-imx/romapi.c
+++ b/arch/arm/mach-imx/romapi.c
@@ -67,6 +67,8 @@ enum boot_device get_boot_device(void)
                 boot_dev = QSPI_BOOT;
                 break;
         case BT_DEV_TYPE_USB:
+               if (!is_imx8ulp() && !is_imx9())
+                       boot_instance = 0;
                 boot_dev = boot_instance + USB_BOOT;
                 break;
         default:

Thanks,
Peng.
> 
> Rasmus

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Rasmus Villemoes]

On 14/10/2022 02.55, Peng Fan wrote:
> Hi Rasmus
> 
> On 10/7/2022 4:08 PM, Rasmus Villemoes wrote:
>> Hi Peng
>>
>> It seems that commit 787f04bb6a (imx: add USB2_BOOT type) broke our
>> board logic which relies on whether get_boot_device() returns USB_BOOT
>> or not. Instrumenting get_boot_device() shows that on our imx8mp board,
>> boot_instance is 3 when we're booting over USB, so boot_dev becomes 20
>> instead of 17 aka USB_BOOT.
>>
>> I assume that for the boards/socs you mentioned in the commit message,
>> boot_instance could be 0 or 1. However, I don't see anywhere that uses
>> that USB2_BOOT define.
>>
>> Short-term, I'll probably just revert 787f04bb6a locally. A bit longer
>> term, I'm thinking I should be doing rom_api_query_boot_infor() and
>> "decoding" the return value myself.
>>
>> Is there any chance you could make some information on that ROM API
>> public so it's possible for outsiders to understand what's going on?
> 
> Could you please try below changes to check whether it fixes your issue?

Well, it seems very likely it would, but could you _please_ answer the
real question so we as a community has a chance of evaluating whether
that's the proper fix or something else entirely is needed. And so that
in the future we as a community would have a chance of objecting to
including 787f04bb6a in the first place.

Is there any chance you could make some information on that ROM API
public so it's possible for outsiders to understand what's going on?

Rasmus

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Peng Fan]

+ Stefano & Fabio

On 10/15/2022 1:53 AM, Rasmus Villemoes wrote:
> On 14/10/2022 02.55, Peng Fan wrote:
>> Hi Rasmus
>>
>> On 10/7/2022 4:08 PM, Rasmus Villemoes wrote:
>>> Hi Peng
>>>
>>> It seems that commit 787f04bb6a (imx: add USB2_BOOT type) broke our
>>> board logic which relies on whether get_boot_device() returns USB_BOOT
>>> or not. Instrumenting get_boot_device() shows that on our imx8mp board,
>>> boot_instance is 3 when we're booting over USB, so boot_dev becomes 20
>>> instead of 17 aka USB_BOOT.
>>>
>>> I assume that for the boards/socs you mentioned in the commit message,
>>> boot_instance could be 0 or 1. However, I don't see anywhere that uses
>>> that USB2_BOOT define.

It is for i.MX8ULP, i.MX93. The i.MX usb feature upstream is a bit slow
in my side, so the macro defined that, but not used for now.

>>>
>>> Short-term, I'll probably just revert 787f04bb6a locally. A bit longer
>>> term, I'm thinking I should be doing rom_api_query_boot_infor() and
>>> "decoding" the return value myself.

You no need to do that, the ROM API is common logic.

>>>
>>> Is there any chance you could make some information on that ROM API
>>> public so it's possible for outsiders to understand what's going on?

What could only help is to ask the ROM team to see whether they have 
plan to public the ROM API details and when. Otherwise you could only
read the code to understand how it works.

>>
>> Could you please try below changes to check whether it fixes your issue?
> 
> Well, it seems very likely it would, but could you _please_ answer the
> real question so we as a community has a chance of evaluating whether
> that's the proper fix or something else entirely is needed. And so that
> in the future we as a community would have a chance of objecting to
> including 787f04bb6a in the first place.

You could help reviewing if you have time.

Thanks,
Peng.
> 
> Is there any chance you could make some information on that ROM API
> public so it's possible for outsiders to understand what's going on?
> 
> Rasmus

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Rasmus Villemoes]

On 18/10/2022 02.43, Peng Fan wrote:
> + Stefano & Fabio
> 

>>>>
>>>> Is there any chance you could make some information on that ROM API
>>>> public so it's possible for outsiders to understand what's going on?
> 
> What could only help is to ask the ROM team to see whether they have
> plan to public the ROM API details and when. Otherwise you could only
> read the code to understand how it works.
> 
>>>
>>> Could you please try below changes to check whether it fixes your issue?
>>
>> Well, it seems very likely it would, but could you _please_ answer the
>> real question so we as a community has a chance of evaluating whether
>> that's the proper fix or something else entirely is needed. And so that
>> in the future we as a community would have a chance of objecting to
>> including 787f04bb6a in the first place.
> 
> You could help reviewing if you have time.

Don't you see the absurdity of on the one hand saying that the only way
to understand the ROM API is to study the U-Boot side of the code, and
on the other hand asking others to review changes to said code?

If the API could be understood from merely reading existing U-Boot code,
than that code is by definition perfect and won't need to be changed.

Now that I know there is a dedicated ROM team, let me rephrase:

Is there any chance you could reach out to said ROM team and ask if they
could make some information on the API public?

[The "you" in the previous questions have always meant NXP, not you
personally.]

Rasmus

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Rasmus Villemoes]

On 26/10/2022 01.42, Rasmus Villemoes wrote:
> On 18/10/2022 02.43, Peng Fan wrote:
>> + Stefano & Fabio
>>
> 
>>>>>
>>>>> Is there any chance you could make some information on that ROM API
>>>>> public so it's possible for outsiders to understand what's going on?
>>
>> What could only help is to ask the ROM team to see whether they have
>> plan to public the ROM API details and when. Otherwise you could only
>> read the code to understand how it works.
>>
>>>>
>>>> Could you please try below changes to check whether it fixes your issue?
>>>
>>> Well, it seems very likely it would, but could you _please_ answer the
>>> real question so we as a community has a chance of evaluating whether
>>> that's the proper fix or something else entirely is needed. And so that
>>> in the future we as a community would have a chance of objecting to
>>> including 787f04bb6a in the first place.
>>
>> You could help reviewing if you have time.
> 
> Don't you see the absurdity of on the one hand saying that the only way
> to understand the ROM API is to study the U-Boot side of the code, and
> on the other hand asking others to review changes to said code?
> 
> If the API could be understood from merely reading existing U-Boot code,
> than that code is by definition perfect and won't need to be changed.
> 
> Now that I know there is a dedicated ROM team, let me rephrase:
> 
> Is there any chance you could reach out to said ROM team and ask if they
> could make some information on the API public?
> 
> [The "you" in the previous questions have always meant NXP, not you
> personally.]

And here we are, half a year later, and mainline U-Boot is still broken.

I'm not gonna offer a tested-by or reviewed-by on that patch you
suggested upthread until and unless the ROM API gets documented.

Rasmus

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Tim Harvey]

On Sun, Apr 16, 2023 at 11:55 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 26/10/2022 01.42, Rasmus Villemoes wrote:
> > On 18/10/2022 02.43, Peng Fan wrote:
> >> + Stefano & Fabio
> >>
> >
> >>>>>
> >>>>> Is there any chance you could make some information on that ROM API
> >>>>> public so it's possible for outsiders to understand what's going on?
> >>
> >> What could only help is to ask the ROM team to see whether they have
> >> plan to public the ROM API details and when. Otherwise you could only
> >> read the code to understand how it works.
> >>
> >>>>
> >>>> Could you please try below changes to check whether it fixes your issue?
> >>>
> >>> Well, it seems very likely it would, but could you _please_ answer the
> >>> real question so we as a community has a chance of evaluating whether
> >>> that's the proper fix or something else entirely is needed. And so that
> >>> in the future we as a community would have a chance of objecting to
> >>> including 787f04bb6a in the first place.
> >>
> >> You could help reviewing if you have time.
> >
> > Don't you see the absurdity of on the one hand saying that the only way
> > to understand the ROM API is to study the U-Boot side of the code, and
> > on the other hand asking others to review changes to said code?
> >
> > If the API could be understood from merely reading existing U-Boot code,
> > than that code is by definition perfect and won't need to be changed.
> >
> > Now that I know there is a dedicated ROM team, let me rephrase:
> >
> > Is there any chance you could reach out to said ROM team and ask if they
> > could make some information on the API public?
> >
> > [The "you" in the previous questions have always meant NXP, not you
> > personally.]
>
> And here we are, half a year later, and mainline U-Boot is still broken.
>
> I'm not gonna offer a tested-by or reviewed-by on that patch you
> suggested upthread until and unless the ROM API gets documented.
>
> Rasmus
>

I just stumbled across this as well after an hour or so of debugging.

It seems to me if we are not going to revert commit 787f04bb6a0af
("imx: add USB2_BOOT type") which breaks IMX8M bootrom booting over
SDP due to boot_instance being non-zero then we should at least accept
Peng's fix which I can verify works.

diff --git a/arch/arm/mach-imx/romapi.c b/arch/arm/mach-imx/romapi.c
index b49e7f80a286..ff0522c2d117 100644
--- a/arch/arm/mach-imx/romapi.c
+++ b/arch/arm/mach-imx/romapi.c
@@ -70,6 +70,8 @@ enum boot_device get_boot_device(void)
                boot_dev = SPI_NOR_BOOT;
                break;
        case BT_DEV_TYPE_USB:
+               if (!is_imx8ulp() && !is_imx9())
+                       boot_instance = 0;
                boot_dev = boot_instance + USB_BOOT;
                break;
        default:

Best Regards,

Tim

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

On Thu, Apr 20, 2023 at 03:13:42PM -0700, Tim Harvey wrote:
> On Sun, Apr 16, 2023 at 11:55 PM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
> >
> > On 26/10/2022 01.42, Rasmus Villemoes wrote:
> > > On 18/10/2022 02.43, Peng Fan wrote:
> > >> + Stefano & Fabio
> > >>
> > >
> > >>>>>
> > >>>>> Is there any chance you could make some information on that ROM API
> > >>>>> public so it's possible for outsiders to understand what's going on?
> > >>
> > >> What could only help is to ask the ROM team to see whether they have
> > >> plan to public the ROM API details and when. Otherwise you could only
> > >> read the code to understand how it works.
> > >>
> > >>>>
> > >>>> Could you please try below changes to check whether it fixes your issue?
> > >>>
> > >>> Well, it seems very likely it would, but could you _please_ answer the
> > >>> real question so we as a community has a chance of evaluating whether
> > >>> that's the proper fix or something else entirely is needed. And so that
> > >>> in the future we as a community would have a chance of objecting to
> > >>> including 787f04bb6a in the first place.
> > >>
> > >> You could help reviewing if you have time.
> > >
> > > Don't you see the absurdity of on the one hand saying that the only way
> > > to understand the ROM API is to study the U-Boot side of the code, and
> > > on the other hand asking others to review changes to said code?
> > >
> > > If the API could be understood from merely reading existing U-Boot code,
> > > than that code is by definition perfect and won't need to be changed.
> > >
> > > Now that I know there is a dedicated ROM team, let me rephrase:
> > >
> > > Is there any chance you could reach out to said ROM team and ask if they
> > > could make some information on the API public?
> > >
> > > [The "you" in the previous questions have always meant NXP, not you
> > > personally.]
> >
> > And here we are, half a year later, and mainline U-Boot is still broken.
> >
> > I'm not gonna offer a tested-by or reviewed-by on that patch you
> > suggested upthread until and unless the ROM API gets documented.

To the NXP folks, is the API truly not documented in any part of the
normal public manuals for these chips? That seems like the kind of
oversight that indeed should be corrected, and internal bugs filed to
get resolved.

> > Rasmus
> >
> 
> I just stumbled across this as well after an hour or so of debugging.
> 
> It seems to me if we are not going to revert commit 787f04bb6a0af
> ("imx: add USB2_BOOT type") which breaks IMX8M bootrom booting over
> SDP due to boot_instance being non-zero then we should at least accept
> Peng's fix which I can verify works.
> 
> diff --git a/arch/arm/mach-imx/romapi.c b/arch/arm/mach-imx/romapi.c
> index b49e7f80a286..ff0522c2d117 100644
> --- a/arch/arm/mach-imx/romapi.c
> +++ b/arch/arm/mach-imx/romapi.c
> @@ -70,6 +70,8 @@ enum boot_device get_boot_device(void)
>                 boot_dev = SPI_NOR_BOOT;
>                 break;
>         case BT_DEV_TYPE_USB:
> +               if (!is_imx8ulp() && !is_imx9())
> +                       boot_instance = 0;
>                 boot_dev = boot_instance + USB_BOOT;
>                 break;
>         default:

And since it's not good to leave code broken either, the above should be
submitted as a proper patch I gather.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: commit 787f04bb6a - imx: add USB2_BOOT type

[Peng Fan]

On 4/24/2023 10:14 AM, Tom Rini wrote:
> On Thu, Apr 20, 2023 at 03:13:42PM -0700, Tim Harvey wrote:
>> On Sun, Apr 16, 2023 at 11:55 PM Rasmus Villemoes
>> <rasmus.villemoes@prevas.dk> wrote:
>>>
>>> On 26/10/2022 01.42, Rasmus Villemoes wrote:
>>>> On 18/10/2022 02.43, Peng Fan wrote:
>>>>> + Stefano & Fabio
>>>>>
>>>>
>>>>>>>>
>>>>>>>> Is there any chance you could make some information on that ROM API
>>>>>>>> public so it's possible for outsiders to understand what's going on?
>>>>>
>>>>> What could only help is to ask the ROM team to see whether they have
>>>>> plan to public the ROM API details and when. Otherwise you could only
>>>>> read the code to understand how it works.
>>>>>
>>>>>>>
>>>>>>> Could you please try below changes to check whether it fixes your issue?
>>>>>>
>>>>>> Well, it seems very likely it would, but could you _please_ answer the
>>>>>> real question so we as a community has a chance of evaluating whether
>>>>>> that's the proper fix or something else entirely is needed. And so that
>>>>>> in the future we as a community would have a chance of objecting to
>>>>>> including 787f04bb6a in the first place.
>>>>>
>>>>> You could help reviewing if you have time.
>>>>
>>>> Don't you see the absurdity of on the one hand saying that the only way
>>>> to understand the ROM API is to study the U-Boot side of the code, and
>>>> on the other hand asking others to review changes to said code?
>>>>
>>>> If the API could be understood from merely reading existing U-Boot code,
>>>> than that code is by definition perfect and won't need to be changed.
>>>>
>>>> Now that I know there is a dedicated ROM team, let me rephrase:
>>>>
>>>> Is there any chance you could reach out to said ROM team and ask if they
>>>> could make some information on the API public?
>>>>
>>>> [The "you" in the previous questions have always meant NXP, not you
>>>> personally.]
>>>
>>> And here we are, half a year later, and mainline U-Boot is still broken.
>>>
>>> I'm not gonna offer a tested-by or reviewed-by on that patch you
>>> suggested upthread until and unless the ROM API gets documented.
> 
> To the NXP folks, is the API truly not documented in any part of the
> normal public manuals for these chips? That seems like the kind of
> oversight that indeed should be corrected, and internal bugs filed to
> get resolved.

Please see https://www.nxp.com/webapp/Download?colCode=IMX93RM
9.11 ROM API. It should be backwards with i.MX8MN/P.

Hope this helps.

Regards,
Peng.

> 
>>> Rasmus
>>>
>>
>> I just stumbled across this as well after an hour or so of debugging.
>>
>> It seems to me if we are not going to revert commit 787f04bb6a0af
>> ("imx: add USB2_BOOT type") which breaks IMX8M bootrom booting over
>> SDP due to boot_instance being non-zero then we should at least accept
>> Peng's fix which I can verify works.
>>
>> diff --git a/arch/arm/mach-imx/romapi.c b/arch/arm/mach-imx/romapi.c
>> index b49e7f80a286..ff0522c2d117 100644
>> --- a/arch/arm/mach-imx/romapi.c
>> +++ b/arch/arm/mach-imx/romapi.c
>> @@ -70,6 +70,8 @@ enum boot_device get_boot_device(void)
>>                  boot_dev = SPI_NOR_BOOT;
>>                  break;
>>          case BT_DEV_TYPE_USB:
>> +               if (!is_imx8ulp() && !is_imx9())
>> +                       boot_instance = 0;
>>                  boot_dev = boot_instance + USB_BOOT;
>>                  break;
>>          default:
> 
> And since it's not good to leave code broken either, the above should be
> submitted as a proper patch I gather.
>