* [PATCH] rockchip: efuse: fix rk3399 reading multiple values @ 2023-04-17 16:09 John Keeping 2023-04-17 17:39 ` Jonas Karlman 0 siblings, 1 reply; 7+ messages in thread From: John Keeping @ 2023-04-17 16:09 UTC (permalink / raw) To: u-boot; +Cc: John Keeping, Simon Glass, Philipp Tomsich, Kever Yang Update rk3399 to match the pattern in the other device-specific implementations to ensure the previous address is cleared when reading multiple blocks. Signed-off-by: John Keeping <john@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); -- 2.40.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: efuse: fix rk3399 reading multiple values 2023-04-17 16:09 [PATCH] rockchip: efuse: fix rk3399 reading multiple values John Keeping @ 2023-04-17 17:39 ` Jonas Karlman 2023-04-17 17:58 ` John Keeping 0 siblings, 1 reply; 7+ messages in thread From: Jonas Karlman @ 2023-04-17 17:39 UTC (permalink / raw) To: John Keeping; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot Hi John, 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. Regards, Jonas > > Signed-off-by: John Keeping <john@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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: efuse: fix rk3399 reading multiple values 2023-04-17 17:39 ` Jonas Karlman @ 2023-04-17 17:58 ` John Keeping 2023-04-17 18:19 ` Jonas Karlman 2023-04-20 11:16 ` John Keeping 0 siblings, 2 replies; 7+ messages in thread From: John Keeping @ 2023-04-17 17:58 UTC (permalink / raw) To: Jonas Karlman; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot 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. > > > > Signed-off-by: John Keeping <john@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); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: efuse: fix rk3399 reading multiple values 2023-04-17 17:58 ` John Keeping @ 2023-04-17 18:19 ` Jonas Karlman 2023-04-18 12:48 ` Jonas Karlman 2023-04-20 11:16 ` John Keeping 1 sibling, 1 reply; 7+ messages in thread From: Jonas Karlman @ 2023-04-17 18:19 UTC (permalink / raw) To: John Keeping; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot On 2023-04-17 19:58, 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. Let me re-test this on one of my rk3399 boards, from what I can recall I made the same change when working on support for the other SoCs and same as you I was expecting it to be reading back bad/wrong data on rk3399. At that time the dump cmd read all 128 byte using one call, unexpectedly the same data was read back regardless the use of clrsetbits or setbits. Will re-test using a full 128 byte read call and report back result. Also look like the EFUSE_STROBE bit is missing from the mask param. Regards, Jonas > >>> >>> Signed-off-by: John Keeping <john@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); >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: efuse: fix rk3399 reading multiple values 2023-04-17 18:19 ` Jonas Karlman @ 2023-04-18 12:48 ` Jonas Karlman 0 siblings, 0 replies; 7+ messages in thread From: Jonas Karlman @ 2023-04-18 12:48 UTC (permalink / raw) To: John Keeping; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot On 2023-04-17 20:19, Jonas Karlman wrote: > On 2023-04-17 19:58, 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. > > Let me re-test this on one of my rk3399 boards, from what I can recall I > made the same change when working on support for the other SoCs and same > as you I was expecting it to be reading back bad/wrong data on rk3399. > > At that time the dump cmd read all 128 byte using one call, unexpectedly > the same data was read back regardless the use of clrsetbits or setbits. > > Will re-test using a full 128 byte read call and report back result. I did seem to recall correctly, the values written to ctrl reg is not what is being read back on rk3399. This is from the read of cpu_id where ctrl is the readl value and val is the value used in setbits call. Here we can see that only the strobe bit is sticky after the setbits call, addr is never read back. rockchip_rk3399_efuse_read: before array read mode, size=5 ctrl=a9 rockchip_rk3399_efuse_read: after array read mode, size=5 ctrl=28c rockchip_rk3399_efuse_read: before setbits, size=4 ctrl=28c val=10002 rockchip_rk3399_efuse_read: after setbits+udelay, size=4 ctrl=28e rockchip_rk3399_efuse_read: before clrbits, size=4 ctrl=28e rockchip_rk3399_efuse_read: after clrbits+udelay, size=4 ctrl=28c rockchip_rk3399_efuse_read: before setbits, size=3 ctrl=28c val=20002 rockchip_rk3399_efuse_read: after setbits+udelay, size=3 ctrl=28e rockchip_rk3399_efuse_read: before clrbits, size=3 ctrl=28e rockchip_rk3399_efuse_read: after clrbits+udelay, size=3 ctrl=28c rockchip_rk3399_efuse_read: before setbits, size=2 ctrl=28c val=30002 rockchip_rk3399_efuse_read: after setbits+udelay, size=2 ctrl=28e rockchip_rk3399_efuse_read: before clrbits, size=2 ctrl=28e rockchip_rk3399_efuse_read: after clrbits+udelay, size=2 ctrl=28c rockchip_rk3399_efuse_read: before setbits, size=1 ctrl=28c val=40002 rockchip_rk3399_efuse_read: after setbits+udelay, size=1 ctrl=28e rockchip_rk3399_efuse_read: before clrbits, size=1 ctrl=28e rockchip_rk3399_efuse_read: after clrbits+udelay, size=1 ctrl=28c rockchip_rk3399_efuse_read: before setbits, size=0 ctrl=28c val=50002 rockchip_rk3399_efuse_read: after setbits+udelay, size=0 ctrl=28e rockchip_rk3399_efuse_read: before clrbits, size=0 ctrl=28e rockchip_rk3399_efuse_read: after clrbits+udelay, size=0 ctrl=28c rockchip_rk3399_efuse_read: before power-down mode, size=-1 ctrl=28c rockchip_rk3399_efuse_read: after power-down mode, size=-1 ctrl=21 And data in efuse is read back correctly. dump_efuse with 16 byte buffer: 00000000: 52 4b 33 99 91 fe 21 54 4d 53 36 31 35 2e 30 30 RK3...!TMS615.00 00000010: 00 00 00 00 04 02 84 26 1c 25 15 1a 08 00 00 00 .......&.%...... 00000020: 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ dump_efuse with 4 byte buffer: 00000000: 52 4b 33 99 RK3. 00000004: 91 fe 21 54 ..!T 00000008: 4d 53 36 31 MS61 0000000c: 35 2e 30 30 5.00 00000010: 00 00 00 00 .... 00000014: 04 02 84 26 ...& 00000018: 1c 25 15 1a .%.. 0000001c: 08 00 00 00 .... 00000020: 00 00 10 00 .... Above was from an old rk3399 board, also tested on a board with a newer OP1 revision that showed the same behavior of the ctrl reg. Regards, Jonas > > Also look like the EFUSE_STROBE bit is missing from the mask param. > > Regards, > Jonas > >> >>>> >>>> Signed-off-by: John Keeping <john@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); >>> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: efuse: fix rk3399 reading multiple values 2023-04-17 17:58 ` John Keeping 2023-04-17 18:19 ` Jonas Karlman @ 2023-04-20 11:16 ` John Keeping 2023-04-20 11:30 ` Jonas Karlman 1 sibling, 1 reply; 7+ messages in thread From: John Keeping @ 2023-04-20 11:16 UTC (permalink / raw) To: Jonas Karlman; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot 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 <john@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); > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: efuse: fix rk3399 reading multiple values 2023-04-20 11:16 ` John Keeping @ 2023-04-20 11:30 ` Jonas Karlman 0 siblings, 0 replies; 7+ messages in thread From: Jonas Karlman @ 2023-04-20 11:30 UTC (permalink / raw) To: John Keeping; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot On 2023-04-20 13:16, John Keeping wrote: > 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. Please see my prior response at [1], my testing showed that it was not needed. That run was on a plain rk3399 and a op1 rk3399 variant showed same result. [1] https://patchwork.ozlabs.org/project/uboot/patch/20230417160914.2838607-1-john@metanate.com/#3098312 Regards, Jonas > >>>> >>>> Signed-off-by: John Keeping <john@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); >>> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-04-20 11:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-17 16:09 [PATCH] rockchip: efuse: fix rk3399 reading multiple values John Keeping 2023-04-17 17:39 ` Jonas Karlman 2023-04-17 17:58 ` John Keeping 2023-04-17 18:19 ` Jonas Karlman 2023-04-18 12:48 ` Jonas Karlman 2023-04-20 11:16 ` John Keeping 2023-04-20 11:30 ` Jonas Karlman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.