Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34633 )
Change subject: soc/samsung/exynos5420: Refactor fimd vidtcon access ......................................................................
soc/samsung/exynos5420: Refactor fimd vidtcon access
Accessing the higher vidtcon variables using pointer arithmetic from the lower address FIMD_CTRL struct is undefined behaviour, since pointers manipulations are not allowed outside the objects they point to. The standard-blessed way is to perform the arithmetic using integer addresses first, and then convert that to a pointer. The end result is the same, but avoids the risk of unsafe optimizations from an over-zealous compiler.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1402096, 1402124, 1402131, 1402169 Change-Id: I13ed23836e8e9076ae0bfd88c05c4f2badac9c49 --- M src/soc/samsung/exynos5420/fimd.c M src/soc/samsung/exynos5420/include/soc/dp.h M src/soc/samsung/exynos5420/include/soc/fimd.h 3 files changed, 9 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/34633/1
diff --git a/src/soc/samsung/exynos5420/fimd.c b/src/soc/samsung/exynos5420/fimd.c index 756d2fb..2b3552a 100644 --- a/src/soc/samsung/exynos5420/fimd.c +++ b/src/soc/samsung/exynos5420/fimd.c @@ -317,9 +317,10 @@ void exynos_fimd_lcd_init(vidinfo_t *vid) { unsigned int cfg = 0, rgb_mode; - unsigned int offset; + struct exynos_fb *fimd;
- offset = exynos_fimd_get_base_offset(); + fimd = (void *)(FIMD_CTRL_ADDR + EXYNOS5_LCD_IF_BASE_OFFSET); + printk(BIOS_SPEW, "%s\n", __func__); exynos5_set_system_display();
@@ -349,19 +350,19 @@ if (!vid->vl_dp) cfg |= EXYNOS_VIDCON1_IVDEN_INVERT;
- lwritel(cfg, &FIMD_CTRL->vidcon1 + offset); + lwritel(cfg, &fimd->vidcon1);
/* set timing */ cfg = EXYNOS_VIDTCON0_VFPD(vid->vl_vfpd - 1); cfg |= EXYNOS_VIDTCON0_VBPD(vid->vl_vbpd - 1); cfg |= EXYNOS_VIDTCON0_VSPW(vid->vl_vspw - 1); - lwritel(cfg, &FIMD_CTRL->vidtcon0 + offset); + lwritel(cfg, &fimd->vidtcon0);
cfg = EXYNOS_VIDTCON1_HFPD(vid->vl_hfpd - 1); cfg |= EXYNOS_VIDTCON1_HBPD(vid->vl_hbpd - 1); cfg |= EXYNOS_VIDTCON1_HSPW(vid->vl_hspw - 1);
- lwritel(cfg, &FIMD_CTRL->vidtcon1 + offset); + lwritel(cfg, &fimd->vidtcon1);
/* set lcd size */ cfg = EXYNOS_VIDTCON2_HOZVAL(vid->vl_col - 1) | @@ -369,7 +370,7 @@ EXYNOS_VIDTCON2_HOZVAL_E(vid->vl_col - 1) | EXYNOS_VIDTCON2_LINEVAL_E(vid->vl_row - 1);
- lwritel(cfg, &FIMD_CTRL->vidtcon2 + offset); + lwritel(cfg, &fimd->vidtcon2); }
/* set display mode */ diff --git a/src/soc/samsung/exynos5420/include/soc/dp.h b/src/soc/samsung/exynos5420/include/soc/dp.h index 28db73a..6b33a76 100644 --- a/src/soc/samsung/exynos5420/include/soc/dp.h +++ b/src/soc/samsung/exynos5420/include/soc/dp.h @@ -884,11 +884,6 @@ /* LCD IF register offset */ #define EXYNOS5_LCD_IF_BASE_OFFSET 0x20000
-static inline u32 exynos_fimd_get_base_offset(void) -{ - return EXYNOS5_LCD_IF_BASE_OFFSET/4; -} - /* * Register offsets */ diff --git a/src/soc/samsung/exynos5420/include/soc/fimd.h b/src/soc/samsung/exynos5420/include/soc/fimd.h index d9d86cb..3e9d6a4 100644 --- a/src/soc/samsung/exynos5420/include/soc/fimd.h +++ b/src/soc/samsung/exynos5420/include/soc/fimd.h @@ -136,7 +136,8 @@ #define OSD_RIGHTBOTX_F_OFFSET 11 #define OSD_RIGHTBOTY_F_OFFSET 0
-#define FIMD_CTRL ((struct exynos_fb *)0x14400000) +#define FIMD_CTRL_ADDR 0x14400000 +#define FIMD_CTRL ((struct exynos_fb *)FIMD_CTRL_ADDR)
/* from u-boot fb.h. It needs to be merged with these dp structs maybe. */ enum {
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34633 )
Change subject: soc/samsung/exynos5420: Refactor fimd vidtcon access ......................................................................
Patch Set 1: Code-Review+2
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34633 )
Change subject: soc/samsung/exynos5420: Refactor fimd vidtcon access ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
When doing the address arithmetic, this patch assumes there is an exynos_fb struct located at that higher address (as opposed to just the vidtcon variables). Do you know if this a safe assumption? I'm not quite sure of the memory layout.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34633 )
Change subject: soc/samsung/exynos5420: Refactor fimd vidtcon access ......................................................................
Patch Set 1:
When doing the address arithmetic, this patch assumes there is an exynos_fb struct located at that higher address (as opposed to just the vidtcon variables). Do you know if this a safe assumption? I'm not quite sure of the memory layout.
No, it looks like only a select amount of registers are mirrored at the other address. Still, I don't think that's a problem here. As long as the code doesn't access the other members, it's fine. This SoC port is ancient anyway, don't invest too much time in it.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34633 )
Change subject: soc/samsung/exynos5420: Refactor fimd vidtcon access ......................................................................
soc/samsung/exynos5420: Refactor fimd vidtcon access
Accessing the higher vidtcon variables using pointer arithmetic from the lower address FIMD_CTRL struct is undefined behaviour, since pointers manipulations are not allowed outside the objects they point to. The standard-blessed way is to perform the arithmetic using integer addresses first, and then convert that to a pointer. The end result is the same, but avoids the risk of unsafe optimizations from an over-zealous compiler.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1402096, 1402124, 1402131, 1402169 Change-Id: I13ed23836e8e9076ae0bfd88c05c4f2badac9c49 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34633 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/soc/samsung/exynos5420/fimd.c M src/soc/samsung/exynos5420/include/soc/dp.h M src/soc/samsung/exynos5420/include/soc/fimd.h 3 files changed, 9 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/soc/samsung/exynos5420/fimd.c b/src/soc/samsung/exynos5420/fimd.c index 756d2fb..2b3552a 100644 --- a/src/soc/samsung/exynos5420/fimd.c +++ b/src/soc/samsung/exynos5420/fimd.c @@ -317,9 +317,10 @@ void exynos_fimd_lcd_init(vidinfo_t *vid) { unsigned int cfg = 0, rgb_mode; - unsigned int offset; + struct exynos_fb *fimd;
- offset = exynos_fimd_get_base_offset(); + fimd = (void *)(FIMD_CTRL_ADDR + EXYNOS5_LCD_IF_BASE_OFFSET); + printk(BIOS_SPEW, "%s\n", __func__); exynos5_set_system_display();
@@ -349,19 +350,19 @@ if (!vid->vl_dp) cfg |= EXYNOS_VIDCON1_IVDEN_INVERT;
- lwritel(cfg, &FIMD_CTRL->vidcon1 + offset); + lwritel(cfg, &fimd->vidcon1);
/* set timing */ cfg = EXYNOS_VIDTCON0_VFPD(vid->vl_vfpd - 1); cfg |= EXYNOS_VIDTCON0_VBPD(vid->vl_vbpd - 1); cfg |= EXYNOS_VIDTCON0_VSPW(vid->vl_vspw - 1); - lwritel(cfg, &FIMD_CTRL->vidtcon0 + offset); + lwritel(cfg, &fimd->vidtcon0);
cfg = EXYNOS_VIDTCON1_HFPD(vid->vl_hfpd - 1); cfg |= EXYNOS_VIDTCON1_HBPD(vid->vl_hbpd - 1); cfg |= EXYNOS_VIDTCON1_HSPW(vid->vl_hspw - 1);
- lwritel(cfg, &FIMD_CTRL->vidtcon1 + offset); + lwritel(cfg, &fimd->vidtcon1);
/* set lcd size */ cfg = EXYNOS_VIDTCON2_HOZVAL(vid->vl_col - 1) | @@ -369,7 +370,7 @@ EXYNOS_VIDTCON2_HOZVAL_E(vid->vl_col - 1) | EXYNOS_VIDTCON2_LINEVAL_E(vid->vl_row - 1);
- lwritel(cfg, &FIMD_CTRL->vidtcon2 + offset); + lwritel(cfg, &fimd->vidtcon2); }
/* set display mode */ diff --git a/src/soc/samsung/exynos5420/include/soc/dp.h b/src/soc/samsung/exynos5420/include/soc/dp.h index 28db73a..6b33a76 100644 --- a/src/soc/samsung/exynos5420/include/soc/dp.h +++ b/src/soc/samsung/exynos5420/include/soc/dp.h @@ -884,11 +884,6 @@ /* LCD IF register offset */ #define EXYNOS5_LCD_IF_BASE_OFFSET 0x20000
-static inline u32 exynos_fimd_get_base_offset(void) -{ - return EXYNOS5_LCD_IF_BASE_OFFSET/4; -} - /* * Register offsets */ diff --git a/src/soc/samsung/exynos5420/include/soc/fimd.h b/src/soc/samsung/exynos5420/include/soc/fimd.h index d9d86cb..3e9d6a4 100644 --- a/src/soc/samsung/exynos5420/include/soc/fimd.h +++ b/src/soc/samsung/exynos5420/include/soc/fimd.h @@ -136,7 +136,8 @@ #define OSD_RIGHTBOTX_F_OFFSET 11 #define OSD_RIGHTBOTY_F_OFFSET 0
-#define FIMD_CTRL ((struct exynos_fb *)0x14400000) +#define FIMD_CTRL_ADDR 0x14400000 +#define FIMD_CTRL ((struct exynos_fb *)FIMD_CTRL_ADDR)
/* from u-boot fb.h. It needs to be merged with these dp structs maybe. */ enum {
-
Jacob Garber (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)