[PATCH 3/3] spl: fit: Load compressed blob to heap buffer

[Loic Poulain]

CONFIG_SYS_LOAD_ADDR is usually configured as the address where
the kernel should be loaded at. It can be problematic to use it
as a generic temporary buffer for FIT compressed blobs.

An example is when the image is a compressed kernel with load
address equal to CONFIG_SYS_LOAD_ADDR, this causes (inplace)
decompression to fail.

Instead we can simply allocate a temporary buffer in the heap

Signed-off-by: Loic Poulain 
---
 common/spl/spl_fit.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 08428660b0..8a807db5ba 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -249,7 +249,7 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
ulong size;
ulong load_addr;
void *load_ptr;
-   void *src;
+   void *src, *src_ptr;
ulong overhead;
int nr_sectors;
uint8_t image_comp, type = -1;
@@ -289,8 +289,6 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
}
 
if (external_data) {
-   void *src_ptr;
-
/* External data */
if (fit_image_get_data_size(fit, node, &len))
return -ENOENT;
@@ -302,10 +300,13 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
return 0;
}
 
-   if (image_comp != IH_COMP_NONE)
-   src_ptr = map_sysmem(ALIGN(CONFIG_SYS_LOAD_ADDR, 
ARCH_DMA_MINALIGN), len);
-   else
+   if (image_comp != IH_COMP_NONE) {
+   src_ptr = malloc_cache_aligned(len + 2 * info->bl_len);
+   if (!src_ptr)
+   return -ENOMEM;
+   } else {
src_ptr = map_sysmem(ALIGN(load_addr, 
ARCH_DMA_MINALIGN), len);
+   }
length = len;
 
overhead = get_aligned_image_overhead(info, offset);
@@ -383,6 +384,9 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
image_info->entry_point = FDT_ERROR;
}
 
+   if (external_data && image_comp != IH_COMP_NONE)
+   free(src_ptr);
+
return 0;
 }
 
-- 
2.34.1

[PATCH 2/3] spl: fit: Add support for LZO compressed images

[Loic Poulain]

Signed-off-by: Loic Poulain 
---
 common/spl/spl_fit.c | 10 ++
 include/spl.h|  4 +++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 1d42cb1d10..08428660b0 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -215,6 +216,8 @@ static inline bool spl_fit_decompression_supported(uint8_t 
comp)
return IS_ENABLED(CONFIG_SPL_GZIP);
case IH_COMP_LZMA:
return IS_ENABLED(CONFIG_SPL_LZMA);
+   case IH_COMP_LZO:
+   return IS_ENABLED(CONFIG_SPL_LZO);
case IH_COMP_NONE:
return true;
}
@@ -357,6 +360,13 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
return -EIO;
}
length = loadEnd - CONFIG_SYS_LOAD_ADDR;
+   } else if (IS_ENABLED(CONFIG_SPL_LZO) && image_comp == IH_COMP_LZO) {
+   size = CONFIG_SYS_BOOTM_LEN;
+   if (lzop_decompress(src, length, load_ptr, &size)) {
+   puts("Uncompressing error\n");
+   return -EIO;
+   }
+   length = size;
} else {
memcpy(load_ptr, src, length);
}
diff --git a/include/spl.h b/include/spl.h
index 8ff20adc28..e07092372a 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -1016,6 +1016,8 @@ int spl_load_fit_image(struct spl_image_info *spl_image,
  */
 static inline bool spl_decompression_enabled(void)
 {
-   return IS_ENABLED(CONFIG_SPL_GZIP) || IS_ENABLED(CONFIG_SPL_LZMA);
+   return IS_ENABLED(CONFIG_SPL_GZIP) || IS_ENABLED(CONFIG_SPL_LZMA) ||
+   IS_ENABLED(CONFIG_SPL_LZO);
 }
+
 #endif
-- 
2.34.1

[PATCH 1/3] spl: fit: Discard decompression if not supported

[Loic Poulain]

And simplify further decompression testing.

Signed-off-by: Loic Poulain 
---
 common/spl/spl_fit.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 70d8d5942d..1d42cb1d10 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -208,6 +208,20 @@ static int get_aligned_image_size(struct spl_load_info 
*info, int data_size,
return (data_size + info->bl_len - 1) / info->bl_len;
 }
 
+static inline bool spl_fit_decompression_supported(uint8_t comp)
+{
+   switch (comp) {
+   case IH_COMP_GZIP:
+   return IS_ENABLED(CONFIG_SPL_GZIP);
+   case IH_COMP_LZMA:
+   return IS_ENABLED(CONFIG_SPL_LZMA);
+   case IH_COMP_NONE:
+   return true;
+   }
+
+   return false;
+}
+
 /**
  * load_simple_fit(): load the image described in a certain FIT node
  * @info:  points to information about the device to load data from
@@ -235,7 +249,7 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
void *src;
ulong overhead;
int nr_sectors;
-   uint8_t image_comp = -1, type = -1;
+   uint8_t image_comp, type = -1;
const void *data;
const void *fit = ctx->fit;
bool external_data = false;
@@ -248,9 +262,11 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
debug("%s ", genimg_get_type_name(type));
}
 
-   if (spl_decompression_enabled()) {
-   fit_image_get_comp(fit, node, &image_comp);
-   debug("%s ", genimg_get_comp_name(image_comp));
+   fit_image_get_comp(fit, node, &image_comp);
+   if (!spl_fit_decompression_supported(image_comp)) {
+   debug("Discard unsupported compression %s ",
+ genimg_get_comp_name(image_comp));
+   image_comp = IH_COMP_NONE;
}
 
if (fit_image_get_load(fit, node, &load_addr)) {
@@ -283,8 +299,7 @@ static int load_simple_fit(struct spl_load_info *info, 
ulong sector,
return 0;
}
 
-   if (spl_decompression_enabled() &&
-   (image_comp == IH_COMP_GZIP || image_comp == IH_COMP_LZMA))
+   if (image_comp != IH_COMP_NONE)
src_ptr = map_sysmem(ALIGN(CONFIG_SYS_LOAD_ADDR, 
ARCH_DMA_MINALIGN), len);
else
src_ptr = map_sysmem(ALIGN(load_addr, 
ARCH_DMA_MINALIGN), len);
-- 
2.34.1

Re: [PATCH] usb: gadget: sdp: Option to enable SDP read register command

[Loic Poulain]

Hi Marek,

On Mon, 14 Aug 2023 at 01:53, Marek Vasut  wrote:
>
> On 8/13/23 10:39, Loic Poulain wrote:
> > The SDP read register command can be used to read any memory
> > mapped address of the device (ddr, registers...). It can then
> > be exploited by an attacker to access sensitive data/values,
> > especially when running SDP from SPL, as SPL runs with highest
> > privileges in ARM secure mode.
> >
> > Without read, SDP still useful to bootstrap and jump on (signed)
> > blob such as u-boot with write and jump commands, but reading
> > is optional in that case (debug purpose).
> >
> > NXP SoCs usually have a dedicated SDP_READ_DISABLE fuse to disable
> > SDP read command in their ROM SDP implementation, so it seems quite
> > reasonable to make it optional from u-boot/spl as well.
>
> If there is a fuse, why not read the fuse and disable READ based on that
> fuse instead ?

Well, fuse is more a way to tune a specific ROM code here, not the software.
It would be more generic to make it a build config like other features, and one
may purposely force SDP READ in SPL, even if disabled at ROM level. That
said we could also introduce a weak board_sdp_read_allowed() function...

Let me know what you prefer.

Regards,
Loic

[PATCH] usb: gadget: sdp: Option to enable SDP read register command

[Loic Poulain]

The SDP read register command can be used to read any memory
mapped address of the device (ddr, registers...). It can then
be exploited by an attacker to access sensitive data/values,
especially when running SDP from SPL, as SPL runs with highest
privileges in ARM secure mode.

Without read, SDP still useful to bootstrap and jump on (signed)
blob such as u-boot with write and jump commands, but reading
is optional in that case (debug purpose).

NXP SoCs usually have a dedicated SDP_READ_DISABLE fuse to disable
SDP read command in their ROM SDP implementation, so it seems quite
reasonable to make it optional from u-boot/spl as well.

Signed-off-by: Loic Poulain 
---
 drivers/usb/gadget/Kconfig | 14 ++
 drivers/usb/gadget/f_sdp.c |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 1cfe602284..50cf7c0dae 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -195,6 +195,13 @@ config USB_FUNCTION_SDP
  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.
 
+config SDP_READ
+   bool "Enable SDP READ command"
+   depends on USB_FUNCTION_SDP
+   default n
+   help
+ Enable SDP secure sensitive SDP read register command.
+
 config USB_FUNCTION_THOR
bool "Enable USB THOR gadget"
help
@@ -344,6 +351,13 @@ config SPL_USB_SDP_SUPPORT
  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.
 
+config SPL_SDP_READ
+   bool "Enable SDP READ command in SPL"
+   depends on SPL_USB_SDP_SUPPORT
+   default n
+   help
+ Enable SDP secure sensitive SDP read register command.
+
 config SPL_SDP_USB_DEV
int "SDP USB controller index in SPL"
default 0
diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
index 4da5a160a0..48a68a50d9 100644
--- a/drivers/usb/gadget/f_sdp.c
+++ b/drivers/usb/gadget/f_sdp.c
@@ -310,6 +310,7 @@ static void sdp_rx_command_complete(struct usb_ep *ep, 
struct usb_request *req)
  be32_to_cpu(cmd->addr), be32_to_cpu(cmd->cnt));
 
switch (be16_to_cpu(cmd->cmd)) {
+#if CONFIG_IS_ENABLED(SDP_READ)
case SDP_READ_REGISTER:
sdp->always_send_status = false;
sdp->error_status = 0x0;
@@ -321,6 +322,7 @@ static void sdp_rx_command_complete(struct usb_ep *ep, 
struct usb_request *req)
printf("Reading %d registers at 0x%08x... ",
   sdp->dnl_bytes_remaining, sdp->dnl_address);
break;
+#endif
case SDP_WRITE_FILE:
sdp->always_send_status = true;
sdp->error_status = SDP_WRITE_FILE_COMPLETE;
-- 
2.34.1

Re: [PATCH v2 3/3] test: dm: mmc: Check block erasing boundaries

[Loic Poulain]

On Fri, 27 Jan 2023 at 15:30, Simon Glass  wrote:
>
> Hi Loic,
>
> On Thu, 26 Jan 2023 at 02:24, Loic Poulain  wrote:
> >
> > Verify that erasing blocks does not impact adjacent ones.
> > - Write four blocks [0 1 2 3]
> > - Erase two blocks [ 1 2 ]
> > - Verify [0 1 2 3 ]
> >
> > Signed-off-by: Loic Poulain 
> > ---
> > v2: Add this change to the series
> >
> >  test/dm/mmc.c | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
>
> This looks good, but can you add the trim command to
> sandbox_mmc_send_cmd()? Then you can test that as well.

The TRIM option is not exposed, it is internally managed inside mmc
depending on the need for it. So from a testing perspective, I can
only test that the mmc erase is doing what we ask correctly.

Regards,
Loic

Re: [PATCH v2 2/3] mmc: erase: Use TRIM erase when available

[Loic Poulain]

Hi Jaehoon,

On Mon, 6 Feb 2023 at 06:05, Jaehoon Chung  wrote:
>
> Hi,
>
> > -Original Message-
> > From: Loic Poulain 
> > Sent: Thursday, January 26, 2023 6:24 PM
> > To: s...@chromium.org; peng@nxp.com; jh80.ch...@samsung.com
> > Cc: u-boot@lists.denx.de; Loic Poulain 
> > Subject: [PATCH v2 2/3] mmc: erase: Use TRIM erase when available
> >
> > The default erase command applies on erase group unit, and
> > simply round down to erase group size. When the start block
> > is not aligned to erase group size (e.g. erasing partition)
> > it causes unwanted erasing of the previous blocks, part of
> > the same erase group (e.g. owned by other logical partition,
> > or by the partition table itself).
> >
> > To prevent this issue, a simple solution is to use TRIM as
> > argument of the Erase command, which is usually supported
> > with eMMC > 4.0, and allow to apply erase operation to write
> > blocks instead of erase group
> >
> > Signed-off-by: Loic Poulain 
> > ---
> > v2: Add mmc unit test change to the series
> >
> >  drivers/mmc/mmc_write.c | 34 +++---
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
> > index 5b7aeeb012..a6f93380dd 100644
> > --- a/drivers/mmc/mmc_write.c
> > +++ b/drivers/mmc/mmc_write.c
> > @@ -15,7 +15,7 @@
> >  #include 
> >  #include "mmc_private.h"
> >
> > -static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> > +static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt, 
> > u32 args)
> >  {
> >   struct mmc_cmd cmd;
> >   ulong end;
> > @@ -52,7 +52,7 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, 
> > lbaint_t blkcnt)
> >   goto err_out;
> >
> >   cmd.cmdidx = MMC_CMD_ERASE;
> > - cmd.cmdarg = MMC_ERASE_ARG;
> > + cmd.cmdarg = args ? args : MMC_ERASE_ARG;
>
> It there any case to pass by other value?

Not at the moment, but it can be used to support eMMC 'Secure Erase' arg.

Regards,
Loic

Re: [PATCH v2 4/5] lib: sha256: Add support for hardware specific sha256_process

[Loic Poulain]

Hi Simon,

On Tue, 7 Feb 2023 at 05:05, Simon Glass  wrote:
>
> Hi Loic,
>
> On Mon, 6 Feb 2023 at 15:12, Loic Poulain  wrote:
> >
> > Hi Simon,
> >
> > Le lun. 6 févr. 2023 à 18:12, Simon Glass  a écrit :
> >>
> >> Hi Loic,
> >>
> >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain  wrote:
> >> >
> >> > Mark sha256_process as weak to allow hardware specific implementation.
> >> > Add parameter for supporting multiple blocks processing.
> >> >
> >> > Signed-off-by: Loic Poulain 
> >> > ---
> >> >  lib/sha256.c | 26 +++---
> >> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >> >
[...]
> >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char 
> >> > *data,
> >> > +  unsigned int blocks)
> >> > +{
> >> > +   if (!blocks)
> >> > +   return;
> >> > +
> >> > +   while (blocks--) {
> >> > +   sha256_process_one(ctx, data);
> >> > +   data += 64;
> >> > +   }
> >> > +}
> >> > +
> >> >  void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t 
> >> > length)
> >> >  {
> >> > uint32_t left, fill;
> >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const 
> >> > uint8_t *input, uint32_t length)
> >> >
> >> > if (left && length >= fill) {
> >> > memcpy((void *) (ctx->buffer + left), (void *) input, 
> >> > fill);
> >> > -   sha256_process(ctx, ctx->buffer);
> >> > +   sha256_process(ctx, ctx->buffer, 1);
> >> > length -= fill;
> >> > input += fill;
> >> > left = 0;
> >> > }
> >> >
> >> > -   while (length >= 64) {
> >> > -   sha256_process(ctx, input);
> >> > -   length -= 64;
> >> > -   input += 64;
> >> > -   }
> >> > +   sha256_process(ctx, input, length / 64);
> >> > +   input += length / 64 * 64;
> >> > +   length = length % 64;
> >> >
> >> > if (length)
> >> > memcpy((void *) (ctx->buffer + left), (void *) input, 
> >> > length);
> >> > --
> >> > 2.7.4
> >> >
> >>
> >> I just came across this patch as it broke minnowmax.
> >
> >
> > Ok, is it a build time or runtime break?
>
> Build, but you need the binary blobs to see it :-(
> >>
> >> This should be using driver model, not weak functions. Please can you
> >> take a look?

Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig;
gcc-11.3.0), and I've not observed any issue (but I had to fake some
of the binary blobs...). Could you share the build problem/error you
encountered? As you mentioned it, Is the error specifically related to
_weak function linking? Would like to have a simple and quick fix
before trying to move on to a more proper DM based solution.

Thanks,
Loic


> >
> >
> > Yes I can look at it in the next few days. I have used weak function 
> > because it’s an architecture feature offered by armv8 instructions, It’s 
> > not strictly speaking an internal device/IP.
>
> Thanks.
>
> Right, same as hardware-accelerated hashing hardware in my book.
>
> See hash.c which has become a mess. We have been trying to make do
> with a list of algos, but given all the updates I think needs a new
> UCLASS_HASH with the same operations as in hash.h
>
> Regards,
> Simon

Re: [PATCH v2 4/5] lib: sha256: Add support for hardware specific sha256_process

[Loic Poulain]

Hi Simon,

Le lun. 6 févr. 2023 à 18:12, Simon Glass  a écrit :

> Hi Loic,
>
> On Wed, 1 Jun 2022 at 12:27, Loic Poulain  wrote:
> >
> > Mark sha256_process as weak to allow hardware specific implementation.
> > Add parameter for supporting multiple blocks processing.
> >
> > Signed-off-by: Loic Poulain 
> > ---
> >  lib/sha256.c | 26 +++---
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/sha256.c b/lib/sha256.c
> > index c1fe93d..50b0b51 100644
> > --- a/lib/sha256.c
> > +++ b/lib/sha256.c
> > @@ -14,6 +14,8 @@
> >  #include 
> >  #include 
> >
> > +#include 
> > +
> >  const uint8_t sha256_der_prefix[SHA256_DER_LEN] = {
> > 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86,
> > 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05,
> > @@ -55,7 +57,7 @@ void sha256_starts(sha256_context * ctx)
> > ctx->state[7] = 0x5BE0CD19;
> >  }
> >
> > -static void sha256_process(sha256_context *ctx, const uint8_t data[64])
> > +static void sha256_process_one(sha256_context *ctx, const uint8_t
> data[64])
> >  {
> > uint32_t temp1, temp2;
> > uint32_t W[64];
> > @@ -186,6 +188,18 @@ static void sha256_process(sha256_context *ctx,
> const uint8_t data[64])
> > ctx->state[7] += H;
> >  }
> >
> > +__weak void sha256_process(sha256_context *ctx, const unsigned char
> *data,
> > +  unsigned int blocks)
> > +{
> > +   if (!blocks)
> > +   return;
> > +
> > +   while (blocks--) {
> > +   sha256_process_one(ctx, data);
> > +   data += 64;
> > +   }
> > +}
> > +
> >  void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t
> length)
> >  {
> > uint32_t left, fill;
> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const
> uint8_t *input, uint32_t length)
> >
> > if (left && length >= fill) {
> > memcpy((void *) (ctx->buffer + left), (void *) input,
> fill);
> > -   sha256_process(ctx, ctx->buffer);
> > +   sha256_process(ctx, ctx->buffer, 1);
> > length -= fill;
> > input += fill;
> > left = 0;
> > }
> >
> > -   while (length >= 64) {
> > -   sha256_process(ctx, input);
> > -   length -= 64;
> > -   input += 64;
> > -   }
> > +   sha256_process(ctx, input, length / 64);
> > +   input += length / 64 * 64;
> > +   length = length % 64;
> >
> > if (length)
> > memcpy((void *) (ctx->buffer + left), (void *) input,
> length);
> > --
> > 2.7.4
> >
>
> I just came across this patch as it broke minnowmax.


Ok, is it a build time or runtime break?


>
> This should be using driver model, not weak functions. Please can you
> take a look?


Yes I can look at it in the next few days. I have used weak function
because it’s an architecture feature offered by armv8 instructions, It’s
not strictly speaking an internal device/IP.

Regards,
Loic

>

Re: [PATCH v2 2/3] mmc: erase: Use TRIM erase when available

[Loic Poulain]

Hi Simon,

On Sat, 28 Jan 2023 at 23:01, Simon Glass  wrote:
>
> Hi Loic,
>
> On Thu, 26 Jan 2023 at 02:24, Loic Poulain  wrote:
> >
> > The default erase command applies on erase group unit, and
> > simply round down to erase group size. When the start block
> > is not aligned to erase group size (e.g. erasing partition)
> > it causes unwanted erasing of the previous blocks, part of
> > the same erase group (e.g. owned by other logical partition,
> > or by the partition table itself).
> >
> > To prevent this issue, a simple solution is to use TRIM as
> > argument of the Erase command, which is usually supported
> > with eMMC > 4.0, and allow to apply erase operation to write
> > blocks instead of erase group
> >
> > Signed-off-by: Loic Poulain 
> > ---
> > v2: Add mmc unit test change to the series
> >
> >  drivers/mmc/mmc_write.c | 34 +++---
> >  1 file changed, 23 insertions(+), 11 deletions(-)
>
> Reviewed-by: Simon Glass 
>
> Please see below
>
> >
> > diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
> > index 5b7aeeb012..a6f93380dd 100644
> > --- a/drivers/mmc/mmc_write.c
> > +++ b/drivers/mmc/mmc_write.c
> > @@ -15,7 +15,7 @@
> >  #include 
> >  #include "mmc_private.h"
> >
> > -static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> > +static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt, 
> > u32 args)
> >  {
> > struct mmc_cmd cmd;
> > ulong end;
> > @@ -52,7 +52,7 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, 
> > lbaint_t blkcnt)
> > goto err_out;
> >
> > cmd.cmdidx = MMC_CMD_ERASE;
> > -   cmd.cmdarg = MMC_ERASE_ARG;
> > +   cmd.cmdarg = args ? args : MMC_ERASE_ARG;
> > cmd.resp_type = MMC_RSP_R1b;
> >
> > err = mmc_send_cmd(mmc, &cmd, NULL);
> > @@ -77,7 +77,7 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t 
> > start, lbaint_t blkcnt)
> >  #endif
> > int dev_num = block_dev->devnum;
> > int err = 0;
> > -   u32 start_rem, blkcnt_rem;
> > +   u32 start_rem, blkcnt_rem, erase_args = 0;
> > struct mmc *mmc = find_mmc_device(dev_num);
> > lbaint_t blk = 0, blk_r = 0;
> > int timeout_ms = 1000;
> > @@ -97,13 +97,25 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t 
> > start, lbaint_t blkcnt)
> >  */
> > err = div_u64_rem(start, mmc->erase_grp_size, &start_rem);
> > err = div_u64_rem(blkcnt, mmc->erase_grp_size, &blkcnt_rem);
> > -   if (start_rem || blkcnt_rem)
> > -   printf("\n\nCaution! Your devices Erase group is 0x%x\n"
> > -  "The erase range would be change to "
> > -  "0x" LBAF "~0x" LBAF "\n\n",
> > -  mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 
> > 1),
> > -  ((start + blkcnt + mmc->erase_grp_size - 1)
> > -  & ~(mmc->erase_grp_size - 1)) - 1);
> > +   if (start_rem || blkcnt_rem) {
> > +   if (mmc->can_trim) {
> > +   /* Trim function applies the erase operation to 
> > write
> > +* blocks instead of erase groups.
> > +*/
> > +   erase_args = MMC_TRIM_ARG;
> > +   } else {
> > +   /* The card ignores all LSB's below the erase group
> > +* size, rounding down the addess to a erase group
> > +* boundary.
> > +*/
> > +   printf("\n\nCaution! Your devices Erase group is 
> > 0x%x\n"
> > +  "The erase range would be change to "
> > +  "0x" LBAF "~0x" LBAF "\n\n",
> > +  mmc->erase_grp_size, start & 
> > ~(mmc->erase_grp_size - 1),
> > +  ((start + blkcnt + mmc->erase_grp_size - 1)
> > +  & ~(mmc->erase_grp_size - 1)) - 1);
>
> Should this return an error, or just go ahead?

It would indeed make sense to return an error since mmc_erase does not
perform what we expect. Now, since this behavior exists for a while,
we may also want to keep it for legacy, though it should be a corner
case...

Regards,
Loic

[PATCH v2 3/3] test: dm: mmc: Check block erasing boundaries

[Loic Poulain]

Verify that erasing blocks does not impact adjacent ones.
- Write four blocks [0 1 2 3]
- Erase two blocks [ 1 2 ]
- Verify [0 1 2 3 ]

Signed-off-by: Loic Poulain 
---
v2: Add this change to the series

 test/dm/mmc.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/test/dm/mmc.c b/test/dm/mmc.c
index f744452ff2..b1eb8bee2f 100644
--- a/test/dm/mmc.c
+++ b/test/dm/mmc.c
@@ -30,7 +30,7 @@ static int dm_test_mmc_blk(struct unit_test_state *uts)
struct udevice *dev;
struct blk_desc *dev_desc;
int i;
-   char write[1024], read[1024];
+   char write[4 * 512], read[4 * 512];
 
ut_assertok(uclass_get_device(UCLASS_MMC, 0, &dev));
ut_assertok(blk_get_device_by_str("mmc", "0", &dev_desc));
@@ -39,14 +39,14 @@ static int dm_test_mmc_blk(struct unit_test_state *uts)
ut_asserteq(512, dev_desc->blksz);
for (i = 0; i < sizeof(write); i++)
write[i] = i;
-   ut_asserteq(2, blk_dwrite(dev_desc, 0, 2, write));
-   ut_asserteq(2, blk_dread(dev_desc, 0, 2, read));
+   ut_asserteq(4, blk_dwrite(dev_desc, 0, 4, write));
+   ut_asserteq(4, blk_dread(dev_desc, 0, 4, read));
ut_asserteq_mem(write, read, sizeof(write));
 
-   /* Now erase them */
-   memset(write, '\0', sizeof(write));
-   ut_asserteq(2, blk_derase(dev_desc, 0, 2));
-   ut_asserteq(2, blk_dread(dev_desc, 0, 2, read));
+   /* Now erase two of them [1 - 2] and verify all blocks */
+   memset(&write[512], '\0', 2 * 512);
+   ut_asserteq(2, blk_derase(dev_desc, 1, 2));
+   ut_asserteq(4, blk_dread(dev_desc, 0, 4, read));
ut_asserteq_mem(write, read, sizeof(write));
 
return 0;
-- 
2.34.1

[PATCH v2 2/3] mmc: erase: Use TRIM erase when available

[Loic Poulain]

The default erase command applies on erase group unit, and
simply round down to erase group size. When the start block
is not aligned to erase group size (e.g. erasing partition)
it causes unwanted erasing of the previous blocks, part of
the same erase group (e.g. owned by other logical partition,
or by the partition table itself).

To prevent this issue, a simple solution is to use TRIM as
argument of the Erase command, which is usually supported
with eMMC > 4.0, and allow to apply erase operation to write
blocks instead of erase group

Signed-off-by: Loic Poulain 
---
v2: Add mmc unit test change to the series

 drivers/mmc/mmc_write.c | 34 +++---
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
index 5b7aeeb012..a6f93380dd 100644
--- a/drivers/mmc/mmc_write.c
+++ b/drivers/mmc/mmc_write.c
@@ -15,7 +15,7 @@
 #include 
 #include "mmc_private.h"
 
-static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
+static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt, u32 
args)
 {
struct mmc_cmd cmd;
ulong end;
@@ -52,7 +52,7 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, 
lbaint_t blkcnt)
goto err_out;
 
cmd.cmdidx = MMC_CMD_ERASE;
-   cmd.cmdarg = MMC_ERASE_ARG;
+   cmd.cmdarg = args ? args : MMC_ERASE_ARG;
cmd.resp_type = MMC_RSP_R1b;
 
err = mmc_send_cmd(mmc, &cmd, NULL);
@@ -77,7 +77,7 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t start, 
lbaint_t blkcnt)
 #endif
int dev_num = block_dev->devnum;
int err = 0;
-   u32 start_rem, blkcnt_rem;
+   u32 start_rem, blkcnt_rem, erase_args = 0;
struct mmc *mmc = find_mmc_device(dev_num);
lbaint_t blk = 0, blk_r = 0;
int timeout_ms = 1000;
@@ -97,13 +97,25 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t 
start, lbaint_t blkcnt)
 */
err = div_u64_rem(start, mmc->erase_grp_size, &start_rem);
err = div_u64_rem(blkcnt, mmc->erase_grp_size, &blkcnt_rem);
-   if (start_rem || blkcnt_rem)
-   printf("\n\nCaution! Your devices Erase group is 0x%x\n"
-  "The erase range would be change to "
-  "0x" LBAF "~0x" LBAF "\n\n",
-  mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 1),
-  ((start + blkcnt + mmc->erase_grp_size - 1)
-  & ~(mmc->erase_grp_size - 1)) - 1);
+   if (start_rem || blkcnt_rem) {
+   if (mmc->can_trim) {
+   /* Trim function applies the erase operation to write
+* blocks instead of erase groups.
+*/
+   erase_args = MMC_TRIM_ARG;
+   } else {
+   /* The card ignores all LSB's below the erase group
+* size, rounding down the addess to a erase group
+* boundary.
+*/
+   printf("\n\nCaution! Your devices Erase group is 0x%x\n"
+  "The erase range would be change to "
+  "0x" LBAF "~0x" LBAF "\n\n",
+  mmc->erase_grp_size, start & 
~(mmc->erase_grp_size - 1),
+  ((start + blkcnt + mmc->erase_grp_size - 1)
+  & ~(mmc->erase_grp_size - 1)) - 1);
+   }
+   }
 
while (blk < blkcnt) {
if (IS_SD(mmc) && mmc->ssr.au) {
@@ -113,7 +125,7 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t 
start, lbaint_t blkcnt)
blk_r = ((blkcnt - blk) > mmc->erase_grp_size) ?
mmc->erase_grp_size : (blkcnt - blk);
}
-   err = mmc_erase_t(mmc, start + blk, blk_r);
+   err = mmc_erase_t(mmc, start + blk, blk_r, erase_args);
if (err)
break;
 
-- 
2.34.1

[PATCH v2 1/3] mmc: Check support for TRIM operations

[Loic Poulain]

When secure/insecure TRIM operations are supported.
When used as erase command argument it applies the
erase operation to write blocks instead of erase
groups.

Signed-off-by: Loic Poulain 
---
v2: Add mmc unit test change to the series

 drivers/mmc/mmc.c | 3 +++
 include/mmc.h | 4 
 2 files changed, 7 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 210703ea46..e5f5ccb5f4 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -2432,6 +2432,9 @@ static int mmc_startup_v4(struct mmc *mmc)
 
mmc->wr_rel_set = ext_csd[EXT_CSD_WR_REL_SET];
 
+   mmc->can_trim =
+   !!(ext_csd[EXT_CSD_SEC_FEATURE] & EXT_CSD_SEC_FEATURE_TRIM_EN);
+
return 0;
 error:
if (mmc->ext_csd) {
diff --git a/include/mmc.h b/include/mmc.h
index 571fa625d0..f6e23625ca 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -241,6 +241,7 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx)
 #define EXT_CSD_HC_WP_GRP_SIZE 221 /* RO */
 #define EXT_CSD_HC_ERASE_GRP_SIZE  224 /* RO */
 #define EXT_CSD_BOOT_MULT  226 /* RO */
+#define EXT_CSD_SEC_FEATURE231 /* RO */
 #define EXT_CSD_GENERIC_CMD6_TIME   248 /* RO */
 #define EXT_CSD_BKOPS_SUPPORT  502 /* RO */
 
@@ -315,6 +316,8 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx)
 #define EXT_CSD_WR_DATA_REL_USR(1 << 0)/* user data 
area WR_REL */
 #define EXT_CSD_WR_DATA_REL_GP(x)  (1 << ((x)+1))  /* GP part (x+1) WR_REL 
*/
 
+#define EXT_CSD_SEC_FEATURE_TRIM_EN(1 << 4) /* Support secure & insecure 
trim */
+
 #define R1_ILLEGAL_COMMAND (1 << 22)
 #define R1_APP_CMD (1 << 5)
 
@@ -687,6 +690,7 @@ struct mmc {
uint tran_speed;
uint legacy_speed; /* speed for the legacy mode provided by the card */
uint read_bl_len;
+   bool can_trim;
 #if CONFIG_IS_ENABLED(MMC_WRITE)
uint write_bl_len;
uint erase_grp_size;/* in 512-byte sectors */
-- 
2.34.1

[PATCH 2/2] mmc: erase: Use TRIM erase when available

[Loic Poulain]

The default erase command applies on erase group unit, and
simply round down to erase group size. When the start block
is not aligned to erase group size (e.g. erasing partition)
it causes unwanted erasing of the previous blocks, part of
the same erase group (e.g. owned by other logical partition,
or by the partition table itself).

To prevent this issue, a simple solution is to use TRIM as
argument of the Erase command, which is usually supported
with eMMC > 4.0, and allow to apply erase operation to write
blocks instead of erase group

Signed-off-by: Loic Poulain 
---
 drivers/mmc/mmc_write.c | 34 +++---
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
index 5b7aeeb012..a6f93380dd 100644
--- a/drivers/mmc/mmc_write.c
+++ b/drivers/mmc/mmc_write.c
@@ -15,7 +15,7 @@
 #include 
 #include "mmc_private.h"
 
-static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
+static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt, u32 
args)
 {
struct mmc_cmd cmd;
ulong end;
@@ -52,7 +52,7 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, 
lbaint_t blkcnt)
goto err_out;
 
cmd.cmdidx = MMC_CMD_ERASE;
-   cmd.cmdarg = MMC_ERASE_ARG;
+   cmd.cmdarg = args ? args : MMC_ERASE_ARG;
cmd.resp_type = MMC_RSP_R1b;
 
err = mmc_send_cmd(mmc, &cmd, NULL);
@@ -77,7 +77,7 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t start, 
lbaint_t blkcnt)
 #endif
int dev_num = block_dev->devnum;
int err = 0;
-   u32 start_rem, blkcnt_rem;
+   u32 start_rem, blkcnt_rem, erase_args = 0;
struct mmc *mmc = find_mmc_device(dev_num);
lbaint_t blk = 0, blk_r = 0;
int timeout_ms = 1000;
@@ -97,13 +97,25 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t 
start, lbaint_t blkcnt)
 */
err = div_u64_rem(start, mmc->erase_grp_size, &start_rem);
err = div_u64_rem(blkcnt, mmc->erase_grp_size, &blkcnt_rem);
-   if (start_rem || blkcnt_rem)
-   printf("\n\nCaution! Your devices Erase group is 0x%x\n"
-  "The erase range would be change to "
-  "0x" LBAF "~0x" LBAF "\n\n",
-  mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 1),
-  ((start + blkcnt + mmc->erase_grp_size - 1)
-  & ~(mmc->erase_grp_size - 1)) - 1);
+   if (start_rem || blkcnt_rem) {
+   if (mmc->can_trim) {
+   /* Trim function applies the erase operation to write
+* blocks instead of erase groups.
+*/
+   erase_args = MMC_TRIM_ARG;
+   } else {
+   /* The card ignores all LSB's below the erase group
+* size, rounding down the addess to a erase group
+* boundary.
+*/
+   printf("\n\nCaution! Your devices Erase group is 0x%x\n"
+  "The erase range would be change to "
+  "0x" LBAF "~0x" LBAF "\n\n",
+  mmc->erase_grp_size, start & 
~(mmc->erase_grp_size - 1),
+  ((start + blkcnt + mmc->erase_grp_size - 1)
+  & ~(mmc->erase_grp_size - 1)) - 1);
+   }
+   }
 
while (blk < blkcnt) {
if (IS_SD(mmc) && mmc->ssr.au) {
@@ -113,7 +125,7 @@ ulong mmc_berase(struct blk_desc *block_dev, lbaint_t 
start, lbaint_t blkcnt)
blk_r = ((blkcnt - blk) > mmc->erase_grp_size) ?
mmc->erase_grp_size : (blkcnt - blk);
}
-   err = mmc_erase_t(mmc, start + blk, blk_r);
+   err = mmc_erase_t(mmc, start + blk, blk_r, erase_args);
if (err)
break;
 
-- 
2.34.1

[PATCH 1/2] mmc: Check support for TRIM operations

[Loic Poulain]

When secure/insecure TRIM operations are supported.
When used as erase command argument it applies the
erase operation to write blocks instead of erase
groups.

Signed-off-by: Loic Poulain 
---
 drivers/mmc/mmc.c | 3 +++
 include/mmc.h | 4 
 2 files changed, 7 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 210703ea46..e5f5ccb5f4 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -2432,6 +2432,9 @@ static int mmc_startup_v4(struct mmc *mmc)
 
mmc->wr_rel_set = ext_csd[EXT_CSD_WR_REL_SET];
 
+   mmc->can_trim =
+   !!(ext_csd[EXT_CSD_SEC_FEATURE] & EXT_CSD_SEC_FEATURE_TRIM_EN);
+
return 0;
 error:
if (mmc->ext_csd) {
diff --git a/include/mmc.h b/include/mmc.h
index 571fa625d0..f6e23625ca 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -241,6 +241,7 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx)
 #define EXT_CSD_HC_WP_GRP_SIZE 221 /* RO */
 #define EXT_CSD_HC_ERASE_GRP_SIZE  224 /* RO */
 #define EXT_CSD_BOOT_MULT  226 /* RO */
+#define EXT_CSD_SEC_FEATURE231 /* RO */
 #define EXT_CSD_GENERIC_CMD6_TIME   248 /* RO */
 #define EXT_CSD_BKOPS_SUPPORT  502 /* RO */
 
@@ -315,6 +316,8 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx)
 #define EXT_CSD_WR_DATA_REL_USR(1 << 0)/* user data 
area WR_REL */
 #define EXT_CSD_WR_DATA_REL_GP(x)  (1 << ((x)+1))  /* GP part (x+1) WR_REL 
*/
 
+#define EXT_CSD_SEC_FEATURE_TRIM_EN(1 << 4) /* Support secure & insecure 
trim */
+
 #define R1_ILLEGAL_COMMAND (1 << 22)
 #define R1_APP_CMD (1 << 5)
 
@@ -687,6 +690,7 @@ struct mmc {
uint tran_speed;
uint legacy_speed; /* speed for the legacy mode provided by the card */
uint read_bl_len;
+   bool can_trim;
 #if CONFIG_IS_ENABLED(MMC_WRITE)
uint write_bl_len;
uint erase_grp_size;/* in 512-byte sectors */
-- 
2.34.1

[PATCH v4 2/2] serial: mxc: Speed-up character transmission

[Loic Poulain]

Instead of waiting for empty FIFO condition before writing a
character, wait for non-full FIFO condition.

This helps in saving several tens of milliseconds during boot
(depending verbosity).

Signed-off-by: Loic Poulain 
Tested-by: Lothar Waßmann 
Acked-by: Pali Rohár 
---
 v2: fixing transfert abort & char corruption commit
 v3: Reordering commits for good bisectability
 v4: no code change

 drivers/serial/serial_mxc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 9e5b987994..cac1922354 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -240,11 +240,11 @@ static void mxc_serial_putc(const char c)
if (c == '\n')
serial_putc('\r');
 
-   writel(c, &mxc_base->txd);
-
/* wait for transmitter to be ready */
-   while (!(readl(&mxc_base->ts) & UTS_TXEMPTY))
+   while (readl(&mxc_base->ts) & UTS_TXFULL)
schedule();
+
+   writel(c, &mxc_base->txd);
 }
 
 /* Test whether a character is in the RX buffer */
@@ -335,7 +335,7 @@ static int mxc_serial_putc(struct udevice *dev, const char 
ch)
struct mxc_serial_plat *plat = dev_get_plat(dev);
struct mxc_uart *const uart = plat->reg;
 
-   if (!(readl(&uart->ts) & UTS_TXEMPTY))
+   if (readl(&uart->ts) & UTS_TXFULL)
return -EAGAIN;
 
writel(ch, &uart->txd);
-- 
2.34.1

[PATCH v4 1/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

The u-boot console may show some corrupted characters when
printing in board_init() due to reset or baudrate change
of the UART (probe) before the TX FIFO has been completely
drained.

To fix this issue, and in case UART is still running, we now
try to flush the FIFO before proceeding to UART reinitialization.
For this we're waiting for Transmitter Complete bit, indicating
that the FIFO and the shift register are empty.

flushing has a 4ms timeout guard, which is normally more than
enough to consume the FIFO @ low baudrate (9600bps).

Signed-off-by: Loic Poulain 
Tested-by: Lothar Waßmann 
---
 v2: Add this commit to the series
 v3: Fix typo & reordering commits for good bisectabilit
 v4: Wait for transfer completion before baudrate change as well

 drivers/serial/serial_mxc.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 82c0d84628..9e5b987994 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* UART Control Register Bit Fields.*/
 #define URXD_CHARRDY   (1<<15)
@@ -144,8 +145,22 @@ struct mxc_uart {
u32 ts;
 };
 
+static void _mxc_serial_flush(struct mxc_uart *base)
+{
+   unsigned int timeout = 4000;
+
+   if (!(readl(&base->cr1) & UCR1_UARTEN) ||
+   !(readl(&base->cr2) & UCR2_TXEN))
+   return;
+
+   while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
+   udelay(1);
+}
+
 static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
 {
+   _mxc_serial_flush(base);
+
writel(0, &base->cr1);
writel(0, &base->cr2);
 
@@ -169,6 +184,8 @@ static void _mxc_serial_setbrg(struct mxc_uart *base, 
unsigned long clk,
 {
u32 tmp;
 
+   _mxc_serial_flush(base);
+
tmp = RFDIV << UFCR_RFDIV_SHF;
if (use_dte)
tmp |= UFCR_DCEDTE;
@@ -252,10 +269,17 @@ static int mxc_serial_init(void)
return 0;
 }
 
+static int mxc_serial_stop(void)
+{
+   _mxc_serial_flush(mxc_base);
+
+   return 0;
+}
+
 static struct serial_device mxc_serial_drv = {
.name   = "mxc_serial",
.start  = mxc_serial_init,
-   .stop   = NULL,
+   .stop   = mxc_serial_stop,
.setbrg = mxc_serial_setbrg,
.putc   = mxc_serial_putc,
.puts   = default_serial_puts,
-- 
2.34.1

Re: [PATCH v3 1/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

On Wed, 11 Jan 2023 at 09:08, Pali Rohár  wrote:
>
> On Wednesday 11 January 2023 08:53:30 Loic Poulain wrote:
> > On Wed, 11 Jan 2023 at 00:53, Pali Rohár  wrote:
> > >
> > > On Tuesday 10 January 2023 20:24:06 Loic Poulain wrote:
> > > > The u-boot console may show some corrupted characters when
> > > > printing in board_init() due to reset of the UART (probe)
> > > > before the TX FIFO has been completely drained.
> > > >
> > > > To fix this issue, and in case UART is still running, we now
> > > > try to flush the FIFO before proceeding to UART reinitialization.
> > > > For this we're waiting for Transmitter Complete bit, indicating
> > > > that the FIFO and the shift register are empty.
> > > >
> > > > flushing has a 4ms timeout guard, which is normally more than
> > > > enough to consume the FIFO @ low baudrate (9600bps).
> > > >
> > > > Signed-off-by: Loic Poulain 
> > > > Tested-by: Lothar Waßmann 
> > >
> > > Hello! Last time when I looked at this driver I was in impression that
> > > also _mxc_serial_setbrg() function requires calling some flush function
> > > at the beginning. Could you please check if it is needed or not? I'm
> > > really not sure.
> >
> > _mxc_serial_setbrg is usually called after init, which now includes that 
> > flush.
>
> I'm looking at it and this function is called also from
> mxc_serial_setbrg() callback, which is used by u-boot to change baudrate
> after the init.

Ok, good point, then it makes sense to add this guard here as well.

Re: [PATCH v2 2/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

On Wed, 11 Jan 2023 at 01:15, Simon Glass  wrote:
>
> Hi,
>
> On Tue, 10 Jan 2023 at 05:42, Loic Poulain  wrote:
> >
> > The u-boot console may show some corrupted characters when
> > printing in board_init() due to reset of the UART (probe)
> > before the TX FIFO has been completely drained.
> >
> > To fix this issue, and in case UART is still running, we now
> > try to flush the FIFO before proceding to UART reinitialization.
> > For this we're waiting for Transmitter Complete bit, indicating
> > that the FIFO and the shift register are empty.
> >
> > flushing has a 4ms timeout guard, which is normally more than
> > enough to consume the FIFO @ low baudrate (9600bps).
> >
> > Signed-off-by: Loic Poulain 
> > ---
> >  v2: Add this commit to the series
> >
> >  drivers/serial/serial_mxc.c | 24 +++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
>
> Can you just use serial_flush()? It is generic and should work with any 
> device.

Not directly, except that serial_flush is specific to DM and
CONSOLE_FLUSH config,
it's also not exactly what we're doing here, as we want to check if
the UART was running
before (re)initialization, and to limit the waiting loop in case the
TX/UART is in bad state or
stuck due to flow control.

Regards,
Loic

Re: [PATCH v3 2/2] serial: mxc: Speed-up character transmission

[Loic Poulain]

On Wed, 11 Jan 2023 at 00:55, Pali Rohár  wrote:
>
> On Tuesday 10 January 2023 20:24:07 Loic Poulain wrote:
> > Instead of waiting for empty FIFO condition before writing a
> > character, wait for non-full FIFO condition.
> >
> > This helps in saving several tens of milliseconds during boot
> > (depending verbosity).
> >
> > Signed-off-by: Loic Poulain 
> > Tested-by: Lothar Waßmann 
> > ---
> >  v2: fixing transfert abort & char corruption commit
> >  v3: Reordering commits for good bisectability
> >
> >  drivers/serial/serial_mxc.c | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > index 3c89781f2c..9899155c00 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -238,11 +238,11 @@ static void mxc_serial_putc(const char c)
> >   if (c == '\n')
> >   serial_putc('\r');
> >
> > - writel(c, &mxc_base->txd);
> > -
> >   /* wait for transmitter to be ready */
> > - while (!(readl(&mxc_base->ts) & UTS_TXEMPTY))
> > + while (readl(&mxc_base->ts) & UTS_TXFULL)
> >   schedule();
> > +
> > + writel(c, &mxc_base->txd);
> >  }
> >
> >  /* Test whether a character is in the RX buffer */
> > @@ -333,7 +333,7 @@ static int mxc_serial_putc(struct udevice *dev, const 
> > char ch)
> >   struct mxc_serial_plat *plat = dev_get_plat(dev);
> >   struct mxc_uart *const uart = plat->reg;
> >
> > - if (!(readl(&uart->ts) & UTS_TXEMPTY))
> > + if (readl(&uart->ts) & UTS_TXFULL)
> >   return -EAGAIN;
> >
> >   writel(ch, &uart->txd);
> > --
> > 2.34.1
> >
>
> Hello!
>
> In this driver there are two mxc_serial_putc() and two mxc_serial_getc()
> function. One for DM and one for non-DM version. It would be really nice
> to fix also second set of functions.

This commit changes both DM/non-DM putc() functions.

Regards,
Loic

Re: [PATCH v3 1/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

On Wed, 11 Jan 2023 at 00:53, Pali Rohár  wrote:
>
> On Tuesday 10 January 2023 20:24:06 Loic Poulain wrote:
> > The u-boot console may show some corrupted characters when
> > printing in board_init() due to reset of the UART (probe)
> > before the TX FIFO has been completely drained.
> >
> > To fix this issue, and in case UART is still running, we now
> > try to flush the FIFO before proceeding to UART reinitialization.
> > For this we're waiting for Transmitter Complete bit, indicating
> > that the FIFO and the shift register are empty.
> >
> > flushing has a 4ms timeout guard, which is normally more than
> > enough to consume the FIFO @ low baudrate (9600bps).
> >
> > Signed-off-by: Loic Poulain 
> > Tested-by: Lothar Waßmann 
>
> Hello! Last time when I looked at this driver I was in impression that
> also _mxc_serial_setbrg() function requires calling some flush function
> at the beginning. Could you please check if it is needed or not? I'm
> really not sure.

_mxc_serial_setbrg is usually called after init, which now includes that flush.

>
> > ---
> >  v2: Add this commit to the series
> >  v3: Fix typo & reordering commits for good bisectability
> >
> >  drivers/serial/serial_mxc.c | 24 +++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > index 82c0d84628..3c89781f2c 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -13,6 +13,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  /* UART Control Register Bit Fields.*/
> >  #define URXD_CHARRDY (1<<15)
> > @@ -144,8 +145,22 @@ struct mxc_uart {
> >   u32 ts;
> >  };
> >
> > +static void _mxc_serial_flush(struct mxc_uart *base)
> > +{
> > + unsigned int timeout = 4000;
> > +
> > + if (!(readl(&base->cr1) & UCR1_UARTEN) ||
> > + !(readl(&base->cr2) & UCR2_TXEN))
> > + return;
> > +
> > + while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
> > + udelay(1);
> > +}
> > +
> >  static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
> >  {
> > + _mxc_serial_flush(base);
> > +
> >   writel(0, &base->cr1);
> >   writel(0, &base->cr2);
> >
> > @@ -252,10 +267,17 @@ static int mxc_serial_init(void)
> >   return 0;
> >  }
> >
> > +static int mxc_serial_stop(void)
> > +{
> > + _mxc_serial_flush(mxc_base);
> > +
> > + return 0;
> > +}
> > +
> >  static struct serial_device mxc_serial_drv = {
> >   .name   = "mxc_serial",
> >   .start  = mxc_serial_init,
> > - .stop   = NULL,
> > + .stop   = mxc_serial_stop,
> >   .setbrg = mxc_serial_setbrg,
> >   .putc   = mxc_serial_putc,
> >   .puts   = default_serial_puts,
>
> Anyway, this code touches _only_ non-DM version of the driver. So same
> fix should be implemented also for DM version because non-DM is now
> legacy and will be removed in the future from U-Boot. Please look at the
> DM version too.

This code impacts both DM and non-DM, as _mxc_serial_init is a common version,
and was the main issue (several init() leading to corrupted char).

Regards,
Loic

[PATCH v3 2/2] serial: mxc: Speed-up character transmission

[Loic Poulain]

Instead of waiting for empty FIFO condition before writing a
character, wait for non-full FIFO condition.

This helps in saving several tens of milliseconds during boot
(depending verbosity).

Signed-off-by: Loic Poulain 
Tested-by: Lothar Waßmann 
---
 v2: fixing transfert abort & char corruption commit
 v3: Reordering commits for good bisectability

 drivers/serial/serial_mxc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 3c89781f2c..9899155c00 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -238,11 +238,11 @@ static void mxc_serial_putc(const char c)
if (c == '\n')
serial_putc('\r');
 
-   writel(c, &mxc_base->txd);
-
/* wait for transmitter to be ready */
-   while (!(readl(&mxc_base->ts) & UTS_TXEMPTY))
+   while (readl(&mxc_base->ts) & UTS_TXFULL)
schedule();
+
+   writel(c, &mxc_base->txd);
 }
 
 /* Test whether a character is in the RX buffer */
@@ -333,7 +333,7 @@ static int mxc_serial_putc(struct udevice *dev, const char 
ch)
struct mxc_serial_plat *plat = dev_get_plat(dev);
struct mxc_uart *const uart = plat->reg;
 
-   if (!(readl(&uart->ts) & UTS_TXEMPTY))
+   if (readl(&uart->ts) & UTS_TXFULL)
return -EAGAIN;
 
writel(ch, &uart->txd);
-- 
2.34.1

[PATCH v3 1/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

The u-boot console may show some corrupted characters when
printing in board_init() due to reset of the UART (probe)
before the TX FIFO has been completely drained.

To fix this issue, and in case UART is still running, we now
try to flush the FIFO before proceeding to UART reinitialization.
For this we're waiting for Transmitter Complete bit, indicating
that the FIFO and the shift register are empty.

flushing has a 4ms timeout guard, which is normally more than
enough to consume the FIFO @ low baudrate (9600bps).

Signed-off-by: Loic Poulain 
Tested-by: Lothar Waßmann 
---
 v2: Add this commit to the series
 v3: Fix typo & reordering commits for good bisectability 

 drivers/serial/serial_mxc.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 82c0d84628..3c89781f2c 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* UART Control Register Bit Fields.*/
 #define URXD_CHARRDY   (1<<15)
@@ -144,8 +145,22 @@ struct mxc_uart {
u32 ts;
 };
 
+static void _mxc_serial_flush(struct mxc_uart *base)
+{
+   unsigned int timeout = 4000;
+
+   if (!(readl(&base->cr1) & UCR1_UARTEN) ||
+   !(readl(&base->cr2) & UCR2_TXEN))
+   return;
+
+   while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
+   udelay(1);
+}
+
 static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
 {
+   _mxc_serial_flush(base);
+
writel(0, &base->cr1);
writel(0, &base->cr2);
 
@@ -252,10 +267,17 @@ static int mxc_serial_init(void)
return 0;
 }
 
+static int mxc_serial_stop(void)
+{
+   _mxc_serial_flush(mxc_base);
+
+   return 0;
+}
+
 static struct serial_device mxc_serial_drv = {
.name   = "mxc_serial",
.start  = mxc_serial_init,
-   .stop   = NULL,
+   .stop   = mxc_serial_stop,
.setbrg = mxc_serial_setbrg,
.putc   = mxc_serial_putc,
.puts   = default_serial_puts,
-- 
2.34.1

[PATCH v2 2/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

The u-boot console may show some corrupted characters when
printing in board_init() due to reset of the UART (probe)
before the TX FIFO has been completely drained.

To fix this issue, and in case UART is still running, we now
try to flush the FIFO before proceding to UART reinitialization.
For this we're waiting for Transmitter Complete bit, indicating
that the FIFO and the shift register are empty.

flushing has a 4ms timeout guard, which is normally more than
enough to consume the FIFO @ low baudrate (9600bps).

Signed-off-by: Loic Poulain 
---
 v2: Add this commit to the series

 drivers/serial/serial_mxc.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index f8c49865be..9899155c00 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* UART Control Register Bit Fields.*/
 #define URXD_CHARRDY   (1<<15)
@@ -144,8 +145,22 @@ struct mxc_uart {
u32 ts;
 };
 
+static void _mxc_serial_flush(struct mxc_uart *base)
+{
+   unsigned int timeout = 4000;
+
+   if (!(readl(&base->cr1) & UCR1_UARTEN) ||
+   !(readl(&base->cr2) & UCR2_TXEN))
+   return;
+
+   while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
+   udelay(1);
+}
+
 static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
 {
+   _mxc_serial_flush(base);
+
writel(0, &base->cr1);
writel(0, &base->cr2);
 
@@ -252,10 +267,17 @@ static int mxc_serial_init(void)
return 0;
 }
 
+static int mxc_serial_stop(void)
+{
+   _mxc_serial_flush(mxc_base);
+
+   return 0;
+}
+
 static struct serial_device mxc_serial_drv = {
.name   = "mxc_serial",
.start  = mxc_serial_init,
-   .stop   = NULL,
+   .stop   = mxc_serial_stop,
.setbrg = mxc_serial_setbrg,
.putc   = mxc_serial_putc,
.puts   = default_serial_puts,
-- 
2.34.1

[PATCH v2 1/2] serial: mxc: Speed-up character transmission

[Loic Poulain]

Instead of waiting for empty FIFO condition before writing a
character, wait for non-full FIFO condition.

This helps in saving several tens of milliseconds during boot
(depending verbosity).

Signed-off-by: Loic Poulain 
---
 v2: Add 2/2 fixing transfert abort & char corruption

 drivers/serial/serial_mxc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 82c0d84628..f8c49865be 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -223,11 +223,11 @@ static void mxc_serial_putc(const char c)
if (c == '\n')
serial_putc('\r');
 
-   writel(c, &mxc_base->txd);
-
/* wait for transmitter to be ready */
-   while (!(readl(&mxc_base->ts) & UTS_TXEMPTY))
+   while (readl(&mxc_base->ts) & UTS_TXFULL)
schedule();
+
+   writel(c, &mxc_base->txd);
 }
 
 /* Test whether a character is in the RX buffer */
@@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char 
ch)
struct mxc_serial_plat *plat = dev_get_plat(dev);
struct mxc_uart *const uart = plat->reg;
 
-   if (!(readl(&uart->ts) & UTS_TXEMPTY))
+   if (readl(&uart->ts) & UTS_TXFULL)
return -EAGAIN;
 
writel(ch, &uart->txd);
-- 
2.34.1

Re: [PATCH v2 2/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

On Tue, 10 Jan 2023 at 13:57, Fabio Estevam  wrote:
>
> > For this we're waiting for Transmitter Complete bit, indicating
> > that the FIFO and the shift register are empty.
> >
> > flushing has a 4ms timeout guard, which is normally more than
> > enough to consume the FIFO @ low baudrate (9600bps).
> >
> > Signed-off-by: Loic Poulain 
> > ---
> >  v2: Add this commit to the series
>
> Should this patch come first in the series?
>
> In case someone is bisecting, the current patch 1/2 may cause serial 
> corruption,
> which is fixed by 2/2.
>
> Can we avoid corruption by swapping the order of these patches?

Indeed, sending a v3.

Thanks,
Loic

[PATCH v2 2/2] serial: mxc: Wait for TX completion before reset

[Loic Poulain]

The u-boot console may show some corrupted characters when
printing in board_init() due to reset of the UART (probe)
before the TX FIFO has been completely drained.

To fix this issue, and in case UART is still running, we now
try to flush the FIFO before proceding to UART reinitialization.
For this we're waiting for Transmitter Complete bit, indicating
that the FIFO and the shift register are empty.

flushing has a 4ms timeout guard, which is normally more than
enough to consume the FIFO @ low baudrate (9600bps).

Signed-off-by: Loic Poulain 
---
 v2: Add this commit to the series

 drivers/serial/serial_mxc.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index f8c49865be..9899155c00 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* UART Control Register Bit Fields.*/
 #define URXD_CHARRDY   (1<<15)
@@ -144,8 +145,22 @@ struct mxc_uart {
u32 ts;
 };
 
+static void _mxc_serial_flush(struct mxc_uart *base)
+{
+   unsigned int timeout = 4000;
+
+   if (!(readl(&base->cr1) & UCR1_UARTEN) ||
+   !(readl(&base->cr2) & UCR2_TXEN))
+   return;
+
+   while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
+   udelay(1);
+}
+
 static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
 {
+   _mxc_serial_flush(base);
+
writel(0, &base->cr1);
writel(0, &base->cr2);
 
@@ -252,10 +267,17 @@ static int mxc_serial_init(void)
return 0;
 }
 
+static int mxc_serial_stop(void)
+{
+   _mxc_serial_flush(mxc_base);
+
+   return 0;
+}
+
 static struct serial_device mxc_serial_drv = {
.name   = "mxc_serial",
.start  = mxc_serial_init,
-   .stop   = NULL,
+   .stop   = mxc_serial_stop,
.setbrg = mxc_serial_setbrg,
.putc   = mxc_serial_putc,
.puts   = default_serial_puts,
-- 
2.34.1

[PATCH v2 1/2] serial: mxc: Speed-up character transmission

[Loic Poulain]

Instead of waiting for empty FIFO condition before writing a
character, wait for non-full FIFO condition.

This helps in saving several tens of milliseconds during boot
(depending verbosity).

Signed-off-by: Loic Poulain 
---
 v2: Add 2/2 fixing transfert abort & char corruption

 drivers/serial/serial_mxc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 82c0d84628..f8c49865be 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -223,11 +223,11 @@ static void mxc_serial_putc(const char c)
if (c == '\n')
serial_putc('\r');
 
-   writel(c, &mxc_base->txd);
-
/* wait for transmitter to be ready */
-   while (!(readl(&mxc_base->ts) & UTS_TXEMPTY))
+   while (readl(&mxc_base->ts) & UTS_TXFULL)
schedule();
+
+   writel(c, &mxc_base->txd);
 }
 
 /* Test whether a character is in the RX buffer */
@@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char 
ch)
struct mxc_serial_plat *plat = dev_get_plat(dev);
struct mxc_uart *const uart = plat->reg;
 
-   if (!(readl(&uart->ts) & UTS_TXEMPTY))
+   if (readl(&uart->ts) & UTS_TXFULL)
return -EAGAIN;
 
writel(ch, &uart->txd);
-- 
2.34.1

Re: [PATCH] serial: mxc: Speed-up character transmission

[Loic Poulain]

Hi Lothar,

On Tue, 10 Jan 2023 at 07:46, Lothar Waßmann  wrote:

> A previous attempt to do this in:
> |commit c7878a0483c59c48a730123bc0f4659fd40921bf
> |Author: Johannes Schneider 
> |Date:   Tue Sep 6 14:15:04 2022 +0200
> |
> |serial: mxc: have putc use the TXFIFO
>
> has been reverted in:
> |commit fc1c1760de38823edbdc2cdd9606dff938a07f6e
> |Author: Fabio Estevam 
> |Date:   Tue Nov 8 08:39:33 2022 -0300
> |
> |Revert "serial: mxc: have putc use the TXFIFO"
> |
> |This reverts commit c7878a0483c59c48a730123bc0f4659fd40921bf.
> |
> |Since commit c7878a0483c5 ("serial: mxc: have putc use the TXFIFO"),
> |serial console corruption can be seen when priting inside board_init().

Thanks for highlighting this. Looked at it and reproduced some sort of
corruption in board_init() as well, which seems due to
reinitialization of the UART while TX is not complete. I'm going to
follow up with a V2, including a fix for this.

Regards,
Loic

[PATCH] serial: mxc: Speed-up character transmission

[Loic Poulain]

Instead of waiting for empty FIFO condition before writing a
character, wait for non-full FIFO condition.

This helps in saving several tens of milliseconds during boot
(depending verbosity).

Signed-off-by: Loic Poulain 
---
 drivers/serial/serial_mxc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 82c0d84628..f8c49865be 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -223,11 +223,11 @@ static void mxc_serial_putc(const char c)
if (c == '\n')
serial_putc('\r');
 
-   writel(c, &mxc_base->txd);
-
/* wait for transmitter to be ready */
-   while (!(readl(&mxc_base->ts) & UTS_TXEMPTY))
+   while (readl(&mxc_base->ts) & UTS_TXFULL)
schedule();
+
+   writel(c, &mxc_base->txd);
 }
 
 /* Test whether a character is in the RX buffer */
@@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char 
ch)
struct mxc_serial_plat *plat = dev_get_plat(dev);
struct mxc_uart *const uart = plat->reg;
 
-   if (!(readl(&uart->ts) & UTS_TXEMPTY))
+   if (readl(&uart->ts) & UTS_TXFULL)
return -EAGAIN;
 
writel(ch, &uart->txd);
-- 
2.34.1

[PATCH v2] configs: imx8m: Enable CONFIG_ARMV8_CRYPTO support

[Loic Poulain]

This enables armv8 crypto extension usage for SHA1/SHA256.

Which speed up sha1/sha256 operations, about 10x faster with
a imx8mm evk for a 20MiB kernel hash verification (12ms vs 165ms).

Signed-off-by: Loic Poulain 
---
 v2: Select ARMV8_CRYPTO in the imx8m common Kconfig

 arch/arm/mach-imx/imx8m/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/imx8m/Kconfig b/arch/arm/mach-imx/imx8m/Kconfig
index 3470160..fe7ef0b 100644
--- a/arch/arm/mach-imx/imx8m/Kconfig
+++ b/arch/arm/mach-imx/imx8m/Kconfig
@@ -4,6 +4,7 @@ config IMX8M
bool
select HAS_CAAM
select ROM_UNIFIED_SECTIONS
+   select ARMV8_CRYPTO
 
 config IMX8MQ
bool
-- 
2.7.4

[PATCH] configs: imx8mm*: Enable CONFIG_ARMV8_CRYPTO support

[Loic Poulain]

Which speed up sha1/sha256 operations, about 10x faster with
a imx8mm evk for a 20MiB kernel hash verification (12ms vs 165ms).

Signed-off-by: Loic Poulain 
---
 configs/imx8mm-cl-iot-gate-optee_defconfig| 1 +
 configs/imx8mm-cl-iot-gate_defconfig  | 1 +
 configs/imx8mm-icore-mx8mm-ctouch2_defconfig  | 1 +
 configs/imx8mm-icore-mx8mm-edimm2.2_defconfig | 1 +
 configs/imx8mm-mx8menlo_defconfig | 1 +
 configs/imx8mm_beacon_defconfig   | 1 +
 configs/imx8mm_data_modul_edm_sbc_defconfig   | 1 +
 configs/imx8mm_evk_defconfig  | 1 +
 configs/imx8mm_evk_fspi_defconfig | 1 +
 configs/imx8mm_venice_defconfig   | 1 +
 10 files changed, 10 insertions(+)

diff --git a/configs/imx8mm-cl-iot-gate-optee_defconfig 
b/configs/imx8mm-cl-iot-gate-optee_defconfig
index 9ab2173..b222756 100644
--- a/configs/imx8mm-cl-iot-gate-optee_defconfig
+++ b/configs/imx8mm-cl-iot-gate-optee_defconfig
@@ -15,6 +15,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_LOAD_ADDR=0x4048
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
diff --git a/configs/imx8mm-cl-iot-gate_defconfig 
b/configs/imx8mm-cl-iot-gate_defconfig
index 73d9388..6186cad 100644
--- a/configs/imx8mm-cl-iot-gate_defconfig
+++ b/configs/imx8mm-cl-iot-gate_defconfig
@@ -16,6 +16,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_ENV_OFFSET_REDUND=0x204000
 CONFIG_SYS_LOAD_ADDR=0x4048
 CONFIG_DISTRO_DEFAULTS=y
diff --git a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig 
b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
index d3e0b05..6dfe027 100644
--- a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
+++ b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
@@ -16,6 +16,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_LOAD_ADDR=0x4048
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
diff --git a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig 
b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
index 29addb5..b47773f 100644
--- a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
+++ b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
@@ -16,6 +16,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_LOAD_ADDR=0x4048
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
diff --git a/configs/imx8mm-mx8menlo_defconfig 
b/configs/imx8mm-mx8menlo_defconfig
index 702162f..e3b94dc 100644
--- a/configs/imx8mm-mx8menlo_defconfig
+++ b/configs/imx8mm-mx8menlo_defconfig
@@ -18,6 +18,7 @@ CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_BOOTCOUNT_BOOTLIMIT=3
 CONFIG_SYS_BOOTCOUNT_ADDR=0x30370090
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_BOOTCOUNT_SINGLEWORD=y
 CONFIG_ENV_OFFSET_REDUND=0xDE00
 CONFIG_SYS_LOAD_ADDR=0x4048
diff --git a/configs/imx8mm_beacon_defconfig b/configs/imx8mm_beacon_defconfig
index e37ce01..467e7d2 100644
--- a/configs/imx8mm_beacon_defconfig
+++ b/configs/imx8mm_beacon_defconfig
@@ -16,6 +16,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_LOAD_ADDR=0x4048
 CONFIG_LTO=y
 CONFIG_FIT=y
diff --git a/configs/imx8mm_data_modul_edm_sbc_defconfig 
b/configs/imx8mm_data_modul_edm_sbc_defconfig
index b246fbd..deca8f6 100644
--- a/configs/imx8mm_data_modul_edm_sbc_defconfig
+++ b/configs/imx8mm_data_modul_edm_sbc_defconfig
@@ -20,6 +20,7 @@ CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_BOOTCOUNT_BOOTLIMIT=3
 CONFIG_SYS_BOOTCOUNT_ADDR=0x30370090
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_BOOTCOUNT_SINGLEWORD=y
 CONFIG_ENV_OFFSET_REDUND=0xFFFC
 CONFIG_IMX_BOOTAUX=y
diff --git a/configs/imx8mm_evk_defconfig b/configs/imx8mm_evk_defconfig
index 7bcedcd..1f30467 100644
--- a/configs/imx8mm_evk_defconfig
+++ b/configs/imx8mm_evk_defconfig
@@ -16,6 +16,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_LOAD_ADDR=0x4048
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
diff --git a/configs/imx8mm_evk_fspi_defconfig 
b/configs/imx8mm_evk_fspi_defconfig
index 21fd7e0..3331c1a 100644
--- a/configs/imx8mm_evk_fspi_defconfig
+++ b/configs/imx8mm_evk_fspi_defconfig
@@ -17,6 +17,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_SYS_LOAD_ADDR=0x4048
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
diff --git a/configs/imx8mm_venice_defconfig b/configs/imx8mm_venice_defconfig
index 2a44bf6..7bf805f 100644
--- a/configs/imx8mm_venice_defconfig
+++ b/configs/imx8mm_venice_defconfig
@@ -16,6 +16,7 @@ CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
+CONFIG_ARMV8_CRYPTO=y
 CONFIG_ENV_OFFSET_REDUND=0xff8000
 CONFIG_SYS_LOAD_ADDR=0x4820
 CONFIG_SYS_MEMTEST_START=0x4000
-- 
2.7.4

Re: [PATCH v2 0/5] Add ARMv8 CE sha1/sha256 support

[Loic Poulain]

Hi Folks,

Any comments on this series? Anyone else to CC?

Thanks,
Loic

On Wed, 1 Jun 2022 at 20:26, Loic Poulain  wrote:
>
> This series adds support for the SHA-1 and SHA-256 Secure Hash Algorithm
> for CPUs that have support of the ARM v8 Crypto Extensions. It Improves
> speed of integrity & signature checking procedures.
>
> V2:
>- Add cover letter & sha256 support.
>- Kconfig default 'y' only if SHA1 and SHA256 selected
>
> Loic Poulain (5):
>   lib: sha1: Add support for hardware specific sha1_process
>   sha1: Fix digest state size/type
>   armv8 SHA-1 using ARMv8 Crypto Extensions:
>   lib: sha256: Add support for hardware specific sha256_process
>   armv8 SHA-256 using ARMv8 Crypto Extensions
>
>  arch/arm/cpu/armv8/Kconfig  |  15 
>  arch/arm/cpu/armv8/Makefile |   2 +
>  arch/arm/cpu/armv8/sha1_ce_core.S   | 132 +++
>  arch/arm/cpu/armv8/sha1_ce_glue.c   |  21 ++
>  arch/arm/cpu/armv8/sha256_ce_core.S | 134 
> 
>  arch/arm/cpu/armv8/sha256_ce_glue.c |  21 ++
>  include/u-boot/sha1.h   |   2 +-
>  lib/sha1.c  |  26 +--
>  lib/sha256.c|  26 +--
>  9 files changed, 364 insertions(+), 15 deletions(-)
>  create mode 100644 arch/arm/cpu/armv8/sha1_ce_core.S
>  create mode 100644 arch/arm/cpu/armv8/sha1_ce_glue.c
>  create mode 100644 arch/arm/cpu/armv8/sha256_ce_core.S
>  create mode 100644 arch/arm/cpu/armv8/sha256_ce_glue.c
>
> --
> 2.7.4
>

[PATCH v2 5/5] armv8 SHA-256 using ARMv8 Crypto Extensions

[Loic Poulain]

This patch adds support for the SHA-256 Secure Hash Algorithm for CPUs
that have support for the SHA-256 part of the ARM v8 Crypto Extensions.

It greatly improves sha-256 based operations, about 17x faster on iMX8M
evk board. ~12ms vs ~208ms for a 20MiB kernel sha-256 verification.

asm implementation is a simplified version of the Linux version (from
Ard Biesheuvel).

Signed-off-by: Loic Poulain 
---
 arch/arm/cpu/armv8/Kconfig  |   4 ++
 arch/arm/cpu/armv8/Makefile |   1 +
 arch/arm/cpu/armv8/sha256_ce_core.S | 134 
 arch/arm/cpu/armv8/sha256_ce_glue.c |  21 ++
 4 files changed, 160 insertions(+)
 create mode 100644 arch/arm/cpu/armv8/sha256_ce_core.S
 create mode 100644 arch/arm/cpu/armv8/sha256_ce_glue.c

diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
index 0b11ca8..0494a08 100644
--- a/arch/arm/cpu/armv8/Kconfig
+++ b/arch/arm/cpu/armv8/Kconfig
@@ -180,6 +180,10 @@ config ARMV8_CE_SHA1
bool "SHA-1 digest algorithm (ARMv8 Crypto Extensions)"
default y if SHA1
 
+config ARMV8_CE_SHA256
+   bool "SHA-256 digest algorithm (ARMv8 Crypto Extensions)"
+   default y if SHA256
+
 endif
 
 endif
diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
index ff2495c..2e4bf9e 100644
--- a/arch/arm/cpu/armv8/Makefile
+++ b/arch/arm/cpu/armv8/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_ARMV8_PSCI) += psci.o
 obj-$(CONFIG_TARGET_BCMNS3) += bcmns3/
 obj-$(CONFIG_XEN) += xen/
 obj-$(CONFIG_ARMV8_CE_SHA1) += sha1_ce_glue.o sha1_ce_core.o
+obj-$(CONFIG_ARMV8_CE_SHA256) += sha256_ce_glue.o sha256_ce_core.o
diff --git a/arch/arm/cpu/armv8/sha256_ce_core.S 
b/arch/arm/cpu/armv8/sha256_ce_core.S
new file mode 100644
index 000..fbae3ca
--- /dev/null
+++ b/arch/arm/cpu/armv8/sha256_ce_core.S
@@ -0,0 +1,134 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * sha256-ce-core.S - core SHA-256 transform using v8 Crypto Extensions
+ *
+ * Copyright (C) 2014 Linaro Ltd 
+ * Copyright (C) 2022 Linaro Ltd 
+ */
+
+ #include 
+ #include 
+ #include 
+ #include 
+
+   .text
+   .arch   armv8-a+crypto
+
+   dga .reqq20
+   dgav.reqv20
+   dgb .reqq21
+   dgbv.reqv21
+
+   t0  .reqv22
+   t1  .reqv23
+
+   dg0q.reqq24
+   dg0v.reqv24
+   dg1q.reqq25
+   dg1v.reqv25
+   dg2q.reqq26
+   dg2v.reqv26
+
+   .macro  add_only, ev, rc, s0
+   mov dg2v.16b, dg0v.16b
+   .ifeq   \ev
+   add t1.4s, v\s0\().4s, \rc\().4s
+   sha256h dg0q, dg1q, t0.4s
+   sha256h2dg1q, dg2q, t0.4s
+   .else
+   .ifnb   \s0
+   add t0.4s, v\s0\().4s, \rc\().4s
+   .endif
+   sha256h dg0q, dg1q, t1.4s
+   sha256h2dg1q, dg2q, t1.4s
+   .endif
+   .endm
+
+   .macro  add_update, ev, rc, s0, s1, s2, s3
+   sha256su0   v\s0\().4s, v\s1\().4s
+   add_only\ev, \rc, \s1
+   sha256su1   v\s0\().4s, v\s2\().4s, v\s3\().4s
+   .endm
+
+   /*
+* The SHA-256 round constants
+*/
+   .align  4
+.Lsha2_rcon:
+   .word   0x428a2f98, 0x71374491, 0xb5c0fbcf, 0xe9b5dba5
+   .word   0x3956c25b, 0x59f111f1, 0x923f82a4, 0xab1c5ed5
+   .word   0xd807aa98, 0x12835b01, 0x243185be, 0x550c7dc3
+   .word   0x72be5d74, 0x80deb1fe, 0x9bdc06a7, 0xc19bf174
+   .word   0xe49b69c1, 0xefbe4786, 0x0fc19dc6, 0x240ca1cc
+   .word   0x2de92c6f, 0x4a7484aa, 0x5cb0a9dc, 0x76f988da
+   .word   0x983e5152, 0xa831c66d, 0xb00327c8, 0xbf597fc7
+   .word   0xc6e00bf3, 0xd5a79147, 0x06ca6351, 0x14292967
+   .word   0x27b70a85, 0x2e1b2138, 0x4d2c6dfc, 0x53380d13
+   .word   0x650a7354, 0x766a0abb, 0x81c2c92e, 0x92722c85
+   .word   0xa2bfe8a1, 0xa81a664b, 0xc24b8b70, 0xc76c51a3
+   .word   0xd192e819, 0xd6990624, 0xf40e3585, 0x106aa070
+   .word   0x19a4c116, 0x1e376c08, 0x2748774c, 0x34b0bcb5
+   .word   0x391c0cb3, 0x4ed8aa4a, 0x5b9cca4f, 0x682e6ff3
+   .word   0x748f82ee, 0x78a5636f, 0x84c87814, 0x8cc70208
+   .word   0x90befffa, 0xa4506ceb, 0xbef9a3f7, 0xc67178f2
+
+   /*
+* void sha256_armv8_ce_process(struct sha256_ce_state *sst,
+*  uint8_t const *src, uint32_t blocks)
+*/
+ENTRY(sha256_armv8_ce_process)
+   /* load round constants */
+   adr x8, .Lsha2_rcon
+   ld1 { v0.4s- v3.4s}, [x8], #64
+   ld1 { v4.4s- v7.4s}, [x8], #64
+   ld1 { v8.4s-v11.4s}, [x8], #64
+   ld1 

[PATCH v2 2/5] sha1: Fix digest state size/type

[Loic Poulain]

sha1 digest size is 5*32-bit => 160-bit. Using 64-bit unsigned long
does not cause issue with the current sha1 implementation, but could
be problematic for vectorized access.

Signed-off-by: Loic Poulain 
---
 include/u-boot/sha1.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/u-boot/sha1.h b/include/u-boot/sha1.h
index 283f103..09fee59 100644
--- a/include/u-boot/sha1.h
+++ b/include/u-boot/sha1.h
@@ -30,7 +30,7 @@ extern const uint8_t sha1_der_prefix[];
 typedef struct
 {
 unsigned long total[2];/*!< number of bytes processed  */
-unsigned long state[5];/*!< intermediate digest state  */
+uint32_t state[5]; /*!< intermediate digest state  */
 unsigned char buffer[64];  /*!< data block being processed */
 }
 sha1_context;
-- 
2.7.4

[PATCH v2 1/5] lib: sha1: Add support for hardware specific sha1_process

[Loic Poulain]

Mark sha1_process as weak to allow hardware specific implementation.
Add parameter to support for multiple blocks processing.

Signed-off-by: Loic Poulain 
---
 lib/sha1.c | 26 +++---
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/sha1.c b/lib/sha1.c
index 8154e1e..e5e42bc 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -25,6 +25,8 @@
 #include 
 #include 
 
+#include 
+
 const uint8_t sha1_der_prefix[SHA1_DER_LEN] = {
0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2b, 0x0e,
0x03, 0x02, 0x1a, 0x05, 0x00, 0x04, 0x14
@@ -65,7 +67,7 @@ void sha1_starts (sha1_context * ctx)
ctx->state[4] = 0xC3D2E1F0;
 }
 
-static void sha1_process(sha1_context *ctx, const unsigned char data[64])
+static void __maybe_unused sha1_process_one(sha1_context *ctx, const unsigned 
char data[64])
 {
unsigned long temp, W[16], A, B, C, D, E;
 
@@ -219,6 +221,18 @@ static void sha1_process(sha1_context *ctx, const unsigned 
char data[64])
ctx->state[4] += E;
 }
 
+__weak void sha1_process(sha1_context *ctx, const unsigned char *data,
+unsigned int blocks)
+{
+   if (!blocks)
+   return;
+
+   while (blocks--) {
+   sha1_process_one(ctx, data);
+   data += 64;
+   }
+}
+
 /*
  * SHA-1 process buffer
  */
@@ -242,17 +256,15 @@ void sha1_update(sha1_context *ctx, const unsigned char 
*input,
 
if (left && ilen >= fill) {
memcpy ((void *) (ctx->buffer + left), (void *) input, fill);
-   sha1_process (ctx, ctx->buffer);
+   sha1_process(ctx, ctx->buffer, 1);
input += fill;
ilen -= fill;
left = 0;
}
 
-   while (ilen >= 64) {
-   sha1_process (ctx, input);
-   input += 64;
-   ilen -= 64;
-   }
+   sha1_process(ctx, input, ilen / 64);
+   input += ilen / 64 * 64;
+   ilen = ilen % 64;
 
if (ilen > 0) {
memcpy ((void *) (ctx->buffer + left), (void *) input, ilen);
-- 
2.7.4

[PATCH v2 4/5] lib: sha256: Add support for hardware specific sha256_process

[Loic Poulain]

Mark sha256_process as weak to allow hardware specific implementation.
Add parameter for supporting multiple blocks processing.

Signed-off-by: Loic Poulain 
---
 lib/sha256.c | 26 +++---
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/sha256.c b/lib/sha256.c
index c1fe93d..50b0b51 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -14,6 +14,8 @@
 #include 
 #include 
 
+#include 
+
 const uint8_t sha256_der_prefix[SHA256_DER_LEN] = {
0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86,
0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05,
@@ -55,7 +57,7 @@ void sha256_starts(sha256_context * ctx)
ctx->state[7] = 0x5BE0CD19;
 }
 
-static void sha256_process(sha256_context *ctx, const uint8_t data[64])
+static void sha256_process_one(sha256_context *ctx, const uint8_t data[64])
 {
uint32_t temp1, temp2;
uint32_t W[64];
@@ -186,6 +188,18 @@ static void sha256_process(sha256_context *ctx, const 
uint8_t data[64])
ctx->state[7] += H;
 }
 
+__weak void sha256_process(sha256_context *ctx, const unsigned char *data,
+  unsigned int blocks)
+{
+   if (!blocks)
+   return;
+
+   while (blocks--) {
+   sha256_process_one(ctx, data);
+   data += 64;
+   }
+}
+
 void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length)
 {
uint32_t left, fill;
@@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t 
*input, uint32_t length)
 
if (left && length >= fill) {
memcpy((void *) (ctx->buffer + left), (void *) input, fill);
-   sha256_process(ctx, ctx->buffer);
+   sha256_process(ctx, ctx->buffer, 1);
length -= fill;
input += fill;
left = 0;
}
 
-   while (length >= 64) {
-   sha256_process(ctx, input);
-   length -= 64;
-   input += 64;
-   }
+   sha256_process(ctx, input, length / 64);
+   input += length / 64 * 64;
+   length = length % 64;
 
if (length)
memcpy((void *) (ctx->buffer + left), (void *) input, length);
-- 
2.7.4

[PATCH v2 3/5] armv8 SHA-1 using ARMv8 Crypto Extensions:

[Loic Poulain]

This patch adds support for the SHA-1 Secure Hash Algorithm for CPUs
that have support for the SHA-1 part of the ARM v8 Crypto Extensions.

It greatly improves sha-1 based operations, about 10x faster on iMX8M
evk board. ~12ms vs ~165ms for a 20MiB kernel sha-1 verification.

asm implementation is a simplified version of the Linux version (from
Ard Biesheuvel).

Signed-off-by: Loic Poulain 
---
 arch/arm/cpu/armv8/Kconfig|  11 
 arch/arm/cpu/armv8/Makefile   |   1 +
 arch/arm/cpu/armv8/sha1_ce_core.S | 132 ++
 arch/arm/cpu/armv8/sha1_ce_glue.c |  21 ++
 4 files changed, 165 insertions(+)
 create mode 100644 arch/arm/cpu/armv8/sha1_ce_core.S
 create mode 100644 arch/arm/cpu/armv8/sha1_ce_glue.c

diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
index 9967376..0b11ca8 100644
--- a/arch/arm/cpu/armv8/Kconfig
+++ b/arch/arm/cpu/armv8/Kconfig
@@ -171,4 +171,15 @@ config ARMV8_SECURE_BASE
 
 endif
 
+menuconfig ARMV8_CRYPTO
+   bool "ARM64 Accelerated Cryptographic Algorithms"
+
+if ARMV8_CRYPTO
+
+config ARMV8_CE_SHA1
+   bool "SHA-1 digest algorithm (ARMv8 Crypto Extensions)"
+   default y if SHA1
+
+endif
+
 endif
diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
index 85fe047..ff2495c 100644
--- a/arch/arm/cpu/armv8/Makefile
+++ b/arch/arm/cpu/armv8/Makefile
@@ -44,3 +44,4 @@ obj-$(CONFIG_TARGET_HIKEY) += hisilicon/
 obj-$(CONFIG_ARMV8_PSCI) += psci.o
 obj-$(CONFIG_TARGET_BCMNS3) += bcmns3/
 obj-$(CONFIG_XEN) += xen/
+obj-$(CONFIG_ARMV8_CE_SHA1) += sha1_ce_glue.o sha1_ce_core.o
diff --git a/arch/arm/cpu/armv8/sha1_ce_core.S 
b/arch/arm/cpu/armv8/sha1_ce_core.S
new file mode 100644
index 000..fbf2714
--- /dev/null
+++ b/arch/arm/cpu/armv8/sha1_ce_core.S
@@ -0,0 +1,132 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * sha1_ce_core.S - SHA-1 secure hash using ARMv8 Crypto Extensions
+ *
+ * Copyright (C) 2014 Linaro Ltd 
+ * Copyright (C) 2022 Linaro Ltd 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+   .text
+   .arch   armv8-a+crypto
+
+   k0  .reqv0
+   k1  .reqv1
+   k2  .reqv2
+   k3  .reqv3
+
+   t0  .reqv4
+   t1  .reqv5
+
+   dga .reqq6
+   dgav.reqv6
+   dgb .reqs7
+   dgbv.reqv7
+
+   dg0q.reqq12
+   dg0s.reqs12
+   dg0v.reqv12
+   dg1s.reqs13
+   dg1v.reqv13
+   dg2s.reqs14
+
+   .macro  add_only, op, ev, rc, s0, dg1
+   .ifc\ev, ev
+   add t1.4s, v\s0\().4s, \rc\().4s
+   sha1h   dg2s, dg0s
+   .ifnb   \dg1
+   sha1\op dg0q, \dg1, t0.4s
+   .else
+   sha1\op dg0q, dg1s, t0.4s
+   .endif
+   .else
+   .ifnb   \s0
+   add t0.4s, v\s0\().4s, \rc\().4s
+   .endif
+   sha1h   dg1s, dg0s
+   sha1\op dg0q, dg2s, t1.4s
+   .endif
+   .endm
+
+   .macro  add_update, op, ev, rc, s0, s1, s2, s3, dg1
+   sha1su0 v\s0\().4s, v\s1\().4s, v\s2\().4s
+   add_only\op, \ev, \rc, \s1, \dg1
+   sha1su1 v\s0\().4s, v\s3\().4s
+   .endm
+
+   .macro  loadrc, k, val, tmp
+   movz\tmp, :abs_g0_nc:\val
+   movk\tmp, :abs_g1:\val
+   dup \k, \tmp
+   .endm
+
+   /*
+* void sha1_armv8_ce_process(uint32_t state[5], uint8_t const *src,
+*uint32_t blocks)
+*/
+ENTRY(sha1_armv8_ce_process)
+   /* load round constants */
+   loadrc  k0.4s, 0x5a827999, w6
+   loadrc  k1.4s, 0x6ed9eba1, w6
+   loadrc  k2.4s, 0x8f1bbcdc, w6
+   loadrc  k3.4s, 0xca62c1d6, w6
+
+   /* load state (4+1 digest states) */
+   ld1 {dgav.4s}, [x0]
+   ldr dgb, [x0, #16]
+
+   /* load input (64 bytes into v8->v11 16B vectors) */
+0: ld1 {v8.4s-v11.4s}, [x1], #64
+   sub w2, w2, #1
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+   rev32   v8.16b, v8.16b
+   rev32   v9.16b, v9.16b
+   rev32   v10.16b, v10.16b
+   rev32   v11.16b, v11.16b
+#endif
+
+1: add t0.4s, v8.4s, k0.4s
+   mov dg0v.16b, dgav.16b
+
+   add_update  c, ev, k0,  8,  9, 10, 11, dgb
+   add_update  c, od, k0,  9, 10, 11,  8
+   add_update  c, ev, k0, 10, 11,  8,  9
+   add_update  c, od, k0, 11,  8,  9, 10
+   add_update  c, ev, k1,  8,  9, 10, 11
+
+   add_update  p, od, k1,  9, 10, 11,  8
+   add_update  p, ev, k1, 10, 11,  8,  9
+ 

[PATCH v2 0/5] Add ARMv8 CE sha1/sha256 support

[Loic Poulain]

This series adds support for the SHA-1 and SHA-256 Secure Hash Algorithm
for CPUs that have support of the ARM v8 Crypto Extensions. It Improves
speed of integrity & signature checking procedures.

V2: 
   - Add cover letter & sha256 support.
   - Kconfig default 'y' only if SHA1 and SHA256 selected

Loic Poulain (5):
  lib: sha1: Add support for hardware specific sha1_process
  sha1: Fix digest state size/type
  armv8 SHA-1 using ARMv8 Crypto Extensions:
  lib: sha256: Add support for hardware specific sha256_process
  armv8 SHA-256 using ARMv8 Crypto Extensions

 arch/arm/cpu/armv8/Kconfig  |  15 
 arch/arm/cpu/armv8/Makefile |   2 +
 arch/arm/cpu/armv8/sha1_ce_core.S   | 132 +++
 arch/arm/cpu/armv8/sha1_ce_glue.c   |  21 ++
 arch/arm/cpu/armv8/sha256_ce_core.S | 134 
 arch/arm/cpu/armv8/sha256_ce_glue.c |  21 ++
 include/u-boot/sha1.h   |   2 +-
 lib/sha1.c  |  26 +--
 lib/sha256.c|  26 +--
 9 files changed, 364 insertions(+), 15 deletions(-)
 create mode 100644 arch/arm/cpu/armv8/sha1_ce_core.S
 create mode 100644 arch/arm/cpu/armv8/sha1_ce_glue.c
 create mode 100644 arch/arm/cpu/armv8/sha256_ce_core.S
 create mode 100644 arch/arm/cpu/armv8/sha256_ce_glue.c

-- 
2.7.4

[PATCH 3/3] armv8 SHA-1 using ARMv8 Crypto Extensions:

[Loic Poulain]

This patch adds support for the SHA-1 Secure Hash Algorithm for CPUs
that have support for the SHA-1 part of the ARM v8 Crypto Extensions.

It greatly improves sha-1 based operations, about 10x faster on iMX8M
evk board. ~12ms vs ~165ms for a 20MiB kernel sha-1 verification.

asm implementation is a simplified version of the Linux version (from
Ard Biesheuvel).

Signed-off-by: Loic Poulain 
---
 arch/arm/cpu/armv8/Kconfig|  10 +++
 arch/arm/cpu/armv8/Makefile   |   1 +
 arch/arm/cpu/armv8/sha1_ce_core.S | 132 ++
 arch/arm/cpu/armv8/sha1_ce_glue.c |  21 ++
 4 files changed, 164 insertions(+)
 create mode 100644 arch/arm/cpu/armv8/sha1_ce_core.S
 create mode 100644 arch/arm/cpu/armv8/sha1_ce_glue.c

diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
index 9967376..675cbb8 100644
--- a/arch/arm/cpu/armv8/Kconfig
+++ b/arch/arm/cpu/armv8/Kconfig
@@ -171,4 +171,14 @@ config ARMV8_SECURE_BASE
 
 endif
 
+menuconfig ARMV8_CRYPTO
+   bool "ARM64 Accelerated Cryptographic Algorithms"
+
+if ARMV8_CRYPTO
+
+config ARMV8_CE_SHA1
+   bool "SHA-1 digest algorithm (ARMv8 Crypto Extensions)"
+default y
+endif
+
 endif
diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
index 85fe047..ff2495c 100644
--- a/arch/arm/cpu/armv8/Makefile
+++ b/arch/arm/cpu/armv8/Makefile
@@ -44,3 +44,4 @@ obj-$(CONFIG_TARGET_HIKEY) += hisilicon/
 obj-$(CONFIG_ARMV8_PSCI) += psci.o
 obj-$(CONFIG_TARGET_BCMNS3) += bcmns3/
 obj-$(CONFIG_XEN) += xen/
+obj-$(CONFIG_ARMV8_CE_SHA1) += sha1_ce_glue.o sha1_ce_core.o
diff --git a/arch/arm/cpu/armv8/sha1_ce_core.S 
b/arch/arm/cpu/armv8/sha1_ce_core.S
new file mode 100644
index 000..f826da3
--- /dev/null
+++ b/arch/arm/cpu/armv8/sha1_ce_core.S
@@ -0,0 +1,132 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * sha1_ce_core.S - SHA-1 secure hash using ARMv8 Crypto Extensions
+ *
+ * Copyright (C) 2014 Linaro Ltd 
+ * Copyright (C) 2022 Linaro Ltd 
+ */
+
+ #include 
+ #include 
+ #include 
+ #include 
+
+   .text
+   .arch   armv8-a+crypto
+
+   k0  .reqv0
+   k1  .reqv1
+   k2  .reqv2
+   k3  .reqv3
+
+   t0  .reqv4
+   t1  .reqv5
+
+   dga .reqq6
+   dgav.reqv6
+   dgb .reqs7
+   dgbv.reqv7
+
+   dg0q.reqq12
+   dg0s.reqs12
+   dg0v.reqv12
+   dg1s.reqs13
+   dg1v.reqv13
+   dg2s.reqs14
+
+   .macro  add_only, op, ev, rc, s0, dg1
+   .ifc\ev, ev
+   add t1.4s, v\s0\().4s, \rc\().4s
+   sha1h   dg2s, dg0s
+   .ifnb   \dg1
+   sha1\op dg0q, \dg1, t0.4s
+   .else
+   sha1\op dg0q, dg1s, t0.4s
+   .endif
+   .else
+   .ifnb   \s0
+   add t0.4s, v\s0\().4s, \rc\().4s
+   .endif
+   sha1h   dg1s, dg0s
+   sha1\op dg0q, dg2s, t1.4s
+   .endif
+   .endm
+
+   .macro  add_update, op, ev, rc, s0, s1, s2, s3, dg1
+   sha1su0 v\s0\().4s, v\s1\().4s, v\s2\().4s
+   add_only\op, \ev, \rc, \s1, \dg1
+   sha1su1 v\s0\().4s, v\s3\().4s
+   .endm
+
+   .macro  loadrc, k, val, tmp
+   movz\tmp, :abs_g0_nc:\val
+   movk\tmp, :abs_g1:\val
+   dup \k, \tmp
+   .endm
+
+   /*
+* void sha1_armv8_ce_process(uint32_t state[5], uint8_t const *src,
+*uint32_t blocks)
+*/
+ENTRY(sha1_armv8_ce_process)
+   /* load round constants */
+   loadrc  k0.4s, 0x5a827999, w6
+   loadrc  k1.4s, 0x6ed9eba1, w6
+   loadrc  k2.4s, 0x8f1bbcdc, w6
+   loadrc  k3.4s, 0xca62c1d6, w6
+
+   /* load state (4+1 digest states) */
+   ld1 {dgav.4s}, [x0]
+   ldr dgb, [x0, #16]
+
+   /* load input (64 bytes into v8->v11 16B vectors) */
+0: ld1 {v8.4s-v11.4s}, [x1], #64
+   sub w2, w2, #1
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+   rev32   v8.16b, v8.16b
+   rev32   v9.16b, v9.16b
+   rev32   v10.16b, v10.16b
+   rev32   v11.16b, v11.16b
+#endif
+
+1: add t0.4s, v8.4s, k0.4s
+   mov dg0v.16b, dgav.16b
+
+   add_update  c, ev, k0,  8,  9, 10, 11, dgb
+   add_update  c, od, k0,  9, 10, 11,  8
+   add_update  c, ev, k0, 10, 11,  8,  9
+   add_update  c, od, k0, 11,  8,  9, 10
+   add_update  c, ev, k1,  8,  9, 10, 11
+
+   add_update  p, od, k1,  9, 10, 11,  8
+   add_update  p, ev, k1, 10, 11,  8,  9
+   ad

[PATCH 2/3] sha1: Fix digest state size/type

[Loic Poulain]

sha1 digest size is 5*32-bit => 160-bit. Using 64-bit unsigned long
does not cause issue with the current sha1 implementation, but could
be problematic for vectorized access.

Signed-off-by: Loic Poulain 
---
 include/u-boot/sha1.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/u-boot/sha1.h b/include/u-boot/sha1.h
index 283f103..09fee59 100644
--- a/include/u-boot/sha1.h
+++ b/include/u-boot/sha1.h
@@ -30,7 +30,7 @@ extern const uint8_t sha1_der_prefix[];
 typedef struct
 {
 unsigned long total[2];/*!< number of bytes processed  */
-unsigned long state[5];/*!< intermediate digest state  */
+uint32_t state[5]; /*!< intermediate digest state  */
 unsigned char buffer[64];  /*!< data block being processed */
 }
 sha1_context;
-- 
2.7.4

[PATCH 1/3] lib: sha1: Add support for hardware specific sha1_process

[Loic Poulain]

Mark sha1_process as weak to allow hardware specific implementation.
Add parameter to support for multiple blocks processing.

Signed-off-by: Loic Poulain 
---
 lib/sha1.c | 26 +++---
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/sha1.c b/lib/sha1.c
index 8154e1e..e5e42bc 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -25,6 +25,8 @@
 #include 
 #include 
 
+#include 
+
 const uint8_t sha1_der_prefix[SHA1_DER_LEN] = {
0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2b, 0x0e,
0x03, 0x02, 0x1a, 0x05, 0x00, 0x04, 0x14
@@ -65,7 +67,7 @@ void sha1_starts (sha1_context * ctx)
ctx->state[4] = 0xC3D2E1F0;
 }
 
-static void sha1_process(sha1_context *ctx, const unsigned char data[64])
+static void __maybe_unused sha1_process_one(sha1_context *ctx, const unsigned 
char data[64])
 {
unsigned long temp, W[16], A, B, C, D, E;
 
@@ -219,6 +221,18 @@ static void sha1_process(sha1_context *ctx, const unsigned 
char data[64])
ctx->state[4] += E;
 }
 
+__weak void sha1_process(sha1_context *ctx, const unsigned char *data,
+unsigned int blocks)
+{
+   if (!blocks)
+   return;
+
+   while (blocks--) {
+   sha1_process_one(ctx, data);
+   data += 64;
+   }
+}
+
 /*
  * SHA-1 process buffer
  */
@@ -242,17 +256,15 @@ void sha1_update(sha1_context *ctx, const unsigned char 
*input,
 
if (left && ilen >= fill) {
memcpy ((void *) (ctx->buffer + left), (void *) input, fill);
-   sha1_process (ctx, ctx->buffer);
+   sha1_process(ctx, ctx->buffer, 1);
input += fill;
ilen -= fill;
left = 0;
}
 
-   while (ilen >= 64) {
-   sha1_process (ctx, input);
-   input += 64;
-   ilen -= 64;
-   }
+   sha1_process(ctx, input, ilen / 64);
+   input += ilen / 64 * 64;
+   ilen = ilen % 64;
 
if (ilen > 0) {
memcpy ((void *) (ctx->buffer + left), (void *) input, ilen);
-- 
2.7.4

[PATCH 1/2] mmc: Add support for wait_dat0 callback

[Loic Poulain]

There is no wait_dat0 mmc ops, causing operations waiting for data
line state change (e.g mmc_switch_voltage) to fallback to a 250ms
active delay. mmc_ops still used when DM_MMC is not enabled, which
is often the case for SPL. The result can be unexpectly long SPL
boot time.

This change adds support for wait_dat0() mmc operation.

Signed-off-by: Loic Poulain 
---
 drivers/mmc/mmc.c | 3 +++
 include/mmc.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index f6ccd83..109f340 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -34,6 +34,9 @@ static int mmc_set_signal_voltage(struct mmc *mmc, uint 
signal_voltage);
 
 static int mmc_wait_dat0(struct mmc *mmc, int state, int timeout_us)
 {
+   if (mmc->cfg->ops->wait_dat0)
+   return mmc->cfg->ops->wait_dat0(mmc, state, timeout_us);
+
return -ENOSYS;
 }
 
diff --git a/include/mmc.h b/include/mmc.h
index 6bdcce8..b7e94e8 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -561,6 +561,7 @@ struct mmc_ops {
int (*getwp)(struct mmc *mmc);
int (*host_power_cycle)(struct mmc *mmc);
int (*get_b_max)(struct mmc *mmc, void *dst, lbaint_t blkcnt);
+   int (*wait_dat0)(struct mmc *mmc, int state, int timeout_us);
 };
 
 static inline int mmc_hs400_prepare_ddr(struct mmc *mmc)
-- 
2.7.4

[PATCH 2/2] mmc: fsl_esdhc_imx: Implement wait_dat0 mmc ops

[Loic Poulain]

Implement wait_dat0 mmc ops callbac, allowing to reduce SPL boot time.

Before (using grabserial):
[0.01 0.01] U-Boot SPL 2021.04-
[0.028257 0.028257] DDRINFO: start DRAM init
[0.028500 0.000243] DDRINFO: DRAM rate 3000MTS
[0.304627 0.276127] DDRINFO:ddrphy calibration done
[0.305647 0.001020] DDRINFO: ddrmix config done
[0.352584 0.046937] SEC0:  RNG instantiated
[0.374299 0.021715] Normal Boot
[0.374675 0.000376] Trying to boot from MMC2
[1.250580 0.875905] NOTICE:  BL31: v2.4(release):lf-5.10.72-2.2.0-0-g5782363f9
[1.251985 0.001405] NOTICE:  BL31: Built : 08:02:40, Apr 12 2022
[1.522560 0.270575]
[1.522734 0.000174]
[1.522788 0.54] U-Boot 2021.04-

After:
[0.01 0.01] U-Boot SPL 2021.04-
[0.001614 0.001614] DDRINFO: start DRAM init
[0.002377 0.000763] DDRINFO: DRAM rate 3000MTS
[0.278494 0.276117] DDRINFO:ddrphy calibration done
[0.279266 0.000772] DDRINFO: ddrmix config done
[0.338432 0.059166] SEC0:  RNG instantiated
[0.339051 0.000619] Normal Boot
[0.339431 0.000380] Trying to boot from MMC2
[0.412587 0.073156] NOTICE:  BL31: v2.4(release):lf-5.15.5-1.0.0-0-g05f788b
[0.414191 0.001604] NOTICE:  BL31: Built : 10:35:26, Apr  6 2022
[0.700685 0.286494]
[0.700793 0.000108]
[0.700845 0.52] U-Boot 2021.04-

Signed-off-by: Loic Poulain 
---
 drivers/mmc/fsl_esdhc_imx.c | 50 ++---
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index 02208a5..ec52f93 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1060,6 +1060,30 @@ static int esdhc_getcd_common(struct fsl_esdhc_priv 
*priv)
return timeout > 0;
 }
 
+static int esdhc_wait_dat0_common(struct fsl_esdhc_priv *priv, int state,
+ int timeout_us)
+{
+   struct fsl_esdhc *regs = priv->esdhc_regs;
+   int ret, err;
+   u32 tmp;
+
+   /* make sure the card clock keep on */
+   esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
+
+   ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp,
+   !!(tmp & PRSSTAT_DAT0) == !!state,
+   timeout_us);
+
+   /* change to default setting, let host control the card clock */
+   esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
+
+   err = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp & 
PRSSTAT_SDOFF, 100);
+   if (err)
+   pr_warn("card clock not gate off as expect.\n");
+
+   return ret;
+}
+
 static int esdhc_reset(struct fsl_esdhc *regs)
 {
ulong start;
@@ -1109,11 +1133,19 @@ static int esdhc_set_ios(struct mmc *mmc)
return esdhc_set_ios_common(priv, mmc);
 }
 
+static int esdhc_wait_dat0(struct mmc *mmc, int state, int timeout_us)
+{
+   struct fsl_esdhc_priv *priv = mmc->priv;
+
+   return esdhc_wait_dat0_common(priv, state, timeout_us);
+}
+
 static const struct mmc_ops esdhc_ops = {
.getcd  = esdhc_getcd,
.init   = esdhc_init,
.send_cmd   = esdhc_send_cmd,
.set_ios= esdhc_set_ios,
+   .wait_dat0  = esdhc_wait_dat0,
 };
 #endif
 
@@ -1576,25 +1608,9 @@ static int __maybe_unused 
fsl_esdhc_set_enhanced_strobe(struct udevice *dev)
 static int fsl_esdhc_wait_dat0(struct udevice *dev, int state,
int timeout_us)
 {
-   int ret, err;
-   u32 tmp;
struct fsl_esdhc_priv *priv = dev_get_priv(dev);
-   struct fsl_esdhc *regs = priv->esdhc_regs;
 
-   /* make sure the card clock keep on */
-   esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
-
-   ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp,
-   !!(tmp & PRSSTAT_DAT0) == !!state,
-   timeout_us);
-
-   /* change to default setting, let host control the card clock */
-   esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
-   err = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp & 
PRSSTAT_SDOFF, 100);
-   if (err)
-   dev_warn(dev, "card clock not gate off as expect.\n");
-
-   return ret;
+   return esdhc_wait_dat0_common(priv, state, timeout_us);
 }
 
 static const struct dm_mmc_ops fsl_esdhc_ops = {
-- 
2.7.4

Re: USB init before using usb_serial_acm gadget?

[Loic Poulain]

On Sun, 17 Apr 2022 at 10:45, Sergey Nazaryev  wrote:
>
> > So I think the right place to call usb_gadget_initialize is probably
> > before registering the acm gadget function into acm_stdio_start(). Can
> > you try this? and submit a follow_up fix patch if working?
>
> It's required for `usb_gadget_initialize` to provide a "USB
> controller index" as an argument. It's pretty obvious how to do it in
> "interactive world" (e.g. to ask user for this number via CLI, like
> it was in commands mentioned in my previous message) but in our case
> we need to provide this number in non-interactive manner.
>
> There is not an elegant solution with hardcoding '0' as a controller
> index like it was done in `ether` gadget [1], but it seems that it
> was a temporary fix ("This is a preparatory work for DM support for
> UDC drivers") that occasionaly became a permament solution.
>
> So, that's why I suggested to add a new subcommand to `usb` for OTG
> initialization: in my opinion, it's better to require an explicit
> call of something like `usb otgstart 0` than hardcoding a '0' into
> the code. One more option is to add a new environment variable to
> explictly specify a controller index, and read this variable from all
> places when it requires.
>
> P.S. I already put `usb_gadget_intiialize(0)` to my board code, and
> ACM gadget started working; so the remaining problem is only to
> decide where is the best place for this call.

Yes, but whatever the solution, we also need to know the controller
index from gadget driver, since we need it for
usb_gadget_handle_interrupts(index). So an env or global variable
would be at least a good start.

Regards,
Loic

>
> [1]: 
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/usb/gadget/ether.c#L2355

Re: USB init before using usb_serial_acm gadget?

[Loic Poulain]

Hi Sergey,


On Thu, 14 Apr 2022 at 20:31, Sergey Nazaryev  wrote:
>
> Hi!
>
> As I can see, recently [1] the implementation of USB ACM gadget has
> been merged into U-boot master. I tried to use it but the problem is
> that running `setenv stdout usbacm` on my board based on STM32MP157
> leads to errors below:
>
> STM32MP> setenv stdout usbacm
> couldn't find an available UDC
> g_dnl_register: failed!, error: -19
> ## Error inserting "stdout" variable, errno=22
>
> After some research I've found that USB OTG controller must be
> initialized somehow before we can actually start using any gadget.
> For instance, on my STM32MP board `dwc2_udc_otg_probe` should be
> called. My research shows that `usb_gadget_initalize` is responsible
> for it; so, for this reason, there are explicit calls of
> `usb_gadget_initialize` (e.g. usb_dnl_dfu [2], usb_dnl_sdp [3])
> before actual usage of any gadget.
>
> However, unlike all other gadgets, usb_serial_acm code and code that
> uses it don't call usb_gadget_initialize at all. Okay, I understand
> that usb_serial_acm shouldn't initialize USB controllers by itself,
> but it's still unclear who must be responsible for it.
>
> So, my main question is: what's the best place for
> `usb_gadget_initialize` call? Should I put it to board-specific code
> (board/vendor/xxx.c) or maybe it's better to put `usb_gadget_initialize`
> into new `usb` subcommand (`usb otgstart` or something like that) and
> call it before `setenv stdout usbacm`?

Yes, you're right. I did not catch this problem because the iMX
board/platform I've tested on automatically initializes the usb
controller in the right role (peripheral) based on devicetree
property... but it's specific to that host driver.

So I think the right place to call usb_gadget_initialize is probably
before registering the acm gadget function into acm_stdio_start(). Can
you try this? and submit a follow_up fix patch if working?

Regards,
Loic

[PATCH] imx8ulp: clock: Fix lcd clock algo

[Loic Poulain]

The div loop uses reassign and reuse parent_rate, which causes
the parent rate reference to be wrong after the first loop, the
resulting clock becomes incorrect for div != 1.

Fixes: 829e06bf4175 ("imx8ulp: clock: Add MIPI DSI clock and DCNano clock")
Signed-off-by: Loic Poulain 
---
 arch/arm/mach-imx/imx8ulp/clock.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/imx8ulp/clock.c 
b/arch/arm/mach-imx/imx8ulp/clock.c
index 91580b2..dbe0f78 100644
--- a/arch/arm/mach-imx/imx8ulp/clock.c
+++ b/arch/arm/mach-imx/imx8ulp/clock.c
@@ -381,10 +381,9 @@ void mxs_set_lcdclk(u32 base_addr, u32 freq_in_khz)
debug("PLL4 rate %ukhz\n", pll4_rate);
 
for (pfd = 12; pfd <= 35; pfd++) {
-   parent_rate = pll4_rate;
-   parent_rate = parent_rate * 18 / pfd;
-
for (div = 1; div <= 64; div++) {
+   parent_rate = pll4_rate;
+   parent_rate = parent_rate * 18 / pfd;
parent_rate = parent_rate / div;
 
for (pcd = 0; pcd < 8; pcd++) {
-- 
2.7.4

Re: [PATCH v3 2/2] usb: gadget: Add CDC ACM function

[Loic Poulain]

Hi Pali,

On Thu, 25 Nov 2021 at 19:10, Pali Rohár  wrote:
>
> On Thursday 25 November 2021 18:16:15 Loic Poulain wrote:
> > Add support for CDC ACM using the new UDC and gadget API. This protocol
> > can be used for serial over USB data transfer and is widely supported
> > by various OS (GNU/Linux, MS-Windows, OSX...). The usual purpose of
> > such link is to access device debug console and can be useful for
> > products not exposing regular UART to the user.
> >
> > A default stdio device named 'usbacm' is created, and can be used
> > to redirect console to USB link over CDC ACM:
> >
> > > setenv stdin usbacm; setenv stdout usbacm
> >
> > Signed-off-by: Loic Poulain 
> > ---
> >  v2: - remove possible infinite recursipe print loop
> >  - Remove acmconsole command, start function along the stdio device
> >  v3: - Use __constant_cpu_to_le16() when possible
> >  - Rename stdio dev to 'usbacm'
> >  - Rename init function to drv_usbacm_init
>
> Hello! Just one question: is in this v3 patch fixed stdout-only
> configuration, or it still has a bug which you described in email?
> https://lore.kernel.org/u-boot/CAMZdPi8185pAiO4w2QYMtWGAFJiX=ej_bjw1cusre990wr7...@mail.gmail.com/
>
> Because neither in above change long nor in driver help description nor
> in driver source code is any comment related to stdout-only device.

No, actually it ended not being an issue, we can have it stdout/stdin
independently. The driver loops on 'irq'-handling to ensure the USB
controller has completely flushed TX before returning.

>
> Note that stdout-only does not have to be too uncommon. For example
> Nokia N900 has enabled UART, keyboard+lcd and usbtty by default and
> sometimes I enabled input only from keyboard and looked at usbtty
> terminal (as stdout-only) if everything is working fine (that every
> pressed key on keyboard was echoed correctly back to usbtty).

Yes, checked this case on a imx7d based board and stdout-only works as
you describe.

Regards,
Loic

[PATCH v3 1/2] lib/circbuf: Make circbuf selectable symbol

[Loic Poulain]

It is currenly only used from usbtty driver but make it properly
selectable via Kconfig symbol, for future usage.

Signed-off-by: Loic Poulain 
---
 v2: no change
 v3: add symbol description

 lib/Kconfig  | 3 +++
 lib/Makefile | 8 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index c535147..4c6ac2b 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -290,6 +290,9 @@ config TRACE_EARLY_ADDR
  the size is too small then the message which says the amount of early
  data being coped will the the same as the
 
+config CIRCBUF
+   bool "Enable circular buffer support"
+
 source lib/dhry/Kconfig
 
 menu "Security support"
diff --git a/lib/Makefile b/lib/Makefile
index 8ba745f..4daeee2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -29,7 +29,13 @@ ifneq ($(CONFIG_CHARSET),)
 obj-y += charset.o
 endif
 endif
-obj-$(CONFIG_USB_TTY) += circbuf.o
+
+ifdef CONFIG_USB_TTY
+obj-y += circbuf.o
+else
+obj-$(CONFIG_CIRCBUF) += circbuf.o
+endif
+
 obj-y += crc8.o
 obj-y += crc16.o
 obj-$(CONFIG_ERRNO_STR) += errno_str.o
-- 
2.7.4

[PATCH v3 2/2] usb: gadget: Add CDC ACM function

[Loic Poulain]

Add support for CDC ACM using the new UDC and gadget API. This protocol
can be used for serial over USB data transfer and is widely supported
by various OS (GNU/Linux, MS-Windows, OSX...). The usual purpose of
such link is to access device debug console and can be useful for
products not exposing regular UART to the user.

A default stdio device named 'usbacm' is created, and can be used
to redirect console to USB link over CDC ACM:

> setenv stdin usbacm; setenv stdout usbacm

Signed-off-by: Loic Poulain 
---
 v2: - remove possible infinite recursipe print loop
 - Remove acmconsole command, start function along the stdio device
 v3: - Use __constant_cpu_to_le16() when possible
 - Rename stdio dev to 'usbacm'
 - Rename init function to drv_usbacm_init

 common/stdio.c  |   3 +
 drivers/usb/gadget/Kconfig  |   9 +
 drivers/usb/gadget/Makefile |   1 +
 drivers/usb/gadget/f_acm.c  | 672 
 include/stdio_dev.h |   1 +
 5 files changed, 686 insertions(+)
 create mode 100644 drivers/usb/gadget/f_acm.c

diff --git a/common/stdio.c b/common/stdio.c
index 4083e4e..e07e4a4 100644
--- a/common/stdio.c
+++ b/common/stdio.c
@@ -381,6 +381,9 @@ int stdio_add_devices(void)
 #ifdef CONFIG_USB_TTY
drv_usbtty_init();
 #endif
+#ifdef CONFIG_USB_FUNCTION_ACM
+   drv_usbacm_init ();
+#endif
if (IS_ENABLED(CONFIG_NETCONSOLE))
drv_nc_init();
 #ifdef CONFIG_JTAG_CONSOLE
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 327ea86..d81a9c5 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -182,6 +182,15 @@ config USB_FUNCTION_THOR
  Enable Tizen's THOR download protocol support in U-Boot. It
  allows downloading images into memory and flash them to target device.
 
+config USB_FUNCTION_ACM
+   bool "Enable CDC ACM gadget"
+   select SYS_STDIO_DEREGISTER
+   select CIRCBUF
+   help
+ ACM serial link. This function can be used to create a stdio device to
+ interoperate with MS-Windows hosts or with the Linux-USB "cdc-acm"
+ driver.
+
 endif # USB_GADGET_DOWNLOAD
 
 config USB_ETHER
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index f560068..d5d891b 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
 obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
 obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
 obj-$(CONFIG_USB_FUNCTION_ROCKUSB) += f_rockusb.o
+obj-$(CONFIG_USB_FUNCTION_ACM) += f_acm.o
 endif
 endif
 ifdef CONFIG_USB_ETHER
diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
new file mode 100644
index 000..388f73d
--- /dev/null
+++ b/drivers/usb/gadget/f_acm.c
@@ -0,0 +1,672 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * USB CDC serial (ACM) function driver
+ *
+ * Copyright (C) 2003 Al Borchers (alborch...@steinerpoint.com)
+ * Copyright (C) 2008 by David Brownell
+ * Copyright (C) 2008 by Nokia Corporation
+ * Copyright (C) 2009 by Samsung Electronics
+ * Copyright (c) 2021, Linaro Ltd 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define REQ_SIZE_MAX   512
+
+struct f_acm {
+   int ctrl_id;
+   int data_id;
+
+   struct usb_ep *ep_in;
+   struct usb_ep *ep_out;
+   struct usb_ep *ep_notify;
+
+   struct usb_request *req_in;
+   struct usb_request *req_out;
+
+   bool connected;
+   bool tx_on;
+
+   circbuf_t rx_buf;
+   circbuf_t tx_buf;
+
+   struct usb_function usb_function;
+
+   struct usb_cdc_line_coding line_coding;
+   u16 handshake_bits;
+#define ACM_CTRL_RTS   BIT(1)  /* unused with full duplex */
+#define ACM_CTRL_DTR   BIT(0)  /* host is ready for data r/w */
+
+   int controller_index;
+};
+
+static struct f_acm *default_acm_function;
+
+static inline struct f_acm *func_to_acm(struct usb_function *f)
+{
+   return container_of(f, struct f_acm, usb_function);
+}
+
+static inline struct f_acm *stdio_to_acm(struct stdio_dev *s)
+{
+   /* stdio dev is cloned on registration, do not use container_of */
+   return s->priv;
+}
+
+static struct usb_interface_assoc_descriptor
+acm_iad_descriptor = {
+   .bLength =  sizeof(acm_iad_descriptor),
+   .bDescriptorType =  USB_DT_INTERFACE_ASSOCIATION,
+   .bFirstInterface =  0,
+   .bInterfaceCount =  2,  // control + data
+   .bFunctionClass =   USB_CLASS_COMM,
+   .bFunctionSubClass =USB_CDC_SUBCLASS_ACM,
+   .bFunctionProtocol =USB_CDC_ACM_PROTO_AT_V25TER,
+};
+
+static struct usb_interface_descriptor acm_control_intf_desc = {
+   .bLength =  USB_DT_INTERFACE_SIZE,
+   .bDescriptorType =  

Re: [PATCH v2 1/2] lib/circbuf: Make circbuf selectable symbol

[Loic Poulain]

Hi Wolfgang,

On Mon, 22 Nov 2021 at 13:50, Wolfgang Denk  wrote:
>
> Dear Loic Poulain,
>
> In message <1637584252-15617-1-git-send-email-loic.poul...@linaro.org> you 
> wrote:
> > It is currenly only used from usbtty driver but make it properly
> > selectable via Kconfig symbol, for future usage.
> >
> > Signed-off-by: Loic Poulain 
> > ---
> >  v2: no change
> >
> >  lib/Kconfig  | 3 +++
> >  lib/Makefile | 8 +++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index c535147..1bd54d8 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -290,6 +290,9 @@ config TRACE_EARLY_ADDR
> > the size is too small then the message which says the amount of 
> > early
> > data being coped will the the same as the
> >
> > +config CIRCBUF
> > +bool
> > +
> >  source lib/dhry/Kconfig
>
> Please add a description what this symbos is doing.

It's not a user selectable symbol, adding description will break that, right?

Regards,
Loic

Re: [PATCH v2 2/2] usb: gadget: Add CDC ACM function

[Loic Poulain]

Hi Pali,

On Mon, 22 Nov 2021 at 16:48, Pali Rohár  wrote:
>
> Hello! I have just few notes, see below.
>
> > diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> > index f560068..d5d891b 100644
> > --- a/drivers/usb/gadget/Makefile
> > +++ b/drivers/usb/gadget/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += 
> > f_mass_storage.o
> >  obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
> >  obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
> >  obj-$(CONFIG_USB_FUNCTION_ROCKUSB) += f_rockusb.o
> > +obj-$(CONFIG_USB_FUNCTION_ACM)   += f_acm.o
> >  endif
> >  endif
> >  ifdef CONFIG_USB_ETHER
> > diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
> > new file mode 100644
> > index 000..3ea3b4a
> > --- /dev/null
> > +++ b/drivers/usb/gadget/f_acm.c
> ...
> > +static struct usb_cdc_header_desc acm_header_desc = {
> > + .bLength =  sizeof(acm_header_desc),
> > + .bDescriptorType =  USB_DT_CS_INTERFACE,
> > + .bDescriptorSubType =   USB_CDC_HEADER_TYPE,
> > + .bcdCDC =   cpu_to_le16(0x0110),
>
> This is global structure initialization. So should not it use
> __constant_cpu_to_le16() macro instead of cpu_to_le16()? I guess that on
> armel or x86 is cpu_to_le16() just identity macro, but on big endian
> systems it is needed to use constant initialization. Other gadget
> drivers are using those __constant_cpu* macros.

Indeed, going to use it.

>
> > +};
> ...
> > +int drv_usb_acm_stdio_init(void)
> > +{
> > + struct stdio_dev stdio;
> > +
> > + strcpy(stdio.name, "stdio_acm");
>
> Just suggestion: What about "usbacm" or just "acm"?

usbacm sounds good.

>
> > + stdio.flags = DEV_FLAGS_INPUT | DEV_FLAGS_OUTPUT;
> > + stdio.tstc = acm_stdio_tstc;
> > + stdio.getc = acm_stdio_getc;
> > + stdio.putc = acm_stdio_putc;
> > + stdio.puts = acm_stdio_puts;
> > + stdio.start = acm_stdio_start;
> > + stdio.stop = acm_stdio_stop;
> > + stdio.priv = NULL;
> > + stdio.ext = 0;
> > +
> > + return stdio_register(&stdio);
> > +}
> > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > index 8fb9a12..d584793 100644
> > --- a/include/stdio_dev.h
> > +++ b/include/stdio_dev.h
> > @@ -103,6 +103,7 @@ int drv_lcd_init(void);
> >  int drv_video_init(void);
> >  int drv_keyboard_init(void);
> >  int drv_usbtty_init(void);
> > +int drv_usb_acm_stdio_init(void);
>
> Other functions do not have stdio in name too. So what about shorting
> it? E.g. just drv_usbacm_init()?

Yes, I've named it _stdio because it's a subpart of the driver. There
is the core cdc_acm function implementation and one client of this
function which is stdio. But I'm fine with shortening that.

Regards,
Loic

[PATCH v2 2/2] usb: gadget: Add CDC ACM function

[Loic Poulain]

Add support for CDC ACM using the new UDC and gadget API. This protocol
can be used for serial over USB data transfer and is widely supported
by various OS (GNU/Linux, MS-Windows, OSX...). The usual purpose of
such link is to access device debug console and can be useful for
products not exposing regular UART to the user.

A default stdio device named 'stdio_acm' is created, and can be used
to redirect console to USB link over CDC ACM:

> setenv stdin stdio_acm; setenv stdout stdio_acm

Signed-off-by: Loic Poulain 
---
 v2: - remove possible infinite recursipe print loop
 - Remove acmconsole command, start function along the stdio device

 common/stdio.c  |   3 +
 drivers/usb/gadget/Kconfig  |   9 +
 drivers/usb/gadget/Makefile |   1 +
 drivers/usb/gadget/f_acm.c  | 672 
 include/stdio_dev.h |   1 +
 5 files changed, 686 insertions(+)
 create mode 100644 drivers/usb/gadget/f_acm.c

diff --git a/common/stdio.c b/common/stdio.c
index 4083e4e..2862823 100644
--- a/common/stdio.c
+++ b/common/stdio.c
@@ -381,6 +381,9 @@ int stdio_add_devices(void)
 #ifdef CONFIG_USB_TTY
drv_usbtty_init();
 #endif
+#ifdef CONFIG_USB_FUNCTION_ACM
+   drv_usb_acm_stdio_init ();
+#endif
if (IS_ENABLED(CONFIG_NETCONSOLE))
drv_nc_init();
 #ifdef CONFIG_JTAG_CONSOLE
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 327ea86..d81a9c5 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -182,6 +182,15 @@ config USB_FUNCTION_THOR
  Enable Tizen's THOR download protocol support in U-Boot. It
  allows downloading images into memory and flash them to target device.
 
+config USB_FUNCTION_ACM
+   bool "Enable CDC ACM gadget"
+   select SYS_STDIO_DEREGISTER
+   select CIRCBUF
+   help
+ ACM serial link. This function can be used to create a stdio device to
+ interoperate with MS-Windows hosts or with the Linux-USB "cdc-acm"
+ driver.
+
 endif # USB_GADGET_DOWNLOAD
 
 config USB_ETHER
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index f560068..d5d891b 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
 obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
 obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
 obj-$(CONFIG_USB_FUNCTION_ROCKUSB) += f_rockusb.o
+obj-$(CONFIG_USB_FUNCTION_ACM) += f_acm.o
 endif
 endif
 ifdef CONFIG_USB_ETHER
diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
new file mode 100644
index 000..3ea3b4a
--- /dev/null
+++ b/drivers/usb/gadget/f_acm.c
@@ -0,0 +1,672 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * USB CDC serial (ACM) function driver
+ *
+ * Copyright (C) 2003 Al Borchers (alborch...@steinerpoint.com)
+ * Copyright (C) 2008 by David Brownell
+ * Copyright (C) 2008 by Nokia Corporation
+ * Copyright (C) 2009 by Samsung Electronics
+ * Copyright (c) 2021, Linaro Ltd 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define REQ_SIZE_MAX   512
+
+struct f_acm {
+   int ctrl_id;
+   int data_id;
+
+   struct usb_ep *ep_in;
+   struct usb_ep *ep_out;
+   struct usb_ep *ep_notify;
+
+   struct usb_request *req_in;
+   struct usb_request *req_out;
+
+   bool connected;
+   bool tx_on;
+
+   circbuf_t rx_buf;
+   circbuf_t tx_buf;
+
+   struct usb_function usb_function;
+
+   struct usb_cdc_line_coding line_coding;
+   u16 handshake_bits;
+#define ACM_CTRL_RTS   BIT(1)  /* unused with full duplex */
+#define ACM_CTRL_DTR   BIT(0)  /* host is ready for data r/w */
+
+   int controller_index;
+};
+
+static struct f_acm *default_acm_function;
+
+static inline struct f_acm *func_to_acm(struct usb_function *f)
+{
+   return container_of(f, struct f_acm, usb_function);
+}
+
+static inline struct f_acm *stdio_to_acm(struct stdio_dev *s)
+{
+   /* stdio dev is cloned on registration, do not use container_of */
+   return s->priv;
+}
+
+static struct usb_interface_assoc_descriptor
+acm_iad_descriptor = {
+   .bLength =  sizeof(acm_iad_descriptor),
+   .bDescriptorType =  USB_DT_INTERFACE_ASSOCIATION,
+   .bFirstInterface =  0,
+   .bInterfaceCount =  2,  // control + data
+   .bFunctionClass =   USB_CLASS_COMM,
+   .bFunctionSubClass =USB_CDC_SUBCLASS_ACM,
+   .bFunctionProtocol =USB_CDC_ACM_PROTO_AT_V25TER,
+};
+
+static struct usb_interface_descriptor acm_control_intf_desc = {
+   .bLength =  USB_DT_INTERFACE_SIZE,
+   .bDescriptorType =  USB_DT_INTERFACE,
+   .bNumEndpoints =1,
+   .bInterfaceClass =  USB_CLASS_COMM,
+   .bI

[PATCH v2 1/2] lib/circbuf: Make circbuf selectable symbol

[Loic Poulain]

It is currenly only used from usbtty driver but make it properly
selectable via Kconfig symbol, for future usage.

Signed-off-by: Loic Poulain 
---
 v2: no change

 lib/Kconfig  | 3 +++
 lib/Makefile | 8 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index c535147..1bd54d8 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -290,6 +290,9 @@ config TRACE_EARLY_ADDR
  the size is too small then the message which says the amount of early
  data being coped will the the same as the
 
+config CIRCBUF
+bool
+
 source lib/dhry/Kconfig
 
 menu "Security support"
diff --git a/lib/Makefile b/lib/Makefile
index 8ba745f..4daeee2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -29,7 +29,13 @@ ifneq ($(CONFIG_CHARSET),)
 obj-y += charset.o
 endif
 endif
-obj-$(CONFIG_USB_TTY) += circbuf.o
+
+ifdef CONFIG_USB_TTY
+obj-y += circbuf.o
+else
+obj-$(CONFIG_CIRCBUF) += circbuf.o
+endif
+
 obj-y += crc8.o
 obj-y += crc16.o
 obj-$(CONFIG_ERRNO_STR) += errno_str.o
-- 
2.7.4

Re: [PATCH 3/3] cmd: add acmconsole command

[Loic Poulain]

Hi Pali,

On Tue, 16 Nov 2021 at 20:36, Pali Rohár  wrote:
>
> Hello!
>
> On Tuesday 16 November 2021 20:05:07 Loic Poulain wrote:
> > Hi Pali,
> >
> > Sorry for the late reply,
> >
> > On Mon, 27 Sept 2021 at 22:04, Pali Rohár  wrote:
> > >
> > > On Thursday 19 August 2021 13:13:06 Loic Poulain wrote:
> > > > This command allows to start CDC ACM function and redirect console
> > > > (stdin, stdout, stderr) to USB (acmconsole start). The console can
> > > > then be accessed through the USB host for debugging purpose. The
> > > > host can stop the session (acmconsole stop) to revert back console
> > > > to serial and unregister CDC ACM USB function.
> > >
> > > Hello!
> > >
> > > Could you rather implement this CDC ACM gadget in a way that calling
> > > 'setenv stdout cdc_acm' (or 'setenv stdout usbtty') would do this
> > > console redirect? Like it is supported for current usbtty or vga output
> > > code.
> > >
> > > Then this acmconsole command would not be needed at all.
> >
> > Yes, that would be good, but AFAIR I restricted this cdc_acm usage to
> > this command because we can't have fine grained control like e.g.
> > using cdc_acm as stdout only, when used, it should necessarily be
> > STDIN (at least). The reason is because of the single-tasking nature
> > of u-boot, and the fact that we need to poll the USB controller for
> > events via the 'usb_gadget_handle_interrupts()' function.
>
> There was already discussion that custom commands for activating drivers
> are against driver model design. See:
> https://lore.kernel.org/u-boot/20211026151742.42b0fcfa@thinkpad/
>
> So I think that there should not be any driver specific command which
> just activates device (in this case console).

That's a bit different here since it's not a standard
device/peripheral driver but a USB function, which is something the
'user' can decide to enable or not, all the other USB functions have
dedicated commands AFAIK. It's not clear otherwise at which place we
should register the acm_console? should it be unconditionally
registered at init?

Regards,
Loic

Re: [PATCH 3/3] cmd: add acmconsole command

[Loic Poulain]

Hi Pali,

Sorry for the late reply,

On Mon, 27 Sept 2021 at 22:04, Pali Rohár  wrote:
>
> On Thursday 19 August 2021 13:13:06 Loic Poulain wrote:
> > This command allows to start CDC ACM function and redirect console
> > (stdin, stdout, stderr) to USB (acmconsole start). The console can
> > then be accessed through the USB host for debugging purpose. The
> > host can stop the session (acmconsole stop) to revert back console
> > to serial and unregister CDC ACM USB function.
>
> Hello!
>
> Could you rather implement this CDC ACM gadget in a way that calling
> 'setenv stdout cdc_acm' (or 'setenv stdout usbtty') would do this
> console redirect? Like it is supported for current usbtty or vga output
> code.
>
> Then this acmconsole command would not be needed at all.

Yes, that would be good, but AFAIR I restricted this cdc_acm usage to
this command because we can't have fine grained control like e.g.
using cdc_acm as stdout only, when used, it should necessarily be
STDIN (at least). The reason is because of the single-tasking nature
of u-boot, and the fact that we need to poll the USB controller for
events via the 'usb_gadget_handle_interrupts()' function. In our case
we simply do that in the getc() function, which is only called for
input consoles in the u-boot mainloop.

An alternative would be to have a u-boot API to register generic
callbacks to be executed in the mainloop, but this is probably a
different thread.

Also we could possibly live with this 'bug' and simply accept that
stdout-only configuration will be broken.

Regards,
Loic

Re: [PATCH 2/3] usb: gadget: Add CDC ACM function

[Loic Poulain]

Hi folks,

Any comments on this?

On Thu, 19 Aug 2021 at 13:02, Loic Poulain  wrote:
>
> Add support for CDC ACM using the new UDC and gadget API. This protocol
> can be used for serial over USB data transfer and is widely supported
> by various OS (GNU/Linux, MS-Windows, OSX...). The usual purpose of
> such link is to access device debug console and can be useful for
> products not exposing regular UART to the user.
>
> Signed-off-by: Loic Poulain 
> ---
>  drivers/usb/gadget/Kconfig  |   9 +
>  drivers/usb/gadget/Makefile |   1 +
>  drivers/usb/gadget/f_acm.c  | 663 
> 
>  3 files changed, 673 insertions(+)
>  create mode 100644 drivers/usb/gadget/f_acm.

[PATCH 3/3] cmd: add acmconsole command

[Loic Poulain]

This command allows to start CDC ACM function and redirect console
(stdin, stdout, stderr) to USB (acmconsole start). The console can
then be accessed through the USB host for debugging purpose. The
host can stop the session (acmconsole stop) to revert back console
to serial and unregister CDC ACM USB function.

Signed-off-by: Loic Poulain 
---
 cmd/Kconfig  |  8 
 cmd/Makefile |  2 ++
 cmd/acmconsole.c | 50 ++
 3 files changed, 60 insertions(+)
 create mode 100644 cmd/acmconsole.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index ffef3cc..7cc9b1c 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1382,6 +1382,14 @@ config CMD_AXI
  Interface) busses, a on-chip interconnect specification for managing
  functional blocks in SoC designs, which is also often used in designs
  involving FPGAs (e.g.  communication with IP cores in Xilinx FPGAs).
+
+config CMD_USB_ACM_CONSOLE
+   bool "ACM USB console"
+   select USB_FUNCTION_ACM
+   help
+ Enable the command "acmconsole" for accessing u-boot console from a
+ USB host through CDC ACM protocol.
+
 endmenu
 
 
diff --git a/cmd/Makefile b/cmd/Makefile
index ed36694..c116091 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_CMD_BOOTMENU) += bootmenu.o
 obj-$(CONFIG_CMD_BOOTSTAGE) += bootstage.o
 obj-$(CONFIG_CMD_BOOTZ) += bootz.o
 obj-$(CONFIG_CMD_BOOTI) += booti.o
+ob-y += acmconsole.o
 obj-$(CONFIG_CMD_BTRFS) += btrfs.o
 obj-$(CONFIG_CMD_BUTTON) += button.o
 obj-$(CONFIG_CMD_CACHE) += cache.o
@@ -174,6 +175,7 @@ obj-$(CONFIG_CMD_FS_UUID) += fs_uuid.o
 obj-$(CONFIG_CMD_USB_MASS_STORAGE) += usb_mass_storage.o
 obj-$(CONFIG_CMD_USB_SDP) += usb_gadget_sdp.o
 obj-$(CONFIG_CMD_THOR_DOWNLOAD) += thordown.o
+obj-$(CONFIG_CMD_USB_ACM_CONSOLE) += acmconsole.o
 obj-$(CONFIG_CMD_XIMG) += ximg.o
 obj-$(CONFIG_CMD_YAFFS2) += yaffs2.o
 obj-$(CONFIG_CMD_SPL) += spl.o
diff --git a/cmd/acmconsole.c b/cmd/acmconsole.c
new file mode 100644
index 000..dac8136
--- /dev/null
+++ b/cmd/acmconsole.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * acmconsole.c -- Console over USB CDC serial (ACM) function gadget function
+ *
+ * Copyright (c) 2021, Linaro Ltd 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+int do_usb_acmconsole(cmd_tbl_t *cmdtp, int flag, int argc, char * const 
argv[])
+{
+   int ret;
+
+   if (argc != 2)
+   return CMD_RET_USAGE;
+
+   if (!strcmp(argv[1], "start")) {
+   ret = g_dnl_register("usb_serial_acm");
+   if (ret)
+   return ret;
+
+   /* ACM function creates a stdio device name 'stdio_acm' */
+   console_assign(stdin, "stdio_acm");
+   console_assign(stdout, "stdio_acm");
+   console_assign(stderr, "stdio_acm");
+   } else if (!strcmp(argv[1], "stop")) {
+   /* default to serial (TODO: restore initial devices) */
+   console_assign(stdin, "serial");
+   console_assign(stdout, "serial");
+   console_assign(stderr, "serial");
+
+   g_dnl_unregister();
+   g_dnl_clear_detach();
+   } else {
+   return CMD_RET_USAGE;
+   }
+
+   return 0;
+}
+
+U_BOOT_CMD(acmconsole, CONFIG_SYS_MAXARGS, 1, do_usb_acmconsole,
+  "Console over USB CDC ACM",
+  "start - start console over USB CDC ACM gadget function\n" \
+  "acmconsole stop - stop USB console");
-- 
2.7.4

[PATCH 2/3] usb: gadget: Add CDC ACM function

[Loic Poulain]

Add support for CDC ACM using the new UDC and gadget API. This protocol
can be used for serial over USB data transfer and is widely supported
by various OS (GNU/Linux, MS-Windows, OSX...). The usual purpose of
such link is to access device debug console and can be useful for
products not exposing regular UART to the user.

Signed-off-by: Loic Poulain 
---
 drivers/usb/gadget/Kconfig  |   9 +
 drivers/usb/gadget/Makefile |   1 +
 drivers/usb/gadget/f_acm.c  | 663 
 3 files changed, 673 insertions(+)
 create mode 100644 drivers/usb/gadget/f_acm.c

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 327ea86..d81a9c5 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -182,6 +182,15 @@ config USB_FUNCTION_THOR
  Enable Tizen's THOR download protocol support in U-Boot. It
  allows downloading images into memory and flash them to target device.
 
+config USB_FUNCTION_ACM
+   bool "Enable CDC ACM gadget"
+   select SYS_STDIO_DEREGISTER
+   select CIRCBUF
+   help
+ ACM serial link. This function can be used to create a stdio device to
+ interoperate with MS-Windows hosts or with the Linux-USB "cdc-acm"
+ driver.
+
 endif # USB_GADGET_DOWNLOAD
 
 config USB_ETHER
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index f560068..d5d891b 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
 obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
 obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
 obj-$(CONFIG_USB_FUNCTION_ROCKUSB) += f_rockusb.o
+obj-$(CONFIG_USB_FUNCTION_ACM) += f_acm.o
 endif
 endif
 ifdef CONFIG_USB_ETHER
diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
new file mode 100644
index 000..5115f1d
--- /dev/null
+++ b/drivers/usb/gadget/f_acm.c
@@ -0,0 +1,663 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * USB CDC serial (ACM) function driver
+ *
+ * Copyright (C) 2003 Al Borchers (alborch...@steinerpoint.com)
+ * Copyright (C) 2008 by David Brownell
+ * Copyright (C) 2008 by Nokia Corporation
+ * Copyright (C) 2009 by Samsung Electronics
+ * Copyright (c) 2021, Linaro Ltd 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define REQ_SIZE_MAX   512
+
+struct f_acm {
+   int ctrl_id;
+   int data_id;
+
+   struct usb_ep *ep_in;
+   struct usb_ep *ep_out;
+   struct usb_ep *ep_notify;
+
+   struct usb_request *req_in;
+   struct usb_request *req_out;
+
+   bool connected;
+   bool tx_on;
+
+   circbuf_t rx_buf;
+   circbuf_t tx_buf;
+
+   struct stdio_dev stdio;
+   struct usb_function usb_function;
+
+   struct usb_cdc_line_coding line_coding;
+   u16 handshake_bits;
+#define ACM_CTRL_RTS   BIT(1)  /* unused with full duplex */
+#define ACM_CTRL_DTR   BIT(0)  /* host is ready for data r/w */
+
+   int controller_index;
+};
+
+static inline struct f_acm *func_to_acm(struct usb_function *f)
+{
+   return container_of(f, struct f_acm, usb_function);
+}
+
+static inline struct f_acm *stdio_to_acm(struct stdio_dev *s)
+{
+   /* stdio dev is cloned on registration, do not use container_of */
+   return s->priv;
+}
+
+static struct usb_interface_assoc_descriptor
+acm_iad_descriptor = {
+   .bLength =  sizeof(acm_iad_descriptor),
+   .bDescriptorType =  USB_DT_INTERFACE_ASSOCIATION,
+   .bFirstInterface =  0,
+   .bInterfaceCount =  2,  // control + data
+   .bFunctionClass =   USB_CLASS_COMM,
+   .bFunctionSubClass =USB_CDC_SUBCLASS_ACM,
+   .bFunctionProtocol =USB_CDC_ACM_PROTO_AT_V25TER,
+};
+
+static struct usb_interface_descriptor acm_control_intf_desc = {
+   .bLength =  USB_DT_INTERFACE_SIZE,
+   .bDescriptorType =  USB_DT_INTERFACE,
+   .bNumEndpoints =1,
+   .bInterfaceClass =  USB_CLASS_COMM,
+   .bInterfaceSubClass =   USB_CDC_SUBCLASS_ACM,
+   .bInterfaceProtocol =   USB_CDC_ACM_PROTO_AT_V25TER,
+};
+
+static struct usb_interface_descriptor acm_data_intf_desc = {
+   .bLength =  sizeof(acm_data_intf_desc),
+   .bDescriptorType =  USB_DT_INTERFACE,
+   .bNumEndpoints =2,
+   .bInterfaceClass =  USB_CLASS_CDC_DATA,
+};
+
+static struct usb_cdc_header_desc acm_header_desc = {
+   .bLength =  sizeof(acm_header_desc),
+   .bDescriptorType =  USB_DT_CS_INTERFACE,
+   .bDescriptorSubType =   USB_CDC_HEADER_TYPE,
+   .bcdCDC =   cpu_to_le16(0x0110),
+};
+
+static struct usb_cdc_call_mgmt_descriptor acm_call_mgmt_desc = {
+   .bLength =  sizeof(acm_call_mgmt_desc),
+   .bDescriptorTyp

[PATCH 1/3] lib/circbuf: Make circbuf selectable symbol

[Loic Poulain]

It is currenly only used from usbtty driver but make it properly
selectable via Kconfig symbol, for future usage.

Signed-off-by: Loic Poulain 
---
 lib/Kconfig  | 3 +++
 lib/Makefile | 8 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index c535147..1bd54d8 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -290,6 +290,9 @@ config TRACE_EARLY_ADDR
  the size is too small then the message which says the amount of early
  data being coped will the the same as the
 
+config CIRCBUF
+bool
+
 source lib/dhry/Kconfig
 
 menu "Security support"
diff --git a/lib/Makefile b/lib/Makefile
index 8ba745f..4daeee2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -29,7 +29,13 @@ ifneq ($(CONFIG_CHARSET),)
 obj-y += charset.o
 endif
 endif
-obj-$(CONFIG_USB_TTY) += circbuf.o
+
+ifdef CONFIG_USB_TTY
+obj-y += circbuf.o
+else
+obj-$(CONFIG_CIRCBUF) += circbuf.o
+endif
+
 obj-y += crc8.o
 obj-y += crc16.o
 obj-$(CONFIG_ERRNO_STR) += errno_str.o
-- 
2.7.4

Re: [U-Boot] DFU and UBIFS Volume upgrade

[Loic Poulain]

Thanks Heiko,

On Wed, 4 Sep 2019 at 07:24, Heiko Schocher  wrote:

> Hello Loic,
>
> added Lukasz as he is the DFU custodian.
>
> Am 03.09.2019 um 09:59 schrieb Loic Poulain:
> > Hi,
> >
> > AFAIU, today it's possible to update a UBI partition via DFU with a new
> UBI
> > blob using 'partubi'.
>
> Yes.
>
> > However, this causes two issues/limitations:
> > - It erases the partition, causing PEB erase counters amnesia (contrary
> to
> > Linux ubiformat)
>
> Yes, patches which fixes this are welcome :-P
>
> > - It's no possible to have a volume-grained upgrade (per UBIFS volume)
>
> I am not to deep in the DFU topic involved, but I think the problem
> is that ubi is not an interface like "nand" or "mmc" it is more something
> like a partition type ...
>
> The big problem with writting an ubi image into nand is, that you
> need to store the image first in RAM and you may have not enough ...
> so may writting volume serverally it may help out here.
>
> On the other side, it is may possible to introduce an ubi interface for
> UBI volumes. But what if your board has not setup yet UBI, nor the UBI
> Volumes? Is there an interface in DFU for setting up something like
> "partitions" ? You need to pass several parameters to create a new
> UBI Volume ...
>

That's a good point. This makes things a bit complicated.


> May Lukasz can say here more ...
>
> I prefer nowadays to boot a linux and use swupdate, which can handle
> UBI Volumes ...
>

I agree, so let's keep that out of DFU.

So, the priority is maybe to preserve erase-counters to keep optimal wear
leveling.
This should be quite simple to fix, I'll look at the UBI spec and come back
with a patch.

Regards,
Loic
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] DFU and UBIFS Volume upgrade

[Loic Poulain]

Hi,

AFAIU, today it's possible to update a UBI partition via DFU with a new UBI
blob using 'partubi'.
However, this causes two issues/limitations:
- It erases the partition, causing PEB erase counters amnesia (contrary to
Linux ubiformat)
- It's no possible to have a volume-grained upgrade (per UBIFS volume)

I saw some (very) old threads on this subject but no conclusion. Did I miss
something? any recommendations?

Regards,
Loic
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot