RE: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx>
> Sent: Friday, February 18, 2022 4:31 PM
> To: Edwin Chiu 邱垂峰 <edwin.chiu@xxxxxxxxxxx>; Edwin Chiu <edwinchiu0505tw@xxxxxxxxx>;
> robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
> daniel.lezcano@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx>
> >> Sent: Monday, February 14, 2022 2:45 PM
> >> To: Edwin Chiu 邱垂峰 <edwin.chiu@xxxxxxxxxxx>; Edwin Chiu
> >> <edwinchiu0505tw@xxxxxxxxx>;
> >> robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> >> robh+linux-kernel@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
> >> daniel.lezcano@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx
> >> 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@sphcmb
> >>>> x0 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


Yes, I will try it in next patch.
Thanks.


邱垂峰 EdwinChiu
智能運算專案
T: +886-3-5786005 ext.2590
edwin.chiu@xxxxxxxxxxx
300 新竹科學園區創新一路19號





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux