Angel Pons 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: 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.
https://review.coreboot.org/c/coreboot/+/34296/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34296/3//COMMIT_MSG@7 PS3, Line 7: Add missing break statement Is it really missing, or is it intentionally not there?
https://review.coreboot.org/c/coreboot/+/34296/3/src/soc/rockchip/rk3288/sdr... File src/soc/rockchip/rk3288/sdram.c:
https://review.coreboot.org/c/coreboot/+/34296/3/src/soc/rockchip/rk3288/sdr... PS3, Line 911: break; To not change the code flow, please add a "fallthrough comment": a comment stating this fallthrough is intentional, so that tools don't complain.