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 X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6A8C6C4320A for ; Wed, 1 Sep 2021 08:13:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 55A1D6103A for ; Wed, 1 Sep 2021 08:13:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242992AbhIAIOD (ORCPT ); Wed, 1 Sep 2021 04:14:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54158 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242921AbhIAIOA (ORCPT ); Wed, 1 Sep 2021 04:14:00 -0400 Received: from mail-lf1-x129.google.com (mail-lf1-x129.google.com [IPv6:2a00:1450:4864:20::129]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5A9A1C0613C1 for ; Wed, 1 Sep 2021 01:13:04 -0700 (PDT) Received: by mail-lf1-x129.google.com with SMTP id c8so4730212lfi.3 for ; Wed, 01 Sep 2021 01:13:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=x2tKAP/1aZjb1THg3HM096zaMHW+UEhDKNrwvOztN+o=; b=NtdFfLlMBvvwmCb3CWPpIKNwfwDjzXrs9rRY0SFXdfUlT+qdz9Su5Ptd4lNpsXckXV wymW4+zGn+9GDI4Qh/wk/gLLDJVfQBuqAEhHLk565ytuia6g8Y2vYC7z0DzwmwRkhU22 stPv6sldloGbiFxEcEprQr93tsb9hhR8/efVxzERsFIqg8RK1O7CSUT2kTmjI0vUyevv 2ihP59qDk8Xly7x43n4Z7pObGikldxv2cCbzbBOlnoDEUtZNk8cf3etRHcqneLLmV6TQ ESaP5kSJzmkCnp0Tr0xyayje1gOJmPDeaO5kJEVx9p6vSQjUVRz5oUUDCulxt/jHKkZL JQiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=x2tKAP/1aZjb1THg3HM096zaMHW+UEhDKNrwvOztN+o=; b=IbwGlKu1fQiSQE+V7U4hvMyJ43qi2jcmSJKMyCUMlHht/tHhroc0vvyjBFKKS691RL 1WsFeF5x1SpxWvBLumpHEsoTfP5c9BCT/PjsTKgj+BrthLl0gXPHBeQM0E80/PkjHeY6 omt6RuIOjqwQgIczfBbi/jLrjAvp69VMSfT5dWWvbpud0ppqDQXLYhRu/aP+a8EWa3Fp SFi6teWvNmkFnlwKalgh5/+Ny7bErsJZEW6wS3PPWQoWUcCoug/58Y9JRz0i29IMbgds HlHi1w2XbU7BWQzzfD7KigmSKPfWdIhDtMq80HRRwrJGtESX+A/ewCDgrt9wmSZkP/Vi 3K4Q== X-Gm-Message-State: AOAM532OsFJdj0JJKm9iDjyT6sVwapXoDrhwPqIRkleos+7XXReFuwjq oJq/QFmQPhHJ0+RUBJjRqvY/1rjQhQCc31waWUwb9w== X-Google-Smtp-Source: ABdhPJzq8baOlGTK2GwvtcD99p4jRPFD/7gsxAm2hrNPt8zCt6zdxwd4ae6WRjr3X66BQKEonrqMI3gmqCxmWhAvYIc= X-Received: by 2002:a19:c107:: with SMTP id r7mr24899153lff.29.1630483982545; Wed, 01 Sep 2021 01:13:02 -0700 (PDT) MIME-Version: 1.0 References: <20210831102141.624725-1-ulf.hansson@linaro.org> In-Reply-To: From: Ulf Hansson Date: Wed, 1 Sep 2021 10:12:26 +0200 Message-ID: Subject: Re: [PATCH 2/2] of: property: fw_devlink: Set 'optional_con_dev' for parse_power_domains To: Saravana Kannan Cc: Rob Herring , DTML , "Rafael J . Wysocki" , Stephen Boyd , Dmitry Osipenko , Linux PM , Linux Kernel Mailing List , Linux ARM Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 31 Aug 2021 at 19:51, Saravana Kannan wrote: > > On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson wrote: > > > > The power-domain DT bindings [1] doesn't enforce a compatible string for a > > provider node, even if this is common to use. In particular, when > > describing a hierarchy with parent/child power-domains, as the psci DT > > bindings [2] for example, it's sometimes not applicable to use a compatible > > string. > > Ok, and fw_devlink handles that -- provider not having a compatible > string is pretty common. In these cases, the parent node is the actual > device that gets probed and registers the provider. So fw_devlink will > create a link from the consumer to the parent device node. Yes, correct. That is working fine and isn't a problem. The first problem (I think) is that fw_devlink creates a fw_devlink from a child provider node (consumer without compatible string) to a parent node (supplier with a compatible string). I don't understand the reason why this is needed, perhaps you can elaborate on why? I come to the second and follow up problem from this behaviour, see below. > > > Therefore, let's set the 'optional_con_dev' to true to avoid creating > > incorrect fw_devlinks for power-domains. > > This part doesn't make sense or is incomplete. What is being done incorrectly? See below. > > > > > [1] Documentation/devicetree/bindings/power/power-domain.yaml > > [2] Documentation/devicetree/bindings/arm/psci.yaml > > > > Signed-off-by: Ulf Hansson > > --- > > > > Some more details of what goes on here. I have added a debug print in > > of_link_to_phandle() to see the fw_devlinks that gets created. > > > > This is what happens on Dragonboard 410c when 'optional_con_dev' isn't set: > > ... > > [ 0.041274] device: 'psci': device_add > > [ 0.041366] OF: Linking power-domain-cpu0 (consumer) to psci (supplier) > > [ 0.041395] OF: Linking power-domain-cpu1 (consumer) to psci (supplier) > > [ 0.041423] OF: Linking power-domain-cpu2 (consumer) to psci (supplier) > > [ 0.041451] OF: Linking power-domain-cpu3 (consumer) to psci (supplier) > > [ 0.041494] device: 'platform:psci--platform:psci': device_add > > [ 0.041556] platform psci: Linked as a sync state only consumer to psci Because we created a fw_devlink for the child provider nodes (consumer) that lacks compatible properties, we end up creating a sync state only devlink. I don't think it serves a purpose, but I may be wrong!? Additionally, the actual devlink that is created, has the same supplier and consumer device, which indicates that this isn't the right thing to do. > > ... > > > > This is what happens on Dragonboard 410c when 'optional_con_dev' is set: > > ... > > [ 0.041179] device: 'psci': device_add > > [ 0.041265] OF: Not linking psci to psci - is descendant > > [ 0.041293] OF: Not linking psci to psci - is descendant > > [ 0.041319] OF: Not linking psci to psci - is descendant > > [ 0.041346] OF: Not linking psci to psci - is descendant > > ... > > Can you please explain what exactly is going on that's wrong here? I > notice that psci is not probed as a device at all. And when you aren't > setting this flag the only difference I see is the creating of a sync > state only link -- which shouldn't matter here because you don't even > have a driver implemented. See above. > > > The relevant dtsi file: > > arch/arm64/boot/dts/qcom/msm8916.dtsi > > > > Kind regards > > Ulf Hansson > > > > --- > > drivers/of/property.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 2babb1807228..4d607fdbea24 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1356,7 +1356,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { > > { .parse_prop = parse_io_channels, }, > > { .parse_prop = parse_interrupt_parent, }, > > { .parse_prop = parse_dmas, .optional = true, }, > > - { .parse_prop = parse_power_domains, }, > > + { .parse_prop = parse_power_domains, .optional_con_dev = true, }, > > This change is just shooting in dark/completely unrelated to the > commit text. This is just saying the actual consumer is a level up > from where the property is listed (eg: remote-endpoint). It just > happens to fix your case for unrelated reasons. Again, see above. > > Definite Nak as this *will* break other cases. In what way will this break other cases? Would you mind elaborating for my understanding and perhaps point me to an example where it will break? > > -Saravana > > > > { .parse_prop = parse_hwlocks, }, > > { .parse_prop = parse_extcon, }, > > { .parse_prop = parse_nvmem_cells, }, > > -- > > 2.25.1 > > Kind regards Uffe