| Message ID | 20220926102523.2367530-4-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: mediatek: More cleanups | expand |
Il 26/09/22 12:25, Chen-Yu Tsai ha scritto: > top_early_divs are registered in the CLK_OF_DECLARE_DRIVER() half of the > topckgen clk driver. Don't try to register it again in the actual probe > function. This gets rid of the "Trying to register duplicate clock ..." > warning. > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> Can't we simply remove the CLK_OF_DECLARE_DRIVER() and top_init_early entirely, and transfer TOP_CSW_F26M_D2 to top_divs[] instead? I get that systimer concern and we have something similar in MT8195, where the TOP_CLK26M_D2 is registered "late". Getting back to MT8192, TOP_CSW_F26M_D2 seems to be used only for: 1. systimer 2. SPMI MST (registered "late"). Being it a fixed factor clock, parented to another fixed clock, it doesn't even have any ON/OFF switch, so I think it would be actually possible to go for the proposed removal... which would further improve this cleanup. Regards, Angelo
>> top_early_divs are registered in the CLK_OF_DECLARE_DRIVER() half of the >> topckgen clk driver. Don't try to register it again in the actual probe >> function. This gets rid of the "Trying to register duplicate clock ..." >> warning. >> >> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > >Can't we simply remove the CLK_OF_DECLARE_DRIVER() and top_init_early entirely, >and transfer TOP_CSW_F26M_D2 to top_divs[] instead? >I get that systimer concern and we have something similar in MT8195, where the >TOP_CLK26M_D2 is registered "late". Another reason for this: Removing the CLK_OF_DECLARE_DRIVER() is good when we want to build our driver as kernel modules because it does not work with kernel modules. thanks, Miles > >Getting back to MT8192, TOP_CSW_F26M_D2 seems to be used only for: >1. systimer >2. SPMI MST (registered "late"). > >Being it a fixed factor clock, parented to another fixed clock, it doesn't >even have any ON/OFF switch, so I think it would be actually possible to go >for the proposed removal... which would further improve this cleanup. > >Regards, >Angelo >
On Tue, Sep 27, 2022 at 6:39 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 26/09/22 12:25, Chen-Yu Tsai ha scritto: > > top_early_divs are registered in the CLK_OF_DECLARE_DRIVER() half of the > > topckgen clk driver. Don't try to register it again in the actual probe > > function. This gets rid of the "Trying to register duplicate clock ..." > > warning. > > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > Can't we simply remove the CLK_OF_DECLARE_DRIVER() and top_init_early entirely, > and transfer TOP_CSW_F26M_D2 to top_divs[] instead? > I get that systimer concern and we have something similar in MT8195, where the > TOP_CLK26M_D2 is registered "late". That was what I initially wanted to do. However I asked MTK whether the system would work fully without systimer, and apparently it is used during suspend (presumably it is supposed to be running?), so making it not functional was a bit concerning. That said, I do plan to rework the systimer stuff. The /2 divider is actually internal to the systimer block, and should not have been modeled the way it is now. Notably, the divider is actually variable. It is only configured and locked by the bootloader. For this I think we have two options: a. Move the /2 fixed factor clock into a standalone device node, like what was done for the MT8195 b. Rework the systimer to internalize the divider, and thus moving the systimer input clock to osc26M. Either one is outside the scope of this series. Option a. works especially well for MT8192, as the configurable divider was removed from the systimer block (only for this chip). > Getting back to MT8192, TOP_CSW_F26M_D2 seems to be used only for: > 1. systimer > 2. SPMI MST (registered "late"). > > Being it a fixed factor clock, parented to another fixed clock, it doesn't > even have any ON/OFF switch, so I think it would be actually possible to go > for the proposed removal... which would further improve this cleanup. As mentioned above, I do have some plans to rework the stuff, but it is kind of beyond the scope of this series, as it changes the device tree binding and ABI. Regards ChenYu
On Wed, Sep 28, 2022 at 9:55 AM Miles Chen <miles.chen@mediatek.com> wrote: > > >> top_early_divs are registered in the CLK_OF_DECLARE_DRIVER() half of the > >> topckgen clk driver. Don't try to register it again in the actual probe > >> function. This gets rid of the "Trying to register duplicate clock ..." > >> warning. > >> > >> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > > >Can't we simply remove the CLK_OF_DECLARE_DRIVER() and top_init_early entirely, > >and transfer TOP_CSW_F26M_D2 to top_divs[] instead? > >I get that systimer concern and we have something similar in MT8195, where the > >TOP_CLK26M_D2 is registered "late". > > Another reason for this: > Removing the CLK_OF_DECLARE_DRIVER() is good when we want to build our driver as > kernel modules because it does not work with kernel modules. I agree. But as I mentioned in my other reply, we need to fix the clock user first before dropping that clock. And there's also the matter of DT backward compatibility. So we need to do it incrementally. ChenYu
>On Wed, Sep 28, 2022 at 9:55 AM Miles Chen <miles.chen@mediatek.com> wrote: >> >> >> top_early_divs are registered in the CLK_OF_DECLARE_DRIVER() half of the >> >> topckgen clk driver. Don't try to register it again in the actual probe >> >> function. This gets rid of the "Trying to register duplicate clock ..." >> >> warning. >> >> >> >> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> >> > >> >Can't we simply remove the CLK_OF_DECLARE_DRIVER() and top_init_early entirely, >> >and transfer TOP_CSW_F26M_D2 to top_divs[] instead? >> >I get that systimer concern and we have something similar in MT8195, where the >> >TOP_CLK26M_D2 is registered "late". >> >> Another reason for this: >> Removing the CLK_OF_DECLARE_DRIVER() is good when we want to build our driver as >> kernel modules because it does not work with kernel modules. > >I agree. But as I mentioned in my other reply, we need to fix the clock >user first before dropping that clock. And there's also the matter of >DT backward compatibility. So we need to do it incrementally. > > >ChenYu Got it. thanks for doing this. thanks, Miles
diff --git a/drivers/clk/mediatek/clk-mt8192.c b/drivers/clk/mediatek/clk-mt8192.c index ebbd2798d9a3..e39012583675 100644 --- a/drivers/clk/mediatek/clk-mt8192.c +++ b/drivers/clk/mediatek/clk-mt8192.c @@ -1235,7 +1235,6 @@ static int clk_mt8192_top_probe(struct platform_device *pdev) 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(top_mtk_muxes, ARRAY_SIZE(top_mtk_muxes), node, &mt8192_clk_lock, top_clk_data);
top_early_divs are registered in the CLK_OF_DECLARE_DRIVER() half of the topckgen clk driver. Don't try to register it again in the actual probe function. This gets rid of the "Trying to register duplicate clock ..." warning. Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- drivers/clk/mediatek/clk-mt8192.c | 1 - 1 file changed, 1 deletion(-)