| Message ID | 20250125011545.15547-2-zhangqing@rock-chips.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: rockchip: Fixed some incorrect commits | expand |
Hi Elaine, Am 25.01.25 um 02:15 schrieb Elaine Zhang: > This reverts commit 1d34b9757523c1ad547bd6d040381f62d74a3189. > > RK3228 Only GPLL and CPLL, GPLL is a common clock, does not allow > dclk_vop to change its frequency, CPLL is used by GMAC, > if dclk_vop use CLK_SET_RATE_PARENT and CLK_SET_RATE_NO_REPARENT flags will > affect the GMAC function. > how do you come to this conclusion? On the RK3228, hdmiphy is the default parent of dclk_vop, a clock that is not even generated by the CRU but is the output of an external HDMI PHY. The CLK_SET_RATE_NO_REPARENT flag ensures that the parent of dclk_vop never changes to sclk_vop_pre (and thus never uses CPLL or GPLL). With CLK_SET_RATE_PARENT we only ensure that we can use all rates of [0] since there is no divider between dclk_vop and hdmiphy. Overall it is pretty much the same situation as for RK3328, which handles these clocks in the same way (see dclk_lcdc for RK3328). Regards, Alex [0] [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c?h=v6.13#n293 > If the client application does not use GMAC and CPLL is free, make this > change on the local branch. > > Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com> > --- > drivers/clk/rockchip/clk-rk3228.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c > index ed602c27b624..9c0284607766 100644 > --- a/drivers/clk/rockchip/clk-rk3228.c > +++ b/drivers/clk/rockchip/clk-rk3228.c > @@ -409,7 +409,7 @@ static struct rockchip_clk_branch rk3228_clk_branches[] __initdata = { > RK2928_CLKSEL_CON(29), 0, 3, DFLAGS), > DIV(0, "sclk_vop_pre", "sclk_vop_src", 0, > RK2928_CLKSEL_CON(27), 8, 8, DFLAGS), > - MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > + MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, 0, > RK2928_CLKSEL_CON(27), 1, 1, MFLAGS), > > FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
Hi Elaine, On 2025-01-25 06:38, Alex Bee wrote: > Hi Elaine, > > Am 25.01.25 um 02:15 schrieb Elaine Zhang: >> This reverts commit 1d34b9757523c1ad547bd6d040381f62d74a3189. >> >> RK3228 Only GPLL and CPLL, GPLL is a common clock, does not allow >> dclk_vop to change its frequency, CPLL is used by GMAC, >> if dclk_vop use CLK_SET_RATE_PARENT and CLK_SET_RATE_NO_REPARENT flags will >> affect the GMAC function. >> > how do you come to this conclusion? > > On the RK3228, hdmiphy is the default parent of dclk_vop, a clock that is > not even generated by the CRU but is the output of an external HDMI PHY. > The CLK_SET_RATE_NO_REPARENT flag ensures that the parent of dclk_vop never > changes to sclk_vop_pre (and thus never uses CPLL or GPLL). With > CLK_SET_RATE_PARENT we only ensure that we can use all rates of [0] since > there is no divider between dclk_vop and hdmiphy. Overall it is pretty much > the same situation as for RK3328, which handles these clocks in the same > way (see dclk_lcdc for RK3328). I came to similar conclusion for v1 of this patch, see [2]. Maybe we should assign clk parent in DT similar to rk3328.dtsi to make it extra clear that hdmiphy should be used as parent? assigned-clocks = <&cru DCLK_VOP>; assigned-clock-parents = <&cru SCLK_HDMI_PHY>; Also for next revert patch you send, please include the patch author in the recipient list :-) [2] https://lore.kernel.org/all/d95d317c-5f6e-42bd-94a9-e1a6c7685e2f@kwiboo.se/ Regards, Jonas > > Regards, > Alex > > [0] [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c?h=v6.13#n293 >> If the client application does not use GMAC and CPLL is free, make this >> change on the local branch. >> >> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com> >> --- >> drivers/clk/rockchip/clk-rk3228.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c >> index ed602c27b624..9c0284607766 100644 >> --- a/drivers/clk/rockchip/clk-rk3228.c >> +++ b/drivers/clk/rockchip/clk-rk3228.c >> @@ -409,7 +409,7 @@ static struct rockchip_clk_branch rk3228_clk_branches[] __initdata = { >> RK2928_CLKSEL_CON(29), 0, 3, DFLAGS), >> DIV(0, "sclk_vop_pre", "sclk_vop_src", 0, >> RK2928_CLKSEL_CON(27), 8, 8, DFLAGS), >> - MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, >> + MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, 0, >> RK2928_CLKSEL_CON(27), 1, 1, MFLAGS), >> >> FACTOR(0, "xin12m", "xin24m", 0, 1, 2), > >
diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c index ed602c27b624..9c0284607766 100644 --- a/drivers/clk/rockchip/clk-rk3228.c +++ b/drivers/clk/rockchip/clk-rk3228.c @@ -409,7 +409,7 @@ static struct rockchip_clk_branch rk3228_clk_branches[] __initdata = { RK2928_CLKSEL_CON(29), 0, 3, DFLAGS), DIV(0, "sclk_vop_pre", "sclk_vop_src", 0, RK2928_CLKSEL_CON(27), 8, 8, DFLAGS), - MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, + MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, 0, RK2928_CLKSEL_CON(27), 1, 1, MFLAGS), FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
This reverts commit 1d34b9757523c1ad547bd6d040381f62d74a3189. RK3228 Only GPLL and CPLL, GPLL is a common clock, does not allow dclk_vop to change its frequency, CPLL is used by GMAC, if dclk_vop use CLK_SET_RATE_PARENT and CLK_SET_RATE_NO_REPARENT flags will affect the GMAC function. If the client application does not use GMAC and CPLL is free, make this change on the local branch. Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com> --- drivers/clk/rockchip/clk-rk3228.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)