Messages in this thread | | | Date | Thu, 21 Apr 2022 11:42:43 +0200 | Subject | Re: [RFC PATCH 0/7] clk: mediatek: Move to struct clk_hw provider APIs | From | AngeloGioacchino Del Regno <> |
| |
Il 21/04/22 08:05, Chen-Yu Tsai ha scritto: > On Wed, Apr 20, 2022 at 8:02 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> Il 19/04/22 18:09, Chen-Yu Tsai ha scritto: >>> On Tue, Apr 19, 2022 at 11:08 PM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@collabora.com> wrote: >>>> >>>> Il 19/04/22 10:12, Chen-Yu Tsai ha scritto: >>>>> Hi everyone, >>>>> >>>>> This is part 2 of my proposed MediaTek clk driver cleanup work [1]. >>>>> >>>> >>>> ..snip.. >>>> >>>>> >>>>> The next phase of the cleanup/improvement shall be to introduce some >>>>> variant of `struct clk_parent_data` to describe clk relationships >>>>> efficiently. >>>>> >>>>> Please have a look. >>>>> >>>> >>>> Hello Chen-Yu, >>>> >>>> I am grateful to see this series, as the MediaTek clock drivers are getting >>>> a bit old, despite new platforms being implemented practically as we speak. >>>> >>>> With this, you surely get that I completely agree with the proposed cleanup >>>> and modernization of the entire MediaTek clocks infrastructure, but I think >>>> that introducing a `struct clk_parent_data` for these drivers is, at this >>>> point, a must, that not only fully justifies these patches, but also "makes >>>> the point" - as the effect of that would be a performance improvement as we >>>> would *at least* avoid lots of clk_cpy_name() in case of parent_hws, or in >>>> case or parent_data where no .fw_name is provided (which would be the case >>>> for most of the clocks). > > BTW, clk_cpy_name() handles const values correctly, i.e. it won't actually > copy them. The performance issue with using names is the clk core has to > match them against the ever increasing list of clks in the system to find > the actual clk object. >
Right.
>>> You and me both. :) And yes, one of the intended results is to make the >>> clk driver probe faster. >>> >>>> That said, my advice would be to add that conversion to declaring clocks >>>> with .hw.init.parent_data and/or .hw.init.parent_hws to this series as to >>>> really make it complete. >>> >>> This series itself already touches a lot of code, even if most of it was >>> done by coccinelle. I'd like to send them separately to not overwhelm >>> people. >>> >>> Also, I haven't actually started on that part yet. It is actually part 3 >>> of my overall plan. I have a good idea of what to do, given I did similar >>> work for the sunxi-ng clk drivers (though half finished...). >> >> Having a good plan means that you're already half-done though :) :) :) >> >> Besides, the reason why I said that you should do the conversion in the same >> series was exactly because your changes are done with coccinelle scripts... >> ...but I thought that you already had something in the works for that. > > The final part won't be doable with coccinelle scripts though. It involves > converting the existing list of clk parent names to IDs. It might be doable > in Perl or Python, but would likely involve a whole lot of string parsing > and pattern matching ... >
It's going to be a bit tricky, yes... but that's "necessary evil", I guess.
>> Since you still need some time for the final part, having this kind of (even >> if partial) modernization is still golden. >> Let's do it in two steps as you prefer then, that's fine for me. >> >>> >>> Most of the clk references are internal to each driver, and those would >>> be mapped from some CLK_ID to some `struct clk_hw *` internally, but all >>> blocks have external parents that need to be modeled as well, and we >>> would likely need global clk name fallbacks for the blocks that don't >>> have parents declared in the device tree, which is unfortunately most >>> of them. Especially the central clock controllers like infracfg or pericfg >>> take many clk inputs, to the point that MediaTek folks were somewhat >>> unwilling to bloat the device tree with them. >>> >>> So it does seem easier to use something like clk_parent_data with >>> `struct clk_hw *` replaced with an index everywhere. This structure >>> would get converted into clk_parent_data by the singular clk registration >>> helpers. >>> >> >> I may not be understanding what you mean by mapping the CLK_ID internally, but >> from what my brain processed, I think that you want to look at, and basically >> replicate, how it's done in the Qualcomm clock drivers (and perhaps standardize >> that in the clock API?). >> >> Specifically, clk/qcom/common.h, struct parent_map. >> >> Though, I admit I haven't looked at the MTK clocks *very deeply*, so I may be >> misunderstanding something. > > Not exactly. All the clocks in the MTK drivers are allocated at runtime, > so we can't use clk_parent_data to point to not-yet-allocated clk_hw-s. > Instead we'll need to have > > struct mtk_clk_parent_data { > unsigned int clk_id; /* Match CLK_XXX_YYY from dt-binding headers */ > ... /* remaining fields same as mtk_clk_parent_data */ > }; > > and create the actual clk_parent_data at runtime by looking up clk_id in > the set of already registered clks: > > int mtk_clk_register_XXX(..., struct mtk_clk_parent_data *pdata, > struct clk_hw_onecell_data *clk_data) { > struct clk_parent_data data = { > .hw = clk_data[pdata->clk_id], > /* copy other fields verbatim */ > }; > ... > } > > Obviously this forces some ordering of how the clks are registered. > I believe the order is already correct, and if it isn't, it would be > easy to detect, and we can reorder things to fix it. >
That's clearer now, thanks for the overview!
>>> This would have to coexist with the existing helpers we have. So I think >>> this work would be combined with the helper API cleanup / alignment with >>> clk provider API. >>> >>> Does that make sense to you? >>> >> >> Yes that does fully make sense to me. >> >>>> Of course, if you have concerns about old platforms that you cannot test, >>>> or for which this kind of conversion would require a huge amount of effort, >>>> then I would go for converting as many as possible as a first step and then >>>> leave the others for later. >>>> >>>> I would envision MT8183, 8186, 8192, 8195 to be a good amount of first >>>> candidates for this great effort. >>> >>> I'm working with MT8183 right now, as it can readily boot mainline to a >>> shell. Depending on the schedule and whose on board with resources, I'd > > * who's onboard * >
I can most probably promise you to spend some time in testing and reviewing.
Besides, when it comes to make things cleaner, proper, eventually as generic as possible (read: upstream style), I'm always in. :-D
Also, I'm adding Nicolas Prado to the loop... as he may find this conversation pretty interesting.
>>> probably handle the other ChromeOS platforms, or delegate it internally. >>> >>> >> >> That sounds like a plan. Besides, I wasn't trying to give you any hurry >> whatsoever - I was simply thinking out loud :)) > > Got it. :) > > > Regards > ChenYu
| |