Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add fall through comment ......................................................................
Patch Set 4: Code-Review+2
Patch Set 3:
Patch Set 3: Code-Review-1
(2 comments)
Patch Set 3: Code-Review-1
If you change the code flow, please test (preferably >1000 times). Unless there is documentation that states that after reading `CONF` it will keep that value, we don't know that for sure. I know that the code really makes it look like this is the way the hardware works, but IMHO, it happens too often that some tool tells us to fix something and we break it instead.
Definitely agree. I'd update the commit message as well.
The difference may be very subtle, but if the cursed sand (hardware) actually care about that register being read twice, things can break. So, even if it seems to be very minor, let's keep the original behavior.
So I did a bit of digging and found the technical reference manual for this chip [0]. There is a state machine on page 281 that describes the control flow for these transitions, and if we are trying to move to the ACCESS state (which is the name of this function) then the fall through from INIT_MEM -> CONF looks intentional, since that is the only way to get to ACCESS. There is another function move_to_config_state() in this file that has a similar (but explicit) fall through, so judging from that I believe this fall through is intentional as well.
Ah, documentation, nice :) Though, it's documenting the hardware state machine. I think, it actually shows that it doesn't matter if we break. The `break` would make us read the hardware state again. The diagram shows us that it would only leave the `Config` state on an explicit `Go` from us. So re-reading the hardware state should provide the same value, and we'd give the `Go`.