Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 15 Mar 2022 11:29:19 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Ionela Voinescu <ionela.voinescu@arm.com>
Cc:     Sudeep Holla <sudeep.holla@arm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 0/3] arch_topology: Correct CPU capacity scaling
Message-ID: <20220315032919.GA217475@leoy-ThinkPad-X240s>
References: <20220313055512.248571-1-leo.yan@linaro.org>
 <Yi+FMrG9NyBnMX0i@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yi+FMrG9NyBnMX0i@arm.com>
Precedence: bulk

Hi Ionela,

On Mon, Mar 14, 2022 at 06:10:58PM +0000, Ionela Voinescu wrote:

[...]

> > Patch 03 is to handle the case for absenting "capacity-dmips-mhz"
> > property in CPU nodes, the patch proceeds to do CPU capacity scaling based
> > on CPU maximum capacity.  Thus it can reflect the correct CPU capacity for
> > Arm platforms with "fast" and "slow" clusters (CPUs in two clusters have
> > the same raw capacity but with different maximum frequencies).
> > 
> 
> In my opinion it's difficult to handle absent "capacity-dmips-mhz"
> properties, as they can be a result of 3 scenarios: potential..
>  1. bug in DT
>  2. unwillingness to fill this information in DT
>  3. suggestion that we're dealing with CPUs with same u-arch
>     (same capacity-dmips-mhz)

For absent "capacity-dmips-mhz" properties, I think we could divide into
two sub classes:

For all CPU nodes are absent "capacity-dmips-mhz" properties, it's
likely all CPUs have the same micro architecture, thus developers are
not necessarily to explictly set the property.

For partial CPUs absent "capacity-dmips-mhz" properties, this is an
usage issue in DT and kernel should handle this as an error and report
it.

> I'm not sure it's up to us to interpret suggestions in the code so I
> believe treating missing information as error is the right choice, which
> is how we're handling this now.

Yes, current kernel means to treat missing info as error, whatever if
all CPUs or partial CPUs are absent "capacity-dmips-mhz" properties.

> For 3. (and patch 03), isn't it easier to populate capacity-dmips-mhz to
> the same value (say 1024) in DT? That is a clear message that we're
> dealing with CPUs with the same u-arch.

"capacity-dmips-mhz" is defined as a _optional_ property in the DT
document (see devicetree/bindings/arm/cpu-capacity.txt).

Current kernel rolls back every CPU raw capacity to 1024 if DT doesn't
bind "capacity-dmips-mhz" properties, given many SoCs with same CPU
u-arch this is right thing to do; here I think kernel should proceed to
scale CPU capacity with its maximum frequency.

When I worked on a platform with a fast and a slow clusters (two clusters
have different max frequencies and with the same CPU u-arch), it's a bit
puzzle when I saw all CPU's capacities are always 1024.  In this case,
since a platform have no CPU capacity modeling, and "capacity-dmips-mhz"
property is not needed to populate in DT, but at the end the kernel
should can reflect the scaled CPU capacity correctly.

Thanks a lot for review,

Leo