Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Nov 2022 16:34:35 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     James Calligeros <jcalligeros99@gmail.com>
Cc:     asahi@lists.linux.dev, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, nm@ti.com, sboyd@kernel.org,
        vireshk@kernel.org
Subject: Re: [PATCH v2] OPP: decouple dt properties in opp_parse_supplies()
Message-ID: <20221103110435.5egvoqwppczmij3t@vireshk-i7>
References: <20221102090228.odyt7ykdkcbdx4lw@vireshk-i7>
 <20221102102011.11623-1-jcalligeros99@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221102102011.11623-1-jcalligeros99@gmail.com>
Precedence: bulk

On 02-11-22, 20:20, James Calligeros wrote:
> On Apple SoCs, all the regulators are controlled transparently
> by the hardware/firmware. The CPUfreq driver requests a pstate as
> described in the OPP table by setting some bits in some registers, and
> the platform handles everything else.
> 
> Not only is there no use for the voltage and current information from
> the kernel's perspective since there's nothing to control, we don't
> even really have a way to reliably model their behaviour.
> 
> What we can do, however, is use energy counter registers in the core
> clusters to determine energy consumption at a given pstate and register
> that with the OPP core using the opp-microwatt property. Given that its
> stated purpose is to enable such behaviour, requiring it to be coupled
> to opp-microvolt is a major design deficiency.

I wasn't asking this. I was rather asking if some code for your
platform calls dev_pm_opp_set_regulators(). I assume No based on what
you provided.

> >I won't call it a fix, we are trying to use this information in a
> >different way here, that's all.
> 
> My interpretation of the commit message for 32bf8bc9a077 is that this
> is the way in which the property was intended to be used, and that it not
> working like this is a bug. Quoting the commit message, "Some of the platforms
> don't expose voltage information, thus it's not possible to use EM
> registration using DT. This patch aims to fix it." 
> 
> There is probably a bigger discussion to be had on whether or not parsing
> opp-microwatt for the purpose of EM registration should be entangled in
> opp_parse_supplies() at all, but that's by the by.

I get it. I will still skip the Fixes tag for now as that may make
people backport this to older kernels, which we may not want as there
are no broken users currently in mainline.

> I noticed no adverse effects from not having opp-microvolt in my testing of
> this patch, possibly because the data is not used to actually puppeteer any
> supplies. This goes back to the greater discussion, though. If we're going
> to use opp_parse_supplies() to do EM registration stuff then having the
> check for a valid representation of an actual VRM should probably happen
> elsewhere, where a valid VRM is actually a hard requirement of the code being
> run.

I figured out that you had to make a lot of unrelated changes for this
simple change, just because of the layout of the routine.

I have included your patch in my series now, cc'd you. Please give
that a try and see if it works or not. Thanks.

I have kept your Authorship and Signed-off, hope that is fine.

-- 
viresh