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

[Saravana Kannan <saravanak@google.com>]

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
>