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

[Liming Sun]

v3 has been posted with Changelog added.

Thanks!
Liming

> -----Original Message-----
> From: Liming Sun
> Sent: Thursday, January 31, 2019 12:18 PM
> To: 'Andy Shevchenko' <andy.shevchenko@xxxxxxxxx>
> 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
> 
> 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