From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D5CA4C433EF for ; Tue, 7 Dec 2021 02:24:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240876AbhLGC2V (ORCPT ); Mon, 6 Dec 2021 21:28:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46072 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232569AbhLGC2V (ORCPT ); Mon, 6 Dec 2021 21:28:21 -0500 Received: from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com [IPv6:2607:f8b0:4864:20::b36]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E1B38C061746 for ; Mon, 6 Dec 2021 18:24:51 -0800 (PST) Received: by mail-yb1-xb36.google.com with SMTP id d10so36934224ybn.0 for ; Mon, 06 Dec 2021 18:24:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2OMixw9Me0yHhWDDFaCGdGZaAxF8n91aCEtHks1Fm+k=; b=CGZbhBmdY5MHv2LFP5YUvDRPTzIFRZu1FsTDtGhF9qgzXDYoAsK68UvPmq/QhFY3QQ YTV2rd/IMY67472eI/UY7WiVkJVpuHBh+G/8pL2L+YMYp74z6CvHMdq66+3k9QBNNe7M DJPZZCKaRNNgydzIVvjFBowrz6D3uPn1B26YVmO/7KotpsKxVZefoSJDStMkcqM8QHPZ VHaQw4MNp+i6h/y4Z+6P57te1oqSWi6wYCAPPH+FKhoZhyxLUWhQ9aNcTG/v2YkqBerO sSpb0CLj897HnY35eaZkBYwTlBj0fofJzClPY3052lGVbwBXiDWlLynvOB7audlWW4Lu KWRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2OMixw9Me0yHhWDDFaCGdGZaAxF8n91aCEtHks1Fm+k=; b=K3VEEmv86rwzWcpFvi9i4l0XD5olu0J7NYmDQYxNYcP8MT0jTIgDhvzZig8Uds4tCw tYPjvFAdYR6ttcoznk+9AOgutdnTzepl9xt32Yz5GGUmWGM0+7GrkF/2D5fG+D58ezqg hNIUb/Rp8NI3JW9fxOo2FHNIcnwNRThaq2qZs07RJnbjsQrEVqCrZ6LjTetCVRzqrGBS 7kZ94nDyihsEQoOI1JYeYTYnL2iGqVzKlm7Ue1xUR6sblOpfLvBv2dZGmZs50iAkOyYT IF/Ua0EN/1MIu0WF4wz4vcb9KqHke3Zdt79mSbb9nSt5FgQZHeqV9Quy5vhBVjC1Z3gl NZkg== X-Gm-Message-State: AOAM532JKZuVIIbtz7l2hNWYHOjm9q0KcxLK80PiE0lASfhGmDINctQS snfI6ufDgprAzAd6gHo+apFsh25qcIjPLdCJY+366Q== X-Google-Smtp-Source: ABdhPJz4iot5xR5ycz2pnVGAMO4lq4a50xzmyMnRXGumF6yGCeBGm4Djut0AsSC8Z9NssHyuMmZxcc3IN0gvH+mqqqk= X-Received: by 2002:a25:6c6:: with SMTP id 189mr47279877ybg.753.1638843890905; Mon, 06 Dec 2021 18:24:50 -0800 (PST) MIME-Version: 1.0 References: <20211125183622.597177-1-dmitry.baryshkov@linaro.org> In-Reply-To: From: Saravana Kannan Date: Mon, 6 Dec 2021 18:24:15 -0800 Message-ID: Subject: Re: [PATCH] of: property: do not create clocks device link for clock controllers To: Dmitry Baryshkov Cc: Rob Herring , Frank Rowand , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Bjorn Andersson , Stephen Boyd , Android Kernel Team , Rob Clark Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Mon, Dec 6, 2021 at 6:00 PM Dmitry Baryshkov wrote: > > Hi Saravana, > > On Tue, 30 Nov 2021 at 03:24, Dmitry Baryshkov > wrote: > > > > On Tue, 30 Nov 2021 at 02:53, Saravana Kannan wrote: > > > > > > On Mon, Nov 29, 2021 at 3:48 PM Saravana Kannan wrote: > > > > > > > > On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov > > > > 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 > > > > > Cc: Stephen Boyd > > > > > Cc: Saravana Kannan > > > > > Signed-off-by: Dmitry Baryshkov > > > > > --- > > > > > 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