Hello Taniya Das,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39612
to review the following change.
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/1
diff --git a/src/soc/qualcomm/sc7180/clock.c b/src/soc/qualcomm/sc7180/clock.c index 2eb5830..f9ddeee 100644 --- a/src/soc/qualcomm/sc7180/clock.c +++ b/src/soc/qualcomm/sc7180/clock.c @@ -18,10 +18,12 @@ #include <delay.h> #include <device/mmio.h> #include <soc/clock.h> +#include <string.h> #include <timer.h> #include <types.h>
#define DIV(div) (2 * div - 1) +#define HALF_DIVIDER(div2x) (div2x ? (div2x - 1) : 0)
struct clock_config qup_cfg[] = { { @@ -117,6 +119,30 @@ }, };
+struct mdss_clock_config mdss_extclk_config[] = { + { + "disp_cc_mdss_esc0_clk", + mdss_reg_ptr(struct sc7180_disp_cc, esc0_rcgr, DISP_CC_BASE), + mdss_reg_ptr(struct sc7180_disp_cc, esc0_cbcr, DISP_CC_BASE), + }, + { + "disp_cc_mdss_pclk0_clk", + mdss_reg_ptr(struct sc7180_disp_cc, pclk0_rcgr, DISP_CC_BASE), + mdss_reg_ptr(struct sc7180_disp_cc, pclk0_cbcr, DISP_CC_BASE), + }, + { + "disp_cc_mdss_byte0_clk", + mdss_reg_ptr(struct sc7180_disp_cc, byte0_rcgr, DISP_CC_BASE), + mdss_reg_ptr(struct sc7180_disp_cc, byte0_cbcr, DISP_CC_BASE), + }, + { + "disp_cc_mdss_byte0_intf_clk", + mdss_reg_ptr(struct sc7180_disp_cc, byte0_rcgr, DISP_CC_BASE), + mdss_reg_ptr(struct sc7180_disp_cc, byte0_intf_cbcr, + DISP_CC_BASE), + }, +}; + static int clock_configure_gpll0(void) { setbits32(&gcc->gpll0.user_ctl_u, 1 << SCALE_FREQ_SHFT); @@ -336,6 +362,90 @@ printk(BIOS_INFO, "L3 Frequency bumped to 1.2096(GHz)\n"); }
+static void * mdss_clock_get_reg_addr(const char *clk_name, + uint32_t mdss_clk_type) +{ + void *reg_addr = NULL; + int i; + int num_clks; + + num_clks = sizeof(mdss_extclk_config) / + sizeof(struct mdss_clock_config); + + for (i = 0; i < num_clks; i++) { + if (strcmp(mdss_extclk_config[i].clk_name, clk_name) != 0) + continue; + + if (mdss_clk_type == 0) + reg_addr = (void *)mdss_extclk_config[i].rcgr; + else if(mdss_clk_type == 1) + reg_addr = (void *)mdss_extclk_config[i].cbcr; + } + return reg_addr; +} + +int mdss_clock_configure(const char *clk_name, uint32_t source, + uint32_t divider, uint32_t m, uint32_t n, uint32_t d_2) +{ + struct clock_config mdss_clk_cfg; + struct sc7180_mdss_rcg *mdss_clk; + uint32_t reg_val; + + /* Find out the RCGR address corresponding to the given clock */ + mdss_clk = mdss_clock_get_reg_addr(clk_name, 0); + + /* clk_name not found in the available clock list */ + if (mdss_clk == NULL) + return -1; + + /* Initialize it with received arguments */ + mdss_clk_cfg.hz = 0; + mdss_clk_cfg.src = source; + mdss_clk_cfg.div = HALF_DIVIDER(divider); + mdss_clk_cfg.m = m; + mdss_clk_cfg.n = n; + mdss_clk_cfg.d_2 = d_2; + + /* configure and set the clock */ + reg_val = (mdss_clk_cfg.src << CLK_CTL_CFG_SRC_SEL_SHFT) | + (mdss_clk_cfg.div << CLK_CTL_CFG_SRC_DIV_SHFT); + + write32(&mdss_clk->cfg, reg_val); + + /* Set m/n/d values for a specific clock */ + if (mdss_clk_cfg.m != 0) { + setbits32(&mdss_clk->cfg, + RCG_MODE_DUAL_EDGE << CLK_CTL_CFG_MODE_SHFT); + write32(&mdss->pclk0_m, mdss_clk_cfg.m & CLK_CTL_RCG_MND_BMSK); + write32(&mdss->pclk0_n, ~(mdss_clk_cfg.n-mdss_clk_cfg.m) & + CLK_CTL_RCG_MND_BMSK); + write32(&mdss->pclk0_d, ~(mdss_clk_cfg.d_2) & + CLK_CTL_RCG_MND_BMSK); + } + + /* Commit config to RCG */ + setbits32(&mdss_clk->cmd, BIT(CLK_CTL_CMD_UPDATE_SHFT)); + + return 0; +} + +int mdss_clock_enable(const char *clk_name, uint32_t clk_on) +{ + void *cbcr; + + /* Find out the RCGR address corresponding to the given clock */ + cbcr = mdss_clock_get_reg_addr(clk_name, 1); + + /* clk_name not found in the available clock list */ + if ((cbcr == NULL) || (clk_on == 0)) + return -1; + + /* Enable clock*/ + clock_enable(cbcr); + + return 0; +} + void clock_init(void) { clock_configure_gpll0(); diff --git a/src/soc/qualcomm/sc7180/include/soc/addressmap.h b/src/soc/qualcomm/sc7180/include/soc/addressmap.h index ee640a2..ff05616 100644 --- a/src/soc/qualcomm/sc7180/include/soc/addressmap.h +++ b/src/soc/qualcomm/sc7180/include/soc/addressmap.h @@ -26,6 +26,7 @@ #define TLMM_WEST_TILE_BASE 0x03500000 #define SILVER_PLL_BASE 0x18280000 #define L3_PLL_BASE 0x18284000 +#define DISP_CC_BASE 0x0AF00000
/* * QUP SERIAL ENGINE BASE ADDRESSES diff --git a/src/soc/qualcomm/sc7180/include/soc/clock.h b/src/soc/qualcomm/sc7180/include/soc/clock.h index 26e3d09..e42f9b3 100644 --- a/src/soc/qualcomm/sc7180/include/soc/clock.h +++ b/src/soc/qualcomm/sc7180/include/soc/clock.h @@ -43,6 +43,9 @@
#define SCALE_FREQ_SHFT 11
+#define mdss_reg_ptr(type, mem, base) \ + ((offsetof(type, mem)) + base) + struct sc7180_clock { u32 cbcr; u32 rcg_cmd; @@ -140,6 +143,33 @@ u32 aoss_cc_apcs_misc; };
+struct sc7180_mdss_rcg { + u32 cmd; + u32 cfg; +}; + +struct sc7180_disp_cc { + u8 _res0[0x2004]; + u32 pclk0_cbcr; + u8 _res1[0x2028 - 0x2008]; + u32 byte0_cbcr; + u32 byte0_intf_cbcr; + u8 _res2[0x2038 - 0x2030]; + u32 esc0_cbcr; + u8 _res3[0x2098 - 0x203C]; + struct sc7180_mdss_rcg pclk0_rcgr; + u32 pclk0_m; + u32 pclk0_n; + u32 pclk0_d; + u8 _res4[0x2110 - 0x20AC]; + struct sc7180_mdss_rcg byte0_rcgr; + u8 _res5[0x2148 - 0x2118]; + struct sc7180_mdss_rcg esc0_rcgr; + u8 _res6[0x10000 - 0x2050]; +}; +check_member(sc7180_disp_cc, byte0_cbcr, 0x2028); +check_member(sc7180_disp_cc, esc0_cbcr, 0x2038); + enum clk_ctl_gpll_user_ctl { CLK_CTL_GPLL_PLLOUT_EVEN_BMSK = 0x2, CLK_CTL_GPLL_PLLOUT_MAIN_SHFT = 0, @@ -280,6 +310,7 @@ static struct sc7180_aoss *const aoss = (void *)AOSS_CC_BASE; static struct sc7180_apss_clock *const apss_silver = (void *)SILVER_PLL_BASE; static struct sc7180_apss_clock *const apss_l3 = (void *)L3_PLL_BASE; +static struct sc7180_disp_cc *const mdss = (void *)DISP_CC_BASE;
void clock_init(void); void clock_reset_aop(void); @@ -288,5 +319,8 @@ void clock_configure_qup(int qup, uint32_t hz); void clock_enable_qup(int qup); void clock_configure_dfsr(int qup); +int mdss_clock_configure(const char *clk_name, uint32_t source, + uint32_t divider, uint32_t m, uint32_t n, uint32_t d); +int mdss_clock_enable(const char *sName, uint32_t clk_on);
#endif // __SOC_QUALCOMM_SC7180_CLOCK_H__
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/clo... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/clo... PS1, Line 365: static void * mdss_clock_get_reg_addr(const char *clk_name, "foo * bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/clo... PS1, Line 381: else if(mdss_clk_type == 1) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/inc... PS1, Line 47: ((offsetof(type, mem)) + base) code indent should use tabs where possible
Taniya Das has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/clo... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/clo... PS1, Line 365: static void * mdss_clock_get_reg_addr(const char *clk_name,
"foo * bar" should be "foo *bar"
Done
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/clo... PS1, Line 381: else if(mdss_clk_type == 1)
space required before the open parenthesis '('
Done
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/1/src/soc/qualcomm/sc7180/inc... PS1, Line 47: ((offsetof(type, mem)) + base)
code indent should use tabs where possible
Done
Hello build bot (Jenkins), Taniya Das, mturney mturney, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39612
to look at the new patch set (#2).
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/2
Hello build bot (Jenkins), Taniya Das, mturney mturney, Julius Werner, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39612
to look at the new patch set (#7).
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/7
mturney mturney has uploaded a new patch set (#8) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/8
Hello build bot (Jenkins), Taniya Das, mturney mturney, Julius Werner, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39612
to look at the new patch set (#12).
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/12
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 15:
(8 comments)
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 353: uint32_t mdss_clk_type) It looks like this should fit into 96 characters.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 369: reg_addr = (void *)mdss_extclk_config[i].cbcr; Is it common that this typo is not 0 or 1? Maybe add an else branch with a debug message?
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 374: int Please use CB_SUCCESS and friends.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 410: CLK_CTL_RCG_MND_BMSK); This should fit into 96 characters.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 424: 1 Please add enums for this.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 16: #define DISP_CC_BASE 0x0AF00000 Please align using tabs.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 34: #define mdss_reg_ptr(type, mem, base) \ Macros should be uppercase.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 35: ((offsetof(type, mem)) + base) This fits into one line.
Hello build bot (Jenkins), Taniya Das, mturney mturney, Julius Werner, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39612
to look at the new patch set (#17).
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/17
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 22:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 13: #define HALF_DIVIDER(div2x) (div2x ? (div2x - 1) : 0) I'm unclear what this macro is doing, it might be better to do this operation explicitly in mdss_clock_configure() and put a comment next to it to explain it.
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 109: struct mdss_clock_config mdss_extclk_config[] = { Please do not bind information with string comparisons. Just do something like this:
enum mdss_clock { MDSS_CLK_ESC0 = 0, MDSS_CLK_PCLK0, MDSS_CLK_BYTE0, MDSS_CLK_BYTE0_INTF,
MDSS_CLK_COUNT };
struct sc7180_mnd_clock *mdss_clock[MDSS_CLK_COUNT] = { [MDSS_CLK_ESC0] = &mdss->esc0, ... };
struct u32 *mdss_cbcr[MDSS_CLK_COUNT] = { [MDSS_CLK_ESC0] = &mdss->esc0_cbcr, ... };
...and then below you can do stuff like...
if (clk >= MDSS_CLK_COUNT) return -1;
clock_enable(mdss_cbcr[clk]);
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 214: ; While we're starting to use these functions for non-boot-critical clocks, it would be a good idea to add timeouts to these btw. You can just write this as
if (!wait_ms(clock_is_off(cbcr_addr), 1)) return -1;
with a reasonable maximum timeout (this would be 1 millisecond, you can make it bigger if necessary). Same goes for clock_enable_vote().
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 38: u32 cbcr; Please fit what you're doing into the existing framework rather than rebuilding a copy next to it. The clocks you're adding here are basically sc7180_mnd_clocks and the existing functions like clock_configure seem to fit exactly what you're trying to do, so please use them.
Your one problem is that your clocks don't have the cbcr register right in front of the rest -- fine, then let's change that. The cbcr field here isn't used much anyway, so just take it out of this struct and put it explicitly in front of the struct for the clocks that need it (e.g. sc7180_qupv3_clock and the qup_wrap and qspi stuff in sc7180_gcc).
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 235: struct mdss_clock_config { Please remove this too, I don't know how it got in here.
Taniya Das has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 26:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 13: #define HALF_DIVIDER(div2x) (div2x ? (div2x - 1) : 0)
I'm unclear what this macro is doing, it might be better to do this operation explicitly in mdss_clo […]
Done
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 109: struct mdss_clock_config mdss_extclk_config[] = {
Please do not bind information with string comparisons. Just do something like this: […]
Done
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 214: ;
While we're starting to use these functions for non-boot-critical clocks, it would be a good idea to […]
We would like to take such cleanups in the next project as it does not break any functionality.
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 38: u32 cbcr;
Please fit what you're doing into the existing framework rather than rebuilding a copy next to it. […]
Done
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 235: struct mdss_clock_config {
Please remove this too, I don't know how it got in here.
Done
Taniya Das has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 26:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 353: uint32_t mdss_clk_type)
It looks like this should fit into 96 characters.
The implementation is updated as per the latest comments from Julius.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 369: reg_addr = (void *)mdss_extclk_config[i].cbcr;
Is it common that this typo is not 0 or 1? Maybe add an else branch with a debug message?
The implementation is updated as per the latest comments from Julius.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 374: int
Please use CB_SUCCESS and friends.
We would like to keep such cleanups for the next project as we have passed CS.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 410: CLK_CTL_RCG_MND_BMSK);
This should fit into 96 characters.
Done
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 424: 1
Please add enums for this.
Implementation is updated as per latest comments from Julius.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 16: #define DISP_CC_BASE 0x0AF00000
Please align using tabs.
Done
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 35: ((offsetof(type, mem)) + base)
This fits into one line.
Implementation updated as per latest comments from Julius.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 26:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 374: int
We would like to keep such cleanups for the next project as we have passed CS.
In this case, considering that existing APIs in this file also use raw integers for return values, I think it makes sense to continue doing that for the new additions. According to my understanding cb_err_t is an option for code to use that wants to, not a requirement.
That said, in general this patch is under review and not yet submitted so "sorry we don't want to touch this anymore" isn't really a valid argument.
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 214: ;
We would like to take such cleanups in the next project as it does not break any functionality.
Sorry, I don't understand (or agree with) this. We are still actively adding new feature code here so clearly this project isn't "done" yet. We haven't cut a stabilization branch on the Chrome OS side yet either. This is an older function but it is now being reused in a new context, so while I admittedly could've thought of this in the original review already, it becomes more important now (since we want to make sure that no problem during display initialization can prevent the system from booting).
If you're worried about this causing unintended problems I am okay with using a very large timeout here (say 100ms or something), I don't really care about the exact number, I just want to make sure it cannot hang the boot completely.
Hello build bot (Jenkins), Taniya Das, mturney mturney, Julius Werner, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39612
to look at the new patch set (#27).
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 116 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/27
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 27:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 370: if (mdss_clk_cfg.m != 0) that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 379: ~(mdss_clk_cfg.d_2) & CLK_CTL_RCG_MND_BMSK); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 379: ~(mdss_clk_cfg.d_2) & CLK_CTL_RCG_MND_BMSK); please, no spaces at the start of a line
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 27:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 342: divider Okay, if this parameter is actually 2x to divide by x, then let's call it half_divider or something to make that a bit more obvious in the function signature.
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 371: { Can't you just call clock_configure_mnd() here?
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 377: m n?
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 133: struct sc7180_mdss_rcg { No longer needed.
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 147: pclk0_rcgr This is the whole clock structure now, not just the RCGR, so shouldn't it just be called `pclk0` (same for the others)?
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 305: uint32_t enum mdss_clock
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 307: uint32_t ditto
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 28:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39612/28/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/28/src/soc/qualcomm/sc7180/cl... PS28, Line 370: if (mdss_clk_cfg.m != 0) that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/39612/28/src/soc/qualcomm/sc7180/cl... PS28, Line 379: ~(mdss_clk_cfg.d_2) & CLK_CTL_RCG_MND_BMSK); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39612/28/src/soc/qualcomm/sc7180/cl... PS28, Line 379: ~(mdss_clk_cfg.d_2) & CLK_CTL_RCG_MND_BMSK); please, no spaces at the start of a line
Hello build bot (Jenkins), Taniya Das, mturney mturney, Julius Werner, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39612
to look at the new patch set (#29).
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 103 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39612/29
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 29: Code-Review+2
(7 comments)
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 342: divider
Okay, if this parameter is actually 2x to divide by x, then let's call it half_divider or something […]
Done
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 371: {
Can't you just call clock_configure_mnd() here?
Done
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/cl... PS27, Line 377: m
n?
Ack
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 133: struct sc7180_mdss_rcg {
No longer needed.
Done
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 147: pclk0_rcgr
This is the whole clock structure now, not just the RCGR, so shouldn't it just be called `pclk0` (sa […]
Done
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 305: uint32_t
enum mdss_clock
Done
https://review.coreboot.org/c/coreboot/+/39612/27/src/soc/qualcomm/sc7180/in... PS27, Line 307: uint32_t
ditto
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 29:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 214: ;
Sorry, I don't understand (or agree with) this. […]
Ack
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 34: #define mdss_reg_ptr(type, mem, base) \
Macros should be uppercase.
Ack
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 35: ((offsetof(type, mem)) + base)
Implementation updated as per latest comments from Julius.
Done
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
sc7180: clock: Add display external clock in coreboot
Add support for display external clock in coreboot for SC7180.
Tested: Display clocks are configured.
Change-Id: Ida222890252b80db738fa1f685b212b3f7c6e689 Signed-off-by: Taniya Das tdas@codeaurora.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/39612 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/soc/qualcomm/sc7180/clock.c M src/soc/qualcomm/sc7180/include/soc/addressmap.h M src/soc/qualcomm/sc7180/include/soc/clock.h 3 files changed, 103 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/soc/qualcomm/sc7180/clock.c b/src/soc/qualcomm/sc7180/clock.c index 1683d70..3702fa6 100644 --- a/src/soc/qualcomm/sc7180/clock.c +++ b/src/soc/qualcomm/sc7180/clock.c @@ -104,6 +104,20 @@ }, };
+static struct sc7180_mnd_clock *mdss_clock[MDSS_CLK_COUNT] = { + [MDSS_CLK_ESC0] = &mdss->esc0, + [MDSS_CLK_PCLK0] = &mdss->pclk0, + [MDSS_CLK_BYTE0] = &mdss->byte0, + [MDSS_CLK_BYTE0_INTF] = &mdss->byte0, +}; + +static u32 *mdss_cbcr[MDSS_CLK_COUNT] = { + [MDSS_CLK_ESC0] = &mdss->esc0_cbcr, + [MDSS_CLK_PCLK0] = &mdss->pclk0_cbcr, + [MDSS_CLK_BYTE0] = &mdss->byte0_cbcr, + [MDSS_CLK_BYTE0_INTF] = &mdss->byte0_intf_cbcr, +}; + static int clock_configure_gpll0(void) { setbits32(&gcc->gpll0.user_ctl_u, 1 << SCALE_FREQ_SHFT); @@ -202,7 +216,7 @@ qspi_core_cfg, hz, ARRAY_SIZE(qspi_core_cfg)); clock_enable(&gcc->qspi_cnoc_ahb_cbcr); - clock_enable(&gcc->qspi_core.cbcr); + clock_enable(&gcc->qspi_core_cbcr); }
int clock_reset_bcr(void *bcr_addr, bool reset) @@ -323,11 +337,63 @@ printk(BIOS_DEBUG, "L3 Frequency bumped to 1.2096(GHz)\n"); }
+int mdss_clock_configure(enum mdss_clock clk_type, uint32_t source, + uint32_t half_divider, uint32_t m, + uint32_t n, uint32_t d_2) +{ + struct clock_config mdss_clk_cfg; + uint32_t reg_val; + + if (clk_type >= MDSS_CLK_COUNT) + return -1; + + /* Initialize it with received arguments */ + mdss_clk_cfg.hz = 0; + mdss_clk_cfg.src = source; + + /* + * client is expected to provide 2n divider value, + * as the divider value in register is in form "2n-1" + */ + mdss_clk_cfg.div = half_divider ? (half_divider - 1) : 0; + mdss_clk_cfg.m = m; + mdss_clk_cfg.n = n; + mdss_clk_cfg.d_2 = d_2; + + /* configure and set the clock */ + reg_val = (mdss_clk_cfg.src << CLK_CTL_CFG_SRC_SEL_SHFT) | + (mdss_clk_cfg.div << CLK_CTL_CFG_SRC_DIV_SHFT); + + write32(&mdss_clock[clk_type]->clock.rcg_cfg, reg_val); + + /* Set m/n/d values for a specific clock */ + if (mdss_clk_cfg.m != 0) + clock_configure_mnd((struct sc7180_clock *)mdss_clock[clk_type], + mdss_clk_cfg.m, mdss_clk_cfg.n, mdss_clk_cfg.d_2); + + /* Commit config to RCG */ + setbits32(&mdss_clock[clk_type]->clock.rcg_cmd, + BIT(CLK_CTL_CMD_UPDATE_SHFT)); + + return 0; +} + +int mdss_clock_enable(enum mdss_clock clk_type) +{ + if (clk_type >= MDSS_CLK_COUNT) + return -1; + + /* Enable clock*/ + clock_enable(mdss_cbcr[clk_type]); + + return 0; +} + void clock_init(void) { clock_configure_gpll0();
- clock_enable_vote(&gcc->qup_wrap0_core_2x.cbcr, + clock_enable_vote(&gcc->qup_wrap0_core_2x_cbcr, &gcc->apcs_clk_br_en1, QUPV3_WRAP0_CORE_2X_CLK_ENA); clock_enable_vote(&gcc->qup_wrap0_core_cbcr, diff --git a/src/soc/qualcomm/sc7180/include/soc/addressmap.h b/src/soc/qualcomm/sc7180/include/soc/addressmap.h index 2b65e99..29c60db 100644 --- a/src/soc/qualcomm/sc7180/include/soc/addressmap.h +++ b/src/soc/qualcomm/sc7180/include/soc/addressmap.h @@ -12,6 +12,7 @@ #define TLMM_WEST_TILE_BASE 0x03500000 #define SILVER_PLL_BASE 0x18280000 #define L3_PLL_BASE 0x18284000 +#define DISP_CC_BASE 0x0AF00000
/* * QUP SERIAL ENGINE BASE ADDRESSES diff --git a/src/soc/qualcomm/sc7180/include/soc/clock.h b/src/soc/qualcomm/sc7180/include/soc/clock.h index d202605..c9ecfb2 100644 --- a/src/soc/qualcomm/sc7180/include/soc/clock.h +++ b/src/soc/qualcomm/sc7180/include/soc/clock.h @@ -32,7 +32,6 @@ #define SCALE_FREQ_SHFT 11
struct sc7180_clock { - u32 cbcr; u32 rcg_cmd; u32 rcg_cfg; }; @@ -58,6 +57,7 @@ };
struct sc7180_qupv3_clock { + u32 cbcr; struct sc7180_mnd_clock mnd_clk; struct sc7180_dfsr_clock dfsr_clk; }; @@ -85,6 +85,7 @@ u32 qup_wrap0_s_ahb_cbcr; u32 qup_wrap0_core_cbcr; u32 qup_wrap0_core_cdivr; + u32 qup_wrap0_core_2x_cbcr; struct sc7180_clock qup_wrap0_core_2x; u8 _res2[0x17030 - 0x17020]; struct sc7180_qupv3_clock qup_wrap0_s[6]; @@ -102,6 +103,7 @@ u8 _res6[0x4b000 - 0x26004]; u32 qspi_bcr; u32 qspi_cnoc_ahb_cbcr; + u32 qspi_core_cbcr; struct sc7180_clock qspi_core; u8 _res7[0x50000 - 0x4b014]; u32 usb3_phy_prim_bcr; @@ -128,6 +130,33 @@ u32 aoss_cc_apcs_misc; };
+struct sc7180_disp_cc { + u8 _res0[0x2004]; + u32 pclk0_cbcr; + u8 _res1[0x2028 - 0x2008]; + u32 byte0_cbcr; + u32 byte0_intf_cbcr; + u8 _res2[0x2038 - 0x2030]; + u32 esc0_cbcr; + u8 _res3[0x2098 - 0x203C]; + struct sc7180_mnd_clock pclk0; + u8 _res4[0x2110 - 0x20AC]; + struct sc7180_mnd_clock byte0; + u8 _res5[0x2148 - 0x2124]; + struct sc7180_mnd_clock esc0; + u8 _res6[0x10000 - 0x215C]; +}; +check_member(sc7180_disp_cc, byte0_cbcr, 0x2028); +check_member(sc7180_disp_cc, esc0_cbcr, 0x2038); + +enum mdss_clock { + MDSS_CLK_ESC0 = 0, + MDSS_CLK_PCLK0, + MDSS_CLK_BYTE0, + MDSS_CLK_BYTE0_INTF, + MDSS_CLK_COUNT +}; + enum clk_ctl_gpll_user_ctl { CLK_CTL_GPLL_PLLOUT_EVEN_BMSK = 0x2, CLK_CTL_GPLL_PLLOUT_MAIN_SHFT = 0, @@ -202,12 +231,6 @@ uint16_t d_2; };
-struct mdss_clock_config { - const char *clk_name; - uintptr_t rcgr; - uintptr_t cbcr; -}; - /* CPU PLL */ #define L_VAL_1516P8MHz 0x4F #define L_VAL_1209P6MHz 0x3F @@ -265,6 +288,7 @@ static struct sc7180_aoss *const aoss = (void *)AOSS_CC_BASE; static struct sc7180_apss_clock *const apss_silver = (void *)SILVER_PLL_BASE; static struct sc7180_apss_clock *const apss_l3 = (void *)L3_PLL_BASE; +static struct sc7180_disp_cc *const mdss = (void *)DISP_CC_BASE;
void clock_init(void); void clock_reset_aop(void); @@ -273,5 +297,8 @@ void clock_configure_qup(int qup, uint32_t hz); void clock_enable_qup(int qup); void clock_configure_dfsr(int qup); +int mdss_clock_configure(enum mdss_clock clk_type, uint32_t source, + uint32_t half_divider, uint32_t m, uint32_t n, uint32_t d); +int mdss_clock_enable(enum mdss_clock clk_type);
#endif // __SOC_QUALCOMM_SC7180_CLOCK_H__
-
build bot (Jenkins) (Code Review) -
Julius Werner (Code Review)
-
mturney mturney (Code Review)
-
Paul Menzel (Code Review)
-
Ravi kumar (Code Review)
-
Taniya Das (Code Review)