[U-Boot] [EXT] Re: [PATCH 4/5] cmd: mvebu: bubt: Add support for legacy Marvell SoCs

[Kostya Porotchkin]

Hi, Jon,

> -----Original Message-----
> From: Jon Nettleton <jon at solid-run.com>
> Sent: Wednesday, August 29, 2018 17:28
> To: Kostya Porotchkin <kostap at marvell.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Stefan Roese <sr at denx.de>
> Subject: [EXT] Re: [U-Boot] [PATCH 4/5] cmd: mvebu: bubt: Add support for
> legacy Marvell SoCs
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, Aug 29, 2018 at 3:36 PM <kostap at marvell.com> wrote:
> >
> > From: Konstantin Porotchkin <kostap at marvell.com>
> >
> > Add support for image load and basic verification in bubt for legacy
> > Marvell SoCs (A38x, A39x, ...)
> >
> > Signed-off-by: Konstantin Porotchkin <kostap at marvell.com>
> > Cc: Igal Liberman <igall at marvell.com>
> > Cc: Stefan Roese <sr at denx.de>
> > ---
> >  cmd/mvebu/bubt.c | 75
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 72 insertions(+), 3 deletions(-)
> >
> > diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c index
> > b4d371f..e10d079 100644
> > --- a/cmd/mvebu/bubt.c
> > +++ b/cmd/mvebu/bubt.c
> > @@ -83,6 +83,33 @@ struct mvebu_image_info {
> >         u32     encrypt_start_offset;
> >         u32     encrypt_size;
> >  };
> > +
> > +#else /* Older Armada SoCs - A38x, A39x, ... */
> > +
> > +#define        LEGACY_HDR_VERSION      1
> > +
> > +struct legacy_image_header {
> > +/*     type    name                    byte order */
> > +       u8      block_id;               /*   0   */
> > +       u8      flags;                  /*   1   */
> > +       u16     nand_pge_size;          /*  2-3  */
> > +       u32     block_size;             /*  4-7  */
> > +       u8      version;                /*   8   */
> > +       u8      hdr_size_msb;           /*   9   */
> > +       u16     hdr_size_lsb;           /* 10-11 */
> > +       u32     source_addr;            /* 12-15 */
> > +       u32     destination_addr;       /* 16-19 */
> > +       u32     execution_addr;         /* 20-23 */
> > +       u8      options;                /*  24   */
> > +       u8      nand_block_size;        /*  25   */
> > +       u8      nand_technology;        /*  26   */
> > +       u8      rsvd4;                  /*  27   */
> > +       u16     rsvd2;                  /* 28-29 */
> > +       u8      ext;                    /*  30   */
> > +       u8      checksum;               /*  31   */
> > +
> > +};
> > +
> >  #endif /* CONFIG_ARMADA_XXX */
> >
> >  struct bubt_dev {
> > @@ -618,11 +645,53 @@ static int check_image_header(void)
> >         return 0;
> >  }
> >
> > -#else /* Not ARMADA? */
> > +#else /* Legacy SoCs */
> > +u8 do_checksum8(u8 *start, u32 len)
> > +{
> > +       u8 sum = 0;
> > +       u8 *startp = start;
> > +
> > +       do {
> > +               sum += *startp;
> > +               startp++;
> > +       } while (--len);
> > +
> > +       return sum;
> > +}
> > +
> >  static int check_image_header(void)
> >  {
> > -       printf("bubt cmd does not support this SoC device or family!\n");
> > -       return -ENOEXEC;
> > +       struct legacy_image_header *hdr =
> > +                       (struct legacy_image_header *)get_load_addr();
> > +       u32 header_len = hdr->hdr_size_lsb + (hdr->hdr_size_msb << 16);
> > +       u8 checksum;
> > +       u8 checksum_ref = hdr->checksum;
> > +
> > +       /*
> > +        * For now compare checksum, and header version. Later we can
> > +        * verify more stuff on the header like interface type, etc
> > +        */
> > +       if (hdr->version != LEGACY_HDR_VERSION) {
> > +               printf("ERROR: Bad HDR Version 0x%x != 0x%x\n",
> > +                      hdr->version, LEGACY_HDR_VERSION);
> > +               return -ENOEXEC;
> > +       }
> > +
> > +       /* The checksum value is discarded from checksum calculation */
> > +       hdr->checksum = 0;
> > +
> > +       checksum = do_checksum8((u8 *)hdr, header_len);
> > +       if (checksum != checksum_ref) {
> > +               printf("Error: Bad Image checksum. 0x%x != 0x%x\n",
> > +                      checksum, checksum_ref);
> > +               return -ENOEXEC;
> > +       }
> > +
> > +       /* Restore the checksum before writing */
> > +       hdr->checksum = checksum_ref;
> > +       printf("Image checksum...OK!\n");
> > +
> > +       return 0;
> >  }
> >  #endif
> >
> 
> Konstantin,
> 
> I already have a more complete integration for the legacy SOCs into the bubt
> command.  It incorporates much of the same code that is used to generate the
> images in kwimage.c.  We haven't submitted it to mainline yet as we were
> waiting for a few more patchsets to land.
[KP] No problem, we will ask Stefan to keep this patch aside and take yours.
I made this change for handling my own tests - it was just handy to use single command for image burning.
> 
> The features I have incorporated are supporting all possible boot devices, image
> flashing from USB devices, and most importantly image verification before
> flashing, so you can't brick your device by flashing an image for eMMC onto the
> SPI device.
> 
> If you could take a look and comment I would be happy to work with you to get
> this support into mainline.
[KP] Sure, no problem. The main reason for this patch series is a fix to broken A38x-GP platform.
If you have better "bubt", let's use it!
> 
> Thanks,
> Jon