linux-kernel - Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <608800b4-2e56-d925-5db9-0ec40f98cbb5@canonical.com>
Date:   Fri, 18 Feb 2022 09:31:04 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Edwin Chiu 邱垂峰 <edwin.chiu@...plus.com>,
        Edwin Chiu <edwinchiu0505tw@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus
 sp7021

On 18/02/2022 09:10, Edwin Chiu 邱垂峰 wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
>> Sent: Monday, February 14, 2022 2:45 PM
>> To: Edwin Chiu 邱垂峰 <edwin.chiu@...plus.com>; Edwin Chiu <edwinchiu0505tw@...il.com>;
>> robh+dt@...nel.org; devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; rafael@...nel.org;
>> daniel.lezcano@...aro.org; linux-pm@...r.kernel.org
>> Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
>>
>> On 14/02/2022 03:55, Edwin Chiu 邱垂峰 wrote:
>>> Hi Krzysztof:
>>>
>>> Please see below answer.
>>>
>>>>> +static struct cpuidle_driver sp7021_idle_driver = {
>>>>> +	.name = "sp7021_idle",
>>>>> +	.owner = THIS_MODULE,
>>>>> +	/*
>>>>> +	 * State at index 0 is standby wfi and considered standard
>>>>> +	 * on all ARM platforms. If in some platforms simple wfi
>>>>> +	 * can't be used as "state 0", DT bindings must be implemented
>>>>> +	 * to work around this issue and allow installing a special
>>>>> +	 * handler for idle state index 0.
>>>>> +	 */
>>>>> +	.states[0] = {
>>>>> +		.enter                  = sp7021_enter_idle_state,
>>>>> +		.exit_latency           = 1,
>>>>> +		.target_residency       = 1,
>>>>> +		.power_usage		= UINT_MAX,
>>>>> +		.name                   = "WFI",
>>>>> +		.desc                   = "ARM WFI",
>>>>
>>>> I have impression that there is no point in having custom driver with WFI...
>>>>
>>>> Still the main question from Daniel and Sudeep stays: why do you need
>>>> this? You copied exactly cpuildle-arm driver, there is nothing
>>>> different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle
>> explicitly.
>>>>
>>>> Unfortunately I cannot understand the explanation here:
>>>> https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmbx0
>>>> 2.sunplus.com.tw/ Why exactly cpuidle-arm does not work in your case?
>>>>
>>> Edwin=> I mean cpuidle-arm driver can't directly use with no modified.
>>>        If someone want to use cpuidle-arm driver, below modification seems necessary.
>>>
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>        Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
>>>        Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
>>>        Static const struct cpuidle_ops sc_smp_ops __initconst = {
>>>             .suspend = sp7021_cpuidle_suspend_enter,
>>>             .init = sp7021_cpuidle_init,
>>>        };
>>>        CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sc_smp_ops); //declare
>> enable method
>>>
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> ~~~~~~~~~~
>>>
>>>        But change cpuilde-arm.c for sunplus driver seems not reasonable.
>>>        That is why I want to submit cpuidle-sunplus.c
>>>        Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.
>>
>> I don't think it is correct. You can use cpuidle-arm, because it is being always initialized with
>> device_initcall(). You either use appropriate compatible in DT or add your compatible to cpuidle-arm.
>>
>> Even if this did not work, then the solution is to use common parts, not to duplicate entire driver.
>> Duplicating is not acceptable.
>>
>> Best regards,
>> Krzysztof
> 
> 
> I do used compatible = "arm,idle-state" in DT and enable generic arm cpuidle driver in menuconfig.
> But there have mount driver fail message due to no cpuidle_ops functions.
> That is why I need added patch code to complete cpuidle driver.
> According your comment, I will try to use common parts and hook some custom code, later. 

I think I understood the motivation behind your driver. The cpuidle-arm
requires enable-method property which usually uses for ARMv7
CPU_METHOD_OF_DECLARE(). You don't have CPU_METHOD_OF_DECLARE().

Now the question: why can't you define CPU_METHOD_OF_DECLARE() just like
many other ARMv7 platforms do?

Best regards,
Krzysztof