[RFC,0/7] clk: mediatek: Move to struct clk_hw provider APIs

Message

Chen-Yu Tsai April 19, 2022, 8:12 a.m. UTC

Hi everyone,

This is part 2 of my proposed MediaTek clk driver cleanup work [1].

Part 2 involves moving the whole MediaTek clk driver library from the
old `struct clk` provider API to the new `struct clk_hw` provider API.

Parts of this series were done with coccinelle scripts, while others
were done by hand. To facilitate review, these parts are currently split
into different patches. As a result however, this series is not fully
bisectable. Later on, the related parts should be squashed together.

Patch 1 and 2 are minor cleanups around code that is touched by later
patches.

Patch 3 starts the switch of the underlying data structure used to hold
clocks from `struct clk_onecell_data` to `struct clk_hw_onecell_data`.
This part is done by hand. The helpers used to allocate the data
structures are changed, but none of the actual call sites, nor the
clk provider API usage.

Patch 4 finishes the switch from `struct clk_onecell_data` to `struct
clk_hw_onecell_data`. All the holding variables and call sites that
involve `struct clk_onecell_data` are changed using coccinelle scripts.

Patch 5 moves most of the MediaTek clk driver library from clk_register*()
to clk_hw_register*, including all intermediate helpers, using
coccinelle scripts.

Patch 6 fixes, by hand, a build error from a call site that was not covered
by the previous patch.

Patch 7 converts the last usage of clk_register*() in the MediaTek clk
drivers.

As mentioned above, this series includes parts that don't build, but are
split out for clarity. These are patches 3 and 5. Once the patches are
reviewed, they shall be squashed together.

This series doesn't cover the new MT8186 clk driver, which is fully
reviewed on the mailing lists, but not yet queued up. I think it would
be better to have that series merged, and I can rerun the coccinelle
scripts to add coverage.

This series will likely conflict with Rex's "Cleanup MediaTek clk reset
drivers" that was posted earlier today. We'll see how these work out.

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.


Thanks
ChenYu

[1] https://lore.kernel.org/linux-clk/[e-mail]/


Chen-Yu Tsai (7):
  clk: mediatek: Make mtk_clk_register_composite() static
  clk: mediatek: apmixed: Drop error message from clk_register() failure
  clk: mediatek: Convert mtk_{alloc,free}_clk_data to struct clk_hw
  clk: mediatek: Replace 'struct clk' with 'struct clk_hw'
  clk: mediatek: Switch to clk_hw provider APIs
  clk: mediatek: mt8173: Fix usage of mtk_clk_register_ref2usb_tx()
  clk: mediatek: mt8173: Switch to clk_hw provider APIs

 drivers/clk/mediatek/clk-apmixed.c           |  12 +-
 drivers/clk/mediatek/clk-cpumux.c            |  50 +++---
 drivers/clk/mediatek/clk-cpumux.h            |   6 +-
 drivers/clk/mediatek/clk-gate.c              |  52 +++---
 drivers/clk/mediatek/clk-gate.h              |   8 +-
 drivers/clk/mediatek/clk-mt2701-aud.c        |   4 +-
 drivers/clk/mediatek/clk-mt2701-bdp.c        |   4 +-
 drivers/clk/mediatek/clk-mt2701-eth.c        |   4 +-
 drivers/clk/mediatek/clk-mt2701-g3d.c        |   4 +-
 drivers/clk/mediatek/clk-mt2701-hif.c        |   4 +-
 drivers/clk/mediatek/clk-mt2701-img.c        |   4 +-
 drivers/clk/mediatek/clk-mt2701-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt2701-vdec.c       |   4 +-
 drivers/clk/mediatek/clk-mt2701.c            |  26 +--
 drivers/clk/mediatek/clk-mt2712-bdp.c        |   4 +-
 drivers/clk/mediatek/clk-mt2712-img.c        |   4 +-
 drivers/clk/mediatek/clk-mt2712-jpgdec.c     |   4 +-
 drivers/clk/mediatek/clk-mt2712-mfg.c        |   4 +-
 drivers/clk/mediatek/clk-mt2712-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt2712-vdec.c       |   4 +-
 drivers/clk/mediatek/clk-mt2712-venc.c       |   4 +-
 drivers/clk/mediatek/clk-mt2712.c            |  28 +--
 drivers/clk/mediatek/clk-mt6765-audio.c      |   4 +-
 drivers/clk/mediatek/clk-mt6765-cam.c        |   4 +-
 drivers/clk/mediatek/clk-mt6765-img.c        |   4 +-
 drivers/clk/mediatek/clk-mt6765-mipi0a.c     |   4 +-
 drivers/clk/mediatek/clk-mt6765-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt6765-vcodec.c     |   4 +-
 drivers/clk/mediatek/clk-mt6765.c            |  12 +-
 drivers/clk/mediatek/clk-mt6779-aud.c        |   4 +-
 drivers/clk/mediatek/clk-mt6779-cam.c        |   4 +-
 drivers/clk/mediatek/clk-mt6779-img.c        |   4 +-
 drivers/clk/mediatek/clk-mt6779-ipe.c        |   4 +-
 drivers/clk/mediatek/clk-mt6779-mfg.c        |   4 +-
 drivers/clk/mediatek/clk-mt6779-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt6779-vdec.c       |   4 +-
 drivers/clk/mediatek/clk-mt6779-venc.c       |   4 +-
 drivers/clk/mediatek/clk-mt6779.c            |  12 +-
 drivers/clk/mediatek/clk-mt6797-img.c        |   4 +-
 drivers/clk/mediatek/clk-mt6797-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt6797-vdec.c       |   4 +-
 drivers/clk/mediatek/clk-mt6797-venc.c       |   4 +-
 drivers/clk/mediatek/clk-mt6797.c            |  22 +--
 drivers/clk/mediatek/clk-mt7622-aud.c        |   4 +-
 drivers/clk/mediatek/clk-mt7622-eth.c        |   8 +-
 drivers/clk/mediatek/clk-mt7622-hif.c        |   8 +-
 drivers/clk/mediatek/clk-mt7622.c            |  30 ++--
 drivers/clk/mediatek/clk-mt7629-eth.c        |   8 +-
 drivers/clk/mediatek/clk-mt7629-hif.c        |   8 +-
 drivers/clk/mediatek/clk-mt7629.c            |  30 ++--
 drivers/clk/mediatek/clk-mt7986-apmixed.c    |   6 +-
 drivers/clk/mediatek/clk-mt7986-eth.c        |  12 +-
 drivers/clk/mediatek/clk-mt7986-infracfg.c   |   4 +-
 drivers/clk/mediatek/clk-mt7986-topckgen.c   |  16 +-
 drivers/clk/mediatek/clk-mt8135.c            |  18 +-
 drivers/clk/mediatek/clk-mt8167-aud.c        |   4 +-
 drivers/clk/mediatek/clk-mt8167-img.c        |   4 +-
 drivers/clk/mediatek/clk-mt8167-mfgcfg.c     |   4 +-
 drivers/clk/mediatek/clk-mt8167-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt8167-vdec.c       |   4 +-
 drivers/clk/mediatek/clk-mt8167.c            |  12 +-
 drivers/clk/mediatek/clk-mt8173-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt8173.c            |  69 ++++----
 drivers/clk/mediatek/clk-mt8183-audio.c      |   4 +-
 drivers/clk/mediatek/clk-mt8183-cam.c        |   4 +-
 drivers/clk/mediatek/clk-mt8183-img.c        |   4 +-
 drivers/clk/mediatek/clk-mt8183-ipu0.c       |   4 +-
 drivers/clk/mediatek/clk-mt8183-ipu1.c       |   4 +-
 drivers/clk/mediatek/clk-mt8183-ipu_adl.c    |   4 +-
 drivers/clk/mediatek/clk-mt8183-ipu_conn.c   |   4 +-
 drivers/clk/mediatek/clk-mt8183-mfgcfg.c     |   4 +-
 drivers/clk/mediatek/clk-mt8183-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt8183-vdec.c       |   4 +-
 drivers/clk/mediatek/clk-mt8183-venc.c       |   4 +-
 drivers/clk/mediatek/clk-mt8183.c            |  25 +--
 drivers/clk/mediatek/clk-mt8192-aud.c        |   4 +-
 drivers/clk/mediatek/clk-mt8192-mm.c         |   4 +-
 drivers/clk/mediatek/clk-mt8192.c            |  21 +--
 drivers/clk/mediatek/clk-mt8195-apmixedsys.c |   6 +-
 drivers/clk/mediatek/clk-mt8195-apusys_pll.c |   6 +-
 drivers/clk/mediatek/clk-mt8195-topckgen.c   |   6 +-
 drivers/clk/mediatek/clk-mt8195-vdo0.c       |   6 +-
 drivers/clk/mediatek/clk-mt8195-vdo1.c       |   6 +-
 drivers/clk/mediatek/clk-mt8516-aud.c        |   4 +-
 drivers/clk/mediatek/clk-mt8516.c            |  12 +-
 drivers/clk/mediatek/clk-mtk.c               | 173 +++++++++----------
 drivers/clk/mediatek/clk-mtk.h               |  25 ++-
 drivers/clk/mediatek/clk-mux.c               |  50 +++---
 drivers/clk/mediatek/clk-mux.h               |   6 +-
 drivers/clk/mediatek/clk-pll.c               |  52 +++---
 drivers/clk/mediatek/clk-pll.h               |   6 +-
 91 files changed, 531 insertions(+), 542 deletions(-)

Comments

AngeloGioacchino Del Regno April 19, 2022, 3:08 p.m. UTC | #1

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).

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.

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.

Cheers,
Angelo

Chen-Yu Tsai April 19, 2022, 4:09 p.m. UTC | #2

On Tue, Apr 19, 2022 at 11:08 PM AngeloGioacchino Del Regno
<[e-mail]> 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).

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...).

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.

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?

> 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
probably handle the other ChromeOS platforms, or delegate it internally.


Regards
ChenYu

AngeloGioacchino Del Regno April 20, 2022, 12:02 p.m. UTC | #3

Il 19/04/22 18:09, Chen-Yu Tsai ha scritto:
> On Tue, Apr 19, 2022 at 11:08 PM AngeloGioacchino Del Regno
> <[e-mail]> 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).
> 
> 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.

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.

> 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
> 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 :))

Regards,
Angelo

> Regards
> ChenYu

Chen-Yu Tsai April 21, 2022, 6:05 a.m. UTC | #4

On Wed, Apr 20, 2022 at 8:02 PM AngeloGioacchino Del Regno
<[e-mail]> wrote:
>
> Il 19/04/22 18:09, Chen-Yu Tsai ha scritto:
> > On Tue, Apr 19, 2022 at 11:08 PM AngeloGioacchino Del Regno
> > <[e-mail]> 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.

> > 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 ...

> 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.

> > 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 *

> > 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

AngeloGioacchino Del Regno April 21, 2022, 9:42 a.m. UTC | #5

Il 21/04/22 08:05, Chen-Yu Tsai ha scritto:
> On Wed, Apr 20, 2022 at 8:02 PM AngeloGioacchino Del Regno
> <[e-mail]> wrote:
>>
>> Il 19/04/22 18:09, Chen-Yu Tsai ha scritto:
>>> On Tue, Apr 19, 2022 at 11:08 PM AngeloGioacchino Del Regno
>>> <[e-mail]> 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

Stephen Boyd April 22, 2022, 1:40 a.m. UTC | #6

Quoting Chen-Yu Tsai (2022-04-20 23:05:10)
> 
> 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.

If this is a common problem, we may need to come up with a generic
solution that either adds a new clk registration API that fills in the
clk_parent_data hw pointer or add another member to struct
clk_parent_data that says "index into this other array of clk_hw
pointers".

Chen-Yu Tsai April 22, 2022, 4:14 a.m. UTC | #7

On Fri, Apr 22, 2022 at 9:40 AM Stephen Boyd <[e-mail]> wrote:
>
> Quoting Chen-Yu Tsai (2022-04-20 23:05:10)
> >
> > 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.
>
> If this is a common problem, we may need to come up with a generic
> solution that either adds a new clk registration API that fills in the
> clk_parent_data hw pointer or add another member to struct
> clk_parent_data that says "index into this other array of clk_hw
> pointers".

Looking through the large clk drivers, at least Rockchip and MediaTek
drivers would benefit from this. And maybe the Tegra ones as well, though
I'm not familiar with them.

Meson, Qcom, and sunxi-ng all use the static clk_hw scheme, so they're
unaffected.

I can think of a couple ways to tackle this:

A. Add a new data structure as I showed above, and a helper to fill in
   |struct clk_parent_data| from said data structure. This basically moves
   what I planned to do for the MediaTek clk driver to the clk provider
   API. This is the least intrusive option.

B. Add the |clk_id| field to |struct clk_parent_data|, and a |struct clk_hw *|
   field to |struct clk_init_data| for the array. Lookup would be done at
   clk registration time in clk_core_populate_parent_map(). This still
   forces checking of the clk_hw pointer and proper ordering of clk
   registration though. And it also bloats the data structures for folks
   not using the feature.

C. Same as B, but keep the pointer to the array around (in clk_core?), and
   move the lookup into clk_core_fill_parent_index(). This provides similar
   behavior to using global clk name or static |struct clk_hw *| values in
   that clk registration order is not restricted.

All the above options are designed around the desire to be able to make
either the new data structure or |struct clk_parent_data| constant.

Thoughts?


Regards
ChenYu

Chen-Yu Tsai May 3, 2022, 4:28 p.m. UTC | #8

Hi Stephen,

On Tue, Apr 19, 2022 at 4:12 PM Chen-Yu Tsai <[e-mail]> wrote:
>
> Hi everyone,
>
> This is part 2 of my proposed MediaTek clk driver cleanup work [1].
>
> Part 2 involves moving the whole MediaTek clk driver library from the
> old `struct clk` provider API to the new `struct clk_hw` provider API.
>
> Parts of this series were done with coccinelle scripts, while others
> were done by hand. To facilitate review, these parts are currently split
> into different patches. As a result however, this series is not fully
> bisectable. Later on, the related parts should be squashed together.
>
> Patch 1 and 2 are minor cleanups around code that is touched by later
> patches.

[...]

> Chen-Yu Tsai (7):
>   clk: mediatek: Make mtk_clk_register_composite() static
>   clk: mediatek: apmixed: Drop error message from clk_register() failure

Could you take a quick look at the first two patches and pick them up?
They are unrelated cleanups that touch the same code sections as the
other patches in this series, and thus were included.


Thanks
ChenYu