Re: [PATCH v1 2/2] clk: rockchip: describe clk_gmac using the new muxgrf type on rk3328

Heiko Stuebner
2017-02-07T00:13:41Z

Hi Elaine,

Am Montag, 6. Februar 2017, 10:50:35 CET schrieb Elaine Zhang:
> With the newly introduced clk type for muxes in the grf we now can
> describe some missing clocks, like the clk_gmac2io and clk_gmac2phy
> that selects between clk_mac2io_src and gmac_clkin based on a bit
> set in the general register files.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>

both patches look good, but I do have a question below.

Anyway, we're very late in the cycle for 4.10, so we'll need to wait until
after the next merge-window.

> ---
> drivers/clk/rockchip/clk-rk3328.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/clk/rockchip/clk-rk3328.c
> b/drivers/clk/rockchip/clk-rk3328.c index 1e384e143504..b04f29774ee7 100644
> --- a/drivers/clk/rockchip/clk-rk3328.c
> +++ b/drivers/clk/rockchip/clk-rk3328.c
> @@ -20,6 +20,7 @@
> #include <dt-bindings/clock/rk3328-cru.h>
> #include "clk.h"
>
> +#define RK3328_GRF_SOC_CON4 0x410
> #define RK3328_GRF_SOC_STATUS0 0x480
> #define RK3328_GRF_MAC_CON1 0x904
> #define RK3328_GRF_MAC_CON2 0x908
> @@ -214,6 +215,8 @@ enum rk3328_plls {
> "gmac_clkin" };
> PNAME(mux_mac2phy_src_p) = { "clk_mac2phy_src",
> "phy_50m_out" };
> +PNAME(mux_mac2io_ext_p) = { "clk_mac2io",
> + "gmac_clkin" };
>
> static struct rockchip_pll_clock rk3328_pll_clks[] __initdata = {
> [apll] = PLL(pll_rk3328, PLL_APLL, "apll", mux_pll_p,
> @@ -680,6 +683,10 @@ enum rk3328_plls {
> COMPOSITE(SCLK_MAC2IO_OUT, "clk_mac2io_out", mux_2plls_p, 0,
> RK3328_CLKSEL_CON(27), 15, 1, MFLAGS, 8, 5, DFLAGS,
> RK3328_CLKGATE_CON(3), 5, GFLAGS),
> + MUXGRF(SCLK_MAC2IO, "clk_mac2io", mux_mac2io_src_p,
> CLK_SET_RATE_NO_REPARENT, + RK3328_GRF_MAC_CON1, 10, 1, MFLAGS),
> + MUXGRF(SCLK_MAC2IO_EXT, "clk_mac2io_ext", mux_mac2io_ext_p,
> CLK_SET_RATE_NO_REPARENT, + RK3328_GRF_SOC_CON4, 14, 1, MFLAGS),
>
> COMPOSITE(SCLK_MAC2PHY_SRC, "clk_mac2phy_src", mux_2plls_p, 0,
> RK3328_CLKSEL_CON(26), 7, 1, MFLAGS, 0, 5, DFLAGS,
> @@ -691,6 +698,8 @@ enum rk3328_plls {
> COMPOSITE_NOMUX(SCLK_MAC2PHY_OUT, "clk_mac2phy_out", "clk_mac2phy", 0,
> RK3328_CLKSEL_CON(26), 8, 2, DFLAGS,
> RK3328_CLKGATE_CON(9), 2, GFLAGS),
> + MUXGRF(SCLK_MAC2PHY, "clk_mac2phy", mux_mac2phy_src_p,
> CLK_SET_RATE_NO_REPARENT, + RK3328_GRF_MAC_CON2, 10, 1, MFLAGS),

You don't set CLK_SET_RATE_PARENT but you do set CLK_SET_RATE_NO_REPARENT -
essentially disabling all automatic clock-changes for them.
Does this mean that these clocks should always only be set "manually" from
something like the assigned-clocks in dts?

This is not a problem in itself, I just want to understand how they should be
used :-)

Thanks
Heiko