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 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=-14.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,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 50A50C432BE for ; Wed, 1 Sep 2021 08:49:01 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0D01D60C3E for ; Wed, 1 Sep 2021 08:49:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 0D01D60C3E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tm6NrFRAu5a8a7Tlkvx/sm3U8IwDPh4i+VyZNU7CQgI=; b=BXePbUV5lDbMG2 CnoBZRGTWmoT9xawYDt6b5xJRG1cROv2DpXffWQNAN2+cByS+GCVRQC4wKWqIXshbssr/IuZN3U8J RCFF0zMkv2guwsGJeWCZ40dhIsbaydc9aSbcTQbduQtCs5dWwOjhJhTqTV1xEJ4i80rG5ryy/bWO0 1E3XUfChE26SOd6z1d6d/n90BMJXJn3WppPaET7t6oJAOqP8dRj16v/XbvRZGTCb6ySrZxf8+bonV QvgQ8Im42bjT9qF59J4iY9XwpKlHPQXBXJNIRegux508im9moW4R7rd9TsuAvv1RYnlYX5GkebsNk HMUvgNv2HzpX7qVVg8OQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mLLt1-004e6G-7I; Wed, 01 Sep 2021 08:46:24 +0000 Received: from mail-lf1-x12b.google.com ([2a00:1450:4864:20::12b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mLLMm-004WCm-DZ for linux-arm-kernel@lists.infradead.org; Wed, 01 Sep 2021 08:13:08 +0000 Received: by mail-lf1-x12b.google.com with SMTP id bq28so4681966lfb.7 for ; Wed, 01 Sep 2021 01:13:03 -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=hGOWEjQF4nrPzPPEyHXcIJriOfbEymJPPRFj1MZpyLaZZ5zodcteDPTu7ITAjBBHjs A1/1js6fThkNsuWg1ou9W+MC3yPUEpWAERaAe3XlBa3KOEk2mKyA0QXYrE/PQK1DWtFG rfRj1mZVAU6VLileIvot4JdPTZOuBPdUm6ZwIJDFGpN53gUVooLt0oROaMVEfF1Fqxjr w3evnVwzEQq0oRr4ACy1Ke7y2YavrwZjSOdGDgPowPDK7HzozpzCzvwicOpifnf+SUUH Ji8QDa57gdq5sjvnYiv2mFvYa+1SlU4i3+BO8rjCocRzXcKR0LS/GcbRCzrtv61LDgK7 rRUg== X-Gm-Message-State: AOAM531N5wk//hW036oLLFtwLs713balHT4vhPDBQrntST/Ox5ka2ALB RxddMJ2Mp1rR3eSxZoCF/ET0brUdnjQsYQNj1zhc/A== 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210901_011304_607434_3B78D5FF X-CRM114-Status: GOOD ( 39.41 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel