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=-16.0 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 autolearn=ham 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 8DFFFC07E99 for ; Fri, 9 Jul 2021 14:11:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7781F613B5 for ; Fri, 9 Jul 2021 14:11:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232050AbhGIOOc (ORCPT ); Fri, 9 Jul 2021 10:14:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56344 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232042AbhGIOOb (ORCPT ); Fri, 9 Jul 2021 10:14:31 -0400 Received: from mail-ua1-x92d.google.com (mail-ua1-x92d.google.com [IPv6:2607:f8b0:4864:20::92d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 872D4C0613E6 for ; Fri, 9 Jul 2021 07:11:48 -0700 (PDT) Received: by mail-ua1-x92d.google.com with SMTP id d2so3647589uan.0 for ; Fri, 09 Jul 2021 07:11:48 -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=xE3GuXPxQPxng6SCV9memDLzNaTsrWlJuTxjZOCkSxg=; b=GrAOP1zSUEmQXCaJ2iewcBz7VjF//nNlpFcdHfxooxPio55+5P9rqbM5m1IBdOQ87l QT9jrNpsbpck1E1mC6SWtRrgKUETw4gUU4YeNXMzGzoJt6KoShPDb4dt6SWqCol8AQGU On9nVbFU5jOx5T9puRYl5UUJXgIf/PImfzvaVhf4YIbgW25qirmG0nLO4746Axs291+e uwKVefmSkXvh88FZqmJ+KxQRMNy5scw/VeZiO2MudUG19RlaXLbMGcE9/Bb6JW65phsP XcYMm5ryF6gwOd+TtV5muvH3PGFKKrHr2ZpIbt+GxJIPlhJ9k82mNcTy4rAFNiRf2IzR C2fw== 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=xE3GuXPxQPxng6SCV9memDLzNaTsrWlJuTxjZOCkSxg=; b=BU75DLrD0LniAf3knI0yO8PNdTZz4ReizeeVSAOWgdJEg5MtoljByO9rT6spRNAxAL KTQ+pYbMRNxXpTJIEm8uwknWxutwrhkWCRmhqY7eEM7h8NDcc82Aka2Xa4+Y1/jwdet0 AJZR82Vic/oQdicVmEPMxoYQNW427BBmzZLJERZOQqdsjR1ncj9HYyeyE0+uipMhMDhi u1EzSXEZPvtpNNVaWNppocfFaFjRiR0FZar46YezBCJn598+W1kKi3oPgAWt+MEQQbRj raIKygf3gKMYfUPy3PkklnAzT9Yhk5s1W2iuPvN0RU9gliHNoK/Jlo2NM+uZV0K+Mcn9 L2ZA== X-Gm-Message-State: AOAM532dRgKJrXsrBjEpd8cqATqbdr7yCDwesZAFRg3NtQHc2zYQDOGi FEKgMQODV4itplFUUH2bUxZn07HTA/mTd2r2Hg2Pkw== X-Google-Smtp-Source: ABdhPJxJUmjEDStUNDJD1nHTQrDVNPoKS5CsDPGXfKJWvU8a14MA901o/PtVmscQQ1ribEEQeSDP1XhFFCy8EzfhT0g= X-Received: by 2002:ab0:42a6:: with SMTP id j35mr35590994uaj.129.1625839907672; Fri, 09 Jul 2021 07:11:47 -0700 (PDT) MIME-Version: 1.0 References: <20210709043136.533205-1-dmitry.baryshkov@linaro.org> <20210709043136.533205-5-dmitry.baryshkov@linaro.org> In-Reply-To: From: Ulf Hansson Date: Fri, 9 Jul 2021 16:11:11 +0200 Message-ID: Subject: Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support To: Dmitry Baryshkov Cc: Andy Gross , Bjorn Andersson , Rob Herring , Stephen Boyd , Taniya Das , Jonathan Marek , Michael Turquette , linux-arm-msm , DTML , linux-clk , "Bryan O'Donoghue" , Mark Brown , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On Fri, 9 Jul 2021 at 15:22, Dmitry Baryshkov wrote: > > On Fri, 9 Jul 2021 at 16:14, Ulf Hansson wrote: > > > > On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov > > wrote: > > > > > > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson wrote: > > > > > > > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov > > > > wrote: > > > > > > > > > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson wrote: > > > > > > > > > > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov > > > > > > wrote: > > > > > > > > > > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power > > > > > > > domain. Currently we used a regulator to enable this domain on demand, > > > > > > > however this has some consequences, as genpd code is not reentrant. > > > > > > > > > > > > > > Teach Qualcomm clock controller code about setting up power domains and > > > > > > > using them for gdsc control. > > > > > > > > > > > > > > Signed-off-by: Dmitry Baryshkov > > > > > > > > > > > > [...] > > > > > > > > > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > > > > > > > index 51ed640e527b..9401d01533c8 100644 > > > > > > > --- a/drivers/clk/qcom/gdsc.c > > > > > > > +++ b/drivers/clk/qcom/gdsc.c > > > > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc, > > > > > > > continue; > > > > > > > scs[i]->regmap = regmap; > > > > > > > scs[i]->rcdev = rcdev; > > > > > > > + scs[i]->pd.dev.parent = desc->dev; > > > > > > > ret = gdsc_init(scs[i]); > > > > > > > if (ret) > > > > > > > return ret; > > > > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc, > > > > > > > continue; > > > > > > > if (scs[i]->parent) > > > > > > > pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd); > > > > > > > + else if (!IS_ERR_OR_NULL(dev->pm_domain)) > > > > > > > > > > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being > > > > > > called for gdsc platform device from the platform bus', to try to > > > > > > attach the device to its corresponding PM domain. > > > > > > > > > > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we > > > > > > shouldn't really try to attach a device to its PM domain, when its OF > > > > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This > > > > > > is because it indicates that the device belongs to a genpd provider > > > > > > itself. In this case, a "power-domains" specifier tells that it has a > > > > > > parent domain. > > > > > > > > > > > > I will post a patch that fixes this asap. > > > > > > > > > > I think there is nothing to fix here. The dispcc/videocc drivers > > > > > provide clocks in addition to the gdsc power domain. And provided > > > > > clocks would definitely benefit from having the dispcc device being > > > > > attached to the power domain which governs clock registers (MMCX in > > > > > our case). Thus I think it is perfectly valid to have: > > > > > > > > > > rpmhpd device: > > > > > - provides MMCX domain. > > > > > > > > > > dispcc device: > > > > > - is attached to the MMCX domain, > > > > > > > > We don't need this, it's redundant and weird to me. > > > > > > > > Also I am kind of worried that you will hit another new path in genpd, > > > > causing locking issues etc, as it has not been designed to work like > > > > this (a provider device and a child domain sharing the same "parent"). > > > > > > So, which domain should the dispcc device belong to? It's registers > > > are powered by the MMCX domain. I can not attach it to the child > > > (GDSC) domain either: in the case of videocc there are 4 child > > > domains. > > > > The dispcc device should *not* be attached to a PM domain. > > > > Instead it should be registered as a genpd provider and the > > corresponding PM domains it provides, should be assigned as child > > domains to the MMCX domain. > > > > This is exactly what the child/parent domain support in genpd is there > > to help with. > > This is done in this patchset. If we stop attaching dispcc to the MMCX > genpd, I'll have to locate it in a different way, but the idea is > implemented here. Right. Perhaps it's not such a bad idea after all as it gives you two things: 1) The handle to the MMCX PM domain, which makes sure it has been registered too before dispcc gets probed. 2) The possibility to control power for the MMCX PM domain via runtime PM for the dispcc device. This seems useful for your use case. > > > > An alternative would be to request that all users of the provided > > > clocks power on one of the child domains. However this is also not > > > perfect. If some generic code (e.g. clock framework) calls into > > > provided clocks (e.g. because of assigned-clock-rates), this can > > > happen w/o proper power domain being powered up yet. > > > > Issues with power on/off synchronization during genpd initializations > > and genpd provider registration, certainly need to be fixed and I am > > happy to help. However, my point is that I think it's a bad idea to > > fix it through modelling the PM domain hierarchy in an incorrect way. > > So, which device should I pass to clk_register to handle runtime PM > for the provided clocks? dispcc, should I not? Right, anything but dispcc seems wrong. > Then if the dispcc is not attached, we will have to manually handle > MMCX from dispcc's runtime pm callbacks. Correct? Yep - and we don't want that either. > > Could you please be more specific, why is it so wrong to attach dispcc > to the MMCX genpd? In the end it seems like I just needed to make my brain feel a little more comfortable with the ideas that you put forward. It should work fine, I think! My apologies for all the noise. KInd regards Uffe