[RESEND] of: property: do not create clocks device link for clock controllers

Commit Message

Dmitry Baryshkov Jan. 18, 2023, 9:11 a.m. UTC

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 (clock
orphans). Later when the parent clock is registered, clocks get
populated properly.

An example of the system where this matters is the SDM8450 MTP board
(see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
breaking display support.

Cc: Bjorn Andersson <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Saravana Kannan <[e-mail]>
Cc: Abel Vesa <[e-mail]>
Signed-off-by: Dmitry Baryshkov <[e-mail]>
---

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 been described
by Abel at ([4]). As we work on adding support for Dual DSI
configurations, the issue becomes more and more important, since binding
the whole display subsystem fails.

Currently the qcom/sdm845-mtp board is already broken and I've just
posted a patch adding Dual DSI variant for the qcom/sdm845-db845c board
([5]).

[1] https://lore.kernel.org/lkml/[e-mail]/
[2] https://lore.kernel.org/lkml/[e-mail]/
[3] https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=[e-mail]/
[4] https://lore.kernel.org/all/[e-mail]/
[5] https://lore.kernel.org/all/[e-mail]/

---
 drivers/of/property.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Rob Herring Jan. 18, 2023, 1:35 p.m. UTC | #1

On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
<[e-mail]> 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 (clock
> orphans). Later when the parent clock is registered, clocks get
> populated properly.
>
> An example of the system where this matters is the SDM8450 MTP board
> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> breaking display support.
>
> Cc: Bjorn Andersson <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Saravana Kannan <[e-mail]>
> Cc: Abel Vesa <[e-mail]>
> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> ---
>
> 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 been described
> by Abel at ([4]). As we work on adding support for Dual DSI
> configurations, the issue becomes more and more important, since binding
> the whole display subsystem fails.

That's ample time to fix this, so I intend to apply this. But I'll
give it a few days for comments.

Rob

Konrad Dybcio Jan. 20, 2023, 9:59 p.m. UTC | #2

On 18.01.2023 10:11, Dmitry Baryshkov 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 (clock
> orphans). Later when the parent clock is registered, clocks get
> populated properly.
> 
> An example of the system where this matters is the SDM8450 MTP board
> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> breaking display support.
> 
> Cc: Bjorn Andersson <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Saravana Kannan <[e-mail]>
> Cc: Abel Vesa <[e-mail]>
> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> ---
Tested-by: Konrad Dybcio <[e-mail]> # SM8350 PDX215


Konrad
> 
> 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 been described
> by Abel at ([4]). As we work on adding support for Dual DSI
> configurations, the issue becomes more and more important, since binding
> the whole display subsystem fails.
> 
> Currently the qcom/sdm845-mtp board is already broken and I've just
> posted a patch adding Dual DSI variant for the qcom/sdm845-db845c board
> ([5]).
> 
> [1] https://lore.kernel.org/lkml/[e-mail]/
> [2] https://lore.kernel.org/lkml/[e-mail]/
> [3] https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=[e-mail]/
> [4] https://lore.kernel.org/all/[e-mail]/
> [5] https://lore.kernel.org/all/[e-mail]/
> 
> ---
>  drivers/of/property.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 134cfc980b70..d323bf26a613 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1297,7 +1297,6 @@ struct supplier_bindings {
>  	bool node_not_dev;
>  };
>  
> -DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
>  DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
>  DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
>  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
> @@ -1327,6 +1326,21 @@ DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
>  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
>  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
>  
> +static struct device_node *parse_clocks(struct device_node *np,
> +					const char *prop_name, int index)
> +{
> +	/*
> +	 * Do not create clock-related device links for clocks controllers,
> +	 * clock orphans will handle missing clock parents automatically.
> +	 */
> +	if (!strcmp(prop_name, "clocks") &&
> +	    of_find_property(np, "#clock-cells", NULL))
> +		return NULL;
> +
> +	return parse_prop_cells(np, prop_name, index, "clocks",
> +				       "#clock-cells");
> +}
> +
>  static struct device_node *parse_gpios(struct device_node *np,
>  				       const char *prop_name, int index)
>  {

Saravana Kannan Jan. 25, 2023, 2:12 a.m. UTC | #3

On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <[e-mail]> wrote:
>
> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> <[e-mail]> 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 (clock
> > orphans). Later when the parent clock is registered, clocks get
> > populated properly.
> >
> > An example of the system where this matters is the SDM8450 MTP board
> > (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> > clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> > breaking display support.
> >
> > Cc: Bjorn Andersson <[e-mail]>
> > Cc: Stephen Boyd <[e-mail]>
> > Cc: Saravana Kannan <[e-mail]>
> > Cc: Abel Vesa <[e-mail]>
> > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > ---
> >
> > 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 been described
> > by Abel at ([4]). As we work on adding support for Dual DSI
> > configurations, the issue becomes more and more important, since binding
> > the whole display subsystem fails.

I did send out a patch series[1] to try and fix this. Heck I even
talked about this in LPC 2022. So I don't think it's accurate to say I
didn't help debug this or fix this. There's some email thread in 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 missed 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 that
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 that
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 Dmitry
but break someone else's use case. That's the whole reason it takes 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.

-Saravana
[1] - https://lore.kernel.org/lkml/[e-mail]/

Rob Herring (Arm) Jan. 25, 2023, 7:09 p.m. UTC | #4

On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote:
> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <[e-mail]> wrote:
> >
> > On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> > <[e-mail]> 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 (clock
> > > orphans). Later when the parent clock is registered, clocks get
> > > populated properly.
> > >
> > > An example of the system where this matters is the SDM8450 MTP board
> > > (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> > > clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> > > breaking display support.
> > >
> > > Cc: Bjorn Andersson <[e-mail]>
> > > Cc: Stephen Boyd <[e-mail]>
> > > Cc: Saravana Kannan <[e-mail]>
> > > Cc: Abel Vesa <[e-mail]>
> > > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > > ---
> > >
> > > 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 been described
> > > by Abel at ([4]). As we work on adding support for Dual DSI
> > > configurations, the issue becomes more and more important, since binding
> > > the whole display subsystem fails.
> 
> I did send out a patch series[1] to try and fix this. Heck I even
> talked about this in LPC 2022. So I don't think it's accurate to say I
> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
> 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 that
> 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 Dmitry
> but break someone else's use case. That's the whole reason it takes 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.

Okay, will give it a bit longer.

Rob

Dmitry Baryshkov Jan. 26, 2023, 10:51 p.m. UTC | #5

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 <[e-mail]> wrote:
>>>
>>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
>>> <[e-mail]> 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 (clock
>>>> orphans). Later when the parent clock is registered, clocks get
>>>> populated properly.
>>>>
>>>> An example of the system where this matters is the SDM8450 MTP board
>>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
>>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
>>>> breaking display support.
>>>>
>>>> Cc: Bjorn Andersson <[e-mail]>
>>>> Cc: Stephen Boyd <[e-mail]>
>>>> Cc: Saravana Kannan <[e-mail]>
>>>> Cc: Abel Vesa <[e-mail]>
>>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>>>> ---
>>>>
>>>> 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 been described
>>>> by Abel at ([4]). As we work on adding support for Dual DSI
>>>> configurations, the issue becomes more and more important, since binding
>>>> the whole display subsystem fails.
>>
>> I did send out a patch series[1] to try and fix this. Heck I even
>> talked about this in LPC 2022. So I don't think it's accurate to say I
>> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
>> 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 that
>> 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 Dmitry
>> but break someone else's use case. That's the whole reason it takes 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 properly. 
Moreover they have been supposed to work in such way for quite a while. 
In other words, I don't think we should save them from this 
-EPROBE_DEFERRED.

Thus I think it is better to let them continue doing their job of 
handling probe deferrals on their own, at least for the time being. And 
then, when your patches are finished, we can think about reenabling 
current behaviour. As a reminder, currently, all Qualcomm platforms 
trying to use double DSI configuration are broken and have to use 
fw_devlink= kernel params.

Stephen, do you have any comments regarding this fw_devlink usage vs CCF?

> 
> Okay, will give it a bit longer.
> 
> Rob

Saravana Kannan Jan. 26, 2023, 11:12 p.m. UTC | #6

On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
<[e-mail]> 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 <[e-mail]> wrote:
> >>>
> >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> >>> <[e-mail]> 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 (clock
> >>>> orphans). Later when the parent clock is registered, clocks get
> >>>> populated properly.
> >>>>
> >>>> An example of the system where this matters is the SDM8450 MTP board
> >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> >>>> breaking display support.
> >>>>
> >>>> Cc: Bjorn Andersson <[e-mail]>
> >>>> Cc: Stephen Boyd <[e-mail]>
> >>>> Cc: Saravana Kannan <[e-mail]>
> >>>> Cc: Abel Vesa <[e-mail]>
> >>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> >>>> ---
> >>>>
> >>>> 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 been described
> >>>> by Abel at ([4]). As we work on adding support for Dual DSI
> >>>> configurations, the issue becomes more and more important, since binding
> >>>> the whole display subsystem fails.
> >>
> >> I did send out a patch series[1] to try and fix this. Heck I even
> >> talked about this in LPC 2022. So I don't think it's accurate to say I
> >> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
> >> 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 that
> >> 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 Dmitry
> >> but break someone else's use case. That's the whole reason it takes 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 properly.
> Moreover they have been supposed to work in such way for quite a while.
> 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 clock
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.

Also, I already fixed the issue you are referring to while not
breaking the conditions I'm referring to. So, I don't know why you are
so opposed to that. See Abel's Tested-by here:
https://lore.kernel.org/lkml/[e-mail]/

> Thus I think it is better to let them continue doing their job of
> handling probe deferrals on their own, at least for the time being.

I'm pretty sure your patch will break other Qualcomm platforms because
they depend on sync_state() callbacks to boot up properly when
all/most of their drivers are built as modules.

> And
> then, when your patches are finished, we can think about reenabling
> current behaviour. As a reminder, currently, all Qualcomm platforms
> trying to use double DSI configuration are broken and have to use
> fw_devlink= kernel params.

I'm/was working on sending out the v2 when I got your email. Hold
tight please. It shouldn't take too long.

-Saravana

Saravana Kannan Jan. 27, 2023, 12:13 a.m. UTC | #7

On Thu, Jan 26, 2023 at 3:12 PM Saravana Kannan <[e-mail]> wrote:
>
> On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
> <[e-mail]> 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 <[e-mail]> wrote:
> > >>>
> > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> > >>> <[e-mail]> 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 (clock
> > >>>> orphans). Later when the parent clock is registered, clocks get
> > >>>> populated properly.
> > >>>>
> > >>>> An example of the system where this matters is the SDM8450 MTP board
> > >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> > >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> > >>>> breaking display support.
> > >>>>
> > >>>> Cc: Bjorn Andersson <[e-mail]>
> > >>>> Cc: Stephen Boyd <[e-mail]>
> > >>>> Cc: Saravana Kannan <[e-mail]>
> > >>>> Cc: Abel Vesa <[e-mail]>
> > >>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > >>>> ---
> > >>>>
> > >>>> 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 been described
> > >>>> by Abel at ([4]). As we work on adding support for Dual DSI
> > >>>> configurations, the issue becomes more and more important, since binding
> > >>>> the whole display subsystem fails.
> > >>
> > >> I did send out a patch series[1] to try and fix this. Heck I even
> > >> talked about this in LPC 2022. So I don't think it's accurate to say I
> > >> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
> > >> 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 that
> > >> 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 Dmitry
> > >> but break someone else's use case. That's the whole reason it takes 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 properly.
> > Moreover they have been supposed to work in such way for quite a while.
> > 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 clock
> 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.
>
> Also, I already fixed the issue you are referring to while not
> breaking the conditions I'm referring to. So, I don't know why you are
> so opposed to that. See Abel's Tested-by here:
> https://lore.kernel.org/lkml/[e-mail]/
>
> > Thus I think it is better to let them continue doing their job of
> > handling probe deferrals on their own, at least for the time being.
>
> I'm pretty sure your patch will break other Qualcomm platforms because
> they depend on sync_state() callbacks to boot up properly when
> all/most of their drivers are built as modules.
>
> > And
> > then, when your patches are finished, we can think about reenabling
> > current behaviour. As a reminder, currently, all Qualcomm platforms
> > trying to use double DSI configuration are broken and have to use
> > fw_devlink= kernel params.
>
> I'm/was working on sending out the v2 when I got your email. Hold
> tight please. It shouldn't take too long.

There!
https://lore.kernel.org/lkml/[e-mail]/

Happy? :)

-Saravana

Dmitry Baryshkov Jan. 28, 2023, 4:41 a.m. UTC | #8

On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <[e-mail]> wrote:
>
> On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
> <[e-mail]> 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 <[e-mail]> wrote:
> > >>>
> > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> > >>> <[e-mail]> 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 (clock
> > >>>> orphans). Later when the parent clock is registered, clocks get
> > >>>> populated properly.
> > >>>>
> > >>>> An example of the system where this matters is the SDM8450 MTP board
> > >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> > >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> > >>>> breaking display support.
> > >>>>
> > >>>> Cc: Bjorn Andersson <[e-mail]>
> > >>>> Cc: Stephen Boyd <[e-mail]>
> > >>>> Cc: Saravana Kannan <[e-mail]>
> > >>>> Cc: Abel Vesa <[e-mail]>
> > >>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > >>>> ---
> > >>>>
> > >>>> 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 been described
> > >>>> by Abel at ([4]). As we work on adding support for Dual DSI
> > >>>> configurations, the issue becomes more and more important, since binding
> > >>>> the whole display subsystem fails.
> > >>
> > >> I did send out a patch series[1] to try and fix this. Heck I even
> > >> talked about this in LPC 2022. So I don't think it's accurate to say I
> > >> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
> > >> 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 that
> > >> 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 Dmitry
> > >> but break someone else's use case. That's the whole reason it takes 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 properly.
> > Moreover they have been supposed to work in such way for quite a while.
> > 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 clock
> 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?

>
> Also, I already fixed the issue you are referring to while not
> breaking the conditions I'm referring to. So, I don't know why you are
> so opposed to that. See Abel's Tested-by here:
> https://lore.kernel.org/lkml/[e-mail]/
>
> > Thus I think it is better to let them continue doing their job of
> > handling probe deferrals on their own, at least for the time being.
>
> I'm pretty sure your patch will break other Qualcomm platforms because
> they depend on sync_state() callbacks to boot up properly when
> all/most of their drivers are built as modules.

Qualcomm platforms did not use sync state for clock controllers. Only
for the icc drivers.

>
> > And
> > then, when your patches are finished, we can think about reenabling
> > current behaviour. As a reminder, currently, all Qualcomm platforms
> > trying to use double DSI configuration are broken and have to use
> > fw_devlink= kernel params.
>
> I'm/was working on sending out the v2 when I got your email. Hold
> tight please. It shouldn't take too long.

I'll give v2 a test next week, thank you!

Saravana Kannan Jan. 28, 2023, 4:54 a.m. UTC | #9

On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <[e-mail]> wrote:
> >
> > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
> > <[e-mail]> 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 <[e-mail]> wrote:
> > > >>>
> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> > > >>> <[e-mail]> 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 (clock
> > > >>>> orphans). Later when the parent clock is registered, clocks get
> > > >>>> populated properly.
> > > >>>>
> > > >>>> An example of the system where this matters is the SDM8450 MTP board
> > > >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> > > >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> > > >>>> breaking display support.
> > > >>>>
> > > >>>> Cc: Bjorn Andersson <[e-mail]>
> > > >>>> Cc: Stephen Boyd <[e-mail]>
> > > >>>> Cc: Saravana Kannan <[e-mail]>
> > > >>>> Cc: Abel Vesa <[e-mail]>
> > > >>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > > >>>> ---
> > > >>>>
> > > >>>> 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 been described
> > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI
> > > >>>> configurations, the issue becomes more and more important, since binding
> > > >>>> the whole display subsystem fails.
> > > >>
> > > >> I did send out a patch series[1] to try and fix this. Heck I even
> > > >> talked about this in LPC 2022. So I don't think it's accurate to say I
> > > >> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
> > > >> 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 that
> > > >> 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 Dmitry
> > > >> but break someone else's use case. That's the whole reason it takes 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 properly.
> > > Moreover they have been supposed to work in such way for quite a while.
> > > 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 clock
> > 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=rpm in the command line. The default is just "on".

> >
> > Also, I already fixed the issue you are referring to while not
> > breaking the conditions I'm referring to. So, I don't know why you are
> > so opposed to that. See Abel's Tested-by here:
> > https://lore.kernel.org/lkml/[e-mail]/
> >
> > > Thus I think it is better to let them continue doing their job of
> > > handling probe deferrals on their own, at least for the time being.
> >
> > I'm pretty sure your patch will break other Qualcomm platforms because
> > they depend on sync_state() callbacks to boot up properly when
> > all/most of their drivers are built as modules.
>
> Qualcomm platforms did not use sync state for clock controllers. Only
> for the icc drivers.
>
> >
> > > And
> > > then, when your patches are finished, we can think about reenabling
> > > current behaviour. As a reminder, currently, all Qualcomm platforms
> > > trying to use double DSI configuration are broken and have to use
> > > fw_devlink= kernel params.
> >
> > I'm/was working on sending out the v2 when I got your email. Hold
> > tight please. It shouldn't take too long.
>
> I'll give v2 a test next week, thank you!

Thanks.

-Saravana

Dmitry Baryshkov Jan. 28, 2023, 5:34 a.m. UTC | #10

28 января 2023 г. 07:54:14 GMT+03:00, Saravana Kannan <[e-mail]> пишет:
>On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov
><[e-mail]> wrote:
>>
>> On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <[e-mail]> wrote:
>> >
>> > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
>> > <[e-mail]> 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 <[e-mail]> wrote:
>> > > >>>
>> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
>> > > >>> <[e-mail]> 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 (clock
>> > > >>>> orphans). Later when the parent clock is registered, clocks get
>> > > >>>> populated properly.
>> > > >>>>
>> > > >>>> An example of the system where this matters is the SDM8450 MTP board
>> > > >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
>> > > >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
>> > > >>>> breaking display support.
>> > > >>>>
>> > > >>>> Cc: Bjorn Andersson <[e-mail]>
>> > > >>>> Cc: Stephen Boyd <[e-mail]>
>> > > >>>> Cc: Saravana Kannan <[e-mail]>
>> > > >>>> Cc: Abel Vesa <[e-mail]>
>> > > >>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>> > > >>>> ---
>> > > >>>>
>> > > >>>> 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 been described
>> > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI
>> > > >>>> configurations, the issue becomes more and more important, since binding
>> > > >>>> the whole display subsystem fails.
>> > > >>
>> > > >> I did send out a patch series[1] to try and fix this. Heck I even
>> > > >> talked about this in LPC 2022. So I don't think it's accurate to say I
>> > > >> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
>> > > >> 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 that
>> > > >> 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 Dmitry
>> > > >> but break someone else's use case. That's the whole reason it takes 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 properly.
>> > > Moreover they have been supposed to work in such way for quite a while.
>> > > 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 clock
>> > 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=rpm in the command line. The default is just "on".

So you plan to switch to rpm at some point?

Saravana Kannan Jan. 28, 2023, 5:55 a.m. UTC | #11

On Fri, Jan 27, 2023 at 9:34 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> 28 января 2023 г. 07:54:14 GMT+03:00, Saravana Kannan <[e-mail]> пишет:
> >On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov
> ><[e-mail]> wrote:
> >>
> >> On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <[e-mail]> wrote:
> >> >
> >> > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
> >> > <[e-mail]> 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 <[e-mail]> wrote:
> >> > > >>>
> >> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> >> > > >>> <[e-mail]> 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 (clock
> >> > > >>>> orphans). Later when the parent clock is registered, clocks get
> >> > > >>>> populated properly.
> >> > > >>>>
> >> > > >>>> An example of the system where this matters is the SDM8450 MTP board
> >> > > >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> >> > > >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> >> > > >>>> breaking display support.
> >> > > >>>>
> >> > > >>>> Cc: Bjorn Andersson <[e-mail]>
> >> > > >>>> Cc: Stephen Boyd <[e-mail]>
> >> > > >>>> Cc: Saravana Kannan <[e-mail]>
> >> > > >>>> Cc: Abel Vesa <[e-mail]>
> >> > > >>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> >> > > >>>> ---
> >> > > >>>>
> >> > > >>>> 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 been described
> >> > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI
> >> > > >>>> configurations, the issue becomes more and more important, since binding
> >> > > >>>> the whole display subsystem fails.
> >> > > >>
> >> > > >> I did send out a patch series[1] to try and fix this. Heck I even
> >> > > >> talked about this in LPC 2022. So I don't think it's accurate to say I
> >> > > >> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
> >> > > >> 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 that
> >> > > >> 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 Dmitry
> >> > > >> but break someone else's use case. That's the whole reason it takes 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 properly.
> >> > > Moreover they have been supposed to work in such way for quite a while.
> >> > > 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 clock
> >> > 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=rpm 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

Saravana Kannan Feb. 2, 2023, 3:27 a.m. UTC | #12

On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <[e-mail]> wrote:
> >
> > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov
> > <[e-mail]> 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 <[e-mail]> wrote:
> > > >>>
> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov
> > > >>> <[e-mail]> 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 (clock
> > > >>>> orphans). Later when the parent clock is registered, clocks get
> > > >>>> populated properly.
> > > >>>>
> > > >>>> An example of the system where this matters is the SDM8450 MTP board
> > > >>>> (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> > > >>>> clocks provided by dsi0_phy and dsi1_phy device tree nodes. However 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 all, thus
> > > >>>> breaking display support.
> > > >>>>
> > > >>>> Cc: Bjorn Andersson <[e-mail]>
> > > >>>> Cc: Stephen Boyd <[e-mail]>
> > > >>>> Cc: Saravana Kannan <[e-mail]>
> > > >>>> Cc: Abel Vesa <[e-mail]>
> > > >>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > > >>>> ---
> > > >>>>
> > > >>>> 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 been described
> > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI
> > > >>>> configurations, the issue becomes more and more important, since binding
> > > >>>> the whole display subsystem fails.
> > > >>
> > > >> I did send out a patch series[1] to try and fix this. Heck I even
> > > >> talked about this in LPC 2022. So I don't think it's accurate to say I
> > > >> didn't help debug this or fix this. There's some email thread in 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 missed 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 that
> > > >> 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 that
> > > >> 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 Dmitry
> > > >> but break someone else's use case. That's the whole reason it takes 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 properly.
> > > Moreover they have been supposed to work in such way for quite a while.
> > > 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 clock
> > 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?
>
> >
> > Also, I already fixed the issue you are referring to while not
> > breaking the conditions I'm referring to. So, I don't know why you are
> > so opposed to that. See Abel's Tested-by here:
> > https://lore.kernel.org/lkml/[e-mail]/
> >
> > > Thus I think it is better to let them continue doing their job of
> > > handling probe deferrals on their own, at least for the time being.
> >
> > I'm pretty sure your patch will break other Qualcomm platforms because
> > they depend on sync_state() callbacks to boot up properly when
> > all/most of their drivers are built as modules.
>
> Qualcomm platforms did not use sync state for clock controllers. Only
> for the icc drivers.
>
> >
> > > And
> > > then, when your patches are finished, we can think about reenabling
> > > current behaviour. As a reminder, currently, all Qualcomm platforms
> > > trying to use double DSI configuration are broken and have to use
> > > fw_devlink= kernel params.
> >
> > I'm/was working on sending out the v2 when I got your email. Hold
> > tight please. It shouldn't take too long.
>
> I'll give v2 a test next week, thank you!

Nudge... I rushed out the series for you.

-Saravana

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 134cfc980b70..d323bf26a613 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1297,7 +1297,6 @@  struct supplier_bindings {
 	bool node_not_dev;
 };
 
-DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
 DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
 DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
 DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
@@ -1327,6 +1326,21 @@  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 
+static struct device_node *parse_clocks(struct device_node *np,
+					const char *prop_name, int index)
+{
+	/*
+	 * Do not create clock-related device links for clocks controllers,
+	 * clock orphans will handle missing clock parents automatically.
+	 */
+	if (!strcmp(prop_name, "clocks") &&
+	    of_find_property(np, "#clock-cells", NULL))
+		return NULL;
+
+	return parse_prop_cells(np, prop_name, index, "clocks",
+				       "#clock-cells");
+}
+
 static struct device_node *parse_gpios(struct device_node *np,
 				       const char *prop_name, int index)
 {