Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bbcdfa45-64c8-72b8-e27c-bea924bf0398@linaro.org>
Date:   Fri, 20 Jan 2023 22:59:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [RESEND PATCH] of: property: do not create clocks device link for
 clock controllers
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org,
        Bjorn Andersson <andersson@kernel.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Saravana Kannan <saravanak@google.com>,
        Abel Vesa <abel.vesa@linaro.org>
References: <20230118091122.2205452-1-dmitry.baryshkov@linaro.org>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20230118091122.2205452-1-dmitry.baryshkov@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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 <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>
> ---
Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> # 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/20211125183622.597177-1-dmitry.baryshkov@linaro.org/
> [2] https://lore.kernel.org/lkml/CAA8EJpqELXvRMPWJdTLCURjwkcMxyPDPj1tVZPkdOT_JVQb4-w@mail.gmail.com/
> [3] https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/
> [4] https://lore.kernel.org/all/YrsdLQrOtg1qdaoE@linaro.org/
> [5] https://lore.kernel.org/all/20230118082048.2198715-1-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 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)
>  {