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:   Fri, 4 Mar 2022 14:37:09 +0000
From:   Sudeep Holla <sudeep.holla@arm.com>
To:     Edwin Chiu =?utf-8?B?6YKx5Z6C5bOw?= <edwin.chiu@sunplus.com>
Cc:     Krzysztof Kozlowski <krzk@kernel.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Edwin Chiu <edwinchiu0505tw@gmail.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "rafael@kernel.org" <rafael@kernel.org>,
        "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus
 sp7021
Message-ID: <YiIkFZNGUihnoVPI@bogus>
References: <cover.1645427180.git.edwinchiu0505tw@gmail.com>
 <1628e048220f066204b8ac27f3cedf7f3cc02963.1645427180.git.edwinchiu0505tw@gmail.com>
 <394261d1-f1df-e80d-3591-10f2d649e731@kernel.org>
 <bcc7a0b58aad4f0989d7d86eaee2c746@sphcmbx02.sunplus.com.tw>
 <748eb0e1-684c-a772-bccd-64b80780192f@kernel.org>
 <fda1e55e576b4cdf9ab412529a3dfc7b@sphcmbx02.sunplus.com.tw>
 <fd39f73e-8317-38c4-6002-8defd784caec@kernel.org>
 <YiCSQCG4NkepeZKs@bogus>
 <ffdcb88f0ea240f68b5e5ec40f0f525c@sphcmbx02.sunplus.com.tw>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ffdcb88f0ea240f68b5e5ec40f0f525c@sphcmbx02.sunplus.com.tw>
Precedence: bulk

On Fri, Mar 04, 2022 at 11:24:32AM +0000, Edwin Chiu 邱垂峰 wrote:
> 
> Thanks your advice.
> Look like key point still only WFI function when cpuidle.

Indeed.

> As I explain before, only enable generic ARM cpuidle driver is not work.

Why do you think it is a must. Most arch(including arm) has default
arch_cpu_idle handler that will be called if no cpuidle driver is active.
It does execute the default WFI, so you don't need a driver to achieve
the same.

> It need enable-method code to assign cpuidle_ops functions.

Correct, but you may not need that driver to be active at all. That is the
main point of these discussions. Sorry if that was not mentioned explicitly
earlier.

> "psci" is one of enable-method, but there have problem in my side due to smc
> or secure code unsupported. > So I create cpuidle-sunplus.c code with
> "sunplus,sc-smp" to let cpuidle code complete for our source code.
> With this structure, I can add more custom low power code in the future.
>

So you want to add custom low power mode support in future, so add the driver
when that is ready. The platform must do WFI even now without the driver
you are adding. Have you checked that ?

> What does it mean for "please document the chosen "sunplus,sc-smp" as bot
> cpu idle and hotplug methods" ?

I meant if you are adding any custom SMP+Idle mentods you need to add the
compatible to [1] or [2] based on what is more appropriate.

> Does it mean "edit yaml file"? (Previously, I submit yaml file also, but Rob
> say I don't need submit when I use compatible="arm,idle-state")

Yes that covers the description of idle states but not the entry method.
There are 2 separate things. You need both "arm,idle-state" and
"sunplus,sc-smp" or "psci" whichever you decide to implement on your
platform. If there is no implementation yet, it is strongly suggested
to go for "psci" unless you have reasons not to. Please add that info
when you submit the custom support, I will check on that again when
you post. But for now you don't need anything.

-- 
Regards,
Sudeep

[1] Documentation/devicetree/bindings/arm/cpu-enable-method/
[2] Documentation/devicetree/bindings/arm/cpus.yaml