Re: [PATCH 0/9] drm/msm/dsi_phy: Replace parent names with clk_hw pointers

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Marijn Suijten <marijn.suijten@somainline.org>
Cc: phone-devel@vger.kernel.org, Stephen Boyd <sboyd@kernel.org>,
	~postmarketos/upstreaming@lists.sr.ht,
	AngeloGioacchino Del Regno 
	<angelogioacchino.delregno@somainline.org>,
	Konrad Dybcio <konrad.dybcio@somainline.org>,
	Martin Botka <martin.botka@somainline.org>,
	Jami Kettunen <jami.kettunen@somainline.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Rob Clark <robdclark@gmail.com>,
	Abhinav Kumar <quic_abhinavk@quicinc.com>,
	Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
	Daniel Vetter <daniel@ffwll.ch>,
	Rajeev Nandan <quic_rajeevny@quicinc.com>,
	Vladimir Lypak <vladimir.lypak@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>, Jonathan Marek <jonathan@marek.ca>,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	freedreno@lists.freedesktop.org
Subject: Re: [PATCH 0/9] drm/msm/dsi_phy: Replace parent names with clk_hw pointers
Date: Tue, 24 May 2022 02:43:01 +0300	[thread overview]
Message-ID: <CAA8EJprEjOWRh98V3sprjXZJZMeR25Bz1U3a_uX_KhRbU48srQ@mail.gmail.com> (raw)
In-Reply-To: <20220523213837.1016542-1-marijn.suijten@somainline.org>

Hi,

On Tue, 24 May 2022 at 00:38, Marijn Suijten
<marijn.suijten@somainline.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