[PATCH] rockchip: efuse: fix rk3399 reading multiple values
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);
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++));
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++));
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++));
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++));
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++));
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-jo...
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++));
participants (2)
-
John Keeping -
Jonas Karlman