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>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	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: Wed, 25 May 2022 11:01:33 +0300	[thread overview]
Message-ID: <CAA8EJpofvyxH22qWs5HLqG-EkKkecbFySXd36YDmK8cdeNaGUg@mail.gmail.com> (raw)
In-Reply-To: <20220524220312.jrdkolu7eoxtcyju@SoMainline.org>

On Wed, 25 May 2022 at 01:03, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2022-05-24 02:43:01, Dmitry Baryshkov wrote:
> > 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.
>
> Leave them as it is, or - as suggested below - clean them up?

Let's leave the names as is for now, convert all clock drivers to
fetch clocks from DT and decide how to continue with clock names
afterwards.

> > 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.
>
> We can at least clean up the names of clocks that are not "exported" by
> the drivers.  However, we should also convert all other clk drivers to
> utilize DT to define clk dependencies instead of depending on global
> names, and already got quite some platforms tackled.  At that point we
> can just convert all names (give or take the often discussed "backwards
> compatbility" between the kernel and some ancient DT someone may still
> be running on their device).
>
> I don't own any device for the SoCs you mentioned, all good from my
> side if you take them.  We should probably note down all clock drivers
> that still need conversion and split them across devs with physical
> access, then I can check what I still have lying around here as well.

Can you please make a google spreadsheet? Then anybody can take a look
and volunteer (or check that the platform is being taken care of).
I have 8064 (and thus I can cover 8960 too), 8016, 8096 on my desk and
qcs404 and 8998 in the remote lab (but I can leave them to somebody
else).

> > > 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?
>
> No worries, it's already much better to have them defined once :), now
> we can just go one step further and move it to dt-bindings.  Great to
> clean up the "magic constant indices" for the DP PHY as well
> (phy-qcom-qmp.c is the only one defining these clocks, right?)

No, phy-qcom-edp.c also uses these magic numbers.

> and I
> think we're fine having them in one header, pending someone suggesting a
> name as I have no idea what to call it nor where to put it.  Under
> dt-bindings/clock most likely, but what common name would we choose?
> Something including qcom and mdss?

dt-bindings/phy/phy-qcom-dsi.h and dt-bindings/phy/phy-qcom-dp.h?

-- 
With best wishes
Dmitry