On Mon, Apr 17, 2023 at 06:58:30PM +0100, John Keeping wrote:
> On Mon, Apr 17, 2023 at 05:39:24PM +0000, Jonas Karlman wrote:
> > On 2023-04-17 18:09, John Keeping wrote:
> > > Update rk3399 to match the pattern in the other device-specific
> > > implementations to ensure the previous address is cleared when reading
> > > multiple blocks.
> >
> > Does this fix a real issue for you?
> >
> > Compared to the other device-specific implementations this reg behaves
> > differently on RK3399. The addr field is not read back, or it gets
> > auto-cleared when EFUSE_STROBE is cleared, the use of clrsetbits_le32
> > may mislead that this reg holds the addr value from prior iteration.
>
> I don't have an RK3399 to test with, but I spotted this when
> experimenting with RK3288's secure efuse which has a similar layout to
> the RK3399 efuses.
>
> Having looked through the RK3399 TRM, I can't see anything about the
> address auto-clearing and the documentation for redundancy read mode
> requiring two strobe cycles suggests that the address is preserved.
>
> Howe are you testing reads? dump_efuse only reads one block at a time
> so it isn't sufficient to trigger an issue here as there is an
> unconditional write to EFUSE_CTRL before this loop.
I found an RK3399 which I had for a previous project and have now tested
this - I can't see any evidence of the address auto-clearing so it looks
to me like this patch fixes a real issue.
> > >
> > > Signed-off-by: John Keeping <j...@metanate.com>
> > > ---
> > > drivers/misc/rockchip-efuse.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/misc/rockchip-efuse.c b/drivers/misc/rockchip-efuse.c
> > > index 2f96b79ea4..d302271239 100644
> > > --- a/drivers/misc/rockchip-efuse.c
> > > +++ b/drivers/misc/rockchip-efuse.c
> > > @@ -207,8 +207,8 @@ static int rockchip_rk3399_efuse_read(struct udevice
> > > *dev, int offset,
> > > udelay(1);
> > >
> > > while (size--) {
> > > - setbits_le32(efuse->base + EFUSE_CTRL,
> > > - EFUSE_STROBE | RK3399_ADDR(offset++));
> > > + clrsetbits_le32(efuse->base + EFUSE_CTRL, RK3399_A_MASK,
> > > + EFUSE_STROBE | RK3399_ADDR(offset++));
> > > udelay(1);
> > > *buffer++ = readl(efuse->base + EFUSE_DOUT);
> > > clrbits_le32(efuse->base + EFUSE_CTRL, EFUSE_STROBE);
> >
