Change in coreboot[master]: soc/mediatek: Change DSI init commands to take flexible length array
Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 5 files changed, 41 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/34778/1
diff --git a/src/mainboard/google/kukui/Makefile.inc b/src/mainboard/google/kukui/Makefile.inc index e34bff1..4f5a201 100644 --- a/src/mainboard/google/kukui/Makefile.inc +++ b/src/mainboard/google/kukui/Makefile.inc @@ -1,4 +1,5 @@ subdirs-y += sdram_params/ +subdirs-y += panel_params/
bootblock-y += boardid.c bootblock-y += bootblock.c diff --git a/src/mainboard/google/kukui/mainboard.c b/src/mainboard/google/kukui/mainboard.c index 6c98689..cd32385 100644 --- a/src/mainboard/google/kukui/mainboard.c +++ b/src/mainboard/google/kukui/mainboard.c @@ -129,7 +129,7 @@ MIPI_DSI_MODE_VIDEO_SYNC_PULSE | MIPI_DSI_MODE_LPM); if (mtk_dsi_init(mipi_dsi_flags, MIPI_DSI_FMT_RGB888, 4, edid, - panel->init) < 0) { + (const struct lcm_init_command *)panel->init) < 0) { printk(BIOS_ERR, "%s: Failed in DSI init.\n", __func__); return false; } diff --git a/src/mainboard/google/kukui/panel.h b/src/mainboard/google/kukui/panel.h index 80dbdb8..a986934 100644 --- a/src/mainboard/google/kukui/panel.h +++ b/src/mainboard/google/kukui/panel.h @@ -23,27 +23,28 @@ const char *name; /* human readable name */ void (*power_on)(void); /* Callback to turn on panel */ struct edid edid; /* edid info of this panel */ - struct lcm_init_table init[]; /* table of init commands */ + char init[]; /* should be a packed array of lcm_init_command */ };
/* Returns the panel description from given ID. */ extern struct panel_description *get_panel_description(int panel_id);
-#define INIT_DCS_CMD(...) { \ - .cmd = LCM_DCS_CMD, \ - .len = sizeof((u8[]){__VA_ARGS__}), \ - .data = {__VA_ARGS__} } +#define INIT_DCS_CMD(...) \ + LCM_DCS_CMD, \ + sizeof((u8[]){__VA_ARGS__}), \ + __VA_ARGS__
-#define INIT_GENERIC_CMD(...) { \ - .cmd = LCM_GENERIC_CMD, \ - .len = sizeof((u8[]){__VA_ARGS__}), \ - .data = {__VA_ARGS__} } +#define INIT_GENERIC_CMD(...) \ + LCM_GENERIC_CMD, \ + sizeof((u8[]){__VA_ARGS__}), \ + __VA_ARGS__
-#define INIT_DELAY_CMD(delay) { \ - .cmd = LCM_DELAY_CMD,\ - .len = delay, } +#define INIT_DELAY_CMD(delay) \ + LCM_DELAY_CMD, \ + (delay)
-#define INIT_END_CMD { .cmd = LCM_END_CMD, } +#define INIT_END_CMD \ + LCM_END_CMD
/* GPIO names */ #define GPIO_LCM_RST_1V8 GPIO(LCM_RST) /* 45 */ diff --git a/src/soc/mediatek/common/dsi.c b/src/soc/mediatek/common/dsi.c index c452fd1..205ff08 100644 --- a/src/soc/mediatek/common/dsi.c +++ b/src/soc/mediatek/common/dsi.c @@ -241,10 +241,10 @@ return false; }
-static void mtk_dsi_cmdq(u8 *data, u8 len, u32 type) +static void mtk_dsi_cmdq(const u8 *data, u8 len, u32 type) { struct stopwatch sw; - u8 *tx_buf = data; + const u8 *tx_buf = data; u8 cmdq_size; u32 reg_val, cmdq_mask, i, config, cmdq_off, intsta_0;
@@ -298,12 +298,19 @@ printk(BIOS_ERR, "dsi send cmd time-out(400uS)\n"); }
-static void mtk_dsi_send_init_commands(struct lcm_init_table *init_cmd) +static void mtk_dsi_send_init_commands(const struct lcm_init_command *cmds) { - assert(init_cmd); + assert(cmds);
- for (; init_cmd->cmd != LCM_END_CMD; init_cmd++) { - u32 cmd = init_cmd->cmd, len = init_cmd->len; + /* The cmds were packed as a single data buffer so we can't simply + * iterate by cmd++; instead we have to parse and scan. + */ + const u8 *buf = (const void *)cmds; + + for (; cmds->cmd != LCM_END_CMD; cmds = (const void *)buf) { + buf += sizeof(*cmds); + + u32 cmd = cmds->cmd, len = cmds->len; u32 type;
switch (cmd) { @@ -350,13 +357,13 @@ return;
} - assert(len <= sizeof(init_cmd->data)); - mtk_dsi_cmdq(init_cmd->data, len, type); + buf += len; + mtk_dsi_cmdq(cmds->data, len, type); } }
-int mtk_dsi_init(u32 mode_flags, u32 format, u32 lanes, - const struct edid *edid, struct lcm_init_table *init_cmd) +int mtk_dsi_init(u32 mode_flags, u32 format, u32 lanes, const struct edid *edid, + const struct lcm_init_command *init_cmds) { int data_rate; u32 bpp = mtk_dsi_get_bits_per_pixel(format); @@ -373,8 +380,8 @@ mtk_dsi_clk_hs_mode_disable(); mtk_dsi_config_vdo_timing(mode_flags, format, lanes, edid, &phy_timing); mtk_dsi_clk_hs_mode_enable(); - if (init_cmd) - mtk_dsi_send_init_commands(init_cmd); + if (init_cmds) + mtk_dsi_send_init_commands(init_cmds); mtk_dsi_set_mode(mode_flags); mtk_dsi_start();
diff --git a/src/soc/mediatek/common/include/soc/dsi_common.h b/src/soc/mediatek/common/include/soc/dsi_common.h index 8597b60..4c26946 100644 --- a/src/soc/mediatek/common/include/soc/dsi_common.h +++ b/src/soc/mediatek/common/include/soc/dsi_common.h @@ -40,16 +40,16 @@ u8 clk_hs_exit; };
-/* Definitions for cmd in lcm_init_table */ +/* Definitions for cmd in lcm_init_command */ #define LCM_END_CMD 0 #define LCM_DELAY_CMD 1 #define LCM_GENERIC_CMD 2 #define LCM_DCS_CMD 3
-struct lcm_init_table { - u32 cmd; - u32 len; - u8 data[7]; +struct lcm_init_command { + u8 cmd; + u8 len; + u8 data[]; };
/* Functions that each SOC should provide. */ @@ -63,6 +63,6 @@ /* Public API provided in common/dsi.c */ int mtk_dsi_bpp_from_format(u32 format); int mtk_dsi_init(u32 mode_flags, u32 format, u32 lanes, const struct edid *edid, - struct lcm_init_table *init_cmd); + const struct lcm_init_command *init_cmds);
#endif // SOC_MEDIATEK_DSI_COMMON_H
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 1:
@jwerner, FYI I'm not sure yet if we should go with this.
Hello Yu-Ping Wu, yongqiang niu, Julius Werner, You-Cheng Syu, jitao shi, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34778
to look at the new patch set (#2).
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 4 files changed, 27 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/34778/2
Hello Yu-Ping Wu, yongqiang niu, Julius Werner, You-Cheng Syu, jitao shi, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34778
to look at the new patch set (#3).
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 4 files changed, 27 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/34778/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/3/src/mainboard/google/kukui/... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/3/src/mainboard/google/kukui/... PS3, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/3/src/mainboard/google/kukui/... PS3, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/3/src/mainboard/google/kukui/... PS3, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/4/src/mainboard/google/kukui/... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/4/src/mainboard/google/kukui/... PS4, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/4/src/mainboard/google/kukui/... PS4, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/4/src/mainboard/google/kukui/... PS4, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Hello Yu-Ping Wu, yongqiang niu, Julius Werner, You-Cheng Syu, jitao shi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34778
to look at the new patch set (#7).
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 4 files changed, 27 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/34778/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/8/src/mainboard/google/kukui/... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/8/src/mainboard/google/kukui/... PS8, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/8/src/mainboard/google/kukui/... PS8, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/8/src/mainboard/google/kukui/... PS8, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/9/src/mainboard/google/kukui/... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/9/src/mainboard/google/kukui/... PS9, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/9/src/mainboard/google/kukui/... PS9, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/9/src/mainboard/google/kukui/... PS9, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/10/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/10/src/mainboard/google/kukui... PS10, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/10/src/mainboard/google/kukui... PS10, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/10/src/mainboard/google/kukui... PS10, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Nicolas Boichat has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 11:
(just a rebase on latest master with no change)
hungte: Can you please help carry https://review.coreboot.org/c/coreboot/+/34733/4 as part of this stack now? (feel free to split it and squash into the other CLs, if that's easier to maintain).
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/11/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/11/src/mainboard/google/kukui... PS11, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/11/src/mainboard/google/kukui... PS11, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/11/src/mainboard/google/kukui... PS11, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Hello Yu-Ping Wu, Nicolas Boichat, yongqiang niu, Julius Werner, You-Cheng Syu, jitao shi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34778
to look at the new patch set (#12).
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 4 files changed, 27 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/34778/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/12/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/12/src/mainboard/google/kukui... PS12, Line 33: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/12/src/mainboard/google/kukui... PS12, Line 38: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/12/src/mainboard/google/kukui... PS12, Line 43: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34778/12/src/soc/mediatek/common/ds... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34778/12/src/soc/mediatek/common/ds... PS12, Line 333: */ Please use one of the allowed comment styles from the coding style.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 33: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 38: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 43: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 13: Code-Review+2
(4 comments)
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 132: (const struct lcm_init_command *)panel->init) < 0) { Just change the function to take a (u8 *) instead, you need casts in there anyway.
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 27: char u8
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 36: __VA_ARGS__ Woah... this is one of those "just crazy enough it could work" things. ^^ I thought about packing it better but I didn't think you could make it the tables look that clean in the end. Very neat!
https://review.coreboot.org/c/coreboot/+/34778/13/src/soc/mediatek/common/ds... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34778/13/src/soc/mediatek/common/ds... PS13, Line 347: buf += sizeof(*init); nit: why not do the whole
buf += sizeof(*init) + len;
at the end?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/14/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/14/src/mainboard/google/kukui... PS14, Line 33: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/14/src/mainboard/google/kukui... PS14, Line 38: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/14/src/mainboard/google/kukui... PS14, Line 43: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Hello Yu-Ping Wu, Nicolas Boichat, Julius Werner, yongqiang niu, Jacob Garber, You-Cheng Syu, jitao shi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34778
to look at the new patch set (#15).
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/mainboard.c M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 4 files changed, 27 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/34778/15
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/15/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/15/src/mainboard/google/kukui... PS15, Line 33: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/15/src/mainboard/google/kukui... PS15, Line 38: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/15/src/mainboard/google/kukui... PS15, Line 43: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/16/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/16/src/mainboard/google/kukui... PS16, Line 33: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/16/src/mainboard/google/kukui... PS16, Line 38: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/16/src/mainboard/google/kukui... PS16, Line 43: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/17/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/17/src/mainboard/google/kukui... PS17, Line 33: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/17/src/mainboard/google/kukui... PS17, Line 38: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/17/src/mainboard/google/kukui... PS17, Line 43: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/18/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/18/src/mainboard/google/kukui... PS18, Line 33: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/18/src/mainboard/google/kukui... PS18, Line 38: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/18/src/mainboard/google/kukui... PS18, Line 43: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 19:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/19/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/19/src/mainboard/google/kukui... PS19, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/19/src/mainboard/google/kukui... PS19, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/19/src/mainboard/google/kukui... PS19, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 19:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 132: (const struct lcm_init_command *)panel->init) < 0) {
Just change the function to take a (u8 *) instead, you need casts in there anyway.
Done
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 27: char
u8
Done
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 36: __VA_ARGS__
Woah... this is one of those "just crazy enough it could work" things. […]
But Jenkins is not happy about it and I'm not sure how to solve that...
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 19: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/13/src/mainboard/google/kukui... PS13, Line 36: __VA_ARGS__
But Jenkins is not happy about it and I'm not sure how to solve that...
Ignore Jenkins in this case, that's a warning and not a hard error for a reason.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 21: Code-Review+2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 22:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/22/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/22/src/mainboard/google/kukui... PS22, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/22/src/mainboard/google/kukui... PS22, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/22/src/mainboard/google/kukui... PS22, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 23:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/23/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/23/src/mainboard/google/kukui... PS23, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/23/src/mainboard/google/kukui... PS23, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/23/src/mainboard/google/kukui... PS23, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Hello Yu-Ping Wu, Nicolas Boichat, Julius Werner, yongqiang niu, Jacob Garber, You-Cheng Syu, jitao shi, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34778
to look at the new patch set (#24).
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 3 files changed, 36 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/34778/24
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 24:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34778/12/src/soc/mediatek/common/ds... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34778/12/src/soc/mediatek/common/ds... PS12, Line 333: */
Please use one of the allowed comment styles from the coding style.
Done
https://review.coreboot.org/c/coreboot/+/34778/13/src/soc/mediatek/common/ds... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34778/13/src/soc/mediatek/common/ds... PS13, Line 347: buf += sizeof(*init);
nit: why not do the whole […]
because if command is delay, len must be ignored.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 24:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34778/24/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel.h:
https://review.coreboot.org/c/coreboot/+/34778/24/src/mainboard/google/kukui... PS24, Line 32: #define INIT_DCS_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/24/src/mainboard/google/kukui... PS24, Line 37: #define INIT_GENERIC_CMD(...) \ Macros with complex values should be enclosed in parentheses
https://review.coreboot.org/c/coreboot/+/34778/24/src/mainboard/google/kukui... PS24, Line 42: #define INIT_DELAY_CMD(delay) \ Macros with complex values should be enclosed in parentheses
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
Patch Set 24: Code-Review+2
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34778 )
Change subject: soc/mediatek: Change DSI init commands to take flexible length array ......................................................................
soc/mediatek: Change DSI init commands to take flexible length array
The fixed size of init command in lcm_init_table is wasting lots of space and we should change to packed array since the command buffer already provides length information.
With this change, BOE panel init commands have been reduced from 4848 bytes to 1309 bytes.
BUG=b:80501386,b:117254947 TEST=emerge-kukui coreboot chromeos-bootimage; Boots properly
Change-Id: I359dde8e6f2e1c0983f4677193bb47a7ae497ca6 Signed-off-by: Hung-Te Lin hungte@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/34778 Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/kukui/panel.h M src/soc/mediatek/common/dsi.c M src/soc/mediatek/common/include/soc/dsi_common.h 3 files changed, 36 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/mainboard/google/kukui/panel.h b/src/mainboard/google/kukui/panel.h index e68567d..321e366 100644 --- a/src/mainboard/google/kukui/panel.h +++ b/src/mainboard/google/kukui/panel.h @@ -23,27 +23,28 @@ struct edid edid; /* edid info of this panel */ enum lb_fb_orientation orientation; /* panel orientation */ void (*power_on)(void); /* Callback to turn on panel */ - struct lcm_init_command init[]; /* table of init commands */ + u8 init[]; /* a packed array of lcm_init_command */ };
/* Returns the panel description from given ID. */ extern struct panel_description *get_panel_description(int panel_id);
-#define INIT_DCS_CMD(...) { \ - .cmd = LCM_DCS_CMD, \ - .len = sizeof((u8[]){__VA_ARGS__}), \ - .data = {__VA_ARGS__} } +#define INIT_DCS_CMD(...) \ + LCM_DCS_CMD, \ + sizeof((u8[]){__VA_ARGS__}), \ + __VA_ARGS__
-#define INIT_GENERIC_CMD(...) { \ - .cmd = LCM_GENERIC_CMD, \ - .len = sizeof((u8[]){__VA_ARGS__}), \ - .data = {__VA_ARGS__} } +#define INIT_GENERIC_CMD(...) \ + LCM_GENERIC_CMD, \ + sizeof((u8[]){__VA_ARGS__}), \ + __VA_ARGS__
-#define INIT_DELAY_CMD(delay) { \ - .cmd = LCM_DELAY_CMD,\ - .len = delay, } +#define INIT_DELAY_CMD(delay) \ + LCM_DELAY_CMD, \ + delay
-#define INIT_END_CMD { .cmd = LCM_END_CMD, } +#define INIT_END_CMD \ + LCM_END_CMD
/* GPIO names */ #define GPIO_LCM_RST_1V8 GPIO(LCM_RST) /* 45 */ diff --git a/src/soc/mediatek/common/dsi.c b/src/soc/mediatek/common/dsi.c index 166bc17..679bec8 100644 --- a/src/soc/mediatek/common/dsi.c +++ b/src/soc/mediatek/common/dsi.c @@ -321,12 +321,25 @@ } }
-static void mtk_dsi_send_init_commands(const struct lcm_init_command *init) +static void mtk_dsi_send_init_commands(const u8 *buf) { - if (!init) + if (!buf) return; + const struct lcm_init_command *init = (const void *)buf;
- for (; init->cmd != LCM_END_CMD; init++) { + /* + * The given commands should be in a buffer containing a packed array of + * lcm_init_command and each element may be in variable size so we have + * to parse and scan. + */ + + for (; init->cmd != LCM_END_CMD; init = (const void *)buf) { + /* + * For some commands like DELAY, the init->len should not be + * counted for buf. + */ + buf += sizeof(*init); + u32 cmd = init->cmd, len = init->len; u32 type;
@@ -374,13 +387,13 @@ return;
} - assert(len <= sizeof(init->data)); + buf += len; mtk_dsi_cmdq(init->data, len, type); } }
int mtk_dsi_init(u32 mode_flags, u32 format, u32 lanes, const struct edid *edid, - const struct lcm_init_command *init_commands) + const u8 *init_commands) { int data_rate; u32 bits_per_pixel = mtk_dsi_get_bits_per_pixel(format); diff --git a/src/soc/mediatek/common/include/soc/dsi_common.h b/src/soc/mediatek/common/include/soc/dsi_common.h index c684aaf..9a00d1d 100644 --- a/src/soc/mediatek/common/include/soc/dsi_common.h +++ b/src/soc/mediatek/common/include/soc/dsi_common.h @@ -350,9 +350,9 @@ #define LCM_DCS_CMD 3
struct lcm_init_command { - u16 cmd; - u16 len; - u8 data[8]; + u8 cmd; + u8 len; + u8 data[]; };
/* Functions that each SOC should provide. */ @@ -365,6 +365,6 @@ /* Public API provided in common/dsi.c */ int mtk_dsi_bpp_from_format(u32 format); int mtk_dsi_init(u32 mode_flags, u32 format, u32 lanes, const struct edid *edid, - const struct lcm_init_command *init_commands); + const u8 *init_commands);
#endif /* SOC_MEDIATEK_DSI_COMMON_H */
-
build bot (Jenkins) (Code Review) -
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Nicolas Boichat (Code Review)
-
Paul Menzel (Code Review)