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

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

[Dmitry Baryshkov]

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 <bjorn.andersson@linaro.org>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 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 a3483484a5a2..f7229e4030e3 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1264,7 +1264,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")
@@ -1294,6 +1293,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)
 {
-- 
2.33.0

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

[Saravana Kannan]

On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Do not create device link for clock controllers.

Nak.

> 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,

Right, because I wrote code to make sure we handle these clock
controller cases properly. If that logic isn't smart enough, let's fix
that.

> but with two PHYs, dispcc doesn't get probed at all, thus
> breaking display support.

Then let's find out why and fix this instead of hiding some
dependencies from fw_devlink. You could be breaking other cases/boards
with this change you are making.

-Saravana

> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  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 a3483484a5a2..f7229e4030e3 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1264,7 +1264,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")
> @@ -1294,6 +1293,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)
>  {
> --
> 2.33.0
>

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

[Saravana Kannan]

On Mon, Nov 29, 2021 at 3:48 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > Do not create device link for clock controllers.
>
> Nak.
>
> > 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,
>
> Right, because I wrote code to make sure we handle these clock
> controller cases properly. If that logic isn't smart enough, let's fix
> that.
>
> > but with two PHYs, dispcc doesn't get probed at all, thus
> > breaking display support.
>
> Then let's find out why and fix this instead of hiding some
> dependencies from fw_devlink. You could be breaking other cases/boards
> with this change you are making.

Btw, forgot to mention. I'll look into this one and try to find the
reason why it wasn't handled automatically. And then come up with a
fix.

If you want to find out why fw_devlink didn't notice the cycle
correctly for the case of 2 PHYs vs 1 PHY, I'd appreciate that too.

Btw, same comment for remote-endpoint. I'll look into what's going on
in that case. Btw, I'm assuming all the code and DT you are testing
this on is already upstream. Can you please confirm that?

-Saravana

>
> -Saravana
>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Stephen Boyd <swboyd@chromium.org>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  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 a3483484a5a2..f7229e4030e3 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1264,7 +1264,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")
> > @@ -1294,6 +1293,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)
> >  {
> > --
> > 2.33.0
> >

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

[Dmitry Baryshkov]

On Tue, 30 Nov 2021 at 02:53, Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Nov 29, 2021 at 3:48 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > Do not create device link for clock controllers.
> >
> > Nak.
> >
> > > 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,
> >
> > Right, because I wrote code to make sure we handle these clock
> > controller cases properly. If that logic isn't smart enough, let's fix
> > that.

As I said, devlink was delaying dispcc probing ,waiting for the second
DSI PHY clock provider.
Thus came my proposal to let clock orphans handle the case (which it
does perfectly).

> >
> > > but with two PHYs, dispcc doesn't get probed at all, thus
> > > breaking display support.
> >
> > Then let's find out why and fix this instead of hiding some
> > dependencies from fw_devlink. You could be breaking other cases/boards
> > with this change you are making.
>
> Btw, forgot to mention. I'll look into this one and try to find the
> reason why it wasn't handled automatically. And then come up with a
> fix.
>
> If you want to find out why fw_devlink didn't notice the cycle
> correctly for the case of 2 PHYs vs 1 PHY, I'd appreciate that too.
>
> Btw, same comment for remote-endpoint. I'll look into what's going on
> in that case. Btw, I'm assuming all the code and DT you are testing
> this on is already upstream. Can you please confirm that?

All the code and basic DT is upstreamed. The DT part I
referenced/posted was written for the custom extender for the
qrb5165-rb5 board that I use here to test MSM DRM driver, but the
result DT should be more or less the same as smd845-mtp.

>
> -Saravana
>
> >
> > -Saravana
> >
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Cc: Stephen Boyd <swboyd@chromium.org>
> > > Cc: Saravana Kannan <saravanak@google.com>
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  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 a3483484a5a2..f7229e4030e3 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1264,7 +1264,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")
> > > @@ -1294,6 +1293,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)
> > >  {
> > > --
> > > 2.33.0
> > >



-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

Hi Saravana,

On Tue, 30 Nov 2021 at 03:24, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 30 Nov 2021 at 02:53, Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Mon, Nov 29, 2021 at 3:48 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > Do not create device link for clock controllers.
> > >
> > > Nak.
> > >
> > > > 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,
> > >
> > > Right, because I wrote code to make sure we handle these clock
> > > controller cases properly. If that logic isn't smart enough, let's fix
> > > that.
>
> As I said, devlink was delaying dispcc probing ,waiting for the second
> DSI PHY clock provider.
> Thus came my proposal to let clock orphans handle the case (which it
> does perfectly).
>
> > >
> > > > but with two PHYs, dispcc doesn't get probed at all, thus
> > > > breaking display support.
> > >
> > > Then let's find out why and fix this instead of hiding some
> > > dependencies from fw_devlink. You could be breaking other cases/boards
> > > with this change you are making.
> >
> > Btw, forgot to mention. I'll look into this one and try to find the
> > reason why it wasn't handled automatically. And then come up with a
> > fix.
> >
> > If you want to find out why fw_devlink didn't notice the cycle
> > correctly for the case of 2 PHYs vs 1 PHY, I'd appreciate that too.
> >
> > Btw, same comment for remote-endpoint. I'll look into what's going on
> > in that case. Btw, I'm assuming all the code and DT you are testing
> > this on is already upstream. Can you please confirm that?
>
> All the code and basic DT is upstreamed. The DT part I
> referenced/posted was written for the custom extender for the
> qrb5165-rb5 board that I use here to test MSM DRM driver, but the
> result DT should be more or less the same as smd845-mtp.

So, is there a way we can assist you in debugging these issues? I
still can not get dual DSI setup to work without this patch (or
without disabling fw_devlink).

>
> >
> > -Saravana
> >
> > >
> > > -Saravana
> > >
> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > Cc: Stephen Boyd <swboyd@chromium.org>
> > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  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 a3483484a5a2..f7229e4030e3 100644
> > > > --- a/drivers/of/property.c
> > > > +++ b/drivers/of/property.c
> > > > @@ -1264,7 +1264,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")
> > > > @@ -1294,6 +1293,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)
> > > >  {
> > > > --
> > > > 2.33.0
> > > >
>
>
>
> --
> With best wishes
> Dmitry



-- 
With best wishes
Dmitry

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

[Saravana Kannan]

On Mon, Dec 6, 2021 at 6:00 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Hi Saravana,
>
> On Tue, 30 Nov 2021 at 03:24, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, 30 Nov 2021 at 02:53, Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Mon, Nov 29, 2021 at 3:48 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > Do not create device link for clock controllers.
> > > >
> > > > Nak.
> > > >
> > > > > 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,
> > > >
> > > > Right, because I wrote code to make sure we handle these clock
> > > > controller cases properly. If that logic isn't smart enough, let's fix
> > > > that.
> >
> > As I said, devlink was delaying dispcc probing ,waiting for the second
> > DSI PHY clock provider.
> > Thus came my proposal to let clock orphans handle the case (which it
> > does perfectly).
> >
> > > >
> > > > > but with two PHYs, dispcc doesn't get probed at all, thus
> > > > > breaking display support.
> > > >
> > > > Then let's find out why and fix this instead of hiding some
> > > > dependencies from fw_devlink. You could be breaking other cases/boards
> > > > with this change you are making.
> > >
> > > Btw, forgot to mention. I'll look into this one and try to find the
> > > reason why it wasn't handled automatically. And then come up with a
> > > fix.
> > >
> > > If you want to find out why fw_devlink didn't notice the cycle
> > > correctly for the case of 2 PHYs vs 1 PHY, I'd appreciate that too.
> > >
> > > Btw, same comment for remote-endpoint. I'll look into what's going on
> > > in that case. Btw, I'm assuming all the code and DT you are testing
> > > this on is already upstream. Can you please confirm that?
> >
> > All the code and basic DT is upstreamed. The DT part I
> > referenced/posted was written for the custom extender for the
> > qrb5165-rb5 board that I use here to test MSM DRM driver, but the
> > result DT should be more or less the same as smd845-mtp.

Can you point me to some upstream DTS file (not dtsi) that you think
will definitely have this issue (ideally you've actually hit it), and
the specific DT nodes in question? That'd make it much easier for me
to jump in and look as I'm not up to speed on all the MSM boards.

> So, is there a way we can assist you in debugging these issues? I
> still can not get dual DSI setup to work without this patch (or
> without disabling fw_devlink).

Sorry I've been a bit swamped. I'll try to take a look at this soon.

Another thing you could do is look at the existing code that detects
these cycles and fixes them up and figure out why it's not noticing a
cycle for your use case or not fixing the cycle correctly. You'll want
to look at calls to fw_devlink_relax_cycle() inside
fw_devlink_create_devlink().

-Saravana

>
> >
> > >
> > > -Saravana
> > >
> > > >
> > > > -Saravana
> > > >
> > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > Cc: Stephen Boyd <swboyd@chromium.org>
> > > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >  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 a3483484a5a2..f7229e4030e3 100644
> > > > > --- a/drivers/of/property.c
> > > > > +++ b/drivers/of/property.c
> > > > > @@ -1264,7 +1264,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")
> > > > > @@ -1294,6 +1293,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)
> > > > >  {
> > > > > --
> > > > > 2.33.0
> > > > >
> >
> >
> >
> > --
> > With best wishes
> > Dmitry
>
>
>
> --
> With best wishes
> Dmitry

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

[Dmitry Baryshkov]

Hi Saravana,

On Tue, 7 Dec 2021 at 05:24, Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Dec 6, 2021 at 6:00 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > Hi Saravana,
> >
> > On Tue, 30 Nov 2021 at 03:24, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Tue, 30 Nov 2021 at 02:53, Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Mon, Nov 29, 2021 at 3:48 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > Do not create device link for clock controllers.
> > > > >
> > > > > Nak.
> > > > >
> > > > > > 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,
> > > > >
> > > > > Right, because I wrote code to make sure we handle these clock
> > > > > controller cases properly. If that logic isn't smart enough, let's fix
> > > > > that.
> > >
> > > As I said, devlink was delaying dispcc probing ,waiting for the second
> > > DSI PHY clock provider.
> > > Thus came my proposal to let clock orphans handle the case (which it
> > > does perfectly).
> > >
> > > > >
> > > > > > but with two PHYs, dispcc doesn't get probed at all, thus
> > > > > > breaking display support.
> > > > >
> > > > > Then let's find out why and fix this instead of hiding some
> > > > > dependencies from fw_devlink. You could be breaking other cases/boards
> > > > > with this change you are making.
> > > >
> > > > Btw, forgot to mention. I'll look into this one and try to find the
> > > > reason why it wasn't handled automatically. And then come up with a
> > > > fix.
> > > >
> > > > If you want to find out why fw_devlink didn't notice the cycle
> > > > correctly for the case of 2 PHYs vs 1 PHY, I'd appreciate that too.
> > > >
> > > > Btw, same comment for remote-endpoint. I'll look into what's going on
> > > > in that case. Btw, I'm assuming all the code and DT you are testing
> > > > this on is already upstream. Can you please confirm that?
> > >
> > > All the code and basic DT is upstreamed. The DT part I
> > > referenced/posted was written for the custom extender for the
> > > qrb5165-rb5 board that I use here to test MSM DRM driver, but the
> > > result DT should be more or less the same as smd845-mtp.
>
> Can you point me to some upstream DTS file (not dtsi) that you think
> will definitely have this issue (ideally you've actually hit it), and
> the specific DT nodes in question? That'd make it much easier for me
> to jump in and look as I'm not up to speed on all the MSM boards.

I've been under the load thanks to the NY holidays.
I have verified that ach/arm64/boot/dts/qcom/sdm845-mtp.dts exhibits
the same behaviour. dispcc being deferred forever because of the
dependencies on dsi phy.

>
> > So, is there a way we can assist you in debugging these issues? I
> > still can not get dual DSI setup to work without this patch (or
> > without disabling fw_devlink).
>
> Sorry I've been a bit swamped. I'll try to take a look at this soon.
>
> Another thing you could do is look at the existing code that detects
> these cycles and fixes them up and figure out why it's not noticing a
> cycle for your use case or not fixing the cycle correctly. You'll want
> to look at calls to fw_devlink_relax_cycle() inside
> fw_devlink_create_devlink().

Could you please post patches that can assist you in debugging this?

>
> -Saravana
>
> >
> > >
> > > >
> > > > -Saravana
> > > >
> > > > >
> > > > > -Saravana
> > > > >
> > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > Cc: Stephen Boyd <swboyd@chromium.org>
> > > > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >  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 a3483484a5a2..f7229e4030e3 100644
> > > > > > --- a/drivers/of/property.c
> > > > > > +++ b/drivers/of/property.c
> > > > > > @@ -1264,7 +1264,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")
> > > > > > @@ -1294,6 +1293,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)
> > > > > >  {


-- 
With best wishes
Dmitry