* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 12:17 ` Tomasz Figa
0 siblings, 0 replies; 40+ messages in thread
From: Tomasz Figa @ 2014-06-27 12:17 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 12:17 ` Tomasz Figa
@ 2014-06-27 15:10 ` Doug Anderson
-1 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 15:10 UTC (permalink / raw)
To: Vikas Sajjan, Vikas Sajjan
Cc: Tomasz Figa, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc, Kukjin Kim, sunil joshi, Linus Walleij,
Stephen Warren, Tomasz Figa
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
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 15:10 ` Doug Anderson
0 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 15:10 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 15:10 ` Doug Anderson
@ 2014-06-27 15:14 ` Tomasz Figa
-1 siblings, 0 replies; 40+ messages in thread
From: Tomasz Figa @ 2014-06-27 15:14 UTC (permalink / raw)
To: Doug Anderson, Vikas Sajjan, Vikas Sajjan
Cc: Tomasz Figa, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc, Kukjin Kim, sunil joshi, Linus Walleij,
Stephen Warren
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
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 15:14 ` Tomasz Figa
0 siblings, 0 replies; 40+ messages in thread
From: Tomasz Figa @ 2014-06-27 15:14 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 15:14 ` Tomasz Figa
@ 2014-06-27 15:22 ` Doug Anderson
-1 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 15:22 UTC (permalink / raw)
To: Tomasz Figa
Cc: Vikas Sajjan, Vikas Sajjan, Tomasz Figa,
linux-arm-kernel@lists.infradead.org, linux-samsung-soc,
Kukjin Kim, sunil joshi, Linus Walleij, Stephen Warren
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
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 15:22 ` Doug Anderson
0 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 15:22 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 15:22 ` Doug Anderson
@ 2014-06-27 15:49 ` Vikas Sajjan
-1 siblings, 0 replies; 40+ messages in thread
From: Vikas Sajjan @ 2014-06-27 15:49 UTC (permalink / raw)
To: Doug Anderson
Cc: Tomasz Figa, Tomasz Figa, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc, Kukjin Kim, sunil joshi, Linus Walleij,
Stephen Warren
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
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 15:49 ` Vikas Sajjan
0 siblings, 0 replies; 40+ messages in thread
From: Vikas Sajjan @ 2014-06-27 15:49 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 12:17 ` Tomasz Figa
@ 2014-06-27 16:10 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2014-06-27 16:10 UTC (permalink / raw)
To: Tomasz Figa, Doug Anderson, Tomasz Figa
Cc: Vikas Sajjan, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc, Kukjin Kim, sunil joshi, Vikas Sajjan,
Linus Walleij
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.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 16:10 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2014-06-27 16:10 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 16:10 ` Stephen Warren
@ 2014-06-27 16:45 ` Doug Anderson
-1 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 16:45 UTC (permalink / raw)
To: Stephen Warren
Cc: Tomasz Figa, Tomasz Figa, Vikas Sajjan,
linux-arm-kernel@lists.infradead.org, linux-samsung-soc,
Kukjin Kim, sunil joshi, Vikas Sajjan, Linus Walleij
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 16:45 ` Doug Anderson
0 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 16:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 16:45 ` Doug Anderson
@ 2014-06-27 18:20 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2014-06-27 18:20 UTC (permalink / raw)
To: Doug Anderson
Cc: Tomasz Figa, Tomasz Figa, Vikas Sajjan,
linux-arm-kernel@lists.infradead.org, linux-samsung-soc,
Kukjin Kim, sunil joshi, Vikas Sajjan, Linus Walleij
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 18:20 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2014-06-27 18:20 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 18:20 ` Stephen Warren
@ 2014-06-27 18:30 ` Doug Anderson
-1 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 18:30 UTC (permalink / raw)
To: Stephen Warren
Cc: Tomasz Figa, Tomasz Figa, Vikas Sajjan,
linux-arm-kernel@lists.infradead.org, linux-samsung-soc,
Kukjin Kim, sunil joshi, Vikas Sajjan, Linus Walleij
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 18:30 ` Doug Anderson
0 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 18:30 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 18:30 ` Doug Anderson
@ 2014-06-27 19:56 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2014-06-27 19:56 UTC (permalink / raw)
To: Doug Anderson
Cc: Tomasz Figa, Tomasz Figa, Vikas Sajjan,
linux-arm-kernel@lists.infradead.org, linux-samsung-soc,
Kukjin Kim, sunil joshi, Vikas Sajjan, Linus Walleij
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 19:56 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2014-06-27 19:56 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
2014-06-27 19:56 ` Stephen Warren
@ 2014-06-27 19:58 ` Doug Anderson
-1 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 19:58 UTC (permalink / raw)
To: Stephen Warren
Cc: Tomasz Figa, Tomasz Figa, Vikas Sajjan,
linux-arm-kernel@lists.infradead.org, linux-samsung-soc,
Kukjin Kim, sunil joshi, Vikas Sajjan, Linus Walleij
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] ARM: dts: Add mask-tpm-reset to the device tree
@ 2014-06-27 19:58 ` Doug Anderson
0 siblings, 0 replies; 40+ messages in thread
From: Doug Anderson @ 2014-06-27 19:58 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread