[SPI_DEVICE|SPI_FLASH] Added a write-protection on en4b/ex4b to avoid… #15326

jeoongp · 2022-10-06T23:50:11Z

… SW overwrite

Issue : as reported in [spi_device] EN4B/EX4B may fail if the next item comes very shortly #14940, when CSB has a short deassertion time, the next sck_csb_asserted_pulse can show up at the next SPI transaction while the previous propagation is still ongoing. Since spi_reg_cfg_addr_4b_en_sync still shows the old value different from spi_cfg_addr_4b_en_o, spi_cfg_addr_4b_en_o is written again by the old value.
In this fix, the HW source-of-truth value is protected till its mirrorred SW reg is updated

Signed-off-by: Joshua Park [email protected]

hw/ip/spi_device/rtl/spid_addr_4b.sv

… SW overwrite - Issue : as reported in lowRISC#14940, hhen CSB has a short deassertion time, the next sck_csb_asserted_pulse can show up at the next SPI transaction while the previous propagation is still ongoing. Since spi_reg_cfg_addr_4b_en_sync still shows the old value different from spi_cfg_addr_4b_en_o, spi_cfg_addr_4b_en_o is written again by the old value. - In this fix, the HW source-of-truth value is protected till its mirrorred SW regs is updated Signed-off-by: Joshua Park <[email protected]>

eunchan · 2022-10-07T01:19:15Z

hw/ip/spi_device/rtl/spid_addr_4b.sv

+  logic addr_4b_en_unlock_condition;
+  logic addr_4b_en_sw_update_condition;
+
+  assign addr_4b_en_lock_condition = spi_reg_cfg_addr_4b_en_sync ? spi_addr_4b_clr_i :


In the doc, the diagram shows lock condition is XOR of spi_reg_cfg_addr_4b_en_sync and spi_cfg_addr_4b_en_o. Here, it uses EN4B/EX4B condition. Is this to retract update (of locked FF) by one SPI clock ?

Line 111 : assign addr_4b_en_sw_update_condition = (spi_reg_cfg_addr_4b_en_sync != spi_cfg_addr_4b_en_o);
corresponds to XOR. XOR output is also marked with addr_4b_en_sw_update_condition in the doc.

addr_4b_en_lock_condition needs en4b/ex4b condition to protect spi_cfg_addr_4b_en_o at the right next cycle after en4b/ex4b.

tjaychen · 2022-10-07T18:29:24Z

hw/ip/spi_device/rtl/spid_addr_4b.sv

-    end else if (spi_csb_asserted_pulse_i
-      && (spi_cfg_addr_4b_en_o != spi_reg_cfg_addr_4b_en_sync)) begin
+    end else if (spi_csb_asserted_pulse_i & !addr_4b_en_locked &
+                 addr_4b_en_sw_update_condition) begin
      // Update


ah this is kind of neat, it's like the reverse of what we're doing in prim_reg_cdc i think.
So to make sure i understand..

when there is a spi host update, that takes priority. If the value is different from the mirrored software value, the software update becomes "locked" until they are the same.

I'm sure you guys have tested, but can we make sure that if the software side needs to change the value before host transaction ever starts that this works correctly? It looks like it should.

@tjaychen your understanding is correct. As I've run regression test with mp 100, they are all passing. But, I am not sure if this corner case was covered by the regression. I will clarify with @weicaiyang once @eunchan confirms this HW fix.

I'm sure you guys have tested, but can we make sure that if the software side needs to change the value before host transaction ever starts that this works correctly? It looks like it should.

CSR addr_4b_en could be updated before or after host sends transactions. But it can be only updated when spi interface is idle. I suppose we don't have race condition that both HW and SW update it at the same time.
https://cs.opensource.google/opentitan/opentitan/+/master:hw/ip/spi_device/dv/env/seq_lib/spi_device_pass_base_vseq.sv;l=323?q=addr_4b_en&ss=opentitan%2Fopentitan:hw%2F&start=11

eunchan · 2022-10-07T20:42:39Z

I think I am OK with this change as this PR removes the issue (short CSb inactive rolls back the addr_4b value). But I think we may use general CDC logic (primitives) for readers to understand more easily.

+1 for merging this. I will delegate to @tjaychen to final stamp. :)

tjaychen · 2022-10-07T21:19:42Z

I think I am OK with this change as this PR removes the issue (short CSb inactive rolls back the addr_4b value). But I think we may use general CDC logic (primitives) for readers to understand more easily.

+1 for merging this. I will delegate to @tjaychen to final stamp. :)

ah yeah agreed with @eunchan if we can re-use cdc primitives it would be better. Which ones did you have in mind?

eunchan · 2022-10-07T22:23:55Z

I think I am OK with this change as this PR removes the issue (short CSb inactive rolls back the addr_4b value). But I think we may use general CDC logic (primitives) for readers to understand more easily.
+1 for merging this. I will delegate to @tjaychen to final stamp. :)

ah yeah agreed with @eunchan if we can re-use cdc primitives it would be better. Which ones did you have in mind?

prim_sync_reqack_data would be enough I think.

tjaychen · 2022-10-07T22:30:07Z

ahhh, are you thinking about using the ack kind of as the "lock" condition?

eunchan · 2022-10-07T22:40:56Z

Kind of. I assume SW updates addr_4b only when it restores the config after power cycles. So it is one-time program only. In that case, asynchronous FIFO is too much in my opinion. By using reqack_data, the `src_ack` comes from `dst_ack`, the acceptance indicator @ SPI_CLK. We could create a logic showing pending bit (connecting to `src_req` and cleared by `src_ack`) and make it visible to SW. Then SW knows when the value has been accepted (committed).

…

On Oct 7, 2022, at 3:30 PM, tjaychen ***@***.***> wrote: ahhh, are you thinking about using the ack kind of as the "lock" condition? — Reply to this email directly, view it on GitHub <#15326 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJDG3S5BJAWJDJN2EYCF43WCCP7TANCNFSM6AAAAAAQ7DK6PQ>. You are receiving this because you were mentioned.

eunchan · 2022-10-07T22:41:53Z

Honestly, I feel guilty by designing this logic poorly at the beginning. :( I didn't think deeply at that time (incl. CSb timing.. ) Thanks for fixing the logic, Joshua!

tjaychen · 2022-10-07T22:50:02Z

Kind of. I assume SW updates addr_4b only when it restores the config after power cycles. So it is one-time program only. In that case, asynchronous FIFO is too much in my opinion. By using reqack_data, the src_ack comes from dst_ack, the acceptance indicator @ SPI_CLK. We could create a logic showing pending bit (connecting to src_req and cleared by src_ack) and make it visible to SW. Then SW knows when the value has been accepted (committed).
…
On Oct 7, 2022, at 3:30 PM, tjaychen @.***> wrote: ahhh, are you thinking about using the ack kind of as the "lock" condition? — Reply to this email directly, view it on GitHub <#15326 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJDG3S5BJAWJDJN2EYCF43WCCP7TANCNFSM6AAAAAAQ7DK6PQ. You are receiving this because you were mentioned.

i would "probably" be careful about assuming that it's one-time only. I know that's the direction Alex gave also, but you never know with these things...something could show up that we completely did not anticipate.

tjaychen · 2022-10-07T22:50:23Z

Honestly, I feel guilty by designing this logic poorly at the beginning. :( I didn't think deeply at that time (incl. CSb timing.. ) Thanks for fixing the logic, Joshua!

ah no worries. We did not exactly have a lot of time, and there were many complicated things in this block :)

eunchan · 2022-10-07T22:54:01Z

Yeah.. That’s possible also. Good thing is, the request can be accepted in a SPI transaction. So, SW may updated frequently without any harm if they want.

…

On Oct 7, 2022, at 3:50 PM, tjaychen ***@***.***> wrote: Kind of. I assume SW updates addr_4b only when it restores the config after power cycles. So it is one-time program only. In that case, asynchronous FIFO is too much in my opinion. By using reqack_data, the src_ack comes from dst_ack, the acceptance indicator @ SPI_CLK. We could create a logic showing pending bit (connecting to src_req and cleared by src_ack) and make it visible to SW. Then SW knows when the value has been accepted (committed). … <x-msg://3/#> On Oct 7, 2022, at 3:30 PM, tjaychen @.***> wrote: ahhh, are you thinking about using the ack kind of as the "lock" condition? — Reply to this email directly, view it on GitHub <#15326 (comment) <#15326 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJDG3S5BJAWJDJN2EYCF43WCCP7TANCNFSM6AAAAAAQ7DK6PQ <https://github.com/notifications/unsubscribe-auth/AAJDG3S5BJAWJDJN2EYCF43WCCP7TANCNFSM6AAAAAAQ7DK6PQ>. You are receiving this because you were mentioned. i would "probably" be careful about assuming that it's one-time only. I know that's the direction Alex gave also, but you never know with these things...something could show up that we completely did not anticipate. — Reply to this email directly, view it on GitHub <#15326 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJDG3VREOTKX3DPNLMNRNTWCCSKJANCNFSM6AAAAAAQ7DK6PQ>. You are receiving this because you were mentioned.

weicaiyang · 2022-10-12T19:16:17Z

@tjaychen @eunchan can we merge this? If we want to make a reusable prim for this, we could do it in another PR. WDYT?

eunchan · 2022-10-12T19:55:07Z

I agree, delegating the approval to @tjaychen

…

On Oct 12, 2022, at 12:16 PM, weicaiyang ***@***.***> wrote: @tjaychen <https://github.com/tjaychen> @eunchan <https://github.com/eunchan> can we merge this? If we want to make a reusable prim for this, we could do it in another PR. WDYT? — Reply to this email directly, view it on GitHub <#15326 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJDG3V2LQKLZHB7WRS44QTWC4FAZANCNFSM6AAAAAAQ7DK6PQ>. You are receiving this because you were mentioned.

tjaychen · 2022-10-13T16:51:04Z

sounds good.

tjaychen · 2022-10-13T16:51:46Z

@eunchan can you approve if it looks good to you? It looks okay to me, but I think you guys had discussed the implementation and probably know the details better than I do.

jeoongp requested a review from eunchan as a code owner October 6, 2022 23:50

github-actions bot reviewed Oct 6, 2022

View reviewed changes

hw/ip/spi_device/rtl/spid_addr_4b.sv Outdated Show resolved Hide resolved

hw/ip/spi_device/rtl/spid_addr_4b.sv Outdated Show resolved Hide resolved

jeoongp force-pushed the spi_flash_fix branch from 125d946 to bcb49d1 Compare October 6, 2022 23:54

github-actions bot reviewed Oct 6, 2022

View reviewed changes

hw/ip/spi_device/rtl/spid_addr_4b.sv Outdated Show resolved Hide resolved

hw/ip/spi_device/rtl/spid_addr_4b.sv Outdated Show resolved Hide resolved

jeoongp force-pushed the spi_flash_fix branch from bcb49d1 to fd023ca Compare October 7, 2022 00:20

eunchan reviewed Oct 7, 2022

View reviewed changes

tjaychen reviewed Oct 7, 2022

View reviewed changes

eunchan approved these changes Oct 13, 2022

View reviewed changes

eunchan merged commit 0681ffa into lowRISC:master Oct 13, 2022

This was referenced Oct 13, 2022

[spi_device] Document update needed for flash/passthrough mode #15463

Closed

[spi_device] EN4B/EX4B may fail if the next item comes very shortly #14940

Closed

jeoongp mentioned this pull request Oct 18, 2022

[spi_device] SW fails to update addr_4b_en in the middle of sim #15543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPI_DEVICE|SPI_FLASH] Added a write-protection on en4b/ex4b to avoid… #15326

[SPI_DEVICE|SPI_FLASH] Added a write-protection on en4b/ex4b to avoid… #15326

jeoongp commented Oct 6, 2022 •

edited

Loading

eunchan Oct 7, 2022

jeoongp Oct 7, 2022

tjaychen Oct 7, 2022

jeoongp Oct 7, 2022

weicaiyang Oct 7, 2022

eunchan commented Oct 7, 2022

tjaychen commented Oct 7, 2022

eunchan commented Oct 7, 2022

tjaychen commented Oct 7, 2022

eunchan commented Oct 7, 2022 via email

eunchan commented Oct 7, 2022

tjaychen commented Oct 7, 2022

tjaychen commented Oct 7, 2022

eunchan commented Oct 7, 2022 via email

weicaiyang commented Oct 12, 2022

eunchan commented Oct 12, 2022 via email

tjaychen commented Oct 13, 2022

tjaychen commented Oct 13, 2022

[SPI_DEVICE|SPI_FLASH] Added a write-protection on en4b/ex4b to avoid… #15326

[SPI_DEVICE|SPI_FLASH] Added a write-protection on en4b/ex4b to avoid… #15326

Conversation

jeoongp commented Oct 6, 2022 • edited Loading

eunchan Oct 7, 2022

Choose a reason for hiding this comment

jeoongp Oct 7, 2022

Choose a reason for hiding this comment

tjaychen Oct 7, 2022

Choose a reason for hiding this comment

jeoongp Oct 7, 2022

Choose a reason for hiding this comment

weicaiyang Oct 7, 2022

Choose a reason for hiding this comment

eunchan commented Oct 7, 2022

tjaychen commented Oct 7, 2022

eunchan commented Oct 7, 2022

tjaychen commented Oct 7, 2022

eunchan commented Oct 7, 2022 via email

eunchan commented Oct 7, 2022

tjaychen commented Oct 7, 2022

tjaychen commented Oct 7, 2022

eunchan commented Oct 7, 2022 via email

weicaiyang commented Oct 12, 2022

eunchan commented Oct 12, 2022 via email

tjaychen commented Oct 13, 2022

tjaychen commented Oct 13, 2022

jeoongp commented Oct 6, 2022 •

edited

Loading