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

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

[Vikas Sajjan]

From: Doug Anderson <dianders@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@chromium.org>
Signed-off-by: Vikas Sajjan <vikas.sajjan@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>;
+	};
 };
 
 &dp {
@@ -210,6 +222,14 @@
 
 
 &pinctrl_0 {
+
+	mask_tpm_reset: mask-tpm-reset {
+		samsung,pins = "gpx0-6";
+		samsung,pin-function = <1>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
 	max98090_irq: max98090-irq {
 		samsung,pins = "gpx0-2";
 		samsung,pin-function = <0>;
-- 
1.7.9.5

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

[Tomasz Figa]

Hi Vikas, Doug,

On 26.06.2014 11:15, Vikas Sajjan wrote:
> From: Doug Anderson <dianders@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@chromium.org>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@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.

>  };
>  
>  &dp {
> @@ -210,6 +222,14 @@
>  
>  
>  &pinctrl_0 {
> +

nit: No need for this blank line.

Best regards,
Tomasz

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

[Doug Anderson]

Tomasz,

On Thu, Jun 26, 2014 at 2:52 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Vikas, Doug,
>
> On 26.06.2014 11:15, Vikas Sajjan wrote:
>> From: Doug Anderson <dianders@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@chromium.org>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@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.


So I think our options are:

* Begrudgingly use the regulator this way.

* Re-propose your pinctrl patch and use hogs.

* 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.


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...

-Doug

[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@samsung.com> wrote:
>> Hi Vikas, Doug,
>>
>> On 26.06.2014 11:15, Vikas Sajjan wrote:
>>> From: Doug Anderson <dianders@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@chromium.org>
>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@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

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

[Doug Anderson]

Tomasz and Vikas,

On Fri, Jun 27, 2014 at 5:17 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> 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@samsung.com> wrote:
>>> Hi Vikas, Doug,
>>>
>>> On 26.06.2014 11:15, Vikas Sajjan wrote:
>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@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.

Thanks, that seems very reasonable to me, too.

Are you going to pick up revitalizing this patch series or are you
hoping Vikas will?  IIRC I was in favor of your patch but it generated
a whole bunch of discussion and never actually landed.

-Doug

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

[Tomasz Figa]

On 27.06.2014 17:10, Doug Anderson wrote:
> Tomasz and Vikas,
> 
> On Fri, Jun 27, 2014 at 5:17 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> 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@samsung.com> wrote:
>>>> Hi Vikas, Doug,
>>>>
>>>> On 26.06.2014 11:15, Vikas Sajjan wrote:
>>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@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.
> 
> Thanks, that seems very reasonable to me, too.
> 
> Are you going to pick up revitalizing this patch series or are you
> hoping Vikas will?  IIRC I was in favor of your patch but it generated
> a whole bunch of discussion and never actually landed.

I will respin it along with few other patches for pinctrl-samsung, but
obviously not before Monday.

Best regards,
Tomasz

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

[Doug Anderson]

Tomasz,

On Fri, Jun 27, 2014 at 8:14 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 27.06.2014 17:10, Doug Anderson wrote:
>> Tomasz and Vikas,
>>
>> On Fri, Jun 27, 2014 at 5:17 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> 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@samsung.com> wrote:
>>>>> Hi Vikas, Doug,
>>>>>
>>>>> On 26.06.2014 11:15, Vikas Sajjan wrote:
>>>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@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.
>>
>> Thanks, that seems very reasonable to me, too.
>>
>> Are you going to pick up revitalizing this patch series or are you
>> hoping Vikas will?  IIRC I was in favor of your patch but it generated
>> a whole bunch of discussion and never actually landed.
>
> I will respin it along with few other patches for pinctrl-samsung, but
> obviously not before Monday.

Sounds like a plan.  Vikas: I'll assume you're going to redo this
patch atop Tomasz's once he posts up his.

-Doug

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

[Vikas Sajjan]

Doug,

On Fri, Jun 27, 2014 at 8:52 PM, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> On Fri, Jun 27, 2014 at 8:14 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 27.06.2014 17:10, Doug Anderson wrote:
>>> Tomasz and Vikas,
>>>
>>> On Fri, Jun 27, 2014 at 5:17 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>> 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@samsung.com> wrote:
>>>>>> Hi Vikas, Doug,
>>>>>>
>>>>>> On 26.06.2014 11:15, Vikas Sajjan wrote:
>>>>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@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.
>>>
>>> Thanks, that seems very reasonable to me, too.
>>>
>>> Are you going to pick up revitalizing this patch series or are you
>>> hoping Vikas will?  IIRC I was in favor of your patch but it generated
>>> a whole bunch of discussion and never actually landed.
>>
>> I will respin it along with few other patches for pinctrl-samsung, but
>> obviously not before Monday.
>
> Sounds like a plan.  Vikas: I'll assume you're going to redo this
> patch atop Tomasz's once he posts up his.

Sure, will respin atop Tomasz's patch series.


>
> -Doug

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

[Stephen Warren]

On 06/27/2014 06:17 AM, Tomasz Figa wrote:
> 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@samsung.com> wrote:
>>> Hi Vikas, Doug,
>>>
>>> On 26.06.2014 11:15, Vikas Sajjan wrote:
>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@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.

Surely there's a driver (or could be a driver) for the TPM chip, and
that driver should have a reset-mask-gpios property, so the driver can
call gpio_get() and gpio_set_output() on the GPIO?

Faking this out via a not-really-a-regulator or pinctrl hogs seems like
an abuse of those features to me.

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

[Doug Anderson]

Stephen,

On Fri, Jun 27, 2014 at 9:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> Surely there's a driver (or could be a driver) for the TPM chip, and
> that driver should have a reset-mask-gpios property, so the driver can
> call gpio_get() and gpio_set_output() on the GPIO?
>
> Faking this out via a not-really-a-regulator or pinctrl hogs seems like
> an abuse of those features to me.

This totally doesn't belong in the TPM chip driver.  This is an
unabashed HACK at the board level.  Without a hog we need a board
driver for this.

To be more specific:

* The TPM needs to be reset when the full system gets reset.  This
unlocks the TPM and allows certain types of updates by the firmware.
The firmware then locks the TPM before jumping to the kernel.

* The TPM is hooked up to the "reset out" line of the CPU so that when
the system does a warm reset it will reset the TPM.

* Unfortunately the CPU asserts the "reset out" line when it's
sleeping (because, of course, sleep is a reset).  This would allow the
kernel to unlock the TPM which it's not supposed to be able to do.

* To solve the problem, it's up to the kernel to "mask" out the reset
line before going to sleep.  Then it's up to the read only firmware to
validate that the kernel properly masked the reset before resuming
from sleep.  If the firmware finds that the kernel cheated and didn't
mask the reset then it will not resume to the kernel and will instead
restart the system.


The above is not beautiful in the least sense.  Getting suspend/resume
working happened very late in the exynos5250-snow project and the
above workaround was the best that we could come up with without
slipping schedules.  It also had the side effect of being less
expensive than other solutions.  Given that the solution was "proven
out" for exynos5250-snow, it was kept in place for similar products.

-Doug

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

[Stephen Warren]

On 06/27/2014 10:45 AM, Doug Anderson wrote:
> Stephen,
> 
> On Fri, Jun 27, 2014 at 9:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> Surely there's a driver (or could be a driver) for the TPM chip, and
>> that driver should have a reset-mask-gpios property, so the driver can
>> call gpio_get() and gpio_set_output() on the GPIO?
>>
>> Faking this out via a not-really-a-regulator or pinctrl hogs seems like
>> an abuse of those features to me.
> 
> This totally doesn't belong in the TPM chip driver.  This is an
> unabashed HACK at the board level.  Without a hog we need a board
> driver for this.
> 
> To be more specific:
> 
> * The TPM needs to be reset when the full system gets reset.  This
> unlocks the TPM and allows certain types of updates by the firmware.
> The firmware then locks the TPM before jumping to the kernel.
> 
> * The TPM is hooked up to the "reset out" line of the CPU so that when
> the system does a warm reset it will reset the TPM.
> 
> * Unfortunately the CPU asserts the "reset out" line when it's
> sleeping (because, of course, sleep is a reset).  This would allow the
> kernel to unlock the TPM which it's not supposed to be able to do.

To me, this does sound precisely like something that should be in the
TPM driver. I guess an overall board driver would be reasonable too,
since the issue probably doesn't to all boards with the TPM.

This kind of thing is *exactly* the kind of thing that's been discussed
in the past re: doing it in pinmux hogs, or GPIO initialization tables
that aren't specific to a driver, and has been rejected. I guess if
people want to change the decisions that's fine, but doing so will open
up the door to all the previously rejected similar use-cases. I'm not
sure how much I care either way, but we really should be consistent
instead of flip-flopping on this kind of issue.

As an aside, why do we even care about this? The kernel clearly can
unlock the TPM simply by failing to set the GPIO up correctly, so at
best this is security through obscurity. If someone actively wanted to
do something bad to the TPM, they'd simply comment out this piece of
code in the kernel. At least that's true with usptream kernels which
aren't validated at all during boot. For a downstream signed kernel,
perhaps this patch makes sense (although I haven't thought about all the
security angles), but then it'd make sense to keep this patch downstream.

> * To solve the problem, it's up to the kernel to "mask" out the reset
> line before going to sleep.  Then it's up to the read only firmware to
> validate that the kernel properly masked the reset before resuming
> from sleep.  If the firmware finds that the kernel cheated and didn't
> mask the reset then it will not resume to the kernel and will instead
> restart the system.
> 
> 
> The above is not beautiful in the least sense.  Getting suspend/resume
> working happened very late in the exynos5250-snow project and the
> above workaround was the best that we could come up with without
> slipping schedules.  It also had the side effect of being less
> expensive than other solutions.  Given that the solution was "proven
> out" for exynos5250-snow, it was kept in place for similar products.
> 
> -Doug

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

[Doug Anderson]

Stephen,

On Fri, Jun 27, 2014 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/27/2014 10:45 AM, Doug Anderson wrote:
>> Stephen,
>>
>> On Fri, Jun 27, 2014 at 9:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> Surely there's a driver (or could be a driver) for the TPM chip, and
>>> that driver should have a reset-mask-gpios property, so the driver can
>>> call gpio_get() and gpio_set_output() on the GPIO?
>>>
>>> Faking this out via a not-really-a-regulator or pinctrl hogs seems like
>>> an abuse of those features to me.
>>
>> This totally doesn't belong in the TPM chip driver.  This is an
>> unabashed HACK at the board level.  Without a hog we need a board
>> driver for this.
>>
>> To be more specific:
>>
>> * The TPM needs to be reset when the full system gets reset.  This
>> unlocks the TPM and allows certain types of updates by the firmware.
>> The firmware then locks the TPM before jumping to the kernel.
>>
>> * The TPM is hooked up to the "reset out" line of the CPU so that when
>> the system does a warm reset it will reset the TPM.
>>
>> * Unfortunately the CPU asserts the "reset out" line when it's
>> sleeping (because, of course, sleep is a reset).  This would allow the
>> kernel to unlock the TPM which it's not supposed to be able to do.
>
> To me, this does sound precisely like something that should be in the
> TPM driver. I guess an overall board driver would be reasonable too,
> since the issue probably doesn't to all boards with the TPM.

Right.  In fact I'd be willing to bet that there are zero other boards
(other than the ChromeOS boards with similar heritage) with this TPM
that do this.  It does seem unfair to burden an Infineon driver with a
board hack when it's something they didn't have anything to do with.

...but if that's the way it has to be then that's the way it has to be...

If consensus is a board driver that's fine too.  It's really all about
coming up with some solution that will make folks happy.


> This kind of thing is *exactly* the kind of thing that's been discussed
> in the past re: doing it in pinmux hogs, or GPIO initialization tables
> that aren't specific to a driver, and has been rejected. I guess if
> people want to change the decisions that's fine, but doing so will open
> up the door to all the previously rejected similar use-cases. I'm not
> sure how much I care either way, but we really should be consistent
> instead of flip-flopping on this kind of issue.
>
> As an aside, why do we even care about this? The kernel clearly can
> unlock the TPM simply by failing to set the GPIO up correctly, so at
> best this is security through obscurity. If someone actively wanted to
> do something bad to the TPM, they'd simply comment out this piece of
> code in the kernel. At least that's true with usptream kernels which
> aren't validated at all during boot. For a downstream signed kernel,
> perhaps this patch makes sense (although I haven't thought about all the
> security angles), but then it'd make sense to keep this patch downstream.

Check out the bullet point about the firmware checking for kernel
cheats.  At resume time the chip actually re-loads read only firmware
out of SPI flash (no choice about this).  This read only firmware can
check the state of the mask pin (which is preserved across sleep wake)
to see if the kernel cheated.

-Doug

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

[Stephen Warren]

On 06/27/2014 12:30 PM, Doug Anderson wrote:
> Stephen,
> 
> On Fri, Jun 27, 2014 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 06/27/2014 10:45 AM, Doug Anderson wrote:
>>> Stephen,
>>>
>>> On Fri, Jun 27, 2014 at 9:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> Surely there's a driver (or could be a driver) for the TPM chip, and
>>>> that driver should have a reset-mask-gpios property, so the driver can
>>>> call gpio_get() and gpio_set_output() on the GPIO?
>>>>
>>>> Faking this out via a not-really-a-regulator or pinctrl hogs seems like
>>>> an abuse of those features to me.
...
>> As an aside, why do we even care about this? The kernel clearly can
>> unlock the TPM simply by failing to set the GPIO up correctly, so at
>> best this is security through obscurity. If someone actively wanted to
>> do something bad to the TPM, they'd simply comment out this piece of
>> code in the kernel. At least that's true with usptream kernels which
>> aren't validated at all during boot. For a downstream signed kernel,
>> perhaps this patch makes sense (although I haven't thought about all the
>> security angles), but then it'd make sense to keep this patch downstream.
> 
> Check out the bullet point about the firmware checking for kernel
> cheats.  At resume time the chip actually re-loads read only firmware
> out of SPI flash (no choice about this).  This read only firmware can
> check the state of the mask pin (which is preserved across sleep wake)
> to see if the kernel cheated.

Ah, that covers the security issues then.

I'd argue that the RO firmware should be setting up GPIOs like this
myself (and the pinmux, from a nice big table), and the kernel simply
not touch it all since it has no direct use for the pin. But I suppose
if the firmware is already baked and read only, that's not possible.

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

[Doug Anderson]

Stephen,

On Fri, Jun 27, 2014 at 12:56 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/27/2014 12:30 PM, Doug Anderson wrote:
>> Stephen,
>>
>> On Fri, Jun 27, 2014 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 06/27/2014 10:45 AM, Doug Anderson wrote:
>>>> Stephen,
>>>>
>>>> On Fri, Jun 27, 2014 at 9:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> Surely there's a driver (or could be a driver) for the TPM chip, and
>>>>> that driver should have a reset-mask-gpios property, so the driver can
>>>>> call gpio_get() and gpio_set_output() on the GPIO?
>>>>>
>>>>> Faking this out via a not-really-a-regulator or pinctrl hogs seems like
>>>>> an abuse of those features to me.
> ...
>>> As an aside, why do we even care about this? The kernel clearly can
>>> unlock the TPM simply by failing to set the GPIO up correctly, so at
>>> best this is security through obscurity. If someone actively wanted to
>>> do something bad to the TPM, they'd simply comment out this piece of
>>> code in the kernel. At least that's true with usptream kernels which
>>> aren't validated at all during boot. For a downstream signed kernel,
>>> perhaps this patch makes sense (although I haven't thought about all the
>>> security angles), but then it'd make sense to keep this patch downstream.
>>
>> Check out the bullet point about the firmware checking for kernel
>> cheats.  At resume time the chip actually re-loads read only firmware
>> out of SPI flash (no choice about this).  This read only firmware can
>> check the state of the mask pin (which is preserved across sleep wake)
>> to see if the kernel cheated.
>
> Ah, that covers the security issues then.
>
> I'd argue that the RO firmware should be setting up GPIOs like this
> myself (and the pinmux, from a nice big table), and the kernel simply
> not touch it all since it has no direct use for the pin. But I suppose
> if the firmware is already baked and read only, that's not possible.

Yes.  Earlier in this thread I said I had no idea why I didn't have
firmware set this up.  I couldn't come up with a good reason... :(

-Doug

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

[Linus Walleij]

On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote:

> From: Doug Anderson <dianders@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@chromium.org>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
(...)
> +       /* 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";

No matter how the discussion ends up, regulator-fixed is wrong.

Either folding it into the TPM driver or using a separate reset driver
is fine with me.

So what about the generic delayed reset GPIO thing?
http://marc.info/?l=linux-kernel&m=140309916607115&w=2

Yours,
Linus Walleij

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

[Doug Anderson]

Hi,

On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote:
>
>> From: Doug Anderson <dianders@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@chromium.org>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
> (...)
>> +       /* 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";
>
> No matter how the discussion ends up, regulator-fixed is wrong.

OK, fair enough.


> Either folding it into the TPM driver or using a separate reset driver
> is fine with me.

OK, Vikas: do you want to code up the driver?


> So what about the generic delayed reset GPIO thing?
> http://marc.info/?l=linux-kernel&m=140309916607115&w=2

That's a neat concept and could be useful in other cases, but I think
it's just as much of a hack as using a regulator.  This is not a reset
signal for the TPM.  This is a signal that will mask the CPU's reset
signal (using a special bit of board-specific logic).


Personally I think Tomasz's idea of using hogs (after his patches
allowing a default output level) is the cleanest, but I think Stephen
didn't like that.

-Doug

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

[Tomasz Figa]

On 08.07.2014 17:27, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote:
>>
>>> From: Doug Anderson <dianders@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@chromium.org>
>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
>> (...)
>>> +       /* 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";
>>
>> No matter how the discussion ends up, regulator-fixed is wrong.
> 
> OK, fair enough.
> 
> 
>> Either folding it into the TPM driver or using a separate reset driver
>> is fine with me.
> 
> OK, Vikas: do you want to code up the driver?
> 
> 
>> So what about the generic delayed reset GPIO thing?
>> http://marc.info/?l=linux-kernel&m=140309916607115&w=2
> 
> That's a neat concept and could be useful in other cases, but I think
> it's just as much of a hack as using a regulator.  This is not a reset
> signal for the TPM.  This is a signal that will mask the CPU's reset
> signal (using a special bit of board-specific logic).
> 
> 
> Personally I think Tomasz's idea of using hogs (after his patches
> allowing a default output level) is the cleanest, but I think Stephen
> didn't like that.

I don't see any benefits of using complex interfaces over simply
initializing the pin to the right value once at boot-up or at
suspend/resume. As far as I understand Doug's explanation of the
problem, nothing else is expected from the OS with respect to this pin.
Moreover it doesn't affect operation of any drivers.

So I'm still for the simplest and effective solution, i.e. hogs.

Best regards,
Tomasz

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

[Doug Anderson]

Vikas,

On Tue, Jul 8, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> On 08.07.2014 17:27, Doug Anderson wrote:
>> Hi,
>>
>> On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote:
>>>
>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
>>> (...)
>>>> +       /* 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";
>>>
>>> No matter how the discussion ends up, regulator-fixed is wrong.
>>
>> OK, fair enough.
>>
>>
>>> Either folding it into the TPM driver or using a separate reset driver
>>> is fine with me.
>>
>> OK, Vikas: do you want to code up the driver?
>>
>>
>>> So what about the generic delayed reset GPIO thing?
>>> http://marc.info/?l=linux-kernel&m=140309916607115&w=2
>>
>> That's a neat concept and could be useful in other cases, but I think
>> it's just as much of a hack as using a regulator.  This is not a reset
>> signal for the TPM.  This is a signal that will mask the CPU's reset
>> signal (using a special bit of board-specific logic).
>>
>>
>> Personally I think Tomasz's idea of using hogs (after his patches
>> allowing a default output level) is the cleanest, but I think Stephen
>> didn't like that.
>
> I don't see any benefits of using complex interfaces over simply
> initializing the pin to the right value once at boot-up or at
> suspend/resume. As far as I understand Doug's explanation of the
> problem, nothing else is expected from the OS with respect to this pin.
> Moreover it doesn't affect operation of any drivers.
>
> So I'm still for the simplest and effective solution, i.e. hogs.

It looks as if Tomasz's patch has landed, so perhaps you could try to
code it up his way.  It should be very simple.  Please make sure to CC
Stephen Warren on the patch so that he is included in the discussion
(and of course include Tomasz, Linus W, etc).

-Doug

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

[Vikas Sajjan]

Doug,

On Wed, Jul 9, 2014 at 8:52 PM, Doug Anderson <dianders@chromium.org> wrote:
> Vikas,
>
> On Tue, Jul 8, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> On 08.07.2014 17:27, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote:
>>>>
>>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
>>>> (...)
>>>>> +       /* 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";
>>>>
>>>> No matter how the discussion ends up, regulator-fixed is wrong.
>>>
>>> OK, fair enough.
>>>
>>>
>>>> Either folding it into the TPM driver or using a separate reset driver
>>>> is fine with me.
>>>
>>> OK, Vikas: do you want to code up the driver?
>>>
>>>
>>>> So what about the generic delayed reset GPIO thing?
>>>> http://marc.info/?l=linux-kernel&m=140309916607115&w=2
>>>
>>> That's a neat concept and could be useful in other cases, but I think
>>> it's just as much of a hack as using a regulator.  This is not a reset
>>> signal for the TPM.  This is a signal that will mask the CPU's reset
>>> signal (using a special bit of board-specific logic).
>>>
>>>
>>> Personally I think Tomasz's idea of using hogs (after his patches
>>> allowing a default output level) is the cleanest, but I think Stephen
>>> didn't like that.
>>
>> I don't see any benefits of using complex interfaces over simply
>> initializing the pin to the right value once at boot-up or at
>> suspend/resume. As far as I understand Doug's explanation of the
>> problem, nothing else is expected from the OS with respect to this pin.
>> Moreover it doesn't affect operation of any drivers.
>>
>> So I'm still for the simplest and effective solution, i.e. hogs.
>
> It looks as if Tomasz's patch has landed, so perhaps you could try to
> code it up his way.  It should be very simple.  Please make sure to CC
> Stephen Warren on the patch so that he is included in the discussion
> (and of course include Tomasz, Linus W, etc).

Can you point me to Tomasz's patchset on which I should rebase this patch on.


>
> -Doug

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

[Doug Anderson]

Vikas,

On Wed, Jul 9, 2014 at 9:35 PM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote:
> Doug,
>
> On Wed, Jul 9, 2014 at 8:52 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Vikas,
>>
>> On Tue, Jul 8, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>>> On 08.07.2014 17:27, Doug Anderson wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote:
>>>>>
>>>>>> From: Doug Anderson <dianders@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@chromium.org>
>>>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
>>>>> (...)
>>>>>> +       /* 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";
>>>>>
>>>>> No matter how the discussion ends up, regulator-fixed is wrong.
>>>>
>>>> OK, fair enough.
>>>>
>>>>
>>>>> Either folding it into the TPM driver or using a separate reset driver
>>>>> is fine with me.
>>>>
>>>> OK, Vikas: do you want to code up the driver?
>>>>
>>>>
>>>>> So what about the generic delayed reset GPIO thing?
>>>>> http://marc.info/?l=linux-kernel&m=140309916607115&w=2
>>>>
>>>> That's a neat concept and could be useful in other cases, but I think
>>>> it's just as much of a hack as using a regulator.  This is not a reset
>>>> signal for the TPM.  This is a signal that will mask the CPU's reset
>>>> signal (using a special bit of board-specific logic).
>>>>
>>>>
>>>> Personally I think Tomasz's idea of using hogs (after his patches
>>>> allowing a default output level) is the cleanest, but I think Stephen
>>>> didn't like that.
>>>
>>> I don't see any benefits of using complex interfaces over simply
>>> initializing the pin to the right value once at boot-up or at
>>> suspend/resume. As far as I understand Doug's explanation of the
>>> problem, nothing else is expected from the OS with respect to this pin.
>>> Moreover it doesn't affect operation of any drivers.
>>>
>>> So I'm still for the simplest and effective solution, i.e. hogs.
>>
>> It looks as if Tomasz's patch has landed, so perhaps you could try to
>> code it up his way.  It should be very simple.  Please make sure to CC
>> Stephen Warren on the patch so that he is included in the discussion
>> (and of course include Tomasz, Linus W, etc).
>
> Can you point me to Tomasz's patchset on which I should rebase this patch on.

You can base it atop (0635c88 pinctrl: samsung: Allow pin value to be
initialized using pinfunc), which appears to be in linux-next.  That
gives you "samsung,pin-val".

You can use that together with a pinctrl "hog" to specify the state of
this pin for the board.

-Doug