RE: [PATCH v2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc

[Liming Sun]

Please see my response inline. Will provide v3 once I solve the comments.

Thanks,
Liming

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Thursday, January 31, 2019 12:02 PM
> To: Liming Sun <lsun@xxxxxxxxxxxx>
> Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>; David
> Woods <dwoods@xxxxxxxxxxxx>; Platform Driver <platform-driver-x86@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
> 
> On Thu, Jan 31, 2019 at 6:53 PM Liming Sun <lsun@xxxxxxxxxxxx> wrote:
> >
> > This commit adds the bootctl platform driver for Mellanox BlueField
> > Soc, which controls the eMMC boot partition swapping and sends SMC
> > calls to ATF running at exception level EL3 to program some system
> > register. This register is only accessible in secure code and is
> > used to enable the watchdog after reboot.
> >
> > Below are the sequences of a typical use case.
> >
> >   1. User-space tool upgrades one eMMC boot partition and requests
> >      the boot partition swapping;
> >
> >   2. The bootctl driver handles such request and sends SMC call
> >      to ATF. ATF programs register BREADCRUMB0 which has value
> >      preserved during software reset. It also programs eMMC to
> >      swap the boot partition;
> >
> >   3. After software reset (rebooting), ATF BL1 (BootRom) checks
> >      register BREADCRUMB0 to enable watchdog if configured;
> >
> >   4. If booting fails, the watchdog timer will trigger rebooting.
> >      In such case, ATF BootRom will switch the boot partition
> >      to the previous one.
> 
> Thanks for an update. My comments below.
> 
> 
> > Reviewed-by: David Woods <dwoods@xxxxxxxxxxxx>
> 
> I'm not sure I see this guy to review v2. Of course if you consider
> all changes minor, you may leave this tag.

He sits besides me for internal review. I'll try to ask him to send
comments to the mailing list.

> 
> > Signed-off-by: Liming Sun <lsun@xxxxxxxxxxxx>
> > ---
> 
> Here should be a changelog what had been done in new version.

Will provide changelog in the v3 email.
Or please correct me if you meant adding changelog in the code or
cover letter.

> > +/* UUID used to probe ATF service. */
> 
> > +static const char * const mlxbf_bootctl_svc_uuid_str =
> > +       "89c036b4-e7d7-11e6-8797-001aca00bfc4";
> 
> static const char *xxx = ...;

Will update it in v3.

> 
> > +/*
> > + * UUID structure used to match the returned value from ATF in
> > + * four 32-bit words. No need to do endian conversion here.
> > + */
> > +union mlxbf_bootctl_uuid {
> > +       guid_t guid;
> > +       u32 words[4];
> > +};
> 
> I'm not sure it's the best you can do. instead of using union, better
> to use conversion from and to corresponding uuid type.

I'll try to figure out another way without the union to compare the
uuid directly using uuid api. Will update it in v3.

> 
> The rest I will comment on v3.
> 
> --
> With Best Regards,
> Andy Shevchenko