diff mbox series

[U-Boot,v2,3/3] usb: gadget: f_sdp: Allow SPL to load and boot FIT via SDP

Message ID 20190604195629.27049-4-sjoerd.simons@collabora.co.uk
State Accepted
Commit 2c72ead7387404eba16c556d2f204c52c36c27f9
Delegated to: Stefano Babic
Headers show
Series Enable usage of SDP for i.MX6 Sabre Auto Boards | expand

Commit Message

Sjoerd Simons June 4, 2019, 7:56 p.m. UTC 
From: Frieder Schrempf <frieder.schrempf@kontron.de>

Add support for loading u-boot FIT images over the USB SDP protocol in
the SPL

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
[Various build fixes]
Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

---

Changes in v2:
- Fix build with CMD_USB_SDP
- Add SoB from Frieder Schrempf

 common/spl/spl_sdp.c       | 12 ++++++---
 drivers/usb/gadget/f_sdp.c | 53 +++++++++++++++++++++++++++++++++-----
 include/sdp.h              |  9 ++++++-
 3 files changed, 63 insertions(+), 11 deletions(-)

Comments

Fabio Estevam June 4, 2019, 8:19 p.m. UTC | #1 
Hi Sjoerd,

On Tue, Jun 4, 2019 at 4:58 PM Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
>
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> Add support for loading u-boot FIT images over the USB SDP protocol in
> the SPL
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> [Various build fixes]
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

Unfortunately, it still fails with imx_usb_loader due to the lack of IVT.

Secure boot also needs IVT, so we need to find a way to insert IVT in
the FIT image.

Tested with UUU and it worked, so this is progress :-)

Tested-by: Fabio Estevam <festevam@gmail.com>

Thanks
Sjoerd Simons June 4, 2019, 8:41 p.m. UTC | #2 
On Tue, 2019-06-04 at 17:19 -0300, Fabio Estevam wrote:
> Hi Sjoerd,
> 
> On Tue, Jun 4, 2019 at 4:58 PM Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> > 
> > Add support for loading u-boot FIT images over the USB SDP protocol
> > in
> > the SPL
> > 
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > [Various build fixes]
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> 
> Unfortunately, it still fails with imx_usb_loader due to the lack of
> IVT.
> 
> Secure boot also needs IVT, so we need to find a way to insert IVT in
> the FIT image.
> 
> Tested with UUU and it worked, so this is progress :-)

Small steps right; Ooi what imx_usb_loader configuration/commands are
you using to test this? (I find its config rather tricky to grasp).

One of the next things I will need to look at is actually secure boot..
That said why does imx_usb_loader if the board isn't locked?

> Tested-by: Fabio Estevam <festevam@gmail.com>
> 
> Thanks
Lukasz Majewski June 4, 2019, 9:46 p.m. UTC | #3 
Hi Fabio,

> Hi Sjoerd,
> 
> On Tue, Jun 4, 2019 at 4:58 PM Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> >
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >
> > Add support for loading u-boot FIT images over the USB SDP protocol
> > in the SPL
> >
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > [Various build fixes]
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>  
> 
> Unfortunately, it still fails with imx_usb_loader due to the lack of
> IVT.

I do observe another issue - one of my "factory" board setup is not
using (till now) OF_CONFIG.

Without u-boot.img being a FitImage wrapped, the imx_usb_loader works
correctly (with current mainline).

After this change I do see the issue with imx_usb_loader.
Unfortunately, applying this patch doesn't help.

I will investigate this issue further tomorrow.

> 
> Secure boot also needs IVT, so we need to find a way to insert IVT in
> the FIT image.
> 
> Tested with UUU and it worked, so this is progress :-)
> 
> Tested-by: Fabio Estevam <festevam@gmail.com>
> 
> Thanks
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Fabio Estevam June 4, 2019, 9:56 p.m. UTC | #4 
On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:

> Small steps right; Ooi what imx_usb_loader configuration/commands are
> you using to test this? (I find its config rather tricky to grasp).

I simply run:

sudo ./imx_usb SPL

and then

sudo ./imx_usb u-boot-dtb.img

I suggest you to try U-Boot 2019.01 on a mx6sabreauto first.

U-Boot 2019.01 is prior to the DM / fit conversion and loading SPL +
u-boot.img with the method above works fine.

> One of the next things I will need to look at is actually secure boot..
> That said why does imx_usb_loader if the board isn't locked?

Not sure what you mean by locked.

We have been using imx_usb_loader for a long time. After DM / fit
comvesion the IVT piece is not added into the final .img.

From the main Makefile:

ifdef CONFIG_SPL_LOAD_FIT
MKIMAGEFLAGS_u-boot.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
-a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
-n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
$(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
else
MKIMAGEFLAGS_u-boot.img = -A $(ARCH) -T firmware -C none -O u-boot \
-a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
-n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
MKIMAGEFLAGS_u-boot-ivt.img = -A $(ARCH) -T firmware_ivt -C none -O u-boot \
-a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
-n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
u-boot-ivt.img: MKIMAGEOUTPUT = u-boot-ivt.img.log
CLEAN_FILES += u-boot-ivt.img.log u-boot-dtb.imx.log SPL.log u-boot.imx.log
endif

we  can see that the ivt is not added for the CONFIG_SPL_LOAD_FIT case.

I tried to change this logic, but so far was not able to make it work.
Sjoerd Simons June 5, 2019, 7:25 a.m. UTC | #5 
On Tue, 2019-06-04 at 18:56 -0300, Fabio Estevam wrote:
> On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> 
> > Small steps right; Ooi what imx_usb_loader configuration/commands
> > are
> > you using to test this? (I find its config rather tricky to grasp).
> 
> I simply run:
> 
> sudo ./imx_usb SPL
> 
> and then
> 
> sudo ./imx_usb u-boot-dtb.img
> 
> I suggest you to try U-Boot 2019.01 on a mx6sabreauto first.

> U-Boot 2019.01 is prior to the DM / fit conversion and loading SPL +
> u-boot.img with the method above works fine.

Right; I've used that method on a fair few boards but none of those use
FIT.

For loading the SPL mx6_usb_sdp_spl.conf of imx_usb_loader has:
`hid,uboot_header,1024,0x10000000,1G,0x00907000,0x31000`

So my guess was that it was simply failing as a fit image doesn't have
a uboot header ;). I did have a quick look at seeing how to convince
imx_usb_loader to upload a FIT image, but failed to do so (hence my
question). uuu turned to be simpler to get to comply :)

> > One of the next things I will need to look at is actually secure
> > boot..
> > That said why does imx_usb_loader if the board isn't locked?
> 
> Not sure what you mean by locked.

Sorry closing the device would be the right jargon for i.mx. My point
really is, isn't it something to be fixed in imx_usb_loader if it can't
upload unsigned FIT images rather then in u-boot?

> We have been using imx_usb_loader for a long time. After DM / fit
> comvesion the IVT piece is not added into the final .img.
> 
> From the main Makefile:
> 
> ifdef CONFIG_SPL_LOAD_FIT
> MKIMAGEFLAGS_u-boot.img = -f auto -A $(ARCH) -T firmware -C none -O
> u-boot \
> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> else
> MKIMAGEFLAGS_u-boot.img = -A $(ARCH) -T firmware -C none -O u-boot \
> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> MKIMAGEFLAGS_u-boot-ivt.img = -A $(ARCH) -T firmware_ivt -C none -O
> u-boot \
> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> u-boot-ivt.img: MKIMAGEOUTPUT = u-boot-ivt.img.log
> CLEAN_FILES += u-boot-ivt.img.log u-boot-dtb.imx.log SPL.log u-
> boot.imx.log
> endif
> 
> we  can see that the ivt is not added for the CONFIG_SPL_LOAD_FIT
> case.
> 
> I tried to change this logic, but so far was not able to make it
> work.

Right; Thanks for the pointer
Lukasz Majewski June 5, 2019, 9:40 a.m. UTC | #6 
Hi Fabio, Sjoerd

> On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> 
> > Small steps right; Ooi what imx_usb_loader configuration/commands
> > are you using to test this? (I find its config rather tricky to
> > grasp).  
> 
> I simply run:
> 
> sudo ./imx_usb SPL
> 
> and then
> 
> sudo ./imx_usb u-boot-dtb.img
> 
> I suggest you to try U-Boot 2019.01 on a mx6sabreauto first.
> 

Tested-by: Lukasz Majewski <lukma@denx.de>

Test HW: i.MX6Q Display5 factory setup.



However, one thing puzzles me - the VID / PID used.When I run uuu
(mfgtools: SHA1: 13d187304f4faa473d2141409419c5b6f052addb):

I see that "Build in config" has following VID/PID:
        SDPU:    SPL             0x0525  0xb4a4  [0x0000..0x04ff] [1]
        SDPV:    SPL1            0x0525  0xb4a4  [0x0500..0x9998]
        SDPU:    SPL             0x0525  0xb4a4  [0x9999..0x9999]

But to make the SDPU command working I had to adjust it to be similar
to sabreauto (CONFIG_USB_GADGET_VENDOR_NUM=0x0525
CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5). Those match to "FB" (fastboot?).

Is this a bug or just the "Build in config" information is outdated?
With VID/PID set as for [1] (and as we use SDPU command, not FB), the
uuu doesn't connect to loaded SPL.



To make it working (on host):

cat << EOF > display5_recovery.lst
uuu_version 1.2.135
SDP: boot -f /srv/tftp/SPL
SDPU: write -f /srv/tftp/u-boot-dtb.img -addr 0x10000000
SDPU: jump -addr 0x10000000
SDPU: done
EOF

sudo ./uuu/uuu display5_recovery.lst


> U-Boot 2019.01 is prior to the DM / fit conversion and loading SPL +
> u-boot.img with the method above works fine.
> 
> > One of the next things I will need to look at is actually secure
> > boot.. That said why does imx_usb_loader if the board isn't
> > locked?  
> 
> Not sure what you mean by locked.
> 
> We have been using imx_usb_loader for a long time. After DM / fit
> comvesion the IVT piece is not added into the final .img.
> 
> From the main Makefile:
> 
> ifdef CONFIG_SPL_LOAD_FIT
> MKIMAGEFLAGS_u-boot.img = -f auto -A $(ARCH) -T firmware -C none -O
> u-boot \ -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> else
> MKIMAGEFLAGS_u-boot.img = -A $(ARCH) -T firmware -C none -O u-boot \
> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> MKIMAGEFLAGS_u-boot-ivt.img = -A $(ARCH) -T firmware_ivt -C none -O
> u-boot \ -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> u-boot-ivt.img: MKIMAGEOUTPUT = u-boot-ivt.img.log
> CLEAN_FILES += u-boot-ivt.img.log u-boot-dtb.imx.log SPL.log
> u-boot.imx.log endif
> 
> we  can see that the ivt is not added for the CONFIG_SPL_LOAD_FIT
> case.
> 
> I tried to change this logic, but so far was not able to make it work.




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Sjoerd Simons June 5, 2019, 10 a.m. UTC | #7 
On Wed, 2019-06-05 at 11:40 +0200, Lukasz Majewski wrote:
> Hi Fabio, Sjoerd
> 
> > On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
> > <sjoerd.simons@collabora.co.uk> wrote:
> > 
> > > Small steps right; Ooi what imx_usb_loader configuration/commands
> > > are you using to test this? (I find its config rather tricky to
> > > grasp).  
> > 
> > I simply run:
> > 
> > sudo ./imx_usb SPL
> > 
> > and then
> > 
> > sudo ./imx_usb u-boot-dtb.img
> > 
> > I suggest you to try U-Boot 2019.01 on a mx6sabreauto first.
> > 
> 
> Tested-by: Lukasz Majewski <lukma@denx.de>
> 
> Test HW: i.MX6Q Display5 factory setup.
> 
> 
> 
> However, one thing puzzles me - the VID / PID used.When I run uuu
> (mfgtools: SHA1: 13d187304f4faa473d2141409419c5b6f052addb):
> 
> I see that "Build in config" has following VID/PID:
>         SDPU:    SPL             0x0525  0xb4a4  [0x0000..0x04ff] [1]
>         SDPV:    SPL1            0x0525  0xb4a4  [0x0500..0x9998]
>         SDPU:    SPL             0x0525  0xb4a4  [0x9999..0x9999]
> 
> But to make the SDPU command working I had to adjust it to be similar
> to sabreauto (CONFIG_USB_GADGET_VENDOR_NUM=0x0525
> CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5). Those match to "FB"
> (fastboot?).
> 
> Is this a bug or just the "Build in config" information is outdated?
> With VID/PID set as for [1] (and as we use SDPU command, not FB), the
> uuu doesn't connect to loaded SPL.

So u-boot for the spl does (arch/arm/mach-imx/spl.c):
```
int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
{
      put_unaligned(CONFIG_USB_GADGET_PRODUCT_NUM + 0xfff, &dev->idProduct);
``

Iotw with CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5 in the config when in
the SPL the product will be 0xb4a4, which uuu recognizes.

Once in the main u-boot image in principle you shoudl be able to use
`FB:`, however it doesn't seem `FB: ucmd` is supported for mainline u-
boot so...
Lukasz Majewski June 5, 2019, 10:31 a.m. UTC | #8 
Hi Sjoerd,

> On Wed, 2019-06-05 at 11:40 +0200, Lukasz Majewski wrote:
> > Hi Fabio, Sjoerd
> >   
> > > On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
> > > <sjoerd.simons@collabora.co.uk> wrote:
> > >   
> > > > Small steps right; Ooi what imx_usb_loader
> > > > configuration/commands are you using to test this? (I find its
> > > > config rather tricky to grasp).    
> > > 
> > > I simply run:
> > > 
> > > sudo ./imx_usb SPL
> > > 
> > > and then
> > > 
> > > sudo ./imx_usb u-boot-dtb.img
> > > 
> > > I suggest you to try U-Boot 2019.01 on a mx6sabreauto first.
> > >   
> > 
> > Tested-by: Lukasz Majewski <lukma@denx.de>
> > 
> > Test HW: i.MX6Q Display5 factory setup.
> > 
> > 
> > 
> > However, one thing puzzles me - the VID / PID used.When I run uuu
> > (mfgtools: SHA1: 13d187304f4faa473d2141409419c5b6f052addb):
> > 
> > I see that "Build in config" has following VID/PID:
> >         SDPU:    SPL             0x0525  0xb4a4  [0x0000..0x04ff]
> > [1] SDPV:    SPL1            0x0525  0xb4a4  [0x0500..0x9998]
> >         SDPU:    SPL             0x0525  0xb4a4  [0x9999..0x9999]
> > 
> > But to make the SDPU command working I had to adjust it to be
> > similar to sabreauto (CONFIG_USB_GADGET_VENDOR_NUM=0x0525
> > CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5). Those match to "FB"
> > (fastboot?).
> > 
> > Is this a bug or just the "Build in config" information is outdated?
> > With VID/PID set as for [1] (and as we use SDPU command, not FB),
> > the uuu doesn't connect to loaded SPL.  
> 
> So u-boot for the spl does (arch/arm/mach-imx/spl.c):
> ```
> int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char
> *name) {
>       put_unaligned(CONFIG_USB_GADGET_PRODUCT_NUM + 0xfff,
> &dev->idProduct); ``

Ach... Right.

Thanks for pointing this out.

> 
> Iotw with CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5 in the config when in
> the SPL the product will be 0xb4a4, which uuu recognizes.
> 
> Once in the main u-boot image in principle you shoudl be able to use
> `FB:`, however it doesn't seem `FB: ucmd` is supported for mainline u-
> boot so...
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Fabio Estevam June 5, 2019, 12:23 p.m. UTC | #9 
On Wed, Jun 5, 2019 at 4:25 AM Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:

> Sorry closing the device would be the right jargon for i.mx. My point
> really is, isn't it something to be fixed in imx_usb_loader if it can't
> upload unsigned FIT images rather then in u-boot?

Yes, I think imx_usb_loader needs to be fixed to load FIT images.

However, since the current FIT format for i.MX6 does not contain an
IVT, this would cause issues when running secure boot.

So in my opinion if we could change the i.MX6 FIT format to also add
an IVT, then I think we could fix the imx_usb_loader and secure boot
issues.
Fabio Estevam June 11, 2019, 12:44 p.m. UTC | #10 
Hi Marek,

On Tue, Jun 4, 2019 at 4:58 PM Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
>
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> Add support for loading u-boot FIT images over the USB SDP protocol in
> the SPL
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> [Various build fixes]
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

Does this look good to you?

Could we have this for 2019.09? It fixes the loading of U-Boot via SDP
on i.MX6 when using FIT image.

Thanks
Fabio Estevam June 11, 2019, 12:48 p.m. UTC | #11 
On Tue, Jun 11, 2019 at 9:44 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Marek,
>
> On Tue, Jun 4, 2019 at 4:58 PM Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> >
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >
> > Add support for loading u-boot FIT images over the USB SDP protocol in
> > the SPL
> >
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > [Various build fixes]
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>
> Does this look good to you?
>
> Could we have this for 2019.09? It fixes the loading of U-Boot via SDP

Ops, I meant 2019.07 :-)
Stefano Babic June 11, 2019, 1:11 p.m. UTC | #12 
Hi Fabio,

On 11/06/19 14:44, Fabio Estevam wrote:
> Hi Marek,
> 
> On Tue, Jun 4, 2019 at 4:58 PM Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
>>
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Add support for loading u-boot FIT images over the USB SDP protocol in
>> the SPL
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> [Various build fixes]
>> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> 
> Does this look good to you?
> 

I have no comments, but from the discussion I felt there are still some
issues and I left it over.

Stefano

> Could we have this for 2019.09? It fixes the loading of U-Boot via SDP
> on i.MX6 when using FIT image.
Fabio Estevam June 11, 2019, 1:14 p.m. UTC | #13 
Hi Stefano,

On Tue, Jun 11, 2019 at 10:11 AM Stefano Babic <sbabic@denx.de> wrote:

> I have no comments, but from the discussion I felt there are still some
> issues and I left it over.

Lukasz reported an issue initially, but after adjusting the VID / PID
he managed to get it to work.

Lukasz, please confirm.

Thanks
Lukasz Majewski June 11, 2019, 1:53 p.m. UTC | #14 
Hi Fabio, Stefano,

> Hi Stefano,
> 
> On Tue, Jun 11, 2019 at 10:11 AM Stefano Babic <sbabic@denx.de> wrote:
> 
> > I have no comments, but from the discussion I felt there are still
> > some issues and I left it over.  
> 
> Lukasz reported an issue initially, but after adjusting the VID / PID
> he managed to get it to work.
> 
> Lukasz, please confirm.

I can confirm that I made it working on v2019-rc3 (on display5) after
adjusting VID/PID with NXP's 'uuu' utility to recovery i.MX6Q board.

> 
> Thanks




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Sjoerd Simons June 17, 2019, 2:26 p.m. UTC | #15 
On Tue, 2019-06-04 at 18:56 -0300, Fabio Estevam wrote:
> On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:

> We have been using imx_usb_loader for a long time. After DM / fit
> comvesion the IVT piece is not added into the final .img.
> 
> From the main Makefile:
> 
> ifdef CONFIG_SPL_LOAD_FIT
> MKIMAGEFLAGS_u-boot.img = -f auto -A $(ARCH) -T firmware -C none -O
> u-boot \
> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> else
> MKIMAGEFLAGS_u-boot.img = -A $(ARCH) -T firmware -C none -O u-boot \
> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> MKIMAGEFLAGS_u-boot-ivt.img = -A $(ARCH) -T firmware_ivt -C none -O
> u-boot \
> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> u-boot-ivt.img: MKIMAGEOUTPUT = u-boot-ivt.img.log
> CLEAN_FILES += u-boot-ivt.img.log u-boot-dtb.imx.log SPL.log u-
> boot.imx.log
> endif
> 
> we  can see that the ivt is not added for the CONFIG_SPL_LOAD_FIT
> case.
> 
> I tried to change this logic, but so far was not able to make it
> work.

So when looking through how secure boot can work from the SPL i can
accross e246bfcfe. Which explains the u-boot SPL expect a signed fit
image with roughly the following layout:

--------------------------------------------------
|     |     |     |   |           |     |        |
| FIT | FIT | FIT |   | U-BOOT    | ATF | U-BOOT |
| FDT | IVT | CSF |   | nodtb.bin |     |   DTB  |
|     |     |     |   |           |     |        |
--------------------------------------------------

The code aligns the IVT at 0x1000 and the standard CSF size is
0x2000. So one has to set CONFIG_FIT_EXTERNAL_OFFSET=0x3000 (assuming the FIT FDT <= 0x1000 bytes) to make CONFIG_SECURE_BOOT work in this setup (even if HAB is off and the image isn't signed). Otherwise the SPL won't pick up the correct location for all the external data.

Seems a bit ugly that the start offset of the external data can't be
introspected from the fdt blob, but oh well. 

The way this seems to work for i.mx8 is that the SECOND_LOADER command
sticks in a IVT header into the fit image. I assume some other tooling
can then stick the CSF data in the appropriate place (anyone have a
pointer?).

Now i guess the first question becomes whether the i.mx6 should follow
the same layout. And if so how to integrate it properly (teach mkimage
about handling firmware_ivt for fit images?). 

Relatedly i was looking for a good description of using HAB for loading
the OS in FIT images, but failed to find a good document. pointers
welcome.
Fabio Estevam June 17, 2019, 2:57 p.m. UTC | #16 
Hi Sjoerd,

Adding Breno in case he has some insights.

On Mon, Jun 17, 2019 at 11:26 AM Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
>
> On Tue, 2019-06-04 at 18:56 -0300, Fabio Estevam wrote:
> > On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
> > <sjoerd.simons@collabora.co.uk> wrote:
>
> > We have been using imx_usb_loader for a long time. After DM / fit
> > comvesion the IVT piece is not added into the final .img.
> >
> > From the main Makefile:
> >
> > ifdef CONFIG_SPL_LOAD_FIT
> > MKIMAGEFLAGS_u-boot.img = -f auto -A $(ARCH) -T firmware -C none -O
> > u-boot \
> > -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> > -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> > $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> > else
> > MKIMAGEFLAGS_u-boot.img = -A $(ARCH) -T firmware -C none -O u-boot \
> > -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> > -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> > MKIMAGEFLAGS_u-boot-ivt.img = -A $(ARCH) -T firmware_ivt -C none -O
> > u-boot \
> > -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> > -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> > u-boot-ivt.img: MKIMAGEOUTPUT = u-boot-ivt.img.log
> > CLEAN_FILES += u-boot-ivt.img.log u-boot-dtb.imx.log SPL.log u-
> > boot.imx.log
> > endif
> >
> > we  can see that the ivt is not added for the CONFIG_SPL_LOAD_FIT
> > case.
> >
> > I tried to change this logic, but so far was not able to make it
> > work.
>
> So when looking through how secure boot can work from the SPL i can
> accross e246bfcfe. Which explains the u-boot SPL expect a signed fit
> image with roughly the following layout:
>
> --------------------------------------------------
> |     |     |     |   |           |     |        |
> | FIT | FIT | FIT |   | U-BOOT    | ATF | U-BOOT |
> | FDT | IVT | CSF |   | nodtb.bin |     |   DTB  |
> |     |     |     |   |           |     |        |
> --------------------------------------------------
>
> The code aligns the IVT at 0x1000 and the standard CSF size is
> 0x2000. So one has to set CONFIG_FIT_EXTERNAL_OFFSET=0x3000 (assuming the FIT FDT <= 0x1000 bytes) to make CONFIG_SECURE_BOOT work in this setup (even if HAB is off and the image isn't signed). Otherwise the SPL won't pick up the correct location for all the external data.
>
> Seems a bit ugly that the start offset of the external data can't be
> introspected from the fdt blob, but oh well.
>
> The way this seems to work for i.mx8 is that the SECOND_LOADER command
> sticks in a IVT header into the fit image. I assume some other tooling
> can then stick the CSF data in the appropriate place (anyone have a
> pointer?).
>
> Now i guess the first question becomes whether the i.mx6 should follow
> the same layout. And if so how to integrate it properly (teach mkimage
> about handling firmware_ivt for fit images?).
>
> Relatedly i was looking for a good description of using HAB for loading
> the OS in FIT images, but failed to find a good document. pointers
> welcome.
>
>
> --
> Sjoerd Simons
> Collabora Ltd.
Breno Matheus Lima June 19, 2019, 2 a.m. UTC | #17 
Hi Sjoerd,

Em seg, 17 de jun de 2019 às 11:26, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> escreveu:
>
> On Tue, 2019-06-04 at 18:56 -0300, Fabio Estevam wrote:
> > On Tue, Jun 4, 2019 at 5:41 PM Sjoerd Simons
> > <sjoerd.simons@collabora.co.uk> wrote:
>
> > We have been using imx_usb_loader for a long time. After DM / fit
> > comvesion the IVT piece is not added into the final .img.
> >
> > From the main Makefile:
> >
> > ifdef CONFIG_SPL_LOAD_FIT
> > MKIMAGEFLAGS_u-boot.img = -f auto -A $(ARCH) -T firmware -C none -O
> > u-boot \
> > -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> > -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
> > $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> > else
> > MKIMAGEFLAGS_u-boot.img = -A $(ARCH) -T firmware -C none -O u-boot \
> > -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> > -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> > MKIMAGEFLAGS_u-boot-ivt.img = -A $(ARCH) -T firmware_ivt -C none -O
> > u-boot \
> > -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
> > -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board"
> > u-boot-ivt.img: MKIMAGEOUTPUT = u-boot-ivt.img.log
> > CLEAN_FILES += u-boot-ivt.img.log u-boot-dtb.imx.log SPL.log u-
> > boot.imx.log
> > endif
> >
> > we  can see that the ivt is not added for the CONFIG_SPL_LOAD_FIT
> > case.
> >
> > I tried to change this logic, but so far was not able to make it
> > work.
>
> So when looking through how secure boot can work from the SPL i can
> accross e246bfcfe. Which explains the u-boot SPL expect a signed fit
> image with roughly the following layout:
>
> --------------------------------------------------
> |     |     |     |   |           |     |        |
> | FIT | FIT | FIT |   | U-BOOT    | ATF | U-BOOT |
> | FDT | IVT | CSF |   | nodtb.bin |     |   DTB  |
> |     |     |     |   |           |     |        |
> --------------------------------------------------
>
> The code aligns the IVT at 0x1000 and the standard CSF size is
> 0x2000. So one has to set CONFIG_FIT_EXTERNAL_OFFSET=0x3000 (assuming the FIT FDT <= 0x1000 bytes) to make CONFIG_SECURE_BOOT work in this setup (even if HAB is off and the image isn't signed). Otherwise the SPL won't pick up the correct location for all the external data.
>
> Seems a bit ugly that the start offset of the external data can't be
> introspected from the fdt blob, but oh well.
>
> The way this seems to work for i.mx8 is that the SECOND_LOADER command
> sticks in a IVT header into the fit image. I assume some other tooling
> can then stick the CSF data in the appropriate place (anyone have a
> pointer?).
>

Thanks for looking at this issue.

We are using similar structure in NXP U-Boot for i.MX8M devices, you
can use dd tool as explained in document below:
https://source.codeaurora.org/external/imx/uboot-imx/tree/doc/imx/habv4/guides/mx8m_mx8mm_secure_boot.txt?h=imx_v2018.03_4.14.98_2.0.0_ga#n301

> Now i guess the first question becomes whether the i.mx6 should follow
> the same layout. And if so how to integrate it properly (teach mkimage
> about handling firmware_ivt for fit images?).
>

IMHO we should follow the same layout as in i.MX8M, as far as I know
in NXP BSP this is handled by imx-mkimage tool:
https://source.codeaurora.org/external/imx/imx-mkimage/?h=imx_4.14.98_2.0.0_ga

Perhaps we can try similar with mkimage tool included in upstream U-Boot?

> Relatedly i was looking for a good description of using HAB for loading
> the OS in FIT images, but failed to find a good document. pointers
> welcome.
>

For HAB API usage you can take a look in HABv4 RVT Guidelines and
Recommendations application note:
https://www.nxp.com/docs/en/application-note/AN12263.pdf

The U-Boot docs in NXP BSP U-Boot also provides an overview:
https://source.codeaurora.org/external/imx/uboot-imx/tree/doc/imx/habv4?h=imx_v2018.03_4.14.98_2.0.0_ga

Thanks,
Breno Lima
Sjoerd Simons June 19, 2019, 9:53 a.m. UTC | #18 
Hey,

On Tue, 2019-06-18 at 23:00 -0300, Breno Matheus Lima wrote:
> > Seems a bit ugly that the start offset of the external data can't
> > be
> > introspected from the fdt blob, but oh well.
> > 
> > The way this seems to work for i.mx8 is that the SECOND_LOADER
> > command
> > sticks in a IVT header into the fit image. I assume some other
> > tooling
> > can then stick the CSF data in the appropriate place (anyone have a
> > pointer?).
> > 
> 
> Thanks for looking at this issue.
> 
> We are using similar structure in NXP U-Boot for i.MX8M devices, you
> can use dd tool as explained in document below:
> https://source.codeaurora.org/external/imx/uboot-imx/tree/doc/imx/habv4/guides/mx8m_mx8mm_secure_boot.txt?h=imx_v2018.03_4.14.98_2.0.0_ga#n301

THat's a quite useful document indeed to explain the mx8m setup a bit
more; Would be lovely to get that in upstream u-boot!

Though from what i can tell mx8m secure boot isn't there yet upstream,
at least judging by the fact there is no correct definition of the RVT
base address for those upstream yet ;) 

> > Now i guess the first question becomes whether the i.mx6 should
> > follow
> > the same layout. And if so how to integrate it properly (teach
> > mkimage
> > about handling firmware_ivt for fit images?).
> > 
> 
> IMHO we should follow the same layout as in i.MX8M, as far as I know
> in NXP BSP this is handled by imx-mkimage tool:
> https://source.codeaurora.org/external/imx/imx-mkimage/?h=imx_4.14.98_2.0.0_ga
> 
> Perhaps we can try similar with mkimage tool included in upstream U-
> Boot?

Right i'd hope the i.mx8m specific bits in u-boot already do that as
well; but for i.MX6 those are overkill since there isn't one big
flash.bin but two seperate bits of data (Though from the documentation
it feels like that might be the case for i.mx8 as well? As there
doesn't seem to be a reason apart from flashing convenience to put the
SPL and the u-boot images in one big file)


> > Relatedly i was looking for a good description of using HAB for
> > loading
> > the OS in FIT images, but failed to find a good document. pointers
> > welcome.
> > 
> 
> For HAB API usage you can take a look in HABv4 RVT Guidelines and
> Recommendations application note:
> https://www.nxp.com/docs/en/application-note/AN12263.pdf
> 
> The U-Boot docs in NXP BSP U-Boot also provides an overview:
> https://source.codeaurora.org/external/imx/uboot-imx/tree/doc/imx/habv4?h=imx_v2018.03_4.14.98_2.0.0_ga

Right all those assume you're using a (z)Image with the IVT/CSF
appended; None of those describe a conventional layout for a FIT image.
Fabio Estevam July 22, 2019, 6:31 p.m. UTC | #19 
Hi Lukasz and Stefano,

On Tue, Jun 11, 2019 at 10:53 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Fabio, Stefano,
>
> > Hi Stefano,
> >
> > On Tue, Jun 11, 2019 at 10:11 AM Stefano Babic <sbabic@denx.de> wrote:
> >
> > > I have no comments, but from the discussion I felt there are still
> > > some issues and I left it over.
> >
> > Lukasz reported an issue initially, but after adjusting the VID / PID
> > he managed to get it to work.
> >
> > Lukasz, please confirm.
>
> I can confirm that I made it working on v2019-rc3 (on display5) after
> adjusting VID/PID with NXP's 'uuu' utility to recovery i.MX6Q board.

Do you think this one could be applied?

Without it, I can not load a fit image via USB on imx6sabresd.

Thanks
Lukasz Majewski July 22, 2019, 8:52 p.m. UTC | #20 
Hi Fabio, Stefano,

> Hi Lukasz and Stefano,
> 
> On Tue, Jun 11, 2019 at 10:53 AM Lukasz Majewski <lukma@denx.de>
> wrote:
> >
> > Hi Fabio, Stefano,
> >  
> > > Hi Stefano,
> > >
> > > On Tue, Jun 11, 2019 at 10:11 AM Stefano Babic <sbabic@denx.de>
> > > wrote: 
> > > > I have no comments, but from the discussion I felt there are
> > > > still some issues and I left it over.  
> > >
> > > Lukasz reported an issue initially, but after adjusting the VID /
> > > PID he managed to get it to work.
> > >
> > > Lukasz, please confirm.  
> >
> > I can confirm that I made it working on v2019-rc3 (on display5)
> > after adjusting VID/PID with NXP's 'uuu' utility to recovery i.MX6Q
> > board.  
> 
> Do you think this one could be applied?

This patch seems to be already applied to master branch of U-Boot:
SHA1: 2c72ead7387404eba16c556d2f204c52c36c27f9

> 
> Without it, I can not load a fit image via USB on imx6sabresd.
> 
> Thanks




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Fabio Estevam July 22, 2019, 8:54 p.m. UTC | #21 
Hi Lukasz,

On Mon, Jul 22, 2019 at 5:52 PM Lukasz Majewski <lukma@denx.de> wrote:

> This patch seems to be already applied to master branch of U-Boot:
> SHA1: 2c72ead7387404eba16c556d2f204c52c36c27f9

Excellent! Thanks
Stefano Babic July 23, 2019, 6:51 a.m. UTC | #22 
Hi Fabio,

On 22/07/19 20:31, Fabio Estevam wrote:
> Hi Lukasz and Stefano,
> 
> On Tue, Jun 11, 2019 at 10:53 AM Lukasz Majewski <lukma@denx.de> wrote:
>>
>> Hi Fabio, Stefano,
>>
>>> Hi Stefano,
>>>
>>> On Tue, Jun 11, 2019 at 10:11 AM Stefano Babic <sbabic@denx.de> wrote:
>>>
>>>> I have no comments, but from the discussion I felt there are still
>>>> some issues and I left it over.
>>>
>>> Lukasz reported an issue initially, but after adjusting the VID / PID
>>> he managed to get it to work.
>>>
>>> Lukasz, please confirm.
>>
>> I can confirm that I made it working on v2019-rc3 (on display5) after
>> adjusting VID/PID with NXP's 'uuu' utility to recovery i.MX6Q board.
> 
> Do you think this one could be applied?
> 
> Without it, I can not load a fit image via USB on imx6sabresd.
> 

This is my mistake, sorry. I supposed I have applied, something went
wrong. I check it, thanks for remind.

Regards,
Stefano
diff mbox series

Patch

diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
index 807256e908..51b245b886 100644
--- a/common/spl/spl_sdp.c
+++ b/common/spl/spl_sdp.c
@@ -25,10 +25,14 @@  static int spl_sdp_load_image(struct spl_image_info *spl_image,
 		return -ENODEV;
 	}
 
-	/* This command typically does not return but jumps to an image */
-	sdp_handle(controller_index);
-	pr_err("SDP ended\n");
+	/*
+	 * This command either loads a legacy image, jumps and never returns,
+	 * or it loads a FIT image and returns it to be handled by the SPL
+	 * code.
+	 */
+	ret = spl_sdp_handle(controller_index, spl_image);
+	debug("SDP ended\n");
 
-	return -EINVAL;
+	return ret;
 }
 SPL_LOAD_IMAGE_METHOD("USB SDP", 0, BOOT_DEVICE_BOARD, spl_sdp_load_image);
diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
index ae97ab2b49..fab7ce6f97 100644
--- a/drivers/usb/gadget/f_sdp.c
+++ b/drivers/usb/gadget/f_sdp.c
@@ -638,7 +638,20 @@  static u32 sdp_jump_imxheader(void *address)
 	return 0;
 }
 
-static void sdp_handle_in_ep(void)
+#ifdef CONFIG_SPL_BUILD
+#ifdef CONFIG_SPL_LOAD_FIT
+static ulong sdp_fit_read(struct spl_load_info *load, ulong sector,
+			  ulong count, void *buf)
+{
+	debug("%s: sector %lx, count %lx, buf %lx\n",
+	      __func__, sector, count, (ulong)buf);
+	memcpy(buf, (void *)(load->dev + sector), count);
+	return count;
+}
+#endif
+#endif
+
+static void sdp_handle_in_ep(struct spl_image_info *spl_image)
 {
 	u8 *data = sdp_func->in_req->buf;
 	u32 status;
@@ -690,10 +703,25 @@  static void sdp_handle_in_ep(void)
 		/* If imx header fails, try some U-Boot specific headers */
 		if (status) {
 #ifdef CONFIG_SPL_BUILD
+			image_header_t *header =
+				sdp_ptr(sdp_func->jmp_address);
+#ifdef CONFIG_SPL_LOAD_FIT
+			if (image_get_magic(header) == FDT_MAGIC) {
+				struct spl_load_info load;
+
+				debug("Found FIT\n");
+				load.dev = header;
+				load.bl_len = 1;
+				load.read = sdp_fit_read;
+				spl_load_simple_fit(spl_image, &load, 0,
+						    header);
+
+				return;
+			}
+#endif
 			/* In SPL, allow jumps to U-Boot images */
 			struct spl_image_info spl_image = {};
-			spl_parse_image_header(&spl_image,
-				(struct image_header *)sdp_func->jmp_address);
+			spl_parse_image_header(&spl_image, header);
 			jump_to_image_no_args(&spl_image);
 #else
 			/* In U-Boot, allow jumps to scripts */
@@ -715,19 +743,32 @@  static void sdp_handle_in_ep(void)
 	};
 }
 
-void sdp_handle(int controller_index)
+#ifndef CONFIG_SPL_BUILD
+int sdp_handle(int controller_index)
+#else
+int spl_sdp_handle(int controller_index, struct spl_image_info *spl_image)
+#endif
 {
 	printf("SDP: handle requests...\n");
 	while (1) {
 		if (ctrlc()) {
 			puts("\rCTRL+C - Operation aborted.\n");
-			return;
+			return -EINVAL;
 		}
 
+#ifdef CONFIG_SPL_BUILD
+		if (spl_image->flags & SPL_FIT_FOUND)
+			return 0;
+#endif
+
 		WATCHDOG_RESET();
 		usb_gadget_handle_interrupts(controller_index);
 
-		sdp_handle_in_ep();
+#ifdef CONFIG_SPL_BUILD
+		sdp_handle_in_ep(spl_image);
+#else
+		sdp_handle_in_ep(NULL);
+#endif
 	}
 }
 
diff --git a/include/sdp.h b/include/sdp.h
index f6252d027f..6ac64fb1f3 100644
--- a/include/sdp.h
+++ b/include/sdp.h
@@ -10,6 +10,13 @@ 
 #define __SDP_H_
 
 int sdp_init(int controller_index);
-void sdp_handle(int controller_index);
+
+#ifdef CONFIG_SPL_BUILD
+#include <spl.h>
+
+int spl_sdp_handle(int controller_index, struct spl_image_info *spl_image);
+#else
+int sdp_handle(int controller_index);
+#endif
 
 #endif /* __SDP_H_ */