Date:   Tue, 24 May 2022 02:43:01 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Marijn Suijten <marijn.suijten@...ainline.org>
Cc:     phone-devel@...r.kernel.org, Stephen Boyd <sboyd@...nel.org>,
        ~postmarketos/upstreaming@...ts.sr.ht,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Martin Botka <martin.botka@...ainline.org>,
        Jami Kettunen <jami.kettunen@...ainline.org>,
        Michael Turquette <mturquette@...libre.com>,
        Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rajeev Nandan <quic_rajeevny@...cinc.com>,
        Vladimir Lypak <vladimir.lypak@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Jonathan Marek <jonathan@...ek.ca>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org
Subject: Re: [PATCH 0/9] drm/msm/dsi_phy: Replace parent names with clk_hw pointers

Hi,

On Tue, 24 May 2022 at 00:38, Marijn Suijten
<marijn.suijten@...ainline.org> wrote:
>
> As stated in [1] I promised to tackle and send this series.
>
> parent_hw pointers are easier to manage and cheaper to use than
> repeatedly formatting the parent name and subsequently leaving the clk
> framework to perform lookups based on that name.
>
> This series starts out by adding extra constructors for divider, mux and
> fixed-factor clocks that have parent_hw(s) pointer argument(s) instead
> of some DT index or name.  Followed by individual patches performing the
> conversion, one DSI PHY at a time.
>
> dsi_phy_28nm_8960 includes an extra fixup to replace "eternal"
> devm_kzalloc allocations (for the lifetime of the device) with
> stack-local char arrays, like all the other DSI PHY drivers.
>
> I couldn't help but notice that clock names are wildly varying:
>
> - Some use underscores in the _clk suffix where others have nothing;
> - Some have an _ after the %d, others have not;
> - Some use a _pll suffix after dsi%d or even _phy_pll suffix.
>
> Are there any thoughts or feelings towards unifying these?
> Theoretically no clock names are used anywhere in the kernel, and
> everything is based on a phandle + index in DT (I have yet to validate
> this).  Obviously no .name/.fw_name will be updated to not break DT.

I'd say, leave them as is. Even if they are historical, we don't have
a strong pressure to change them.

Significant number of older platforms still use names to identify the
clock. And moreover apq8096/msm8960 uses dsi1/dsi2 instead of
dsi0/dsi1.

Probably we should call the next cycle "The Cycle of clocks cleaning".
I can volunteer to take care of 8960/8064/8016/8996, as at least I can
test them. But if you wish, you (or anybody else of course) can take
any of these platforms too, just ping me, so that I won't spend time
duplicating somebody's efforts.

> Which, by the way, is there a particular reason for:
>
>   #define DSI_BYTE_PLL_CLK              0
>   #define DSI_PIXEL_PLL_CLK             1
>
> To not be in the dt-bindings and used in the DT?

Before my restructure of the DSI PHY subsys, each driver defined them
separately. And the idea of moving them to a dt-bindings header didn't
come to my mind. Feel free to do so, it looks like a good idea.
Just as a note, DP PHY also uses 0 for the link clock and 1 for the
pixel clock. What do you think about having a single header for these
names?

>
> And with enough future improvements out of the way, let's round out this
> patch-series by stating that it has been successfully tested on:
>
> - Sony Nile Discovery (Xperia XA2 Ultra): 14nm;
> - Sony Seine PDX201 (Xperia 10II): 14nm;
> - Sony Loire Suzu (Xperia X): 28nm.
>
> And no diff is observed in debugfs's clk_summary.
>
> Unfortunately all other devices in my collection with a 7/10nm DSI PHY
> have a DSC panel which we have yet to get working.

I will test it on RB3 (10nm) and RB5 (7nm) during one of the next few days.

>
> [1]: https://lore.kernel.org/linux-arm-msm/20220502214235.s5plebunh4ttjhge@SoMainline.org/
>
> Marijn Suijten (9):
>   clk: divider: Introduce devm_clk_hw_register_divider_parent_hw()
>   clk: mux: Introduce devm_clk_hw_register_mux_parent_hws()
>   clk: fixed-factor: Introduce *clk_hw_register_fixed_factor_parent_hw()
>   drm/msm/dsi_phy_28nm: Replace parent names with clk_hw pointers
>   drm/msm/dsi_phy_28nm_8960: Replace parent names with clk_hw pointers
>   drm/msm/dsi_phy_28nm_8960: Use stack memory for temporary clock names
>   drm/msm/dsi_phy_14nm: Replace parent names with clk_hw pointers
>   drm/msm/dsi_phy_10nm: Replace parent names with clk_hw pointers
>   drm/msm/dsi_phy_7nm: Replace parent names with clk_hw pointers
>
>  drivers/clk/clk-fixed-factor.c                | 57 ++++++++++--
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c    | 92 ++++++++-----------
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c    | 36 ++++----
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c    | 52 +++++------
>  .../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c   | 26 ++----
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     | 92 +++++++++----------
>  include/linux/clk-provider.h                  | 34 +++++++
>  7 files changed, 209 insertions(+), 180 deletions(-)
>
> --
> 2.36.1



-- 
With best wishes
Dmitry

Powered by blists - more mailing lists