Re: [PATCH] cpuidle: kirkwood: Move out of mach directory, add DT.

[Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>]

On Friday 28 December 2012 09:44 PM, Rob Herring wrote:
> On 12/28/2012 09:49 AM, Andrew Lunn wrote:
>> On Fri, Dec 28, 2012 at 08:55:31AM -0600, Rob Herring wrote:
>>> On 12/28/2012 08:35 AM, Andrew Lunn wrote:
>>>> On Fri, Dec 28, 2012 at 08:18:42AM -0600, Rob Herring wrote:
>>>>> On 12/28/2012 06:47 AM, Andrew Lunn wrote:
>>>>>> Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and
>>>>>> into drivers/cpuidle. Convert the driver into a platform driver and
>>>>>> add a device tree binding. Add a DT node to instantiate the driver for
>>>>>> boards converted to DT, and a platform data for old style boards.
>>>>>
>>>>> Is this an old comment? I don't see any platform data.
>>>>
>>>> There is no platform data, since all the driver needs is an address of
>>>> the DDR control register. The code to create a platform device entry
>>>> is in common.c hunk.
>>>
>>> So you should say "a platform device for old style boards".
>>
>> Hi Rob
>>
>> O.K. I will change it.
>>
>>>>>> +		cpuidle@1418 {
>>>>>> +			compatible = "marvell,kirkwood-cpuidle";
>>>>>> +			reg = <0x1418 0x4>;
>>>>>> +		};
>>>>>> +
>>>>>
>>>>> This is describing what linux wants, not the hardware. This is a common
>>>>> problem with cpuidle drivers in that they use shared registers. I don't
>>>>> have a good solution, but this doesn't belong in DTS.
>>>>
>>>> Do you have a bad solution?
>>>
>>> Ha! :) I should say I don't have a clear, obvious solution.
>>>
>>> Don't do a platform driver and just check the machine compatible
>>> property which is what I did for highbank.
>>
>> Yes, i've seen your cpuidle-calxeda.c. I can follow that model, but i
>> still somehow need the address of the SDRAM controller "Operation
>> Register".
>>
>>> Have the machine code create
>>> the platform device. Not *all* platform devices have to be created based
>>> on the DTB. Create an MFD driver for the whole block of registers.
>>
>> The block of registers is for controlling the SDRAM. Its not really a
>> MFD. The cpuidle code is putting the SDRAM controller into self
>> refresh mode and then doing a WFI.
>
> It is multi-function in the sense that multiple subsystems needing to
> access shared registers. If you have ECC, then you would need to give
> EDAC subsystem access to ECC related registers.
>
>>>
>>>> I could just hard code the address, since its the same for all
>>>> kirkwood SoCs. Also, the register is not being used by any other
>>>> code on kirkwood, so its not shared.
>>>
>>> Then describe it based on the reference manual, but you need to do so
>>> assuming you are using all the other registers. I assume there are other
>>> registers at say 0x1414 or 0x141c. You have to be careful if you create
>>> separate nodes for each register or sub-group of registers. It needs to
>>> work no matter what the OS expectation is.
>>
>> I don't understand what you are try to explain here. It makes little
>> sense for the cpuidle driver to take all the SDRAM control registers.
>
> Exactly. The same is true of the dts. It makes little sense to describe
> only 1 register of a h/w block. Perhaps the memory controller subsystem
> (drivers/memory) can be expanded to include self-refresh functions.
> Entering self-refresh can be tricky, so it might not really be possible
> to define a common api here.
>
Putting DDR/SDRAM into self refresh means, you no longer have RAM
available to execute and any code CPU needs to execute after that has
to be executed either from lock down cache with no cache evictions or
from some internal on chip memory with caches disabled to avoid accesses
to DDR. Not sure how below code works if the first writel puts DDR into
self refresh. Is cpu_do_idle() code copied to some internal memory ?

+	writel(0x7, ddr_operation_base);
+	cpu_do_idle();

May be I am missing something but i have worked on such a code for OMAP3
kind of devices and seen major issues with DDR self refresh and CPU
entering into idle state.

Regards
Santosh