Date:   Wed, 1 Nov 2023 15:48:14 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     "yu-chang.lee" <yu-chang.lee@...iatek.com>
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Markus Schneider-Pargmann <msp@...libre.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, Rob Herring <robh@...nel.org>,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        "Garmin . Chang" <Garmin.Chang@...iatek.com>
Subject: Re: [PATCH] clk: mediatek: mt8188: Set clock parent for TOP_EDP and TOP_DP

On Mon, Oct 30, 2023 at 6:57 PM yu-chang.lee <yu-chang.lee@...iatek.com> wrote:
>
> Modify MT8188 edp, dp clock and add .determine_rate() callback to
> dynamically set parent for TOP_EDP and TOP_DP. By separating tvdpll
> output into different interval, we can make sure VCO operate at optimal
> condition.

This patch does two things: a) separate eDP and DP clocks onto two separate
PLLs, and b) customize how the parent (divider) is selected. They should
be separate patches.

(a) can be done like how Angelo proposed for MT8195 [1]. If PLL separation
is all you need, please post a patch like it.

It's unclear to me how (b) is an improvement over existing code. The MediaTek
clock driver library already specifies proper ranges for the PLLs, and the
mux code should go through all the clock parents and choose one that can
generate a clock rate closest to the requested one while using one of the
dividers and a PLL rate within spec. If that isn't working, it should be
fixed, not worked around.


Regards
ChenYu

[1] https://lore.kernel.org/linux-mediatek/20231018103546.48174-1-angelogioacchino.delregno@collabora.com/

>
> Signed-off-by: yu-chang.lee <yu-chang.lee@...iatek.com>
> Signed-off-by: Garmin.Chang <Garmin.Chang@...iatek.com>
>
> ---
>  drivers/clk/mediatek/clk-mt8188-topckgen.c | 74 +++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8188-topckgen.c b/drivers/clk/mediatek/clk-mt8188-topckgen.c
> index e330a4f9a0c3..921fb31fe4b1 100644
> --- a/drivers/clk/mediatek/clk-mt8188-topckgen.c
> +++ b/drivers/clk/mediatek/clk-mt8188-topckgen.c
> @@ -487,6 +487,74 @@ static const char * const dp_parents[] = {
>         "tvdpll2_d16"
>  };
>
> +#define VCO_OPP_LOW    7000000
> +#define VCO_OPP_MEDIUM 200000000
> +
> +enum dp_edp_parent_index {
> +       PLL_D4 = 2,
> +       PLL_D8,
> +       PLL_D16
> +};
> +
> +static int dp_clk_determine_rate(struct clk_hw *hw,
> +                                  struct clk_rate_request *req)
> +{
> +       const char *clk_hw_name = clk_hw_get_name(hw);
> +       unsigned int pow, parent_id, is_edp = 0;
> +       struct clk_rate_request parent_req = *req;
> +       struct clk_hw *parent_hw = clk_hw_get_parent(hw);
> +       unsigned long parent_rate = clk_hw_get_rate(parent_hw);
> +       int ret = 0;
> +
> +       if (!strncmp(clk_hw_name, "top_edp", 7))
> +               is_edp = 1;
> +
> +       if (req->rate < VCO_OPP_LOW)
> +               pow = PLL_D16;
> +       else if (req->rate < VCO_OPP_MEDIUM)
> +               pow = PLL_D8;
> +       else
> +               pow = PLL_D4;
> +
> +       if (is_edp)
> +               parent_id = (pow << 1) - 1;
> +       else
> +               parent_id = pow << 1;
> +
> +       /* re-parenting and determine rate */
> +       if (clk_hw_can_set_rate_parent(hw)) {
> +               parent_hw = clk_hw_get_parent_by_index(hw, parent_id);
> +               ret = __clk_determine_rate(parent_hw, &parent_req);
> +               if (ret)
> +                       return ret;
> +
> +               req->best_parent_hw = parent_hw;
> +               req->rate = req->best_parent_rate = parent_req.rate;
> +       } else {
> +               req->best_parent_hw = parent_hw;
> +               req->rate = req->best_parent_rate = parent_rate;
> +       }
> +
> +       return ret;
> +}
> +
> +static struct clk_ops dp_clk_ops;
> +
> +static void dp_clk_ops_init(void)
> +{
> +       dp_clk_ops = mtk_mux_gate_clr_set_upd_ops;
> +       dp_clk_ops.determine_rate = dp_clk_determine_rate;
> +}
> +
> +#define DP_MUX(_id, _name, _parents, _mux_ofs, _mux_set_ofs,   \
> +                _mux_clr_ofs, _shift, _width, _gate, _upd_ofs, \
> +                _upd)  \
> +       GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs,  \
> +               _mux_set_ofs, _mux_clr_ofs, _shift, _width,     \
> +               _gate, _upd_ofs, _upd, CLK_SET_RATE_PARENT,     \
> +               dp_clk_ops)
> +
> +
>  static const char * const edp_parents[] = {
>         "clk26m",
>         "tvdpll1_d2",
> @@ -1038,9 +1106,9 @@ static const struct mtk_mux top_mtk_muxes[] = {
>         MUX_GATE_CLR_SET_UPD(CLK_TOP_SSPM, "top_sspm",
>                              sspm_parents, 0x080, 0x084, 0x088, 24, 4, 31, 0x08, 3),
>         /* CLK_CFG_9 */
> -       MUX_GATE_CLR_SET_UPD(CLK_TOP_DP, "top_dp",
> +       DP_MUX(CLK_TOP_DP, "top_dp",
>                              dp_parents, 0x08C, 0x090, 0x094, 0, 4, 7, 0x08, 4),
> -       MUX_GATE_CLR_SET_UPD(CLK_TOP_EDP, "top_edp",
> +       DP_MUX(CLK_TOP_EDP, "top_edp",
>                              edp_parents, 0x08C, 0x090, 0x094, 8, 4, 15, 0x08, 5),
>         MUX_GATE_CLR_SET_UPD(CLK_TOP_DPI, "top_dpi",
>                              dpi_parents, 0x08C, 0x090, 0x094, 16, 4, 23, 0x08, 6),
> @@ -1267,6 +1335,8 @@ static int clk_mt8188_topck_probe(struct platform_device *pdev)
>                 goto free_top_data;
>         }
>
> +       dp_clk_ops_init();
> +
>         r = mtk_clk_register_fixed_clks(top_fixed_clks, ARRAY_SIZE(top_fixed_clks),
>                                         top_clk_data);
>         if (r)
> --
> 2.18.0
>
>

Powered by blists - more mailing lists