MIME-Version: 1.0
References: <20230118091122.2205452-1-dmitry.baryshkov@linaro.org>
 <CAL_JsqJ=0neiZ4wkPiMqJMT4E1O_xO0uLrTmEGUcnZMqxkw4UQ@mail.gmail.com>
 <CAGETcx8Xy5OzsbW3123esxsbQJq-SqDkP1S5g2mmwzoCz4shtQ@mail.gmail.com>
 <20230125190926.GA2697290-robh@kernel.org> <505fc434-c31f-726e-b1cb-0bbfd5f83490@linaro.org>
 <CAGETcx-f9vy7MDB2vFWP9CL26UY7W65oJArvhzksCu8QG6Y4nw@mail.gmail.com>
 <CAA8EJpo-mFxq+eGW=YaMxea+zi2Z64QWocuO36aNsM7Wmtu3sA@mail.gmail.com>
 <CAGETcx-=2O_ZMLGSZrF7Q9Of3pbnieWFRJvzjTe0EVfGpgFSPQ@mail.gmail.com> <68B27FAF-5D14-476C-A22D-4615487EDEA6@linaro.org>
In-Reply-To: <68B27FAF-5D14-476C-A22D-4615487EDEA6@linaro.org>
From:   Saravana Kannan <saravanak@google.com>
Date:   Fri, 27 Jan 2023 21:55:47 -0800
Message-ID: <CAGETcx-sWZz8GWdhUqsXb9EKkKeiOi53SJrLUJGTOG1q9mZG0w@mail.gmail.com>
Subject: Re: [RESEND PATCH] of: property: do not create clocks device link for
 clock controllers
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc:     Rob Herring <robh@kernel.org>, Stephen Boyd <sboyd@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org,
        Bjorn Andersson <andersson@kernel.org>,
        Abel Vesa <abel.vesa@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jan 27, 2023 at 9:34 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> 28 =D1=8F=D0=BD=D0=B2=D0=B0=D1=80=D1=8F 2023 =D0=B3. 07:54:14 GMT+03:00, =
Saravana Kannan <saravanak@google.com> =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
> >On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov
> ><dmitry.baryshkov@linaro.org> wrote:
> >>
> >> On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <saravanak@google.com> w=
rote:
> >> >
> >> > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
> >> > <dmitry.baryshkov@linaro.org> wrote:
> >> > >
> >> > > On 25/01/2023 21:09, Rob Herring wrote:
> >> > > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote:
> >> > > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org=
> wrote:
> >> > > >>>
> >> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> >> > > >>> <dmitry.baryshkov@linaro.org> wrote:
> >> > > >>>>
> >> > > >>>> Do not create device link for clock controllers. Some of the =
clocks
> >> > > >>>> provided to the device via OF can be the clocks that are just=
 parents to
> >> > > >>>> the clocks provided by this clock controller. Clock subsystem=
 already
> >> > > >>>> has support for handling missing clock parents correctly (clo=
ck
> >> > > >>>> orphans). Later when the parent clock is registered, clocks g=
et
> >> > > >>>> populated properly.
> >> > > >>>>
> >> > > >>>> An example of the system where this matters is the SDM8450 MT=
P board
> >> > > >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispc=
c uses
> >> > > >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. H=
owever the
> >> > > >>>> dispcc itself provides clocks to both PHYs, to the PHY parent=
 device,
> >> > > >>>> etc. With just dsi0_phy in place devlink is able to break the
> >> > > >>>> dependency, but with two PHYs, dispcc doesn't get probed at a=
ll, thus
> >> > > >>>> breaking display support.
> >> > > >>>>
> >> > > >>>> Cc: Bjorn Andersson <andersson@kernel.org>
> >> > > >>>> Cc: Stephen Boyd <sboyd@kernel.org>
> >> > > >>>> Cc: Saravana Kannan <saravanak@google.com>
> >> > > >>>> Cc: Abel Vesa <abel.vesa@linaro.org>
> >> > > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> > > >>>> ---
> >> > > >>>>
> >> > > >>>> This patch has been posted a year ago in January 2022 ([1]). =
Since that time
> >> > > >>>> Saravana failed to produce patches to assist in debugging the=
 issue
> >> > > >>>> ([2]) or to fix the issue ([3]). The issue we observe has bee=
n described
> >> > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI
> >> > > >>>> configurations, the issue becomes more and more important, si=
nce binding
> >> > > >>>> the whole display subsystem fails.
> >> > > >>
> >> > > >> I did send out a patch series[1] to try and fix this. Heck I ev=
en
> >> > > >> talked about this in LPC 2022. So I don't think it's accurate t=
o say I
> >> > > >> didn't help debug this or fix this. There's some email thread i=
n lore
> >> > > >> where Abel gave more details and I figured out the issue and we=
 didn't
> >> > > >> need any more debugging. And then I sent out [1]. Sorry I misse=
d you
> >> > > >> in the cc lise for [1] -- I try to keep track of everyone to cc=
 but
> >> > > >> things slip through the cracks sometimes. But at the same time,=
 it's
> >> > > >> easy to check for emails from me before saying I didn't help or=
 didn't
> >> > > >> send out fixes :)
> >> > > >>
> >> > > >> If you do try to give [1] a shot, there are a bunch of bugs tha=
t
> >> > > >> people pointed out for which I gave fixes on top of [1] in the
> >> > > >> replies. I was supposed to work on v2 over the holidays, but th=
at
> >> > > >> didn't happen because of stuff outside my control.
> >> > > >>
> >> > > >>> That's ample time to fix this, so I intend to apply this. But =
I'll
> >> > > >>> give it a few days for comments.
> >> > > >>
> >> > > >> Rob, I'd recommend not applying this because it'll fix it for D=
mitry
> >> > > >> but break someone else's use case. That's the whole reason it t=
akes me
> >> > > >> a while to send out patches -- it's easy to fix it for a subset=
 of
> >> > > >> devices, but fixing something without breaking someone else is =
harder
> >> > > >> (I still believe it's doable) and it takes a while to test them=
 on all
> >> > > >> the devices I want to test before sending them out.
> >> > >
> >> > > This case is really simple, I think. Clock controllers (and
> >> > > clock-core-framework) are prepared to handle clock orphans properl=
y.
> >> > > Moreover they have been supposed to work in such way for quite a w=
hile.
> >> > > In other words, I don't think we should save them from this
> >> > > -EPROBE_DEFERRED.
> >> >
> >> > A clock controller can depend on other clock controllers for non clo=
ck
> >> > tree reasons. For example, it might need a clock ON to access its
> >> > registers. So, while the CCF can handle orphans properly, that's not
> >> > the only dependency. Also, fw_devlink is not just about probing
> >> > either. It also has to do with proper sync_state() callbacks.
> >>
> >> Just a question, please excuse if I'm misunderstanding it. Does
> >> fw_devlink created this way also impose any runtime PM dependencies?
> >
> >If you set fw_devlink=3Drpm in the command line. The default is just "on=
".
>
> So you plan to switch to rpm at some point?

Ideally, but it's a loooong way off. I need to fix all the issues
people are pointing out right now before I try to go for that being
the default.

-Saravana