mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] ARM: i.MX6: boot: detect USB serial downloader more reliable
@ 2021-08-11 17:36 Marco Felsch
  2021-08-11 18:30 ` Ahmad Fatoum
  0 siblings, 1 reply; 3+ messages in thread
From: Marco Felsch @ 2021-08-11 17:36 UTC (permalink / raw)
  To: barebox

The problem with the BootROM is that the SCR registers are not set
accordingly in case of a failed primary boot. E.g. if the device is
configured to boot from an eMMC and the eMMC is empty or image is
corrupt, the BootROM goes into 'recovery boot mode' (reference manual
Figure 8-1) and the last possible recovery option is the serial
downloader. In such case the SRC registers still indicate that the
device was booted from an eMMC instead of serial-download.

This commit ports the U-Boot commit [1] with slightly adaptions suggested
by Ahmad to Barebox. Also we need to reorder the imx6_init() else we
reset the otg-controller to early.

[1] https://source.denx.de/u-boot/u-boot/-/commit/e203dcf23e9eabc2e4f3d0b079457cd1516f2081

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 arch/arm/mach-imx/boot.c  | 27 +++++++++++++++++++++++++++
 arch/arm/mach-imx/imx6.c  |  4 ++--
 include/soc/fsl/fsl_udc.h | 11 +++++++++++
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 2b66bbf71e..fcb3c10064 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -8,6 +8,7 @@
 #include <magicvar.h>
 
 #include <io.h>
+#include <mach/clock-imx6.h>
 #include <mach/generic.h>
 #include <mach/imx25-regs.h>
 #include <mach/imx27-regs.h>
@@ -23,6 +24,7 @@
 #include <mach/imx8mq.h>
 #include <mach/imx6.h>
 
+#include <soc/fsl/fsl_udc.h>
 
 static void
 imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
@@ -397,6 +399,11 @@ static u32 imx6_get_src_boot_mode(void __iomem *src_base)
 	return readl(src_base + IMX6_SRC_SBMR1);
 }
 
+static inline bool imx6_usboh3_clk_active(void)
+{
+	return (readl(MXC_CCM_CCGR6) & 0x3) == 0x3;
+}
+
 void imx6_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *src_base = IOMEM(MX6_SRC_BASE_ADDR);
@@ -410,6 +417,26 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 
 	bootsrc = imx53_bootsource_internal(bootmode);
 
+	/*
+	 * imx6_bootsource_serial() can't detect cases where the boot ROM
+	 * decided to use the serial downloader as a fall back (primary
+	 * boot source failed).
+	 *
+	 * Infer that the boot ROM used the USB serial downloader by
+	 * checking whether both the UDC and the clock enabling access
+	 * to its MMIO region are currently active...
+	 * This assumes:
+	 * - On fresh boots, PBL doesn't itself start a stopped UDC
+	 * - In barebox proper, boot source is saved before the UDC driver
+	 *   may enable the UDC
+	 */
+
+	if (imx6_usboh3_clk_active() &&
+	    is_chipidea_udc_running(IOMEM(MX6_OTG_BASE_ADDR))) {
+		*src = BOOTSOURCE_SERIAL;
+		return;
+	}
+
 	if (imx6_bootsource_serial(sbmr2) ||
 	    imx6_bootsource_serial_forced(bootsrc)) {
 		*src = BOOTSOURCE_SERIAL;
diff --git a/arch/arm/mach-imx/imx6.c b/arch/arm/mach-imx/imx6.c
index 9ccb391384..fa4cb093c3 100644
--- a/arch/arm/mach-imx/imx6.c
+++ b/arch/arm/mach-imx/imx6.c
@@ -205,10 +205,10 @@ int imx6_init(void)
 	void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR);
 	u64 mx6_uid;
 
-	imx6_init_lowlevel();
-
 	imx6_boot_save_loc();
 
+	imx6_init_lowlevel();
+
 	mx6_silicon_revision = imx6_cpu_revision();
 	mx6_uid = imx6_uid();
 
diff --git a/include/soc/fsl/fsl_udc.h b/include/soc/fsl/fsl_udc.h
index b983f714c5..0b409a9f6b 100644
--- a/include/soc/fsl/fsl_udc.h
+++ b/include/soc/fsl/fsl_udc.h
@@ -1,6 +1,9 @@
 #ifndef __FSL_UDC_H
 #define __FSL_UDC_H
 
+#include <linux/types.h>
+#include <io.h>
+
 /* USB DR device mode registers (Little Endian) */
 struct usb_dr_device {
 	/* Capability register */
@@ -380,4 +383,12 @@ int imx_barebox_start_usb(void __iomem *dr, void *dest);
 int imx8mm_barebox_load_usb(void *dest);
 int imx8mm_barebox_start_usb(void *dest);
 
+static inline bool is_chipidea_udc_running(void __iomem *dr)
+{
+	struct usb_dr_device __iomem *dr_regs = dr;
+
+	return (readl(&dr_regs->usbmode) & USB_MODE_CTRL_MODE_DEVICE)
+		&& (readl(&dr_regs->usbcmd) & USB_CMD_RUN_STOP);
+}
+
 #endif /* __FSL_UDC_H */
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ARM: i.MX6: boot: detect USB serial downloader more reliable
  2021-08-11 17:36 [PATCH] ARM: i.MX6: boot: detect USB serial downloader more reliable Marco Felsch
@ 2021-08-11 18:30 ` Ahmad Fatoum
  2021-08-12  7:38   ` Marco Felsch
  0 siblings, 1 reply; 3+ messages in thread
From: Ahmad Fatoum @ 2021-08-11 18:30 UTC (permalink / raw)
  To: Marco Felsch, barebox

On 11.08.21 19:36, Marco Felsch wrote:
> The problem with the BootROM is that the SCR registers are not set

s/SCR/SRC/ ?

> accordingly in case of a failed primary boot. E.g. if the device is
> configured to boot from an eMMC and the eMMC is empty or image is
> corrupt, the BootROM goes into 'recovery boot mode' (reference manual
> Figure 8-1) and the last possible recovery option is the serial
> downloader. In such case the SRC registers still indicate that the
> device was booted from an eMMC instead of serial-download.
> 
> This commit ports the U-Boot commit [1] with slightly adaptions suggested
> by Ahmad to Barebox. Also we need to reorder the imx6_init() else we
> reset the otg-controller to early.
> 
> [1] https://source.denx.de/u-boot/u-boot/-/commit/e203dcf23e9eabc2e4f3d0b079457cd1516f2081
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  arch/arm/mach-imx/boot.c  | 27 +++++++++++++++++++++++++++
>  arch/arm/mach-imx/imx6.c  |  4 ++--
>  include/soc/fsl/fsl_udc.h | 11 +++++++++++
>  3 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
> index 2b66bbf71e..fcb3c10064 100644
> --- a/arch/arm/mach-imx/boot.c
> +++ b/arch/arm/mach-imx/boot.c
> @@ -8,6 +8,7 @@
>  #include <magicvar.h>
>  
>  #include <io.h>
> +#include <mach/clock-imx6.h>
>  #include <mach/generic.h>
>  #include <mach/imx25-regs.h>
>  #include <mach/imx27-regs.h>
> @@ -23,6 +24,7 @@
>  #include <mach/imx8mq.h>
>  #include <mach/imx6.h>
>  
> +#include <soc/fsl/fsl_udc.h>
>  
>  static void
>  imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
> @@ -397,6 +399,11 @@ static u32 imx6_get_src_boot_mode(void __iomem *src_base)
>  	return readl(src_base + IMX6_SRC_SBMR1);
>  }
>  
> +static inline bool imx6_usboh3_clk_active(void)
> +{
> +	return (readl(MXC_CCM_CCGR6) & 0x3) == 0x3;
> +}
> +
>  void imx6_get_boot_source(enum bootsource *src, int *instance)
>  {
>  	void __iomem *src_base = IOMEM(MX6_SRC_BASE_ADDR);
> @@ -410,6 +417,26 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
>  
>  	bootsrc = imx53_bootsource_internal(bootmode);
>  
> +	/*
> +	 * imx6_bootsource_serial() can't detect cases where the boot ROM
> +	 * decided to use the serial downloader as a fall back (primary
> +	 * boot source failed).
> +	 *
> +	 * Infer that the boot ROM used the USB serial downloader by
> +	 * checking whether both the UDC and the clock enabling access
> +	 * to its MMIO region are currently active...
> +	 * This assumes:
> +	 * - On fresh boots, PBL doesn't itself start a stopped UDC
> +	 * - In barebox proper, boot source is saved before the UDC driver
> +	 *   may enable the UDC
> +	 */
> +
> +	if (imx6_usboh3_clk_active() &&
> +	    is_chipidea_udc_running(IOMEM(MX6_OTG_BASE_ADDR))) {
> +		*src = BOOTSOURCE_SERIAL;
> +		return;
> +	}
> +
>  	if (imx6_bootsource_serial(sbmr2) ||
>  	    imx6_bootsource_serial_forced(bootsrc)) {
>  		*src = BOOTSOURCE_SERIAL;
> diff --git a/arch/arm/mach-imx/imx6.c b/arch/arm/mach-imx/imx6.c
> index 9ccb391384..fa4cb093c3 100644
> --- a/arch/arm/mach-imx/imx6.c
> +++ b/arch/arm/mach-imx/imx6.c
> @@ -205,10 +205,10 @@ int imx6_init(void)
>  	void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR);
>  	u64 mx6_uid;
>  
> -	imx6_init_lowlevel();
> -
>  	imx6_boot_save_loc();
>  
> +	imx6_init_lowlevel();

A comment inside imx6_init_lowlevel hinting at the dependency w.r.t 
imx6_boot_save_loc would be nice.

Apart from these nitpicks:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> +
>  	mx6_silicon_revision = imx6_cpu_revision();
>  	mx6_uid = imx6_uid();
>  
> diff --git a/include/soc/fsl/fsl_udc.h b/include/soc/fsl/fsl_udc.h
> index b983f714c5..0b409a9f6b 100644
> --- a/include/soc/fsl/fsl_udc.h
> +++ b/include/soc/fsl/fsl_udc.h
> @@ -1,6 +1,9 @@
>  #ifndef __FSL_UDC_H
>  #define __FSL_UDC_H
>  
> +#include <linux/types.h>
> +#include <io.h>
> +
>  /* USB DR device mode registers (Little Endian) */
>  struct usb_dr_device {
>  	/* Capability register */
> @@ -380,4 +383,12 @@ int imx_barebox_start_usb(void __iomem *dr, void *dest);
>  int imx8mm_barebox_load_usb(void *dest);
>  int imx8mm_barebox_start_usb(void *dest);
>  
> +static inline bool is_chipidea_udc_running(void __iomem *dr)
> +{
> +	struct usb_dr_device __iomem *dr_regs = dr;
> +
> +	return (readl(&dr_regs->usbmode) & USB_MODE_CTRL_MODE_DEVICE)
> +		&& (readl(&dr_regs->usbcmd) & USB_CMD_RUN_STOP);
> +}
> +
>  #endif /* __FSL_UDC_H */
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ARM: i.MX6: boot: detect USB serial downloader more reliable
  2021-08-11 18:30 ` Ahmad Fatoum
@ 2021-08-12  7:38   ` Marco Felsch
  0 siblings, 0 replies; 3+ messages in thread
From: Marco Felsch @ 2021-08-12  7:38 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On 21-08-11 20:30, Ahmad Fatoum wrote:
> On 11.08.21 19:36, Marco Felsch wrote:
> > The problem with the BootROM is that the SCR registers are not set
> 
> s/SCR/SRC/ ?

of course :)

> > accordingly in case of a failed primary boot. E.g. if the device is
> > configured to boot from an eMMC and the eMMC is empty or image is
> > corrupt, the BootROM goes into 'recovery boot mode' (reference manual
> > Figure 8-1) and the last possible recovery option is the serial
> > downloader. In such case the SRC registers still indicate that the
> > device was booted from an eMMC instead of serial-download.
> > 
> > This commit ports the U-Boot commit [1] with slightly adaptions suggested
> > by Ahmad to Barebox. Also we need to reorder the imx6_init() else we
> > reset the otg-controller to early.
> > 
> > [1] https://source.denx.de/u-boot/u-boot/-/commit/e203dcf23e9eabc2e4f3d0b079457cd1516f2081
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  arch/arm/mach-imx/boot.c  | 27 +++++++++++++++++++++++++++
> >  arch/arm/mach-imx/imx6.c  |  4 ++--
> >  include/soc/fsl/fsl_udc.h | 11 +++++++++++
> >  3 files changed, 40 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
> > index 2b66bbf71e..fcb3c10064 100644
> > --- a/arch/arm/mach-imx/boot.c
> > +++ b/arch/arm/mach-imx/boot.c
> > @@ -8,6 +8,7 @@
> >  #include <magicvar.h>
> >  
> >  #include <io.h>
> > +#include <mach/clock-imx6.h>
> >  #include <mach/generic.h>
> >  #include <mach/imx25-regs.h>
> >  #include <mach/imx27-regs.h>
> > @@ -23,6 +24,7 @@
> >  #include <mach/imx8mq.h>
> >  #include <mach/imx6.h>
> >  
> > +#include <soc/fsl/fsl_udc.h>
> >  
> >  static void
> >  imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
> > @@ -397,6 +399,11 @@ static u32 imx6_get_src_boot_mode(void __iomem *src_base)
> >  	return readl(src_base + IMX6_SRC_SBMR1);
> >  }
> >  
> > +static inline bool imx6_usboh3_clk_active(void)
> > +{
> > +	return (readl(MXC_CCM_CCGR6) & 0x3) == 0x3;
> > +}
> > +
> >  void imx6_get_boot_source(enum bootsource *src, int *instance)
> >  {
> >  	void __iomem *src_base = IOMEM(MX6_SRC_BASE_ADDR);
> > @@ -410,6 +417,26 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
> >  
> >  	bootsrc = imx53_bootsource_internal(bootmode);
> >  
> > +	/*
> > +	 * imx6_bootsource_serial() can't detect cases where the boot ROM
> > +	 * decided to use the serial downloader as a fall back (primary
> > +	 * boot source failed).
> > +	 *
> > +	 * Infer that the boot ROM used the USB serial downloader by
> > +	 * checking whether both the UDC and the clock enabling access
> > +	 * to its MMIO region are currently active...
> > +	 * This assumes:
> > +	 * - On fresh boots, PBL doesn't itself start a stopped UDC
> > +	 * - In barebox proper, boot source is saved before the UDC driver
> > +	 *   may enable the UDC
> > +	 */
> > +
> > +	if (imx6_usboh3_clk_active() &&
> > +	    is_chipidea_udc_running(IOMEM(MX6_OTG_BASE_ADDR))) {
> > +		*src = BOOTSOURCE_SERIAL;
> > +		return;
> > +	}
> > +
> >  	if (imx6_bootsource_serial(sbmr2) ||
> >  	    imx6_bootsource_serial_forced(bootsrc)) {
> >  		*src = BOOTSOURCE_SERIAL;
> > diff --git a/arch/arm/mach-imx/imx6.c b/arch/arm/mach-imx/imx6.c
> > index 9ccb391384..fa4cb093c3 100644
> > --- a/arch/arm/mach-imx/imx6.c
> > +++ b/arch/arm/mach-imx/imx6.c
> > @@ -205,10 +205,10 @@ int imx6_init(void)
> >  	void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR);
> >  	u64 mx6_uid;
> >  
> > -	imx6_init_lowlevel();
> > -
> >  	imx6_boot_save_loc();
> >  
> > +	imx6_init_lowlevel();
> 
> A comment inside imx6_init_lowlevel hinting at the dependency w.r.t 
> imx6_boot_save_loc would be nice.

Make sense.

Regards,
  Marco

> Apart from these nitpicks:
> 
> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> > +
> >  	mx6_silicon_revision = imx6_cpu_revision();
> >  	mx6_uid = imx6_uid();
> >  
> > diff --git a/include/soc/fsl/fsl_udc.h b/include/soc/fsl/fsl_udc.h
> > index b983f714c5..0b409a9f6b 100644
> > --- a/include/soc/fsl/fsl_udc.h
> > +++ b/include/soc/fsl/fsl_udc.h
> > @@ -1,6 +1,9 @@
> >  #ifndef __FSL_UDC_H
> >  #define __FSL_UDC_H
> >  
> > +#include <linux/types.h>
> > +#include <io.h>
> > +
> >  /* USB DR device mode registers (Little Endian) */
> >  struct usb_dr_device {
> >  	/* Capability register */
> > @@ -380,4 +383,12 @@ int imx_barebox_start_usb(void __iomem *dr, void *dest);
> >  int imx8mm_barebox_load_usb(void *dest);
> >  int imx8mm_barebox_start_usb(void *dest);
> >  
> > +static inline bool is_chipidea_udc_running(void __iomem *dr)
> > +{
> > +	struct usb_dr_device __iomem *dr_regs = dr;
> > +
> > +	return (readl(&dr_regs->usbmode) & USB_MODE_CTRL_MODE_DEVICE)
> > +		&& (readl(&dr_regs->usbcmd) & USB_CMD_RUN_STOP);
> > +}
> > +
> >  #endif /* __FSL_UDC_H */
> > 
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-12  7:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11 17:36 [PATCH] ARM: i.MX6: boot: detect USB serial downloader more reliable Marco Felsch
2021-08-11 18:30 ` Ahmad Fatoum
2021-08-12  7:38   ` Marco Felsch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox