[U-Boot] [PATCH v1 5/7] spl: add serial download protocol (SDP) support

[Stefano Babic]

Hi Stefan,

On 09/08/2017 02:59, Stefan Agner wrote:
> Stefano,
> 
> One question below:
> 
> On 2017-08-04 16:38, Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner at toradex.com>
>>
>> Add USB serial download protocol support to SPL. If the SoC started
>> in recovery mode the SPL will immediately switch to SDP and wait for
>> further downloads/commands from the host side.
>>
>> Signed-off-by: Stefan Agner <stefan.agner at toradex.com>
>> ---
>>
>>  common/spl/Kconfig          |  6 ++++++
>>  common/spl/Makefile         |  1 +
>>  common/spl/spl_sdp.c        | 38 ++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/gadget/Makefile |  1 +
>>  4 files changed, 46 insertions(+)
>>  create mode 100644 common/spl/spl_sdp.c
>>
>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>> index 4de81392b0..95378b98a0 100644
>> --- a/common/spl/Kconfig
>> +++ b/common/spl/Kconfig
>> @@ -646,6 +646,12 @@ config SPL_DFU_RAM
>>  
>>  endchoice
>>  
>> +config SPL_USB_SDP_SUPPORT
>> +	bool "Support SDP (Serial Download Protocol)"
>> +	help
>> +	  Enable Serial Download Protocol (SDP) device support in SPL. This
>> +	  allows to download images into memory and execute (jump to) them
>> +	  using the same protocol as implemented by the i.MX family's boot ROM.
>>  endif
>>  
>>  config SPL_WATCHDOG_SUPPORT
>> diff --git a/common/spl/Makefile b/common/spl/Makefile
>> index 47a64dd7d0..a979560acf 100644
>> --- a/common/spl/Makefile
>> +++ b/common/spl/Makefile
>> @@ -29,4 +29,5 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
>>  obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
>>  obj-$(CONFIG_SPL_SPI_LOAD) += spl_spi.o
>>  obj-$(CONFIG_SPL_RAM_SUPPORT) += spl_ram.o
>> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += spl_sdp.o
>>  endif
>> diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
>> new file mode 100644
>> index 0000000000..faa74b8bec
>> --- /dev/null
>> +++ b/common/spl/spl_sdp.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * (C) Copyright 2016 Toradex
>> + * Author: Stefan Agner <stefan.agner at toradex.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <spl.h>
>> +#include <usb.h>
>> +#include <g_dnl.h>
>> +#include <sdp.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +static int spl_sdp_load_image(struct spl_image_info *spl_image,
>> +			      struct spl_boot_device *bootdev)
>> +{
>> +	int ret;
>> +
>> +	g_dnl_clear_detach();
>> +	g_dnl_register("usb_dnl_sdp");
>> +
>> +	ret = sdp_init();
>> +	if (ret) {
>> +		error("SDP init failed: %d", ret);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = sdp_handle();
>> +	if (ret) {
>> +		error("SDP failed: %d", ret);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +SPL_LOAD_IMAGE_METHOD("USB SDP", 0, BOOT_DEVICE_UART, spl_sdp_load_image);
> 
> 
> We currently use BOOT_DEVICE_UART when serial downloader boot mode is
> detected. This can be either USB or UART...
> 

Right

> In fact, USB is probably much more often used since only 6UL/ULL have
> UART serial downloader support afact...

The misunderstanding is originated from NXP's documentation. The
official name is "Serial Downloader" and this can be easy confused as
"UART" downloader. Even if documentation is correct, it can be bad
interpreted.

To be correct, we have a "serial protocol" over a device that can be
either USB or UART.

> 
> There is BOOT_DEVICE_BOARD which is used by e.g. Sunxi in case the boot
> ROM mechanism is used, what do you think, should be change that?

Good point. To be consistent (thanks for hint, I was not aware of this
in sunxi) we should change it, yes.

> 
> Ideally we should be able to discriminate between USB and UART. But I
> don't think its possible... (the USBPH0_PWD method likely does not work
> since even in the UART case the boot ROM will initialize the USB PHY
> first, at least according to the flow diagram in ULL's Chapter 9...)

I agree with you - we should get rid of undocumented feature inside ROM,
whose behaviour could be changed with SOC revision number.

The use case is fixed - if a board loads from USB, it will always load
from USB. A runtime detection is difficult and even overkilling for this
application.

> 
> The i.MX 7 which has the new Boot Information block actually only
> support USB serial downloader...
> 
> Thoughts?

As far as I can see and apart of names, we should reach to have this
mainlined as USB downloader. It covers quite all cases.

Best regards,
Stefano


> 
> --
> Stefan
> 
> 
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 6a007d1bcb..7258099c1c 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_USB_ETHER) += epautoconf.o config.o usbstring.o
>>  ifdef CONFIG_SPL_BUILD
>>  obj-$(CONFIG_SPL_USB_GADGET_SUPPORT) += g_dnl.o
>>  obj-$(CONFIG_SPL_DFU_SUPPORT) += f_dfu.o
>> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += f_sdp.o
>>  endif
>>  
>>  # new USB gadget layer dependencies


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================