Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add missing break statement ......................................................................
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.
[0]: http://rockchip.fr/Rockchip%20RK3288%20TRM%20V1.0%20Part%201-System%20and%20...