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 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 7ACF3C04FF3 for ; Mon, 24 May 2021 17:05:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5437961406 for ; Mon, 24 May 2021 17:05:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233376AbhEXRGs (ORCPT ); Mon, 24 May 2021 13:06:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39752 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233221AbhEXRGq (ORCPT ); Mon, 24 May 2021 13:06:46 -0400 Received: from mail-ua1-x92b.google.com (mail-ua1-x92b.google.com [IPv6:2607:f8b0:4864:20::92b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 54A42C06138B for ; Mon, 24 May 2021 10:05:17 -0700 (PDT) Received: by mail-ua1-x92b.google.com with SMTP id c6so9007289uat.0 for ; Mon, 24 May 2021 10:05:17 -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=eZESf2+UaWJEsGgPUeGFqkYRrVF4ir0zTy9O8j+JZj0=; b=F7m/3fCzNq2FjVowZE3b6kOUYNK0yc/QidlGDaLPRVfmD9v1gRRu7OVOrRBpTgyBAq RN6dpQp83jnaFaGvshIYnM+xXVDEAHcTxHRfUPB/SmG3IQsfVTPBnvWXIelgpaqEIIRp RbgRetkJQ+yYYTmV5IcLZ929/NeBfAGKyLUmXky1ShOLjOahGqBcLXtVlRg11DvEYpYd eIADl4LA6Dvl82BB+Ief4vDAB6PximvEmlgWhpWBcG4HyoBXvYyA9llRubapIvYM1pJi IwFJz0jr7NufaHk0RMW3QO5k29azEzEUM8yNELd2fPWiK5/1i9kbxxYnfWv9zDUII/Nj ggDg== 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=eZESf2+UaWJEsGgPUeGFqkYRrVF4ir0zTy9O8j+JZj0=; b=KVWWdaCRPVKpB96Kbks4xiiAGVv3A9hZnPKeXK62FcFjIgS3Enhjl320M2h1YaBYPT Dz/V3hJqgEqTmACCZ6jLc/cHCesBBz3ZDR1BEJI7Lb7PEYnAzDnXyH0QGWRxBzSTzlMD SSvN/j6Ng/4b3r53V4lF9zhuwrGj4DD8uYNziNtkZe1ghgaJFjFliF24F4gDHwTAL1f2 7jQnYmhW5RQPz2VzXF0n4OpnkCTj+nNFRHPLVWY4GSfhOTmPF0w9P2UXaNXXTtzKZm+w LKNhC/onoQAZURNC7wYeAbpdZ/xMVCZKwrxLfRirSDeIC5vlMcGih73Wi3kA/l9BJw4P EvoQ== X-Gm-Message-State: AOAM531cSjUb3b6Q3rIceqUzxLNwFeVY5BBuRelMwG+BA9eStQbdAnaZ YT6INHQAl3MA1LVV6iSB2wjG6DjEt1wz9UD5MXYvWw== X-Google-Smtp-Source: ABdhPJwVnMsBxavUG6WhLD7276brAnw7avHoO4W5DwCb7iPsdb9pe0vuL7T8bKsnZFn6ouLaui6EqThOSbhN4pDxRHE= X-Received: by 2002:ab0:d8f:: with SMTP id i15mr4749618uak.104.1621875916371; Mon, 24 May 2021 10:05:16 -0700 (PDT) MIME-Version: 1.0 References: <20210523231335.8238-1-digetx@gmail.com> <20210523231335.8238-14-digetx@gmail.com> In-Reply-To: <20210523231335.8238-14-digetx@gmail.com> From: Ulf Hansson Date: Mon, 24 May 2021 19:04:40 +0200 Message-ID: Subject: Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain To: Dmitry Osipenko Cc: Thierry Reding , Jonathan Hunter , =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , =?UTF-8?Q?Nikola_Milosavljevi=C4=87?= , Peter Geis , Nicolas Chauvet , Viresh Kumar , Stephen Boyd , Matt Merhar , Paul Fertser , Mark Brown , Liam Girdwood , Krzysztof Kozlowski , Mikko Perttunen , Linux Kernel Mailing List , linux-tegra , DTML , Linux PM , Nathan Chancellor , linux-clk Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 24 May 2021 at 01:13, Dmitry Osipenko wrote: > > NVIDIA Tegra SoCs have multiple power domains, each domain corresponds > to an external SoC power rail. Core power domain covers vast majority of > hardware blocks within a Tegra SoC. The voltage of a power domain should > be set to a level which satisfies all devices within the power domain. > Add support for the core power domain which controls voltage state of the > domain. This allows us to support system-wide DVFS on Tegra20-210 SoCs. > The PMC powergate domains now are sub-domains of the core domain, this > requires device-tree updating, older DTBs are unaffected. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Paul Fertser # PAZ00 T20 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko [...] > + > +static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np) > +{ > + static struct lock_class_key tegra_core_domain_lock_class; > + struct generic_pm_domain *genpd; > + const char *rname = "core"; > + int err; > + > + genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL); > + if (!genpd) > + return -ENOMEM; > + > + genpd->name = np->name; > + genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state; > + genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state; > + > + err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1); > + if (err) > + return dev_err_probe(pmc->dev, err, > + "failed to set core OPP regulator\n"); > + > + err = pm_genpd_init(genpd, NULL, false); > + if (err) { > + dev_err(pmc->dev, "failed to init core genpd: %d\n", err); > + return err; > + } > + > + /* > + * We have a "PMC pwrgate -> Core" hierarchy of the power domains > + * where PMC needs to resume and change performance (voltage) of the > + * Core domain from the PMC GENPD on/off callbacks, hence we need > + * to annotate the lock in order to remove confusion from the > + * lockdep checker when a nested access happens. > + */ Can you elaborate a bit more on this? Are you saying that when the child domain (PMC pwrgate) gets powered off, you want to drop its aggregated votes it may hold for the performance state, as otherwise it may affect the parent domain (core domain)? I guess this would be a valid scenario to optimize for, especially if you have more than one child domain of the core power domain, right? If you have only one child domain, would it be sufficient to assign ->power_on|off() callbacks for the core domain and deal with the performance stare votes from there instead? > + lockdep_set_class(&genpd->mlock, &tegra_core_domain_lock_class); > + > + err = of_genpd_add_provider_simple(np, genpd); > + if (err) { > + dev_err(pmc->dev, "failed to add core genpd: %d\n", err); > + goto remove_genpd; > + } > + > + return 0; > + > +remove_genpd: > + pm_genpd_remove(genpd); > + > + return err; > +} [...] > +static void tegra_pmc_sync_state(struct device *dev) > +{ > + int err; > + > + pmc->core_domain_state_synced = true; > + > + /* this is a no-op if core regulator isn't used */ > + mutex_lock(&pmc->powergates_lock); > + err = dev_pm_opp_sync_regulators(dev); > + mutex_unlock(&pmc->powergates_lock); > + > + if (err) > + dev_err(dev, "failed to sync regulators: %d\n", err); > +} > + Nitpick. Would you mind splitting the "sync_state" thingy out into a separate patch on top of $subject patch? I think it would be nice, especially since it shares a function via include/soc/tegra/common.h - that would make it clear to what part that belongs to. > static struct platform_driver tegra_pmc_driver = { > .driver = { > .name = "tegra-pmc", > @@ -3680,6 +3822,7 @@ static struct platform_driver tegra_pmc_driver = { > #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM) > .pm = &tegra_pmc_pm_ops, > #endif > + .sync_state = tegra_pmc_sync_state, > }, > .probe = tegra_pmc_probe, > }; > diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h > index af41ad80ec21..135a6956a18c 100644 > --- a/include/soc/tegra/common.h > +++ b/include/soc/tegra/common.h > @@ -23,6 +23,8 @@ struct tegra_core_opp_params { > #ifdef CONFIG_ARCH_TEGRA > bool soc_is_tegra(void); > > +bool tegra_soc_core_domain_state_synced(void); > + > int devm_tegra_core_dev_init_opp_table(struct device *dev, > struct tegra_core_opp_params *params); > #else > @@ -31,6 +33,11 @@ static inline bool soc_is_tegra(void) > return false; > } > > +static inline bool tegra_soc_core_domain_state_synced(void) > +{ > + return false; > +} > + > static inline int > devm_tegra_core_dev_init_opp_table(struct device *dev, > struct tegra_core_opp_params *params) Kind regards Uffe