lkml.org 
[lkml]   [2019]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 18/18] arm64: dts: hikey: Convert to the hierarchical CPU topology layout
    On Thu, Jul 18, 2019 at 12:48:14PM +0200, Ulf Hansson wrote:
    > On Tue, 16 Jul 2019 at 16:47, Sudeep Holla <sudeep.holla@arm.com> wrote:
    > >
    > > On Mon, May 13, 2019 at 09:23:00PM +0200, Ulf Hansson wrote:
    > > > To enable the OS to manage last-man standing activities for a CPU, while an
    > > > idle state for a group of CPUs is selected, let's convert the Hikey
    > > > platform into using the hierarchical CPU topology layout.
    > > >
    > > > Cc: Wei Xu <xuwei5@hisilicon.com>
    > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
    > > > ---
    > > >
    > > > Changes:
    > > > - None.
    > > >
    > > > ---
    > > > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 87 ++++++++++++++++++++---
    > > > 1 file changed, 76 insertions(+), 11 deletions(-)
    > > >
    > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
    > > > index 108e2a4227f6..36ff460f428f 100644
    > > > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
    > > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
    > > > cpus {
    > >
    > > [...]
    > >
    > > > @@ -70,9 +128,8 @@
    > > > };
    > > >
    > > > CLUSTER_SLEEP: cluster-sleep {
    > > > - compatible = "arm,idle-state";
    > > > - local-timer-stop;
    > > > - arm,psci-suspend-param = <0x1010000>;
    > > > + compatible = "domain-idle-state";
    > > > + arm,psci-suspend-param = <0x1000000>;
    > > > entry-latency-us = <1000>;
    > > > exit-latency-us = <700>;
    > > > min-residency-us = <2700>;
    > >
    > > Again this must be original format and as per PSCI spec, your patch
    > > changes this cluster sleep state into cluster retention state which I
    > > think is not what you intended.
    >
    > If the hierarchical topology is used, the parameter for cluster states
    > are ORed with the deepest idle state for the CPU.
    >
    > CPU_SLEEP: 0x0010000
    > CLUSTER_SLEEP: 0x1000000
    >
    > After the ORed operation
    > CLUSTER_SLEEP: 0x1010000
    >
    > So, this indeed works as expected.
    >

    Yes, it works. But we are not XOR-ing so what's wrong in keeping the
    StateType as required and be compliant to specification. Why do we need
    to make the state param on it's own non-complaint.

    What's wrong in retaining CLUSTER_SLEEP as 0x1010000 so that it reflects
    the state level and type correctly on it's own ?

    > However, are you saying that ORing the state parameters like above has
    > other problems? I am reading your other replies...
    >

    Yes OR-ing may have other problems but the point I made here was more on
    PSCI spec compliance for each suspend parameter values independently.

    --
    Regards,
    Sudeep

    \
     
     \ /
      Last update: 2019-07-18 15:13    [W:4.679 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site