| Message ID | 20230206152928.918562-20-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MediaTek clocks: full module build and cleanups | expand |
On Mon, Feb 6, 2023 at 11:30 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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@collabora.com> > --- > 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. ChenYu
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@collabora.com> 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@collabora.com> >> --- >> 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). Is MT8183's cpufreq working after this change, or is it still not behaving? If not, can you please provide a log for me to check? I don't have any MT8183 device locally, for now at least. Thanks, Angelo
On Tue, Feb 7, 2023 at 8:14 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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@collabora.com> 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@collabora.com> > >> --- > >> 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
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), FACTOR(CLK_TOP_F26M_CK_D2, "csw_f26m_ck_d2", "clk26m", 1, 2), 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); - - of_clk_add_hw_provider(node, of_clk_hw_onecell_get, top_clk_data); -} - -CLK_OF_DECLARE_DRIVER(mt8183_topckgen, "mediatek,mt8183-topckgen", - clk_mt8183_top_init_early); - /* Register mux notifier for MFG mux */ static int clk_mt8183_reg_mfg_mux_notifier(struct device *dev, struct clk *clk) { @@ -851,134 +828,53 @@ static int clk_mt8183_reg_mfg_mux_notifier(struct device *dev, struct clk *clk) return devm_mtk_clk_mux_notifier_register(dev, clk, mfg_mux_nb); } -static int clk_mt8183_top_probe(struct platform_device *pdev) -{ - void __iomem *base; - struct device_node *node = pdev->dev.of_node; - int ret; - - base = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(base)) - return PTR_ERR(base); - - mtk_clk_register_fixed_clks(top_fixed_clks, ARRAY_SIZE(top_fixed_clks), - top_clk_data); - - mtk_clk_register_factors(top_early_divs, ARRAY_SIZE(top_early_divs), - top_clk_data); - - mtk_clk_register_factors(top_divs, ARRAY_SIZE(top_divs), top_clk_data); - - mtk_clk_register_muxes(&pdev->dev, top_muxes, - ARRAY_SIZE(top_muxes), node, - &mt8183_clk_lock, top_clk_data); - - mtk_clk_register_composites(&pdev->dev, top_aud_comp, - ARRAY_SIZE(top_aud_comp), base, - &mt8183_clk_lock, top_clk_data); - - mtk_clk_register_gates(&pdev->dev, node, top_clks, - ARRAY_SIZE(top_clks), top_clk_data); - - ret = clk_mt8183_reg_mfg_mux_notifier(&pdev->dev, - top_clk_data->hws[CLK_TOP_MUX_MFG]->clk); - if (ret) - return ret; - - return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, - top_clk_data); -} - -static int clk_mt8183_mcu_probe(struct platform_device *pdev) -{ - struct clk_hw_onecell_data *clk_data; - struct device_node *node = pdev->dev.of_node; - void __iomem *base; - - base = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(base)) - return PTR_ERR(base); - - clk_data = mtk_alloc_clk_data(CLK_MCU_NR_CLK); - - mtk_clk_register_composites(&pdev->dev, mcu_muxes, - ARRAY_SIZE(mcu_muxes), base, - &mt8183_clk_lock, clk_data); - - return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data); -} - -static const struct of_device_id of_match_clk_mt8183[] = { - { - .compatible = "mediatek,mt8183-topckgen", - .data = clk_mt8183_top_probe, - }, { - .compatible = "mediatek,mt8183-mcucfg", - .data = clk_mt8183_mcu_probe, - }, { - /* sentinel */ - } -}; - -static int clk_mt8183_probe(struct platform_device *pdev) -{ - int (*clk_probe)(struct platform_device *pdev); - int r; - - clk_probe = of_device_get_match_data(&pdev->dev); - if (!clk_probe) - return -EINVAL; - - r = clk_probe(pdev); - if (r) - dev_err(&pdev->dev, - "could not register clock provider: %s: %d\n", - pdev->name, r); - - return r; -} - static const struct mtk_clk_desc infra_desc = { .clks = infra_clks, .num_clks = ARRAY_SIZE(infra_clks), .rst_desc = &clk_rst_desc, }; +static const struct mtk_clk_desc mcu_desc = { + .composite_clks = mcu_muxes, + .num_composite_clks = ARRAY_SIZE(mcu_muxes), + .clk_lock = &mt8183_clk_lock, +}; + static const struct mtk_clk_desc peri_desc = { .clks = peri_clks, .num_clks = ARRAY_SIZE(peri_clks), }; -static const struct of_device_id of_match_clk_mt8183_simple[] = { +static const struct mtk_clk_desc topck_desc = { + .fixed_clks = top_fixed_clks, + .num_fixed_clks = ARRAY_SIZE(top_fixed_clks), + .factor_clks = top_divs, + .num_factor_clks = ARRAY_SIZE(top_divs), + .mux_clks = top_muxes, + .num_mux_clks = ARRAY_SIZE(top_muxes), + .composite_clks = top_aud_comp, + .num_composite_clks = ARRAY_SIZE(top_aud_comp), + .clks = top_clks, + .num_clks = ARRAY_SIZE(top_clks), + .clk_lock = &mt8183_clk_lock, + .clk_notifier_func = clk_mt8183_reg_mfg_mux_notifier, + .mfg_clk_idx = CLK_TOP_MUX_MFG, +}; + +static const struct of_device_id of_match_clk_mt8183[] = { { .compatible = "mediatek,mt8183-infracfg", .data = &infra_desc }, + { .compatible = "mediatek,mt8183-mcucfg", .data = &mcu_desc }, { .compatible = "mediatek,mt8183-pericfg", .data = &peri_desc, }, + { .compatible = "mediatek,mt8183-topckgen", .data = &topck_desc }, { /* sentinel */ } }; -static struct platform_driver clk_mt8183_simple_drv = { +static struct platform_driver clk_mt8183_drv = { .probe = mtk_clk_simple_probe, .remove = mtk_clk_simple_remove, - .driver = { - .name = "clk-mt8183-simple", - .of_match_table = of_match_clk_mt8183_simple, - }, -}; - -static struct platform_driver clk_mt8183_drv = { - .probe = clk_mt8183_probe, .driver = { .name = "clk-mt8183", .of_match_table = of_match_clk_mt8183, }, }; - -static int __init clk_mt8183_init(void) -{ - int ret = platform_driver_register(&clk_mt8183_drv); - - if (ret) - return ret; - return platform_driver_register(&clk_mt8183_simple_drv); -} - -arch_initcall(clk_mt8183_init); +module_platform_driver(clk_mt8183_drv)
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@collabora.com> --- drivers/clk/mediatek/clk-mt8183.c | 160 ++++++------------------------ 1 file changed, 28 insertions(+), 132 deletions(-)