Re: [PATCH u-boot-mvebu 5/5] arm: mvebu: a37xx: Add support for reading Security OTP values

["Marek Behún" <marek.behun@nic.cz>]

On Thu, 17 Feb 2022 17:50:31 +0100
Pali Rohár <pali@kernel.org> wrote:

> On Thursday 17 February 2022 15:31:10 Marek Behún wrote:
> > On Thu, 17 Feb 2022 10:26:19 +0100
> > Pali Rohár <pali@kernel.org> wrote:
> >   
> > > Only secure CM3 core can access Security OTP. It is not possible via A53  
> > 
> > It is not possible for the A53 core (on which U-Boot is running) to read
> > it directly.
> >   
> > > core on which is running U-Boot. Marvell for this purpose defined mbox API  
> > 
> > For this purpose Marvell defined...
> >   
> > > for sending OTP commands between CM and A53 cores.  
> >                                    ^CM3
> >   
> > > Implement this Marvell mbox API via U-Boot fuse API.  
> > 
> > Implement these Marvell fuse reading mbox commands via ....
> >   
> > > Banks 0-43 are used for accessing Security OTP (44 rows with 67 bits via 44
> > > banks and words 0-2).  
> > 
> > Note that of the 67 bits, the 3 upper bits are: 1 lock bit and 2
> > auxiliary bits (meant for testing during the manufacture of the SOC, as
> > I understand it).
> > 
> > Also note that the lock bit and the auxiliary bits are not readable
> > via Marvell commands.
> > 
> > With CZ.NIC's commands the lock bit is readable.
> >   
> > > Write support is not implemented yet.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  arch/arm/mach-mvebu/armada3700/efuse.c | 40 ++++++++++++++++++++++++--
> > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-mvebu/armada3700/efuse.c b/arch/arm/mach-mvebu/armada3700/efuse.c
> > > index 03778f17ea49..274d9c72c073 100644
> > > --- a/arch/arm/mach-mvebu/armada3700/efuse.c
> > > +++ b/arch/arm/mach-mvebu/armada3700/efuse.c
> > > @@ -8,6 +8,7 @@
> > >  #include <common.h>
> > >  #include <asm/io.h>
> > >  #include <linux/delay.h>
> > > +#include <mach/mbox.h>
> > >  #include <mach/soc.h>
> > >  
> > >  #define OTP_NB_REG_BASE		((void __iomem *)MVEBU_REGISTER(0x12600))
> > > @@ -77,6 +78,42 @@ static void otp_read_parallel(void __iomem *base, u32 *data, u32 count)
> > >  	}
> > >  }
> > >  
> > > +static int rwtm_otp_read(u8 row, u32 word, u32 *data)
> > > +{
> > > +	u32 out[3];
> > > +	u32 in[2];
> > > +	int res;
> > > +
> > > +	/*
> > > +	 * MBOX_CMD_OTP_READ_32B command is supported by Marvell fuse.bin
> > > +	 * firmware and also by new (yet unreleased) CZ.NIC wtmi firmware.  
> > 
> > Marvell's, CZ.NIC's, and drop the "(yet unreleased)", because you'll
> > need to send another patch that drops it afterwards.
> >   
> > > +	 * But this command does not provide access to lock bit.
> > > +	 */
> > > +	if (word < 2) {
> > > +		in[0] = row;
> > > +		in[1] = word * 32;
> > > +		res = mbox_do_cmd(MBOX_CMD_OTP_READ_32B, in, 2, out, 2);
> > > +		if (res != -ENOSYS) {
> > > +			if (!res)
> > > +				*data = out[0];
> > > +			return res;
> > > +		}
> > > +		/* Fallback for old version of CZ.NIC wtmi firmware. */
> > > +	}  
> > 
> > I am afraid this is not correct, because Marvell's firmware reads the
> > efuse without Error Correction. So it is possible for Marvell's command
> > to return different value than CZ.NIC's command.
> > 
> > You need to determine whether CZ.NIC's command is supported, and use it
> > if it is, otherwise use Marvell's command. Or you need to define
> > whether and when the Error Correction is supposed to be used, or
> > something.  
> 
> Seems that this U-Boot fuse API is low level API, so it probably would
> be better to always read without ECC correction (which is provided by
> Marvell OTP API). As ECC is stored in other bits, it is possible to read
> everything needed for ECC correction via this API.
> 
> This could simplify patch: Lock bit read via CZ.NIC API (as there is no
> other API) and other bits read via Marvell API (which is going to be
> supported also by CZ.NIC firmware).

Ok, as long as turris_mox.c reads OTP with Error Correction, fuse can
be kept low level.

Marek

> > But doing what you are doing here can make Turris MOX boards read
> > different values. I know of at least one board where serial number or
> > MAC address needs Error Correction.
> > 
> > Marek