| Message ID | 20231124-amlogic-v6-4-upstream-dsi-ccf-vim3-v9-8-95256ed139e6@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/meson: add support for MIPI DSI Display | expand |
On Fri 24 Nov 2023 at 09:41, Neil Armstrong <neil.armstrong@linaro.org> wrote: > In order to setup the DSI clock, let's make the unused VCLK2 clock path > configuration via CCF. > > The nocache option is removed from following clocks: > - vclk2_sel > - vclk2_input > - vclk2_div > - vclk2 > - vclk_div1 > - vclk2_div2_en > - vclk2_div4_en > - vclk2_div6_en > - vclk2_div12_en > - vclk2_div2 > - vclk2_div4 > - vclk2_div6 > - vclk2_div12 > - cts_encl_sel > > vclk2 and vclk2_div uses the newly introduced vclk regmap driver > to handle the enable and reset bits. > > In order to set a rate on cts_encl via the vclk2 clock path, > the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order > to keep CCF from selection a parent. > The parents of cts_encl_sel & vclk2_sel are expected to be defined > in DT. > > The following clock scheme is to be used for DSI: > > xtal > \_ gp0_pll_dco > \_ gp0_pll > |- vclk2_sel > | \_ vclk2_input > | \_ vclk2_div > | \_ vclk2 > | \_ vclk2_div1 > | \_ cts_encl_sel > | \_ cts_encl -> to VPU LCD Encoder > |- mipi_dsi_pxclk_sel > \_ mipi_dsi_pxclk_div > \_ mipi_dsi_pxclk -> to DSI controller > > The mipi_dsi_pxclk_div is set as RO in order to use the same GP0 > for mipi_dsi_pxclk and vclk2_input. Could you explain a bit more this part of about the RO ops ? Maybe I'm missing something. You would be relying on the reset being always the way it. It is probable but not safe. A way to deal with the shared GP0 would be to: * cut rate propagation at mipi_dsi_pxclk_sel (already done) and (vclk2_sel - TBD) ... * Set GP0 base rate through assigned-clock-rate (which you already in patch 11) With this, I'm not sure anything needs to be RO for the rates to be set properly for each subtree. Also, with the subtree above and your example in patch 11, it looks odd that PXCLK is manually set through DT while ENCL is not. Both are input of dsi driver. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/clk/meson/g12a.c | 68 +++++++++++++++++++++++++++++++++--------------- > 1 file changed, 47 insertions(+), 21 deletions(-) > > diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c > index cadd824336ad..fb3d9196a1fd 100644 > --- a/drivers/clk/meson/g12a.c > +++ b/drivers/clk/meson/g12a.c > @@ -22,6 +22,7 @@ > #include "clk-regmap.h" > #include "clk-cpu-dyndiv.h" > #include "vid-pll-div.h" > +#include "vclk.h" > #include "meson-eeclk.h" > #include "g12a.h" > > @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = { > .ops = &clk_regmap_mux_ops, > .parent_hws = g12a_vclk_parent_hws, > .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws), > - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, > + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, No sure CLK_SET_RATE_PARENT is wise here. What you manually set in DT for the GP0, is likely to change because of this, isn't it ? > }, > }; > > @@ -3193,7 +3194,7 @@ static struct clk_regmap g12a_vclk2_input = { > .ops = &clk_regmap_gate_ops, > .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_sel.hw }, > .num_parents = 1, > - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3215,19 +3216,32 @@ static struct clk_regmap g12a_vclk_div = { > }; > > static struct clk_regmap g12a_vclk2_div = { > - .data = &(struct clk_regmap_div_data){ > - .offset = HHI_VIID_CLK_DIV, > - .shift = 0, > - .width = 8, > + .data = &(struct clk_regmap_vclk_div_data){ > + .div = { > + .reg_off = HHI_VIID_CLK_DIV, > + .shift = 0, > + .width = 8, > + }, > + .enable = { > + .reg_off = HHI_VIID_CLK_DIV, > + .shift = 16, > + .width = 1, > + }, > + .reset = { > + .reg_off = HHI_VIID_CLK_DIV, > + .shift = 17, > + .width = 1, > + }, > + .flags = CLK_DIVIDER_ROUND_CLOSEST, > }, > .hw.init = &(struct clk_init_data){ > .name = "vclk2_div", > - .ops = &clk_regmap_divider_ops, > + .ops = &clk_regmap_vclk_div_ops, > .parent_hws = (const struct clk_hw *[]) { > &g12a_vclk2_input.hw > }, > .num_parents = 1, > - .flags = CLK_GET_RATE_NOCACHE, > + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, > }, > }; > > @@ -3246,16 +3260,24 @@ static struct clk_regmap g12a_vclk = { > }; > > static struct clk_regmap g12a_vclk2 = { > - .data = &(struct clk_regmap_gate_data){ > - .offset = HHI_VIID_CLK_CNTL, > - .bit_idx = 19, > + .data = &(struct clk_regmap_vclk_data){ > + .enable = { > + .reg_off = HHI_VIID_CLK_CNTL, > + .shift = 19, > + .width = 1, > + }, > + .reset = { > + .reg_off = HHI_VIID_CLK_CNTL, > + .shift = 15, > + .width = 1, > + }, > }, > .hw.init = &(struct clk_init_data) { > .name = "vclk2", > - .ops = &clk_regmap_gate_ops, > + .ops = &clk_regmap_vclk_ops, > .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_div.hw }, > .num_parents = 1, > - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, > + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, > }, > }; > > @@ -3339,7 +3361,7 @@ static struct clk_regmap g12a_vclk2_div1 = { > .ops = &clk_regmap_gate_ops, > .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, > .num_parents = 1, > - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3353,7 +3375,7 @@ static struct clk_regmap g12a_vclk2_div2_en = { > .ops = &clk_regmap_gate_ops, > .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, > .num_parents = 1, > - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3367,7 +3389,7 @@ static struct clk_regmap g12a_vclk2_div4_en = { > .ops = &clk_regmap_gate_ops, > .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, > .num_parents = 1, > - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3381,7 +3403,7 @@ static struct clk_regmap g12a_vclk2_div6_en = { > .ops = &clk_regmap_gate_ops, > .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, > .num_parents = 1, > - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3395,7 +3417,7 @@ static struct clk_regmap g12a_vclk2_div12_en = { > .ops = &clk_regmap_gate_ops, > .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, > .num_parents = 1, > - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3461,6 +3483,7 @@ static struct clk_fixed_factor g12a_vclk2_div2 = { > &g12a_vclk2_div2_en.hw > }, > .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3474,6 +3497,7 @@ static struct clk_fixed_factor g12a_vclk2_div4 = { > &g12a_vclk2_div4_en.hw > }, > .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3487,6 +3511,7 @@ static struct clk_fixed_factor g12a_vclk2_div6 = { > &g12a_vclk2_div6_en.hw > }, > .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3500,6 +3525,7 @@ static struct clk_fixed_factor g12a_vclk2_div12 = { > &g12a_vclk2_div12_en.hw > }, > .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > }, > }; > > @@ -3561,7 +3587,7 @@ static struct clk_regmap g12a_cts_encl_sel = { > .ops = &clk_regmap_mux_ops, > .parent_hws = g12a_cts_parent_hws, > .num_parents = ARRAY_SIZE(g12a_cts_parent_hws), > - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, > + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > }, > }; > > @@ -3717,7 +3743,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_sel = { > .ops = &clk_regmap_mux_ops, > .parent_hws = g12a_mipi_dsi_pxclk_parent_hws, > .num_parents = ARRAY_SIZE(g12a_mipi_dsi_pxclk_parent_hws), > - .flags = CLK_SET_RATE_NO_REPARENT, > + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > }, > }; > > @@ -3729,7 +3755,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_div = { > }, > .hw.init = &(struct clk_init_data){ > .name = "mipi_dsi_pxclk_div", > - .ops = &clk_regmap_divider_ops, > + .ops = &clk_regmap_divider_ro_ops, > .parent_hws = (const struct clk_hw *[]) { > &g12a_mipi_dsi_pxclk_sel.hw > },
On 24/11/2023 15:12, Jerome Brunet wrote: > > On Fri 24 Nov 2023 at 09:41, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> In order to setup the DSI clock, let's make the unused VCLK2 clock path >> configuration via CCF. >> >> The nocache option is removed from following clocks: >> - vclk2_sel >> - vclk2_input >> - vclk2_div >> - vclk2 >> - vclk_div1 >> - vclk2_div2_en >> - vclk2_div4_en >> - vclk2_div6_en >> - vclk2_div12_en >> - vclk2_div2 >> - vclk2_div4 >> - vclk2_div6 >> - vclk2_div12 >> - cts_encl_sel >> >> vclk2 and vclk2_div uses the newly introduced vclk regmap driver >> to handle the enable and reset bits. >> >> In order to set a rate on cts_encl via the vclk2 clock path, >> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order >> to keep CCF from selection a parent. >> The parents of cts_encl_sel & vclk2_sel are expected to be defined >> in DT. >> >> The following clock scheme is to be used for DSI: >> >> xtal >> \_ gp0_pll_dco >> \_ gp0_pll >> |- vclk2_sel >> | \_ vclk2_input >> | \_ vclk2_div >> | \_ vclk2 >> | \_ vclk2_div1 >> | \_ cts_encl_sel >> | \_ cts_encl -> to VPU LCD Encoder >> |- mipi_dsi_pxclk_sel >> \_ mipi_dsi_pxclk_div >> \_ mipi_dsi_pxclk -> to DSI controller >> >> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0 >> for mipi_dsi_pxclk and vclk2_input. > > Could you explain a bit more this part of about the RO ops ? > Maybe I'm missing something. > > You would be relying on the reset being always the way it. It is > probable but not safe. > > A way to deal with the shared GP0 would be to: > * cut rate propagation at mipi_dsi_pxclk_sel (already done) and > (vclk2_sel - TBD) ... > * Set GP0 base rate through assigned-clock-rate (which you already in > patch 11) > > With this, I'm not sure anything needs to be RO for the rates to be set > properly for each subtree. > > Also, with the subtree above and your example in patch 11, it looks odd that > PXCLK is manually set through DT while ENCL is not. Both are input of > dsi driver. So the deal is about dynamic setup of clocks for DSI bridges, not really for panels where we can probably know in advance the clock setup. In this particular case, we need to keep a ratio between the vclk and the DSI bitclk, the DSI bitclk is taken from mipi_dsi_pxclk and vclk is derived from gp0 via vclk2. If we set the bitclk rate via mipi_dsi_pxclk, CCF will try to use mipi_dsi_pxclk_div to achieve the rate, and it does it everytime I tried, breaking the vclk/bitclk ratio, and we have no way to know the gp0 rate in this case. I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk ratios, since it doesn't exist on AXG. Not sure we would ever need it... and none of the other upstream DSI drivers supports such setups. The main reasons I set only mipi_dsi_pxclk in DT is because : 1) the DSI controller requires a bitclk to respond, pclk is not enough 2) GP0 is disabled with an invalid config at cold boot, thus we cannot rely on a default/safe rate on an initial prepare_enable(). This permits setting initial valid state for the DSI controller, while the actual bitclk and vclk are calculated dynamically with panel/bridge runtime parameters. For the record, the samsung-dsim used fixed rate set from DT, and they moved from that in order to support more panel and bridges. But they're quite lucky because usually the DSI PLL is included in the PHY, this makes the Amlogic design quite unusual (like most multimedia stuf...). Neil > >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/clk/meson/g12a.c | 68 +++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 47 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c >> index cadd824336ad..fb3d9196a1fd 100644 >> --- a/drivers/clk/meson/g12a.c >> +++ b/drivers/clk/meson/g12a.c >> @@ -22,6 +22,7 @@ >> #include "clk-regmap.h" >> #include "clk-cpu-dyndiv.h" >> #include "vid-pll-div.h" >> +#include "vclk.h" >> #include "meson-eeclk.h" >> #include "g12a.h" >> >> @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = { >> .ops = &clk_regmap_mux_ops, >> .parent_hws = g12a_vclk_parent_hws, >> .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws), >> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, >> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > > No sure CLK_SET_RATE_PARENT is wise here. > What you manually set in DT for the GP0, is likely to change because of > this, isn't it ? > >> }, >> }; >> >> @@ -3193,7 +3194,7 @@ static struct clk_regmap g12a_vclk2_input = { >> .ops = &clk_regmap_gate_ops, >> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_sel.hw }, >> .num_parents = 1, >> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3215,19 +3216,32 @@ static struct clk_regmap g12a_vclk_div = { >> }; >> >> static struct clk_regmap g12a_vclk2_div = { >> - .data = &(struct clk_regmap_div_data){ >> - .offset = HHI_VIID_CLK_DIV, >> - .shift = 0, >> - .width = 8, >> + .data = &(struct clk_regmap_vclk_div_data){ >> + .div = { >> + .reg_off = HHI_VIID_CLK_DIV, >> + .shift = 0, >> + .width = 8, >> + }, >> + .enable = { >> + .reg_off = HHI_VIID_CLK_DIV, >> + .shift = 16, >> + .width = 1, >> + }, >> + .reset = { >> + .reg_off = HHI_VIID_CLK_DIV, >> + .shift = 17, >> + .width = 1, >> + }, >> + .flags = CLK_DIVIDER_ROUND_CLOSEST, >> }, >> .hw.init = &(struct clk_init_data){ >> .name = "vclk2_div", >> - .ops = &clk_regmap_divider_ops, >> + .ops = &clk_regmap_vclk_div_ops, >> .parent_hws = (const struct clk_hw *[]) { >> &g12a_vclk2_input.hw >> }, >> .num_parents = 1, >> - .flags = CLK_GET_RATE_NOCACHE, >> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, >> }, >> }; >> >> @@ -3246,16 +3260,24 @@ static struct clk_regmap g12a_vclk = { >> }; >> >> static struct clk_regmap g12a_vclk2 = { >> - .data = &(struct clk_regmap_gate_data){ >> - .offset = HHI_VIID_CLK_CNTL, >> - .bit_idx = 19, >> + .data = &(struct clk_regmap_vclk_data){ >> + .enable = { >> + .reg_off = HHI_VIID_CLK_CNTL, >> + .shift = 19, >> + .width = 1, >> + }, >> + .reset = { >> + .reg_off = HHI_VIID_CLK_CNTL, >> + .shift = 15, >> + .width = 1, >> + }, >> }, >> .hw.init = &(struct clk_init_data) { >> .name = "vclk2", >> - .ops = &clk_regmap_gate_ops, >> + .ops = &clk_regmap_vclk_ops, >> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_div.hw }, >> .num_parents = 1, >> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, >> }, >> }; >> >> @@ -3339,7 +3361,7 @@ static struct clk_regmap g12a_vclk2_div1 = { >> .ops = &clk_regmap_gate_ops, >> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >> .num_parents = 1, >> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3353,7 +3375,7 @@ static struct clk_regmap g12a_vclk2_div2_en = { >> .ops = &clk_regmap_gate_ops, >> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >> .num_parents = 1, >> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3367,7 +3389,7 @@ static struct clk_regmap g12a_vclk2_div4_en = { >> .ops = &clk_regmap_gate_ops, >> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >> .num_parents = 1, >> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3381,7 +3403,7 @@ static struct clk_regmap g12a_vclk2_div6_en = { >> .ops = &clk_regmap_gate_ops, >> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >> .num_parents = 1, >> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3395,7 +3417,7 @@ static struct clk_regmap g12a_vclk2_div12_en = { >> .ops = &clk_regmap_gate_ops, >> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >> .num_parents = 1, >> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3461,6 +3483,7 @@ static struct clk_fixed_factor g12a_vclk2_div2 = { >> &g12a_vclk2_div2_en.hw >> }, >> .num_parents = 1, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3474,6 +3497,7 @@ static struct clk_fixed_factor g12a_vclk2_div4 = { >> &g12a_vclk2_div4_en.hw >> }, >> .num_parents = 1, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3487,6 +3511,7 @@ static struct clk_fixed_factor g12a_vclk2_div6 = { >> &g12a_vclk2_div6_en.hw >> }, >> .num_parents = 1, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3500,6 +3525,7 @@ static struct clk_fixed_factor g12a_vclk2_div12 = { >> &g12a_vclk2_div12_en.hw >> }, >> .num_parents = 1, >> + .flags = CLK_SET_RATE_PARENT, >> }, >> }; >> >> @@ -3561,7 +3587,7 @@ static struct clk_regmap g12a_cts_encl_sel = { >> .ops = &clk_regmap_mux_ops, >> .parent_hws = g12a_cts_parent_hws, >> .num_parents = ARRAY_SIZE(g12a_cts_parent_hws), >> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, >> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >> }, >> }; >> >> @@ -3717,7 +3743,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_sel = { >> .ops = &clk_regmap_mux_ops, >> .parent_hws = g12a_mipi_dsi_pxclk_parent_hws, >> .num_parents = ARRAY_SIZE(g12a_mipi_dsi_pxclk_parent_hws), >> - .flags = CLK_SET_RATE_NO_REPARENT, >> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >> }, >> }; >> >> @@ -3729,7 +3755,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_div = { >> }, >> .hw.init = &(struct clk_init_data){ >> .name = "mipi_dsi_pxclk_div", >> - .ops = &clk_regmap_divider_ops, >> + .ops = &clk_regmap_divider_ro_ops, >> .parent_hws = (const struct clk_hw *[]) { >> &g12a_mipi_dsi_pxclk_sel.hw >> }, >
On Fri 24 Nov 2023 at 16:15, Neil Armstrong <neil.armstrong@linaro.org> wrote: > On 24/11/2023 15:12, Jerome Brunet wrote: >> On Fri 24 Nov 2023 at 09:41, Neil Armstrong <neil.armstrong@linaro.org> >> wrote: >> >>> In order to setup the DSI clock, let's make the unused VCLK2 clock path >>> configuration via CCF. >>> >>> The nocache option is removed from following clocks: >>> - vclk2_sel >>> - vclk2_input >>> - vclk2_div >>> - vclk2 >>> - vclk_div1 >>> - vclk2_div2_en >>> - vclk2_div4_en >>> - vclk2_div6_en >>> - vclk2_div12_en >>> - vclk2_div2 >>> - vclk2_div4 >>> - vclk2_div6 >>> - vclk2_div12 >>> - cts_encl_sel >>> >>> vclk2 and vclk2_div uses the newly introduced vclk regmap driver >>> to handle the enable and reset bits. >>> >>> In order to set a rate on cts_encl via the vclk2 clock path, >>> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order >>> to keep CCF from selection a parent. >>> The parents of cts_encl_sel & vclk2_sel are expected to be defined >>> in DT. >>> >>> The following clock scheme is to be used for DSI: >>> >>> xtal >>> \_ gp0_pll_dco >>> \_ gp0_pll >>> |- vclk2_sel >>> | \_ vclk2_input >>> | \_ vclk2_div >>> | \_ vclk2 >>> | \_ vclk2_div1 >>> | \_ cts_encl_sel >>> | \_ cts_encl -> to VPU LCD Encoder >>> |- mipi_dsi_pxclk_sel >>> \_ mipi_dsi_pxclk_div >>> \_ mipi_dsi_pxclk -> to DSI controller >>> >>> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0 >>> for mipi_dsi_pxclk and vclk2_input. >> Could you explain a bit more this part of about the RO ops ? >> Maybe I'm missing something. >> You would be relying on the reset being always the way it. It is >> probable but not safe. >> A way to deal with the shared GP0 would be to: >> * cut rate propagation at mipi_dsi_pxclk_sel (already done) and >> (vclk2_sel - TBD) ... >> * Set GP0 base rate through assigned-clock-rate (which you already in >> patch 11) >> With this, I'm not sure anything needs to be RO for the rates to be set >> properly for each subtree. >> Also, with the subtree above and your example in patch 11, it looks odd >> that >> PXCLK is manually set through DT while ENCL is not. Both are input of >> dsi driver. > > So the deal is about dynamic setup of clocks for DSI bridges, not really > for panels where we can probably know in advance the clock setup. > > In this particular case, we need to keep a ratio between the vclk and the > DSI bitclk, the DSI bitclk is taken from mipi_dsi_pxclk and vclk is derived > from gp0 via vclk2. > > If we set the bitclk rate via mipi_dsi_pxclk, CCF will try to use mipi_dsi_pxclk_div > to achieve the rate, If you have CLK_RATE_PARENT on mipi_dsi_pxclk_sel, I'm not surprised it does that, but you don't :/ I'm quite surprised it would do that, or even could. From your example setting 96Mhz on both gp0 and mipi_dsi_pxclk, since you've proposed RO-OPS, I suppose the divider is assumed to be 1 and stay like that forever. With rate propagation disabled mipi_dsi_pxclk_sel and GP0 is 96Mhz, CCF would have no choice but picking 1 as divider, so I don't understand how CCF would pick anything else and how RO-OPS help > and it does it everytime I tried, breaking the vclk/bitclk ratio, > and we have no way to know the gp0 rate in this case. If you really want to ensure the divider value is always 1, why not use a divider table allowing only 1 ? Adding a comment in the g12 clock driver would nice because this not obvious. It would be safer than relying on the reset value. > > I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk ratios, > since it doesn't exist on AXG. Not sure we would ever need it... and none > of the other upstream DSI drivers supports such setups. > > The main reasons I set only mipi_dsi_pxclk in DT is because : > 1) the DSI controller requires a bitclk to respond, pclk is not enough > 2) GP0 is disabled with an invalid config at cold boot, thus we cannot > rely on a default/safe rate on an initial prepare_enable(). > This permits setting initial valid state for the DSI controller, while > the actual bitclk and vclk are calculated dynamically with panel/bridge > runtime parameters. Nothing against setting rate in DT when it is static. Setting it then overriding it is not easy to follow. To work around GP0 not being set, assuming you want to keep rate propagation as it is, you could call clk_set_rate() on cts_encl (possibly w/o enabling it) to force a setup on gp0 then clk_prepare_enable() on pxclk. You'd get a your safe rate on GP0 and the clock you need on pxclk. It is a bit hackish. Might be better to claim gp0 in your driver to manage it directly, cutting rate propagation above it to control each branch of the subtree as you need. It seems you need to have control over that anyway and it would be clear GP0 is expected to belong to DSI. > > For the record, the samsung-dsim used fixed rate set from DT, and they moved > from that in order to support more panel and bridges. > > But they're quite lucky because usually the DSI PLL is included in the PHY, > this makes the Amlogic design quite unusual (like most multimedia stuf...). > > Neil > >> >>> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> drivers/clk/meson/g12a.c | 68 +++++++++++++++++++++++++++++++++--------------- >>> 1 file changed, 47 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c >>> index cadd824336ad..fb3d9196a1fd 100644 >>> --- a/drivers/clk/meson/g12a.c >>> +++ b/drivers/clk/meson/g12a.c >>> @@ -22,6 +22,7 @@ >>> #include "clk-regmap.h" >>> #include "clk-cpu-dyndiv.h" >>> #include "vid-pll-div.h" >>> +#include "vclk.h" >>> #include "meson-eeclk.h" >>> #include "g12a.h" >>> @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = { >>> .ops = &clk_regmap_mux_ops, >>> .parent_hws = g12a_vclk_parent_hws, >>> .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws), >>> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, >>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >> No sure CLK_SET_RATE_PARENT is wise here. >> What you manually set in DT for the GP0, is likely to change because of >> this, isn't it ? >> >>> }, >>> }; >>> @@ -3193,7 +3194,7 @@ static struct clk_regmap g12a_vclk2_input = { >>> .ops = &clk_regmap_gate_ops, >>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_sel.hw }, >>> .num_parents = 1, >>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3215,19 +3216,32 @@ static struct clk_regmap g12a_vclk_div = { >>> }; >>> static struct clk_regmap g12a_vclk2_div = { >>> - .data = &(struct clk_regmap_div_data){ >>> - .offset = HHI_VIID_CLK_DIV, >>> - .shift = 0, >>> - .width = 8, >>> + .data = &(struct clk_regmap_vclk_div_data){ >>> + .div = { >>> + .reg_off = HHI_VIID_CLK_DIV, >>> + .shift = 0, >>> + .width = 8, >>> + }, >>> + .enable = { >>> + .reg_off = HHI_VIID_CLK_DIV, >>> + .shift = 16, >>> + .width = 1, >>> + }, >>> + .reset = { >>> + .reg_off = HHI_VIID_CLK_DIV, >>> + .shift = 17, >>> + .width = 1, >>> + }, >>> + .flags = CLK_DIVIDER_ROUND_CLOSEST, >>> }, >>> .hw.init = &(struct clk_init_data){ >>> .name = "vclk2_div", >>> - .ops = &clk_regmap_divider_ops, >>> + .ops = &clk_regmap_vclk_div_ops, >>> .parent_hws = (const struct clk_hw *[]) { >>> &g12a_vclk2_input.hw >>> }, >>> .num_parents = 1, >>> - .flags = CLK_GET_RATE_NOCACHE, >>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, >>> }, >>> }; >>> @@ -3246,16 +3260,24 @@ static struct clk_regmap g12a_vclk = { >>> }; >>> static struct clk_regmap g12a_vclk2 = { >>> - .data = &(struct clk_regmap_gate_data){ >>> - .offset = HHI_VIID_CLK_CNTL, >>> - .bit_idx = 19, >>> + .data = &(struct clk_regmap_vclk_data){ >>> + .enable = { >>> + .reg_off = HHI_VIID_CLK_CNTL, >>> + .shift = 19, >>> + .width = 1, >>> + }, >>> + .reset = { >>> + .reg_off = HHI_VIID_CLK_CNTL, >>> + .shift = 15, >>> + .width = 1, >>> + }, >>> }, >>> .hw.init = &(struct clk_init_data) { >>> .name = "vclk2", >>> - .ops = &clk_regmap_gate_ops, >>> + .ops = &clk_regmap_vclk_ops, >>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_div.hw }, >>> .num_parents = 1, >>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, >>> }, >>> }; >>> @@ -3339,7 +3361,7 @@ static struct clk_regmap g12a_vclk2_div1 = { >>> .ops = &clk_regmap_gate_ops, >>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>> .num_parents = 1, >>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3353,7 +3375,7 @@ static struct clk_regmap g12a_vclk2_div2_en = { >>> .ops = &clk_regmap_gate_ops, >>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>> .num_parents = 1, >>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3367,7 +3389,7 @@ static struct clk_regmap g12a_vclk2_div4_en = { >>> .ops = &clk_regmap_gate_ops, >>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>> .num_parents = 1, >>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3381,7 +3403,7 @@ static struct clk_regmap g12a_vclk2_div6_en = { >>> .ops = &clk_regmap_gate_ops, >>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>> .num_parents = 1, >>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3395,7 +3417,7 @@ static struct clk_regmap g12a_vclk2_div12_en = { >>> .ops = &clk_regmap_gate_ops, >>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>> .num_parents = 1, >>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3461,6 +3483,7 @@ static struct clk_fixed_factor g12a_vclk2_div2 = >>> { >>> &g12a_vclk2_div2_en.hw >>> }, >>> .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3474,6 +3497,7 @@ static struct clk_fixed_factor g12a_vclk2_div4 = >>> { >>> &g12a_vclk2_div4_en.hw >>> }, >>> .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3487,6 +3511,7 @@ static struct clk_fixed_factor g12a_vclk2_div6 = >>> { >>> &g12a_vclk2_div6_en.hw >>> }, >>> .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3500,6 +3525,7 @@ static struct clk_fixed_factor g12a_vclk2_div12 >>> = { >>> &g12a_vclk2_div12_en.hw >>> }, >>> .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> }, >>> }; >>> @@ -3561,7 +3587,7 @@ static struct clk_regmap g12a_cts_encl_sel = { >>> .ops = &clk_regmap_mux_ops, >>> .parent_hws = g12a_cts_parent_hws, >>> .num_parents = ARRAY_SIZE(g12a_cts_parent_hws), >>> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, >>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >>> }, >>> }; >>> @@ -3717,7 +3743,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_sel >>> = { >>> .ops = &clk_regmap_mux_ops, >>> .parent_hws = g12a_mipi_dsi_pxclk_parent_hws, >>> .num_parents = ARRAY_SIZE(g12a_mipi_dsi_pxclk_parent_hws), >>> - .flags = CLK_SET_RATE_NO_REPARENT, >>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >>> }, >>> }; >>> @@ -3729,7 +3755,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_div >>> = { >>> }, >>> .hw.init = &(struct clk_init_data){ >>> .name = "mipi_dsi_pxclk_div", >>> - .ops = &clk_regmap_divider_ops, >>> + .ops = &clk_regmap_divider_ro_ops, >>> .parent_hws = (const struct clk_hw *[]) { >>> &g12a_mipi_dsi_pxclk_sel.hw >>> }, >>
Hi, On 24/11/2023 16:32, Jerome Brunet wrote: > > On Fri 24 Nov 2023 at 16:15, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> On 24/11/2023 15:12, Jerome Brunet wrote: >>> On Fri 24 Nov 2023 at 09:41, Neil Armstrong <neil.armstrong@linaro.org> >>> wrote: >>> >>>> In order to setup the DSI clock, let's make the unused VCLK2 clock path >>>> configuration via CCF. >>>> >>>> The nocache option is removed from following clocks: >>>> - vclk2_sel >>>> - vclk2_input >>>> - vclk2_div >>>> - vclk2 >>>> - vclk_div1 >>>> - vclk2_div2_en >>>> - vclk2_div4_en >>>> - vclk2_div6_en >>>> - vclk2_div12_en >>>> - vclk2_div2 >>>> - vclk2_div4 >>>> - vclk2_div6 >>>> - vclk2_div12 >>>> - cts_encl_sel >>>> >>>> vclk2 and vclk2_div uses the newly introduced vclk regmap driver >>>> to handle the enable and reset bits. >>>> >>>> In order to set a rate on cts_encl via the vclk2 clock path, >>>> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order >>>> to keep CCF from selection a parent. >>>> The parents of cts_encl_sel & vclk2_sel are expected to be defined >>>> in DT. >>>> >>>> The following clock scheme is to be used for DSI: >>>> >>>> xtal >>>> \_ gp0_pll_dco >>>> \_ gp0_pll >>>> |- vclk2_sel >>>> | \_ vclk2_input >>>> | \_ vclk2_div >>>> | \_ vclk2 >>>> | \_ vclk2_div1 >>>> | \_ cts_encl_sel >>>> | \_ cts_encl -> to VPU LCD Encoder >>>> |- mipi_dsi_pxclk_sel >>>> \_ mipi_dsi_pxclk_div >>>> \_ mipi_dsi_pxclk -> to DSI controller >>>> >>>> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0 >>>> for mipi_dsi_pxclk and vclk2_input. >>> Could you explain a bit more this part of about the RO ops ? >>> Maybe I'm missing something. >>> You would be relying on the reset being always the way it. It is >>> probable but not safe. >>> A way to deal with the shared GP0 would be to: >>> * cut rate propagation at mipi_dsi_pxclk_sel (already done) and >>> (vclk2_sel - TBD) ... >>> * Set GP0 base rate through assigned-clock-rate (which you already in >>> patch 11) >>> With this, I'm not sure anything needs to be RO for the rates to be set >>> properly for each subtree. >>> Also, with the subtree above and your example in patch 11, it looks odd >>> that >>> PXCLK is manually set through DT while ENCL is not. Both are input of >>> dsi driver. >> >> So the deal is about dynamic setup of clocks for DSI bridges, not really >> for panels where we can probably know in advance the clock setup. >> >> In this particular case, we need to keep a ratio between the vclk and the >> DSI bitclk, the DSI bitclk is taken from mipi_dsi_pxclk and vclk is derived >> from gp0 via vclk2. >> >> If we set the bitclk rate via mipi_dsi_pxclk, CCF will try to use mipi_dsi_pxclk_div >> to achieve the rate, > > If you have CLK_RATE_PARENT on mipi_dsi_pxclk_sel, I'm not surprised it > does that, but you don't :/ I'm quite surprised it would do that, or > even could. Hmm, I need to recheck the clock tree again... seems I got lost in the different revisions... > > From your example setting 96Mhz on both gp0 and mipi_dsi_pxclk, since > you've proposed RO-OPS, I suppose the divider is assumed to be 1 and > stay like that forever. > > With rate propagation disabled mipi_dsi_pxclk_sel and GP0 is 96Mhz, > CCF would have no choice but picking 1 as divider, so I don't understand > how CCF would pick anything else and how RO-OPS help > >> and it does it everytime I tried, breaking the vclk/bitclk ratio, >> and we have no way to know the gp0 rate in this case. > > If you really want to ensure the divider value is always 1, why not use a > divider table allowing only 1 ? Adding a comment in the g12 clock driver > would nice because this not obvious. It would be safer than relying on > the reset value. Indeed, will switch to that > >> >> I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk ratios, >> since it doesn't exist on AXG. Not sure we would ever need it... and none >> of the other upstream DSI drivers supports such setups. >> >> The main reasons I set only mipi_dsi_pxclk in DT is because : >> 1) the DSI controller requires a bitclk to respond, pclk is not enough >> 2) GP0 is disabled with an invalid config at cold boot, thus we cannot >> rely on a default/safe rate on an initial prepare_enable(). >> This permits setting initial valid state for the DSI controller, while >> the actual bitclk and vclk are calculated dynamically with panel/bridge >> runtime parameters. > > Nothing against setting rate in DT when it is static. Setting it then > overriding it is not easy to follow. Yup, would be simpler to only have parenting set in DT, since it must stay static, I'm fine trying to move rate setup to code. > > To work around GP0 not being set, assuming you want to keep rate > propagation as it is, you could call clk_set_rate() on cts_encl (possibly w/o > enabling it) to force a setup on gp0 then clk_prepare_enable() on > pxclk. You'd get a your safe rate on GP0 and the clock you need on pxclk. > > It is a bit hackish. Might be better to claim gp0 in your driver to > manage it directly, cutting rate propagation above it to control each > branch of the subtree as you need. It seems you need to have control over > that anyway and it would be clear GP0 is expected to belong to DSI. Controlling the PLL from the DSI controller seems violating too much layers, DSI controller driver is not feed directly by the PLL so it's a non-sense regarding DT properties. Setting a safe clock from the DSI controller probe is an idea, but again I don't know which value I should use... I'll review the clk parenting flags and try to hack something. Thanks, Neil > >> >> For the record, the samsung-dsim used fixed rate set from DT, and they moved >> from that in order to support more panel and bridges. >> >> But they're quite lucky because usually the DSI PLL is included in the PHY, >> this makes the Amlogic design quite unusual (like most multimedia stuf...). >> >> Neil >> >>> >>>> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> drivers/clk/meson/g12a.c | 68 +++++++++++++++++++++++++++++++++--------------- >>>> 1 file changed, 47 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c >>>> index cadd824336ad..fb3d9196a1fd 100644 >>>> --- a/drivers/clk/meson/g12a.c >>>> +++ b/drivers/clk/meson/g12a.c >>>> @@ -22,6 +22,7 @@ >>>> #include "clk-regmap.h" >>>> #include "clk-cpu-dyndiv.h" >>>> #include "vid-pll-div.h" >>>> +#include "vclk.h" >>>> #include "meson-eeclk.h" >>>> #include "g12a.h" >>>> @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = { >>>> .ops = &clk_regmap_mux_ops, >>>> .parent_hws = g12a_vclk_parent_hws, >>>> .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws), >>>> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, >>>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >>> No sure CLK_SET_RATE_PARENT is wise here. >>> What you manually set in DT for the GP0, is likely to change because of >>> this, isn't it ? >>> >>>> }, >>>> }; >>>> @@ -3193,7 +3194,7 @@ static struct clk_regmap g12a_vclk2_input = { >>>> .ops = &clk_regmap_gate_ops, >>>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_sel.hw }, >>>> .num_parents = 1, >>>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3215,19 +3216,32 @@ static struct clk_regmap g12a_vclk_div = { >>>> }; >>>> static struct clk_regmap g12a_vclk2_div = { >>>> - .data = &(struct clk_regmap_div_data){ >>>> - .offset = HHI_VIID_CLK_DIV, >>>> - .shift = 0, >>>> - .width = 8, >>>> + .data = &(struct clk_regmap_vclk_div_data){ >>>> + .div = { >>>> + .reg_off = HHI_VIID_CLK_DIV, >>>> + .shift = 0, >>>> + .width = 8, >>>> + }, >>>> + .enable = { >>>> + .reg_off = HHI_VIID_CLK_DIV, >>>> + .shift = 16, >>>> + .width = 1, >>>> + }, >>>> + .reset = { >>>> + .reg_off = HHI_VIID_CLK_DIV, >>>> + .shift = 17, >>>> + .width = 1, >>>> + }, >>>> + .flags = CLK_DIVIDER_ROUND_CLOSEST, >>>> }, >>>> .hw.init = &(struct clk_init_data){ >>>> .name = "vclk2_div", >>>> - .ops = &clk_regmap_divider_ops, >>>> + .ops = &clk_regmap_vclk_div_ops, >>>> .parent_hws = (const struct clk_hw *[]) { >>>> &g12a_vclk2_input.hw >>>> }, >>>> .num_parents = 1, >>>> - .flags = CLK_GET_RATE_NOCACHE, >>>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, >>>> }, >>>> }; >>>> @@ -3246,16 +3260,24 @@ static struct clk_regmap g12a_vclk = { >>>> }; >>>> static struct clk_regmap g12a_vclk2 = { >>>> - .data = &(struct clk_regmap_gate_data){ >>>> - .offset = HHI_VIID_CLK_CNTL, >>>> - .bit_idx = 19, >>>> + .data = &(struct clk_regmap_vclk_data){ >>>> + .enable = { >>>> + .reg_off = HHI_VIID_CLK_CNTL, >>>> + .shift = 19, >>>> + .width = 1, >>>> + }, >>>> + .reset = { >>>> + .reg_off = HHI_VIID_CLK_CNTL, >>>> + .shift = 15, >>>> + .width = 1, >>>> + }, >>>> }, >>>> .hw.init = &(struct clk_init_data) { >>>> .name = "vclk2", >>>> - .ops = &clk_regmap_gate_ops, >>>> + .ops = &clk_regmap_vclk_ops, >>>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_div.hw }, >>>> .num_parents = 1, >>>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, >>>> }, >>>> }; >>>> @@ -3339,7 +3361,7 @@ static struct clk_regmap g12a_vclk2_div1 = { >>>> .ops = &clk_regmap_gate_ops, >>>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>>> .num_parents = 1, >>>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3353,7 +3375,7 @@ static struct clk_regmap g12a_vclk2_div2_en = { >>>> .ops = &clk_regmap_gate_ops, >>>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>>> .num_parents = 1, >>>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3367,7 +3389,7 @@ static struct clk_regmap g12a_vclk2_div4_en = { >>>> .ops = &clk_regmap_gate_ops, >>>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>>> .num_parents = 1, >>>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3381,7 +3403,7 @@ static struct clk_regmap g12a_vclk2_div6_en = { >>>> .ops = &clk_regmap_gate_ops, >>>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>>> .num_parents = 1, >>>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3395,7 +3417,7 @@ static struct clk_regmap g12a_vclk2_div12_en = { >>>> .ops = &clk_regmap_gate_ops, >>>> .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, >>>> .num_parents = 1, >>>> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3461,6 +3483,7 @@ static struct clk_fixed_factor g12a_vclk2_div2 = >>>> { >>>> &g12a_vclk2_div2_en.hw >>>> }, >>>> .num_parents = 1, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3474,6 +3497,7 @@ static struct clk_fixed_factor g12a_vclk2_div4 = >>>> { >>>> &g12a_vclk2_div4_en.hw >>>> }, >>>> .num_parents = 1, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3487,6 +3511,7 @@ static struct clk_fixed_factor g12a_vclk2_div6 = >>>> { >>>> &g12a_vclk2_div6_en.hw >>>> }, >>>> .num_parents = 1, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3500,6 +3525,7 @@ static struct clk_fixed_factor g12a_vclk2_div12 >>>> = { >>>> &g12a_vclk2_div12_en.hw >>>> }, >>>> .num_parents = 1, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> }, >>>> }; >>>> @@ -3561,7 +3587,7 @@ static struct clk_regmap g12a_cts_encl_sel = { >>>> .ops = &clk_regmap_mux_ops, >>>> .parent_hws = g12a_cts_parent_hws, >>>> .num_parents = ARRAY_SIZE(g12a_cts_parent_hws), >>>> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, >>>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >>>> }, >>>> }; >>>> @@ -3717,7 +3743,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_sel >>>> = { >>>> .ops = &clk_regmap_mux_ops, >>>> .parent_hws = g12a_mipi_dsi_pxclk_parent_hws, >>>> .num_parents = ARRAY_SIZE(g12a_mipi_dsi_pxclk_parent_hws), >>>> - .flags = CLK_SET_RATE_NO_REPARENT, >>>> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >>>> }, >>>> }; >>>> @@ -3729,7 +3755,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_div >>>> = { >>>> }, >>>> .hw.init = &(struct clk_init_data){ >>>> .name = "mipi_dsi_pxclk_div", >>>> - .ops = &clk_regmap_divider_ops, >>>> + .ops = &clk_regmap_divider_ro_ops, >>>> .parent_hws = (const struct clk_hw *[]) { >>>> &g12a_mipi_dsi_pxclk_sel.hw >>>> }, >>> >
>> >>> >>> I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk ratios, >>> since it doesn't exist on AXG. Not sure we would ever need it... and none >>> of the other upstream DSI drivers supports such setups. >>> >>> The main reasons I set only mipi_dsi_pxclk in DT is because : >>> 1) the DSI controller requires a bitclk to respond, pclk is not enough >>> 2) GP0 is disabled with an invalid config at cold boot, thus we cannot >>> rely on a default/safe rate on an initial prepare_enable(). >>> This permits setting initial valid state for the DSI controller, while >>> the actual bitclk and vclk are calculated dynamically with panel/bridge >>> runtime parameters. >> Nothing against setting rate in DT when it is static. Setting it then >> overriding it is not easy to follow. > > Yup, would be simpler to only have parenting set in DT, since it must > stay static, I'm fine trying to move rate setup to code. > >> To work around GP0 not being set, assuming you want to keep rate >> propagation as it is, you could call clk_set_rate() on cts_encl (possibly w/o >> enabling it) to force a setup on gp0 then clk_prepare_enable() on >> pxclk. You'd get a your safe rate on GP0 and the clock you need on pxclk. >> It is a bit hackish. Might be better to claim gp0 in your driver to >> manage it directly, cutting rate propagation above it to control each >> branch of the subtree as you need. It seems you need to have control over >> that anyway and it would be clear GP0 is expected to belong to DSI. > > Controlling the PLL from the DSI controller seems violating too much layers, > DSI controller driver is not feed directly by the PLL so it's a non-sense > regarding DT properties. Not sure what you mean by this. You have shown in your the commit message that the DSI clocks make significant subtree. I don't see a problem if you need to manage the root of that subtree. I'd be great if you didn't need to, but it is what it is ... > > Setting a safe clock from the DSI controller probe is an idea, but again I > don't know which value I should use... You mentionned that the problem comes DSI bridges that needs to change at runtime. I don't know much about those TBH, but is there anyway you can come up with a static GP0 rate that would then be able to divide to serve all the rates bridge would need in your use case ? GP0 can go a lot higher than ~100MHz and there are dividers unused in the tree it seems. I suppose there is a finite number of required rate for each use case ? If there are not too many and there is a common divider that allows a common rate GP0 can do, it would solve your problem. It's a lot of if but It is worth checking. This is how audio works and DT assigned rate is a good match in this case. > > I'll review the clk parenting flags and try to hack something. > > Thanks, > Neil > >
Hi, On 27/11/2023 09:38, Jerome Brunet wrote: > >>> >>>> >>>> I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk ratios, >>>> since it doesn't exist on AXG. Not sure we would ever need it... and none >>>> of the other upstream DSI drivers supports such setups. >>>> >>>> The main reasons I set only mipi_dsi_pxclk in DT is because : >>>> 1) the DSI controller requires a bitclk to respond, pclk is not enough >>>> 2) GP0 is disabled with an invalid config at cold boot, thus we cannot >>>> rely on a default/safe rate on an initial prepare_enable(). >>>> This permits setting initial valid state for the DSI controller, while >>>> the actual bitclk and vclk are calculated dynamically with panel/bridge >>>> runtime parameters. >>> Nothing against setting rate in DT when it is static. Setting it then >>> overriding it is not easy to follow. >> >> Yup, would be simpler to only have parenting set in DT, since it must >> stay static, I'm fine trying to move rate setup to code. >> >>> To work around GP0 not being set, assuming you want to keep rate >>> propagation as it is, you could call clk_set_rate() on cts_encl (possibly w/o >>> enabling it) to force a setup on gp0 then clk_prepare_enable() on >>> pxclk. You'd get a your safe rate on GP0 and the clock you need on pxclk. >>> It is a bit hackish. Might be better to claim gp0 in your driver to >>> manage it directly, cutting rate propagation above it to control each >>> branch of the subtree as you need. It seems you need to have control over >>> that anyway and it would be clear GP0 is expected to belong to DSI. >> >> Controlling the PLL from the DSI controller seems violating too much layers, >> DSI controller driver is not feed directly by the PLL so it's a non-sense >> regarding DT properties. > > Not sure what you mean by this. You have shown in your the commit > message that the DSI clocks make significant subtree. I don't see a > problem if you need to manage the root of that subtree. I'd be great if > you didn't need to, but it is what it is ... I really think the choice of PLL should not be done by the DSI controller, but by the Video pipeline driver or statically until we can do this. My point is that we should only define the clocks that are related to each hardware, for example the whole VCLK/VCLK2 clocks should be defined for the VPU HW, then only the few endpoint clocks should be defined for the HDMI or DSI controllers, PHY clock and ENCI/ENCP for HDMI, DSI and ENCL for DSI. The big plan is to entirely switch to CCF for VPU, but first I want to have DSI working, and since DSI needs GP0, we need CCF for that so the intermediate plan is to have partial CCF handling only for DSI with fixed clock tree in DT, then in the future the Meson DRM driver would set up the appropriate clock tree for HDMI, DSI, Composite and perhaps DP for T7 SoCs then the controller bridge will call the clk_set_rate() in the same design I did for DSI. Here's the tracked item: https://gitlab.com/amlogic-foss/mainline-linux-issues-tracker/-/issues/9 CCF clock control is a mandatory item to solved dual-head display: https://gitlab.com/amlogic-foss/mainline-linux-issues-tracker/-/issues/6 > >> >> Setting a safe clock from the DSI controller probe is an idea, but again I >> don't know which value I should use... > > You mentionned that the problem comes DSI bridges that needs to change > at runtime. I don't know much about those TBH, but is there > anyway you can come up with a static GP0 rate that would then be able to > divide to serve all the rates bridge would need in your use case ? No, there's no such things in the DSI world, MIPI only specifies the electrical and transport layer, everything else is custom per vendor. > > GP0 can go a lot higher than ~100MHz and there are dividers unused in the > tree it seems. > > I suppose there is a finite number of required rate for each use case ? > If there are not too many and there is a common divider that allows a > common rate GP0 can do, it would solve your problem. It's a lot of if > but It is worth checking. > > This is how audio works and DT assigned rate is a good match in this case. Yeah I know, but I would love it but no... > >> >> I'll review the clk parenting flags and try to hack something. >> >> Thanks, >> Neil >> >> Thanks, Neil
diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c index cadd824336ad..fb3d9196a1fd 100644 --- a/drivers/clk/meson/g12a.c +++ b/drivers/clk/meson/g12a.c @@ -22,6 +22,7 @@ #include "clk-regmap.h" #include "clk-cpu-dyndiv.h" #include "vid-pll-div.h" +#include "vclk.h" #include "meson-eeclk.h" #include "g12a.h" @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = { .ops = &clk_regmap_mux_ops, .parent_hws = g12a_vclk_parent_hws, .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws), - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, }, }; @@ -3193,7 +3194,7 @@ static struct clk_regmap g12a_vclk2_input = { .ops = &clk_regmap_gate_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_sel.hw }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3215,19 +3216,32 @@ static struct clk_regmap g12a_vclk_div = { }; static struct clk_regmap g12a_vclk2_div = { - .data = &(struct clk_regmap_div_data){ - .offset = HHI_VIID_CLK_DIV, - .shift = 0, - .width = 8, + .data = &(struct clk_regmap_vclk_div_data){ + .div = { + .reg_off = HHI_VIID_CLK_DIV, + .shift = 0, + .width = 8, + }, + .enable = { + .reg_off = HHI_VIID_CLK_DIV, + .shift = 16, + .width = 1, + }, + .reset = { + .reg_off = HHI_VIID_CLK_DIV, + .shift = 17, + .width = 1, + }, + .flags = CLK_DIVIDER_ROUND_CLOSEST, }, .hw.init = &(struct clk_init_data){ .name = "vclk2_div", - .ops = &clk_regmap_divider_ops, + .ops = &clk_regmap_vclk_div_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_input.hw }, .num_parents = 1, - .flags = CLK_GET_RATE_NOCACHE, + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, }, }; @@ -3246,16 +3260,24 @@ static struct clk_regmap g12a_vclk = { }; static struct clk_regmap g12a_vclk2 = { - .data = &(struct clk_regmap_gate_data){ - .offset = HHI_VIID_CLK_CNTL, - .bit_idx = 19, + .data = &(struct clk_regmap_vclk_data){ + .enable = { + .reg_off = HHI_VIID_CLK_CNTL, + .shift = 19, + .width = 1, + }, + .reset = { + .reg_off = HHI_VIID_CLK_CNTL, + .shift = 15, + .width = 1, + }, }, .hw.init = &(struct clk_init_data) { .name = "vclk2", - .ops = &clk_regmap_gate_ops, + .ops = &clk_regmap_vclk_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2_div.hw }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE, }, }; @@ -3339,7 +3361,7 @@ static struct clk_regmap g12a_vclk2_div1 = { .ops = &clk_regmap_gate_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3353,7 +3375,7 @@ static struct clk_regmap g12a_vclk2_div2_en = { .ops = &clk_regmap_gate_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3367,7 +3389,7 @@ static struct clk_regmap g12a_vclk2_div4_en = { .ops = &clk_regmap_gate_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3381,7 +3403,7 @@ static struct clk_regmap g12a_vclk2_div6_en = { .ops = &clk_regmap_gate_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3395,7 +3417,7 @@ static struct clk_regmap g12a_vclk2_div12_en = { .ops = &clk_regmap_gate_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_vclk2.hw }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3461,6 +3483,7 @@ static struct clk_fixed_factor g12a_vclk2_div2 = { &g12a_vclk2_div2_en.hw }, .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3474,6 +3497,7 @@ static struct clk_fixed_factor g12a_vclk2_div4 = { &g12a_vclk2_div4_en.hw }, .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3487,6 +3511,7 @@ static struct clk_fixed_factor g12a_vclk2_div6 = { &g12a_vclk2_div6_en.hw }, .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3500,6 +3525,7 @@ static struct clk_fixed_factor g12a_vclk2_div12 = { &g12a_vclk2_div12_en.hw }, .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, }, }; @@ -3561,7 +3587,7 @@ static struct clk_regmap g12a_cts_encl_sel = { .ops = &clk_regmap_mux_ops, .parent_hws = g12a_cts_parent_hws, .num_parents = ARRAY_SIZE(g12a_cts_parent_hws), - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE, + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, }, }; @@ -3717,7 +3743,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_sel = { .ops = &clk_regmap_mux_ops, .parent_hws = g12a_mipi_dsi_pxclk_parent_hws, .num_parents = ARRAY_SIZE(g12a_mipi_dsi_pxclk_parent_hws), - .flags = CLK_SET_RATE_NO_REPARENT, + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, }, }; @@ -3729,7 +3755,7 @@ static struct clk_regmap g12a_mipi_dsi_pxclk_div = { }, .hw.init = &(struct clk_init_data){ .name = "mipi_dsi_pxclk_div", - .ops = &clk_regmap_divider_ops, + .ops = &clk_regmap_divider_ro_ops, .parent_hws = (const struct clk_hw *[]) { &g12a_mipi_dsi_pxclk_sel.hw },
In order to setup the DSI clock, let's make the unused VCLK2 clock path configuration via CCF. The nocache option is removed from following clocks: - vclk2_sel - vclk2_input - vclk2_div - vclk2 - vclk_div1 - vclk2_div2_en - vclk2_div4_en - vclk2_div6_en - vclk2_div12_en - vclk2_div2 - vclk2_div4 - vclk2_div6 - vclk2_div12 - cts_encl_sel vclk2 and vclk2_div uses the newly introduced vclk regmap driver to handle the enable and reset bits. In order to set a rate on cts_encl via the vclk2 clock path, the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order to keep CCF from selection a parent. The parents of cts_encl_sel & vclk2_sel are expected to be defined in DT. The following clock scheme is to be used for DSI: xtal \_ gp0_pll_dco \_ gp0_pll |- vclk2_sel | \_ vclk2_input | \_ vclk2_div | \_ vclk2 | \_ vclk2_div1 | \_ cts_encl_sel | \_ cts_encl -> to VPU LCD Encoder |- mipi_dsi_pxclk_sel \_ mipi_dsi_pxclk_div \_ mipi_dsi_pxclk -> to DSI controller The mipi_dsi_pxclk_div is set as RO in order to use the same GP0 for mipi_dsi_pxclk and vclk2_input. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/clk/meson/g12a.c | 68 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 21 deletions(-)