Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220419081246.2546159-1-wenst@chromium.org> <3591fcc1-d34a-b40a-4e78-edcf9d2ddf08@collabora.com>
 <CAGXv+5HhcQbdGLQFtgPnvzVbSKaQ5GQGvmjwPVOVxjrYnMh1dg@mail.gmail.com>
 <ca8048f4-2e75-a49b-6c54-9f6abba6ead3@collabora.com> <CAGXv+5G2Uyq5fdMUnhr5wOm6O8M3dHNKGaSP2L_Vv1o28mXmOQ@mail.gmail.com>
 <20220422014044.16530C385A7@smtp.kernel.org>
In-Reply-To: <20220422014044.16530C385A7@smtp.kernel.org>
From:   Chen-Yu Tsai <wenst@chromium.org>
Date:   Fri, 22 Apr 2022 12:14:36 +0800
Message-ID: <CAGXv+5HJ2EmxH49L4a3t-gGcQpNLwgi74=uZ1iB1fNzSFz7Gbw@mail.gmail.com>
Subject: Re: [RFC PATCH 0/7] clk: mediatek: Move to struct clk_hw provider APIs
To:     Stephen Boyd <sboyd@kernel.org>
Cc:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Chun-Jie Chen <chun-jie.chen@mediatek.com>,
        Miles Chen <miles.chen@mediatek.com>,
        Rex-BC Chen <rex-bc.chen@mediatek.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-clk@vger.kernel.org, linux-mediatek@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 22, 2022 at 9:40 AM Stephen Boyd <sboyd@kernel.org> 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