[PATCH] ARM: dts: Add mask-tpm-reset to the device tree

[Tomasz Figa]

Hi Doug,

On 26.06.2014 17:25, Doug Anderson wrote:
> Tomasz,
> 
> On Thu, Jun 26, 2014 at 2:52 AM, Tomasz Figa <t.figa at samsung.com> wrote:
>> Hi Vikas, Doug,
>>
>> On 26.06.2014 11:15, Vikas Sajjan wrote:
>>> From: Doug Anderson <dianders at chromium.org>
>>>
>>> The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from
>>> being reset across sleep/wake.  If we don't set it to anything then
>>> the TPM will be reset.  U-Boot will detect this as invalid
>>> and will reset the system on resume time. This GPIO can always be low
>>> and not hurt anything.  It will get pulled back high again during a
>>> normal warm reset when it will default back to an input.
>>>
>>> To properly preserve the TPM state across suspend/resume and to make
>>> the chrome U-Boot happy, properly set the GPIO to mask the
>>> reset to the TPM.
>>>
>>> Signed-off-by: Doug Anderson <dianders at chromium.org>
>>> Signed-off-by: Vikas Sajjan <vikas.sajjan at samsung.com>
>>> ---
>>>  arch/arm/boot/dts/exynos5420-peach-pit.dts |   20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>>> index 7649982..8fd990a 100644
>>> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
>>> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>>> @@ -87,6 +87,18 @@
>>>               pinctrl-0 = <&usb301_vbus_en>;
>>>               enable-active-high;
>>>       };
>>> +
>>> +     /* We need GPX0_6 to be low at sleep time; just keep it low always */
>>> +     mask_tpm_reset_regulator: mask-tpm-reset-regulator {
>>> +     compatible = "regulator-fixed";
>>> +     regulator-name = "mask-tpm-reset ";
>>> +             gpio = <&gpx0 6 0>;
>>> +             enable-active-low;
>>> +             regulator-boot-on;
>>> +             regulator-always-on;
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&mask_tpm_reset>;
>>> +     };
>>
>> I don't think this pin is supposed to be a real regulator. If I'm right,
>> you should just add a hog for it, if you don't have a proper driver to
>> handle it.
> 
> Yes, I agree that it shouldn't really be a regulator, but there's not
> a whole lot of choice.  The pin needs to actually be driven low, not
> just pulled low.  Without your proposed patch (pinctrl: samsung: Allow
> pin value to be initialized using pinfunc) I don't think it's possible
> to actually drive a pin low with a hog.  I could be wrong, though.
> 

Uhm, I was convinced that this patch was already in. So I think your use
case is definitely a good reason to get back to this patch and use the
facility it provides to solve your problem.

> 
> So I think our options are:
> 
> * Begrudgingly use the regulator this way.
> 
> * Re-propose your pinctrl patch and use hogs.

I'd prefer this one, as stated above. I think the reasons are obvious:
no made up devices and re-using infrastructure useful for other purposes
as well.

> 
> * Implement a tiny driver that will just hold a GPIO in a certain
> state and instantiate that driver here.
> 
> * Decide that really the firmware should have set this pin and that if
> someone wants suspend/resume to work then they'll just have to update
> their firmware.  Queue the debate about what firmware vs. kernel's job
> is and commence yelling about the fact that it's hard to get an
> official RW firmware update approved.  Note: I'm not quite sure why I
> didn't have the firmware init this GPIO to begin with (so that if the
> kernel didn't do anything it would just work).  Probably just me being
> stupid.
> 

Right, ideally the firmware should be making operating system's life
easier not harder, but we all know that even if there are examples of
good firmware, there will always be the opposite as well... So the
kernel should be able to work anyway.

> 
> Ah, one other thing that could justify this being its own special
> driver (though you might be able to use the regulator framework for it
> too?).  Technically you'll save a tiny amount of power in the system
> if you leave this GPIO high while the system is running and only drive
> it low when the system goes to suspend.  That's because there's a
> pretty strong external pullup on this pin.  The amount of power is
> probably not noticeable and the savings is only possible when the
> system is on (and in high power state) anyway, but...

This could be also handled by adding a sleep hog, i.e. "sleep" state for
the pinctrl device itself and adding proper suspend/resume callbacks to
the pinctrl-samsung driver (I'm not sure whether a call to
pinctrl_select_state() would work in syscore ops).

Best regards,
Tomasz