Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operation ......................................................................
soc/nvidia/tegra124: Correct bitwise operation
We want to selectively enable those bits, not overwrite the whole register, so use |=.
Change-Id: Ia8c0ea5a58e25b3b58ed82caba20f8e49a30fb68 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1287070 --- M src/soc/nvidia/tegra124/sor.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/34560/1
diff --git a/src/soc/nvidia/tegra124/sor.c b/src/soc/nvidia/tegra124/sor.c index 52b909e..5f26d1c 100644 --- a/src/soc/nvidia/tegra124/sor.c +++ b/src/soc/nvidia/tegra124/sor.c @@ -351,7 +351,7 @@ reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON || APBDEV_PMC_IO_DPD2_REQ_CODE_DEFAULT_MASK);
- reg_val = up ? APBDEV_PMC_IO_DPD2_REQ_LVDS_ON | + reg_val |= up ? APBDEV_PMC_IO_DPD2_REQ_LVDS_ON | APBDEV_PMC_IO_DPD2_REQ_CODE_DPD_OFF : APBDEV_PMC_IO_DPD2_REQ_LVDS_OFF | APBDEV_PMC_IO_DPD2_REQ_CODE_DPD_ON;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... File src/soc/nvidia/tegra124/sor.c:
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... PS1, Line 351: reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON || That logical or is suspicious...
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... File src/soc/nvidia/tegra124/sor.c:
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... PS1, Line 351: reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON ||
That logical or is suspicious...
The previous &= clears the bits in positions 25, 30, and 31, and then the APBDEV constants in the next line set some of them. Using an &= for this line would clear the whole variable, while an |= re-enables some of those bits instead (that was my reasoning).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... File src/soc/nvidia/tegra124/sor.c:
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... PS1, Line 351: reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON ||
The previous &= clears the bits in positions 25, 30, and 31, and then the APBDEV constants in the ne […]
I was talking about || at end of line 351, and expecting it to only clear bit 0. I agree on the change you made already for line 354.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... File src/soc/nvidia/tegra124/sor.c:
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... PS1, Line 351: reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON ||
I was talking about || at end of line 351, and expecting it to only clear bit 0. […]
Oops, Gerrit's highlighting mixed me up. Thanks, I'll fix that too.
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34560
to look at the new patch set (#2).
Change subject: soc/nvidia/tegra124: Correct bitwise operators ......................................................................
soc/nvidia/tegra124: Correct bitwise operators
We are treating reg_val like a bit mask, so use bitwise or instead of boolean or, and use |= to enable certain bits instead of overwriting the whole variable.
Change-Id: Ia8c0ea5a58e25b3b58ed82caba20f8e49a30fb68 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1287070 --- M src/soc/nvidia/tegra124/sor.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/34560/2
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operators ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... File src/soc/nvidia/tegra124/sor.c:
https://review.coreboot.org/c/coreboot/+/34560/1/src/soc/nvidia/tegra124/sor... PS1, Line 351: reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON ||
Oops, Gerrit's highlighting mixed me up. Thanks, I'll fix that too.
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operators ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34560 )
Change subject: soc/nvidia/tegra124: Correct bitwise operators ......................................................................
soc/nvidia/tegra124: Correct bitwise operators
We are treating reg_val like a bit mask, so use bitwise or instead of boolean or, and use |= to enable certain bits instead of overwriting the whole variable.
Change-Id: Ia8c0ea5a58e25b3b58ed82caba20f8e49a30fb68 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1287070 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34560 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/nvidia/tegra124/sor.c 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/soc/nvidia/tegra124/sor.c b/src/soc/nvidia/tegra124/sor.c index 52b909e..1c151f5 100644 --- a/src/soc/nvidia/tegra124/sor.c +++ b/src/soc/nvidia/tegra124/sor.c @@ -348,10 +348,10 @@ }
reg_val = READL(pmc_base + APBDEV_PMC_IO_DPD2_REQ); - reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON || + reg_val &= ~(APBDEV_PMC_IO_DPD2_REQ_LVDS_ON | APBDEV_PMC_IO_DPD2_REQ_CODE_DEFAULT_MASK);
- reg_val = up ? APBDEV_PMC_IO_DPD2_REQ_LVDS_ON | + reg_val |= up ? APBDEV_PMC_IO_DPD2_REQ_LVDS_ON | APBDEV_PMC_IO_DPD2_REQ_CODE_DPD_OFF : APBDEV_PMC_IO_DPD2_REQ_LVDS_OFF | APBDEV_PMC_IO_DPD2_REQ_CODE_DPD_ON;
-
Jacob Garber (Code Review) -
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)