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=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no 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 57777C43462 for ; Wed, 12 May 2021 13:51:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1D7B460FE6 for ; Wed, 12 May 2021 13:51:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231308AbhELNwT (ORCPT ); Wed, 12 May 2021 09:52:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53472 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231204AbhELNwR (ORCPT ); Wed, 12 May 2021 09:52:17 -0400 Received: from mail-vk1-xa33.google.com (mail-vk1-xa33.google.com [IPv6:2607:f8b0:4864:20::a33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5FA86C06174A for ; Wed, 12 May 2021 06:51:08 -0700 (PDT) Received: by mail-vk1-xa33.google.com with SMTP id 31so3596120vkl.12 for ; Wed, 12 May 2021 06:51:08 -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=Jowz1jnkMOr3gH5yQSgGMVN6x7Lry8Su5OlE7bU8MPI=; b=x8zdvCKn2bNSMcUkWXc+bpIp2E636IiqxMYB9KGWoZUar2n/Bf0XGdc9a+mhfCveWy PDPrNaXNtGyqg5Re5lKi11lHdMNdatRwBgV0zPz6wqlPfyc+jsCsdVtB9+Pcaau6VrtM mvkwqrCJ0kF44hjW7u0JjznAUBXS3xVIUwz2TEDI4vqBSsSX9EIL8caTXZ5w2+KXanWr /2xkp7EmD63TzYJOHRFlYk580hc5jTj6kKnvwBDCPD8r2pNYWruX+1FGR6IJCvfgsy49 WjqTud2XfJCO4zXEKLRH5eplfPoQtKLzGzf0wpBGxPEgbjJzSATCogbNwsnN7Y4XJSt3 SWhQ== 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=Jowz1jnkMOr3gH5yQSgGMVN6x7Lry8Su5OlE7bU8MPI=; b=VowqKaqmqLVwFocad3UDvfj1gw0cFfpb1ogrkm5RCUqqX+QM2HpfszMMBlI0fICvus 4LHipuySgNV7n4Vq4fxWQgLDq4DeuU0eEoKcBl3PNVN/SpCtv3bkgkBtVFUJvefTbN/X qufbBLknkRk9rs/EdyUnq9QLOtFMvuSHp1D6OUNfUB57NWt8KjRwB0cPLCbZF+oFYj0t EPCmkwMhrTFnxL5RtFMP92o52rqcUIpwSI7ZRmZYr6M712jAtA2rEWx2z5EKEUPNsyOg 9P+7SIc+1IqtVW4p56CE49SoEF7L0QYuUBqnAC0o3/KyGvIVq65aXptXHizmSfweaRCm W8hQ== X-Gm-Message-State: AOAM530YIGdKTyNl36icgTh8kM7/hkkAmjRm2GZ/FhtEe0ilivYVYbM9 YelVofHh53Ck+SC+q5TGcql0gYnwGyY8HJDaSxcBYQ== X-Google-Smtp-Source: ABdhPJx8zR9ROGiDsPfhddCCRFlE4zhaPq5y1bpybJCuT4h5Kz2qpp6Svv+S8YHJKW2TJ4bd2N59hBdWm2IOHM72cxI= X-Received: by 2002:a1f:5504:: with SMTP id j4mr28021678vkb.7.1620827467457; Wed, 12 May 2021 06:51:07 -0700 (PDT) MIME-Version: 1.0 References: <20201224111210.1214-1-rojay@codeaurora.org> <20201224111210.1214-4-rojay@codeaurora.org> <6bfec3e6-3d26-7ade-d836-032273856ce2@codeaurora.org> <20210429075054.vrotcbldbaivfh2d@vireshk-i7> <3743d729-4287-a389-72e2-2201ee59601d@codeaurora.org> In-Reply-To: From: Ulf Hansson Date: Wed, 12 May 2021 15:50:31 +0200 Message-ID: Subject: Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states' To: Rajendra Nayak Cc: Viresh Kumar , Bjorn Andersson , Roja Rani Yarubandi , Rob Herring , Wolfram Sang , Stephen Boyd , Doug Anderson , Sai Prakash Ranjan , Matthias Kaehlcke , akashast@codeaurora.org, msavaliy@qti.qualcomm.com, parashar@codeaurora.org, Linux PM , DTML , Linux Kernel Mailing List , linux-arm-msm , Andy Gross , linux-i2c@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 10 May 2021 at 08:37, Rajendra Nayak wrote: > > > > On 5/7/2021 2:36 PM, Ulf Hansson wrote: > > On Tue, 4 May 2021 at 09:18, Rajendra Nayak wrote: > >> > >> > >> []... > >>>>>> > >>>>>> Ulf, Viresh, I think we discussed this at the time of introducing the > >>>>>> performance states. > >>>>>> > >>>>>> The client's state does not affect if its performance_state should > >>>>>> be included in the calculation of the aggregated performance_state, so > >>>>>> each driver that needs to keep some minimum performance state needs to > >>>>>> have these two snippets. > >>>>>> > >>>>>> Would it not make sense to on enable/disable re-evaluate the > >>>>>> performance_state and potentially reconfigure the hardware > >>>>>> automatically? > >>>>> > >>>>> I agree, this will be repeated across multiple drivers which would > >>>>> need some minimal vote while they are active, handling this during > >>>>> genpd enable/disable in genpd core makes sense. > >>>> > >>>> Initially that's what we tried out, but we realized that it was > >>>> difficult to deal with this internally in genpd, but more importantly > >>>> it also removed some flexibility from consumers and providers. See > >>>> commit 68de2fe57a8f ("PM / Domains: Make genpd performance states > >>>> orthogonal to the idlestates"). > >>>> > >>>> As a matter of fact this was quite recently discussed [1], which also > >>>> pointed out some issues when using the "required-opps" in combination, > >>>> but perhaps that got resolved? Viresh? > >>> > >>> So I looked again at that thread in detail today. The basic idea was > >>> to enable/disable the genpd from within the OPP core and there were > >>> doubts on how to do that efficiently as there are cases where domains > >>> may be enabled for an OPP, but not for others.. etc. etc. > >>> > >>> I am not sure if I consider that thread as part of the discussion we > >>> are having here, they may be related, but that thread doesn't block > >>> anything to be done in the genpd core. > >> > >> That's true, the 2 threads are different in the sense that one talks > >> about having OPP core managing power on/off along with setting perf state, > >> while the other talks about genpd core managing a default perf state > >> along with power on/off, but they are similar in the sense that both > >> are related to the discussion whether we should treat powering on and off > >> a domain related to setting its performance state or if it should be > >> considered completely orthogonal. > >> > >> I think the clock framework treats setting clock rates and turning > >> on/off a clock orthogonal because there is an inherent assumption that > >> once the clock is turned off, what rate it was set to should not matter, > >> and it can be running at the same rate when we turn the clock back on. > >> > >> I guess we can have the same assumption here that a perf state of a > >> power domain should not matter if the power domain is turned off > >> and hence the perf state need not be dropped explicitly during power off, > >> atleast that should be true for the qcom power domains supporting perf > >> state upstream. > >> > >> Should that be the approach taken here? I guess that would mean the patch > >> I had proposed earlier [1] to manage this in the genpd core would have to set the default > >> perf state at attach and remove it only during a detach of the device to > >> the pm_domain, and not manage it during the runtime_suspend/resume of the device. > > > > Right, I think this would be a step in the right direction, but it's > > not sufficient to solve the complete problem. As you also point out > > below. > > > >> > >>>> A consumer driver > >>>> can no longer make its vote for its device to stick around, when the > >>>> device becomes runtime suspended - and how do we know that we never > >>>> need to support such a case? > >> > >> The above approach should take care of this but the down side of it would be, > >> unlike in the case of clocks where the devices assigning a default clock rate > >> might be doing so on a device specific clock (rarely shared with other devices) > >> in case of power domain, and especially in the qcom implementation of these > >> power domains which support perf state, these can be large domains with lots of devices, > >> and any device being active (not necessarily wanting any default perf state) will keep > >> the domain at the default perf state, requested by a device which isn't really active. > > > > Yep, this certainly sounds suboptimal. To me, this isn't good enough. > > > >> > >>> What about doing this just for the assigned-performance-state case as > >>> the clients don't want to play with it at all. > >> > >> well, thats possible too, but you obviously can't reuse the same bindings > >> in such cases > > > > Not sure I understand the issue with the DT binding? Let me elaborate > > on how I think we could move forward. > > > > It looks like we have two problems to solve: > > > > *) We need a new DT binding. > > If that becomes a generic property along the lines of the > > "assigned-performance-state" as suggested - or if we decide to add a > > SoC specific binding via using an additional cell in "power-domains" > > (suggested by Rob), doesn't really matter much to me. The arguments > > for the new DT property are very much similar to why we added > > "assigned-clock-rates" for clocks. > > > > **) We want to avoid boiler-plate code in drivers to manage > > "assigned-performance-state" for their devices. > > No matter what DT property we decide on (generic or SoC specific), we > > should be able to manage this from the PM domain (genpd) layer. No > > changes in the drivers should be needed. > > If a generic binding is used, we could consider to let genpd > > internally manage the whole thing (DT parsing and updating performance > > state votes for assigned-performance-state only). > > Sure, so for starters does that mean I should re-spin my series which > adds the generic 'assigned-performance-states' bindings and see if Rob > is OK with that? I am guessing you are OK with the way that binding gets > used within genpd core in that series, or would you want it to be handled > differently? A re-spin would definitely move this forward. If we can convince Rob about the generic DT binding, I will certainly agree to change genpd to help/manage the DT parsing. Although, I am not sure yet what is the best. Let genpd to do the parsing internally in genpd_add_device() or just provide a helper function that can be called from an ->attach|detach_dev() callback. In the end it boils done to decide whether we should use the ->start|stop() callbacks or let genpd internally manage the "assigned performance state". Honestly, I would prefer to look at how the code would need to be changed, before answering this. > > > If we go for an SoC specific binding, the genpd provider needs to be > > updated. It can manage DT parsing from the ->attach|detach_dev() > > callbacks and update performance votes from the ->start|stop() > > callbacks. > > We could also consider a hybrid of these two solutions. > >> > >> [1] https://lore.kernel.org/patchwork/patch/1284042/ Kind regards Uffe