Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add fallthrough comment ......................................................................
soc/rockchip/rk3288: Add fallthrough comment
To break, or not to break, that is the question. -- Hamlet, probably
The case statement for INIT_MEM does not have a break, and so falls through to the one for CONF. Is this intentional, or is it a mistake? Well, it doesn't completely matter, since if a break were added the state would be CONF on the next iteration of the while loop, and so it would execute the CONF case anyway. To be safe however, let's leave the existing control flow and add a comment that the fallthrough is intentional.
Change-Id: I1d0cfea07211c54d6a906f5a7481c2c760f8ef0d Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1291959 --- M src/soc/rockchip/rk3288/sdram.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/34296/1
diff --git a/src/soc/rockchip/rk3288/sdram.c b/src/soc/rockchip/rk3288/sdram.c index 808ff963..f5d1f74 100644 --- a/src/soc/rockchip/rk3288/sdram.c +++ b/src/soc/rockchip/rk3288/sdram.c @@ -908,6 +908,7 @@ while ((read32(&ddr_pctl_regs->stat) & PCTL_STAT_MSK) != CONF) ; + /* fallthrough */ case CONF: write32(&ddr_pctl_regs->sctl, GO_STATE); while ((read32(&ddr_pctl_regs->stat) & PCTL_STAT_MSK)
Hello Angel Pons, Julius Werner, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34296
to look at the new patch set (#2).
Change subject: soc/rockchip/rk3288: Add fall through comment ......................................................................
soc/rockchip/rk3288: Add fall through comment
To break, or not to break, that is the question. -- Hamlet, probably
The case statement for INIT_MEM does not have a break, and so falls through to the one for CONF. Is this intentional, or is it a mistake? Well, it doesn't completely matter, since if a break were added the state would be CONF on the next iteration of the while loop, and so it would execute the CONF case anyway. To be safe however, let's leave the existing control flow and add a comment that the fall through is intentional.
Change-Id: I1d0cfea07211c54d6a906f5a7481c2c760f8ef0d Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1291959 --- M src/soc/rockchip/rk3288/sdram.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/34296/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add fall through comment ......................................................................
Patch Set 2: Code-Review+2
nit: You're right that it should be equivalent, but for consistency I think adding the break would be better.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add fall through comment ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+2
nit: You're right that it should be equivalent, but for consistency I think adding the break would be better.
If that was the original intention, then I agree
Hello Julius Werner, Angel Pons, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34296
to look at the new patch set (#3).
Change subject: soc/rockchip/rk3288: Add missing break statement ......................................................................
soc/rockchip/rk3288: Add missing break statement
To break, or not to break, that is the question. -- Hamlet, probably
The case statement for INIT_MEM does not have a break, and so falls through to the one for CONF. Is this intentional, or is it a mistake? Well, it doesn't completely matter, since if a break were added the state would be CONF on the next iteration of the while loop, and so it would execute the CONF case anyway. Let's add a break statement to be consistent with the rest of the cases.
Change-Id: I1d0cfea07211c54d6a906f5a7481c2c760f8ef0d Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1291959 --- M src/soc/rockchip/rk3288/sdram.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/34296/3
Julius Werner 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+2
HAOUAS Elyes 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+2
Nico Huber 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
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.
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.
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...
Jacob Garber has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add missing break statement ......................................................................
Removed Code-Review+2 by HAOUAS Elyes ehaouas@noos.fr
Jacob Garber has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add missing break statement ......................................................................
Removed Code-Review+2 by Julius Werner jwerner@chromium.org
Hello HAOUAS Elyes, Angel Pons, Julius Werner, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34296
to look at the new patch set (#4).
Change subject: soc/rockchip/rk3288: Add fall through comment ......................................................................
soc/rockchip/rk3288: Add fall through comment
Judging from the state machine on page 281 of the Rockchip RK3288 Technical Reference Manual (Rev 1.0 - Jun 2015), the fall through from the INIT_MEM -> CONF states is intentional, since that is the only way to get to the ACCESS state. Add a comment to explain this.
Change-Id: I1d0cfea07211c54d6a906f5a7481c2c760f8ef0d Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1291959 --- M src/soc/rockchip/rk3288/sdram.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/34296/4
Jacob Garber 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:
(2 comments)
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?
Done
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 […]
Done
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`.
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34296 )
Change subject: soc/rockchip/rk3288: Add fall through comment ......................................................................
soc/rockchip/rk3288: Add fall through comment
Judging from the state machine on page 281 of the Rockchip RK3288 Technical Reference Manual (Rev 1.0 - Jun 2015), the fall through from the INIT_MEM -> CONF states is intentional, since that is the only way to get to the ACCESS state. Add a comment to explain this.
Change-Id: I1d0cfea07211c54d6a906f5a7481c2c760f8ef0d Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1291959 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34296 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/rockchip/rk3288/sdram.c 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/soc/rockchip/rk3288/sdram.c b/src/soc/rockchip/rk3288/sdram.c index 808ff963..53c594a 100644 --- a/src/soc/rockchip/rk3288/sdram.c +++ b/src/soc/rockchip/rk3288/sdram.c @@ -908,6 +908,7 @@ while ((read32(&ddr_pctl_regs->stat) & PCTL_STAT_MSK) != CONF) ; + /* fall through - enter config next to get to access state */ case CONF: write32(&ddr_pctl_regs->sctl, GO_STATE); while ((read32(&ddr_pctl_regs->stat) & PCTL_STAT_MSK)
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Jacob Garber (Code Review)
-
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)