| Message ID | 20220915072458.18232-6-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MediaTek SoC safe clock muxing and GPU clocks | expand |
On Thu, Sep 15, 2022 at 3:25 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > The MFG_BG3D is a gate to enable/disable clock output to the GPU, > but the actual output is decided by multiple muxes; in particular: > mfg_ck_fast_ref muxes between "slow" (top_mfg_core_tmp) and > "fast" (MFGPLL) clock, while top_mfg_core_tmp muxes between the > 26MHz clock and various system PLLs. > > This also implies that "top_mfg_core_tmp" is a parent of the > "mfg_ck_fast_ref" mux (and not vice-versa), so reparent the I don't see where this was the case though? I think what you meant was that the direct parent for "mfg_bg3d" is "mfg_ck_fast_ref, not "top_mfg_core_tmp"? > MFG_BG3D gate to the latter and add the CLK_SET_RATE_PARENT > flag to it: this way we ensure propagating rate changes that > are requested on MFG_BG3D along its entire clock tree. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Otherwise, Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > --- > drivers/clk/mediatek/clk-mt8195-mfg.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/mediatek/clk-mt8195-mfg.c b/drivers/clk/mediatek/clk-mt8195-mfg.c > index 9411c556a5a9..c94cb71bd9b9 100644 > --- a/drivers/clk/mediatek/clk-mt8195-mfg.c > +++ b/drivers/clk/mediatek/clk-mt8195-mfg.c > @@ -17,10 +17,12 @@ static const struct mtk_gate_regs mfg_cg_regs = { > }; > > #define GATE_MFG(_id, _name, _parent, _shift) \ > - GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr) > + GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, \ > + _shift, &mtk_clk_gate_ops_setclr, \ > + CLK_SET_RATE_PARENT) > > static const struct mtk_gate mfg_clks[] = { > - GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "top_mfg_core_tmp", 0), > + GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_ck_fast_ref", 0), > }; > > static const struct mtk_clk_desc mfg_desc = { > -- > 2.37.2 >
Il 26/09/22 05:27, Chen-Yu Tsai ha scritto: > On Thu, Sep 15, 2022 at 3:25 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> The MFG_BG3D is a gate to enable/disable clock output to the GPU, >> but the actual output is decided by multiple muxes; in particular: >> mfg_ck_fast_ref muxes between "slow" (top_mfg_core_tmp) and >> "fast" (MFGPLL) clock, while top_mfg_core_tmp muxes between the >> 26MHz clock and various system PLLs. >> >> This also implies that "top_mfg_core_tmp" is a parent of the >> "mfg_ck_fast_ref" mux (and not vice-versa), so reparent the > > I don't see where this was the case though? I think what you meant > was that the direct parent for "mfg_bg3d" is "mfg_ck_fast_ref, not > "top_mfg_core_tmp"? > MFG_BG3D's direct parent is mfg_ck_fast_ref - yes - but in the commit message I am explaining how the clock tree for MFG_BG3D really is and, in particular, I'm then explaining that: * parenting MFG_BG3D to "top_mfg_core_tmp" is wrong; because * "top_mfg_core_tmp" is a parent of "mfg_ck_fast_ref" (not the other way around). So, the question in your comment is addressed just a little later.... >> MFG_BG3D gate to the latter and add the CLK_SET_RATE_PARENT ...here, where I say "reparent MFG_BG3D to the latter", where "the latter" is, exactly "mfg_ck_fast_ref". I hope you now understand what I am trying to communicate :-) However, if even after that you still think that the commit description should be rewritten in some less tangled and/or more understandable way, I definitely can do that. Please confirm :-) >> flag to it: this way we ensure propagating rate changes that >> are requested on MFG_BG3D along its entire clock tree. >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > Otherwise, > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > >> --- >> drivers/clk/mediatek/clk-mt8195-mfg.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/mediatek/clk-mt8195-mfg.c b/drivers/clk/mediatek/clk-mt8195-mfg.c >> index 9411c556a5a9..c94cb71bd9b9 100644 >> --- a/drivers/clk/mediatek/clk-mt8195-mfg.c >> +++ b/drivers/clk/mediatek/clk-mt8195-mfg.c >> @@ -17,10 +17,12 @@ static const struct mtk_gate_regs mfg_cg_regs = { >> }; >> >> #define GATE_MFG(_id, _name, _parent, _shift) \ >> - GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr) >> + GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, \ >> + _shift, &mtk_clk_gate_ops_setclr, \ >> + CLK_SET_RATE_PARENT) >> >> static const struct mtk_gate mfg_clks[] = { >> - GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "top_mfg_core_tmp", 0), >> + GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_ck_fast_ref", 0), >> }; >> >> static const struct mtk_clk_desc mfg_desc = { >> -- >> 2.37.2 >>
On Mon, Sep 26, 2022 at 4:36 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 26/09/22 05:27, Chen-Yu Tsai ha scritto: > > On Thu, Sep 15, 2022 at 3:25 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@collabora.com> wrote: > >> > >> The MFG_BG3D is a gate to enable/disable clock output to the GPU, > >> but the actual output is decided by multiple muxes; in particular: > >> mfg_ck_fast_ref muxes between "slow" (top_mfg_core_tmp) and > >> "fast" (MFGPLL) clock, while top_mfg_core_tmp muxes between the > >> 26MHz clock and various system PLLs. > >> > >> This also implies that "top_mfg_core_tmp" is a parent of the > >> "mfg_ck_fast_ref" mux (and not vice-versa), so reparent the > > > > I don't see where this was the case though? I think what you meant > > was that the direct parent for "mfg_bg3d" is "mfg_ck_fast_ref, not > > "top_mfg_core_tmp"? > > > > MFG_BG3D's direct parent is mfg_ck_fast_ref - yes - but in the commit message > I am explaining how the clock tree for MFG_BG3D really is and, in particular, > I'm then explaining that: > * parenting MFG_BG3D to "top_mfg_core_tmp" is wrong; because > * "top_mfg_core_tmp" is a parent of "mfg_ck_fast_ref" (not the other way around). > > So, the question in your comment is addressed just a little later.... > > >> MFG_BG3D gate to the latter and add the CLK_SET_RATE_PARENT > > ...here, where I say "reparent MFG_BG3D to the latter", where "the latter" is, > exactly "mfg_ck_fast_ref". > > I hope you now understand what I am trying to communicate :-) > > However, if even after that you still think that the commit description should > be rewritten in some less tangled and/or more understandable way, I definitely > can do that. > > Please confirm :-) I think This also implies that "top_mfg_core_tmp" is a parent of the "mfg_ck_fast_ref" mux (and not vice-versa) actually confused me. Maybe just say The clock gate comes after all the muxes, so its parent is mfg_ck_fast_ref, not top_mfg_core_tmp. Reparent mfg_bg3d to the latter to match the hardware ... Since you are fixing the topology, could you also add a fixes tag? Thanks ChenYu > >> flag to it: this way we ensure propagating rate changes that > >> are requested on MFG_BG3D along its entire clock tree. > >> > >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > > > Otherwise, > > > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > > > > >> --- > >> drivers/clk/mediatek/clk-mt8195-mfg.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/clk/mediatek/clk-mt8195-mfg.c b/drivers/clk/mediatek/clk-mt8195-mfg.c > >> index 9411c556a5a9..c94cb71bd9b9 100644 > >> --- a/drivers/clk/mediatek/clk-mt8195-mfg.c > >> +++ b/drivers/clk/mediatek/clk-mt8195-mfg.c > >> @@ -17,10 +17,12 @@ static const struct mtk_gate_regs mfg_cg_regs = { > >> }; > >> > >> #define GATE_MFG(_id, _name, _parent, _shift) \ > >> - GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr) > >> + GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, \ > >> + _shift, &mtk_clk_gate_ops_setclr, \ > >> + CLK_SET_RATE_PARENT) > >> > >> static const struct mtk_gate mfg_clks[] = { > >> - GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "top_mfg_core_tmp", 0), > >> + GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_ck_fast_ref", 0), > >> }; > >> > >> static const struct mtk_clk_desc mfg_desc = { > >> -- > >> 2.37.2 > >> > >
Il 26/09/22 10:57, Chen-Yu Tsai ha scritto: > On Mon, Sep 26, 2022 at 4:36 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> Il 26/09/22 05:27, Chen-Yu Tsai ha scritto: >>> On Thu, Sep 15, 2022 at 3:25 PM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@collabora.com> wrote: >>>> >>>> The MFG_BG3D is a gate to enable/disable clock output to the GPU, >>>> but the actual output is decided by multiple muxes; in particular: >>>> mfg_ck_fast_ref muxes between "slow" (top_mfg_core_tmp) and >>>> "fast" (MFGPLL) clock, while top_mfg_core_tmp muxes between the >>>> 26MHz clock and various system PLLs. >>>> >>>> This also implies that "top_mfg_core_tmp" is a parent of the >>>> "mfg_ck_fast_ref" mux (and not vice-versa), so reparent the >>> >>> I don't see where this was the case though? I think what you meant >>> was that the direct parent for "mfg_bg3d" is "mfg_ck_fast_ref, not >>> "top_mfg_core_tmp"? >>> >> >> MFG_BG3D's direct parent is mfg_ck_fast_ref - yes - but in the commit message >> I am explaining how the clock tree for MFG_BG3D really is and, in particular, >> I'm then explaining that: >> * parenting MFG_BG3D to "top_mfg_core_tmp" is wrong; because >> * "top_mfg_core_tmp" is a parent of "mfg_ck_fast_ref" (not the other way around). >> >> So, the question in your comment is addressed just a little later.... >> >>>> MFG_BG3D gate to the latter and add the CLK_SET_RATE_PARENT >> >> ...here, where I say "reparent MFG_BG3D to the latter", where "the latter" is, >> exactly "mfg_ck_fast_ref". >> >> I hope you now understand what I am trying to communicate :-) >> >> However, if even after that you still think that the commit description should >> be rewritten in some less tangled and/or more understandable way, I definitely >> can do that. >> >> Please confirm :-) > > I think > > This also implies that "top_mfg_core_tmp" is a parent of the > "mfg_ck_fast_ref" mux (and not vice-versa) > > actually confused me. > > Maybe just say > > The clock gate comes after all the muxes, so its parent is > mfg_ck_fast_ref, not top_mfg_core_tmp. Reparent mfg_bg3d to > the latter to match the hardware ... > > Since you are fixing the topology, could you also add a fixes tag? > > Yeah, sure! I'll send a new version most probably today. Thanks for the review(s)! Cheers, Angelo > Thanks > ChenYu > > >>>> flag to it: this way we ensure propagating rate changes that >>>> are requested on MFG_BG3D along its entire clock tree. >>>> >>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>> >>> Otherwise, >>> >>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> >>> >>> >>>> --- >>>> drivers/clk/mediatek/clk-mt8195-mfg.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/clk/mediatek/clk-mt8195-mfg.c b/drivers/clk/mediatek/clk-mt8195-mfg.c >>>> index 9411c556a5a9..c94cb71bd9b9 100644 >>>> --- a/drivers/clk/mediatek/clk-mt8195-mfg.c >>>> +++ b/drivers/clk/mediatek/clk-mt8195-mfg.c >>>> @@ -17,10 +17,12 @@ static const struct mtk_gate_regs mfg_cg_regs = { >>>> }; >>>> >>>> #define GATE_MFG(_id, _name, _parent, _shift) \ >>>> - GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr) >>>> + GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, \ >>>> + _shift, &mtk_clk_gate_ops_setclr, \ >>>> + CLK_SET_RATE_PARENT) >>>> >>>> static const struct mtk_gate mfg_clks[] = { >>>> - GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "top_mfg_core_tmp", 0), >>>> + GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_ck_fast_ref", 0), >>>> }; >>>> >>>> static const struct mtk_clk_desc mfg_desc = { >>>> -- >>>> 2.37.2 >>>> >> >>
diff --git a/drivers/clk/mediatek/clk-mt8195-mfg.c b/drivers/clk/mediatek/clk-mt8195-mfg.c index 9411c556a5a9..c94cb71bd9b9 100644 --- a/drivers/clk/mediatek/clk-mt8195-mfg.c +++ b/drivers/clk/mediatek/clk-mt8195-mfg.c @@ -17,10 +17,12 @@ static const struct mtk_gate_regs mfg_cg_regs = { }; #define GATE_MFG(_id, _name, _parent, _shift) \ - GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr) + GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, \ + _shift, &mtk_clk_gate_ops_setclr, \ + CLK_SET_RATE_PARENT) static const struct mtk_gate mfg_clks[] = { - GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "top_mfg_core_tmp", 0), + GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_ck_fast_ref", 0), }; static const struct mtk_clk_desc mfg_desc = {
The MFG_BG3D is a gate to enable/disable clock output to the GPU, but the actual output is decided by multiple muxes; in particular: mfg_ck_fast_ref muxes between "slow" (top_mfg_core_tmp) and "fast" (MFGPLL) clock, while top_mfg_core_tmp muxes between the 26MHz clock and various system PLLs. This also implies that "top_mfg_core_tmp" is a parent of the "mfg_ck_fast_ref" mux (and not vice-versa), so reparent the MFG_BG3D gate to the latter and add the CLK_SET_RATE_PARENT flag to it: this way we ensure propagating rate changes that are requested on MFG_BG3D along its entire clock tree. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/clk/mediatek/clk-mt8195-mfg.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)