Re: [PATCH v1 19/45] clk: mediatek: mt8183: Convert all remaining clocks to common probe

[Chen-Yu Tsai]

On Tue, Feb 7, 2023 at 8:14 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> Il 07/02/23 10:58, Chen-Yu Tsai ha scritto:
> > On Mon, Feb 6, 2023 at 11:30 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>
> >> Switch to mtk_clk_simple_{probe,remove}() for infracfg and topckgen
> >> clocks on MT8183 to allow full module build for clock drivers.
> >> In order to do this, like done for other MediaTek clock drivers, it
> >> was necessary to join top_early_divs with top_divs and to stop
> >> registering the `clk13m` clock early.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> >> ---
> >>   drivers/clk/mediatek/clk-mt8183.c | 160 ++++++------------------------
> >>   1 file changed, 28 insertions(+), 132 deletions(-)
> >>
> >> diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
> >> index 0fad2cf7f41b..035fdd02f0be 100644
> >> --- a/drivers/clk/mediatek/clk-mt8183.c
> >> +++ b/drivers/clk/mediatek/clk-mt8183.c
> >> @@ -25,11 +25,8 @@ static const struct mtk_fixed_clk top_fixed_clks[] = {
> >>          FIXED_CLK(CLK_TOP_UNIVP_192M, "univpll_192m", "univpll", 192000000),
> >>   };
> >>
> >> -static const struct mtk_fixed_factor top_early_divs[] = {
> >> -       FACTOR(CLK_TOP_CLK13M, "clk13m", "clk26m", 1, 2),
> >> -};
> >> -
> >>   static const struct mtk_fixed_factor top_divs[] = {
> >> +       FACTOR(CLK_TOP_CLK13M, "clk13m", "clk26m", 1, 2),
> >
> > A clock with the same name is now present in the DT, and so this clock
> > would fail to register. We should drop this one completely and point
> > any references to it internally to "csw_f26m_ck_d2".
> >
> >>          FACTOR(CLK_TOP_F26M_CK_D2, "csw_f26m_ck_d2", "clk26m", 1, 2),
> >
> > MT8192 and MT8195 aren't affected because they only have "csw_f26m_ck_d2",
> > which systimer was referencing.
> >
> >>          FACTOR_FLAGS(CLK_TOP_SYSPLL_CK, "syspll_ck", "mainpll", 1, 1, 0),
> >>          FACTOR_FLAGS(CLK_TOP_SYSPLL_D2, "syspll_d2", "syspll_ck", 1, 2, 0),
> >> @@ -809,26 +806,6 @@ static const struct mtk_clk_rst_desc clk_rst_desc = {
> >>          .rst_bank_nr = ARRAY_SIZE(infra_rst_ofs),
> >>   };
> >>
> >> -static struct clk_hw_onecell_data *top_clk_data;
> >> -
> >> -static void clk_mt8183_top_init_early(struct device_node *node)
> >> -{
> >> -       int i;
> >> -
> >> -       top_clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK);
> >> -
> >> -       for (i = 0; i < CLK_TOP_NR_CLK; i++)
> >> -               top_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> >> -
> >> -       mtk_clk_register_factors(top_early_divs, ARRAY_SIZE(top_early_divs),
> >> -                       top_clk_data);
> >
> > And since we used to not do error checking, the name conflict was OK.
> > With the new common probe, it's not.
> >
>
> That makes me proud of my changes to extend the new common probe mechanism,
> as this is one of (hopefully not) many wrongs that slipped through without
> any apparent issue.
> Anyway, there was no reference to this clk13m (nor CLK_TOP_CLK13M) anywhere
> so I changed this commit to just "forget about this clock" (advertising the
> reason in the commit description, of course).

I think I should send this as a separate patch as a follow-up to the systimer
changes. And we should keep the CLK_TOP_CLK13M entry valid, since that's
the entry referenced in old DTs, but change its name to "csw_f26m_ck_d2".

In short we are actually merging CLK_TOP_CLK13M and CLK_TOP_F26M_CK_D2,
with the former surviving but with a name change. CLK_TOP_F26M_CK_D2
is only referenced internally in TOPCKGEN.

> Is MT8183's cpufreq working after this change, or is it still not behaving?

Yes it's back.

ChenYu