ARM: dts: Add mask-tpm-reset to the device tree
Subsystems (2)
PATCH 1/1 ARM: dts: Add mask-tpm-reset to the device tree
From: Doug Anderson <[email protected]>
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.
--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
arch/arm/boot/dts/exynos5420-peach-pit.dts +20 -0
@@ -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>;
Signature · 1 line
1.7.9.5
PATCH 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Hi Vikas, Doug,
Quoted 29 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +2
-0@@ -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 */
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 10 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +10
-0+ 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.
arch/arm/boot/dts/exynos5420-peach-pit.dts line 225
Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +1
-0}; &dp {@@ -210,6 +222,14 @@ &pinctrl_0 { +
nit: No need for this blank line.
Best regards,
Tomasz
PATCH 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Tomasz,
Quoted 38 lines · Tomasz Figa
Hi Vikas, Doug,
Quoted 35 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +8
-0@@ -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;
Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 4 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +4
-0+ 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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Hi Doug,
Quoted 50 lines · Doug Anderson
Tomasz,
Quoted 38 lines · Tomasz Figa
Hi Vikas, Doug,
Quoted 35 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +8
-0@@ -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;Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 4 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +4
-0+ 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.
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 4 lines
* Implement a tiny driver that will just hold a GPIO in a certain state and instantiate that driver here.
Quoted 9 lines
* 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.
Quoted 9 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Tomasz and Vikas,
Quoted 60 lines · Tomasz Figa
Hi Doug,
Quoted 50 lines · Doug Anderson
Tomasz,
Quoted 38 lines · Tomasz Figa
Hi Vikas, Doug,
Quoted 35 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +8
-0@@ -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;Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 4 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +4
-0+ 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.
Quoted 6 lines
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.
Quoted 3 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Quoted 67 lines · Doug Anderson
Tomasz and Vikas,
Quoted 60 lines · Tomasz Figa
Hi Doug,
Quoted 50 lines · Doug Anderson
Tomasz,
Quoted 38 lines · Tomasz Figa
Hi Vikas, Doug,
Quoted 35 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +8
-0@@ -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;Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 4 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +4
-0+ 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.
Quoted 6 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Tomasz,
Quoted 68 lines · Tomasz Figa
Quoted 67 lines · Doug Anderson
Tomasz and Vikas,
Quoted 63 lines · Tomasz Figa
Hi Doug,
Quoted 50 lines · Doug Anderson
Tomasz,
Quoted 38 lines · Tomasz Figa
Hi Vikas, Doug,
Quoted 35 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +8
-0@@ -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;Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 4 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +4
-0+ 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.
Quoted 6 lines
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.
Quoted 8 lines
Quoted 5 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Doug,
Quoted 77 lines · Doug Anderson
Tomasz,
Quoted 74 lines · Tomasz Figa
Quoted 72 lines · Doug Anderson
Tomasz and Vikas,
Quoted 60 lines · Tomasz Figa
Hi Doug,
Quoted 50 lines · Doug Anderson
Tomasz,
Quoted 38 lines · Tomasz Figa
Hi Vikas, Doug,
Quoted 35 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +8
-0@@ -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;Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 4 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +4
-0+ 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.
Quoted 6 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 2 lines
-Doug
PATCH 1/1 ARM: dts: Add mask-tpm-reset to the device tree
In reply to Tomasz FigaQuoted 58 lines · Tomasz Figa
Hi Doug,
Quoted 55 lines · Doug Anderson
Tomasz,
Quoted 38 lines · Tomasz Figa
Hi Vikas, Doug,
Quoted 35 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>--- arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/exynos5420-peach-pit.dts +8
-0@@ -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;Quoted 8 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts line 101
Quoted 4 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +4
-0+ 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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Stephen,
Quoted 6 lines · Stephen Warren
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()andgpio_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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Quoted 20 lines · Doug Anderson
Stephen,
Quoted 6 lines · Stephen Warren
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()andgpio_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.
Quoted 6 lines
* 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.
Quoted 16 lines
* 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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Stephen,
Quoted 24 lines · Stephen Warren
Quoted 23 lines · Doug Anderson
Stephen,
Quoted 6 lines · Stephen Warren
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()andgpio_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.
Quoted 7 lines
Quoted 3 lines
* 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.
Quoted 8 lines
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.
Quoted 8 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Quoted 13 lines · Doug Anderson
Stephen,
Quoted 10 lines · Stephen Warren
Quoted 9 lines · Doug Anderson
Stephen,
Quoted 6 lines · Stephen Warren
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()andgpio_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.
...
Quoted 9 lines
Quoted 8 lines
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.
Quoted 5 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Stephen,
Quoted 30 lines · Stephen Warren
Quoted 13 lines · Doug Anderson
Stephen,
Quoted 10 lines · Stephen Warren
Quoted 9 lines · Doug Anderson
Stephen,
Quoted 6 lines · Stephen Warren
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()andgpio_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.
...
Quoted 9 lines
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
In reply to Vikas SajjanQuoted 15 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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 <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>
(...)
arch/arm/boot/dts/exynos5420-peach-pit.dts line 93
Quoted 3 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +3
-0+ /* 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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Hi,
Quoted 15 lines · Linus Walleij
Quoted 13 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>(...)
arch/arm/boot/dts/exynos5420-peach-pit.dts line 93
Quoted 3 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +3
-0+ /* 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.
Quoted 2 lines
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?
Quoted 2 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Quoted 45 lines · Doug Anderson
Hi,
Quoted 15 lines · Linus Walleij
Quoted 13 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>(...)
arch/arm/boot/dts/exynos5420-peach-pit.dts line 93
Quoted 3 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +3
-0+ /* 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.
Quoted 2 lines
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?
Quoted 2 lines
So what about the generic delayed reset GPIO thing?
http://marc.info/?l=linux-kernel&m=140309916607115&w=2That'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).
Quoted 3 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Vikas,
Quoted 50 lines · Tomasz Figa
Quoted 45 lines · Doug Anderson
Hi,
Quoted 15 lines · Linus Walleij
Quoted 13 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>(...)
arch/arm/boot/dts/exynos5420-peach-pit.dts line 93
Quoted 3 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +3
-0+ /* 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.
Quoted 2 lines
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?
Quoted 2 lines
So what about the generic delayed reset GPIO thing?
http://marc.info/?l=linux-kernel&m=140309916607115&w=2That'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).
Quoted 3 lines
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.
Quoted 7 lines
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 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Doug,
Quoted 59 lines · Doug Anderson
Vikas,
Quoted 56 lines · Tomasz Figa
Quoted 45 lines · Doug Anderson
Hi,
Quoted 15 lines · Linus Walleij
Quoted 13 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>(...)
arch/arm/boot/dts/exynos5420-peach-pit.dts line 93
Quoted 3 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +3
-0+ /* 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.
Quoted 2 lines
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?
Quoted 2 lines
So what about the generic delayed reset GPIO thing?
http://marc.info/?l=linux-kernel&m=140309916607115&w=2That'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).
Quoted 3 lines
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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 2 lines
-Doug
PATCH 1/1 ARM: dts: Add mask-tpm-reset to the device tree
Vikas,
Quoted 62 lines · Vikas Sajjan
Doug,
Quoted 59 lines · Doug Anderson
Vikas,
Quoted 56 lines · Tomasz Figa
Quoted 45 lines · Doug Anderson
Hi,
Quoted 15 lines · Linus Walleij
Quoted 13 lines · Vikas Sajjan
From: Doug Anderson <[email protected]>
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.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Vikas Sajjan <[email protected]>(...)
arch/arm/boot/dts/exynos5420-peach-pit.dts line 93
Quoted 3 lines
arch/arm/boot/dts/exynos5420-peach-pit.dts +3
-0+ /* 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.
Quoted 2 lines
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?
Quoted 2 lines
So what about the generic delayed reset GPIO thing?
http://marc.info/?l=linux-kernel&m=140309916607115&w=2That'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).
Quoted 3 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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