| Message ID | 1403774127-21892-1-git-send-email-vikas.sajjan@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vikas, Doug, On 26.06.2014 11:15, Vikas Sajjan wrote: > From: Doug Anderson <dianders@chromium.org> > > The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from > being reset across sleep/wake. If we don't set it to anything then > the TPM will be reset. U-Boot will detect this as invalid > and will reset the system on resume time. This GPIO can always be low > and not hurt anything. It will get pulled back high again during a > normal warm reset when it will default back to an input. > > To properly preserve the TPM state across suspend/resume and to make > the chrome U-Boot happy, properly set the GPIO to mask the > reset to the TPM. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> > --- > arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts > index 7649982..8fd990a 100644 > --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts > +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts > @@ -87,6 +87,18 @@ > pinctrl-0 = <&usb301_vbus_en>; > enable-active-high; > }; > + > + /* We need GPX0_6 to be low at sleep time; just keep it low always */ > + mask_tpm_reset_regulator: mask-tpm-reset-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "mask-tpm-reset "; > + gpio = <&gpx0 6 0>; > + enable-active-low; > + regulator-boot-on; > + regulator-always-on; > + pinctrl-names = "default"; > + pinctrl-0 = <&mask_tpm_reset>; > + }; I don't think this pin is supposed to be a real regulator. If I'm right, you should just add a hog for it, if you don't have a proper driver to handle it. > }; > > &dp { > @@ -210,6 +222,14 @@ > > > &pinctrl_0 { > + nit: No need for this blank line. Best regards, Tomasz
Tomasz, On Thu, Jun 26, 2014 at 2:52 AM, Tomasz Figa <t.figa@samsung.com> wrote: > Hi Vikas, Doug, > > On 26.06.2014 11:15, Vikas Sajjan wrote: >> From: Doug Anderson <dianders@chromium.org> >> >> The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from >> being reset across sleep/wake. If we don't set it to anything then >> the TPM will be reset. U-Boot will detect this as invalid >> and will reset the system on resume time. This GPIO can always be low >> and not hurt anything. It will get pulled back high again during a >> normal warm reset when it will default back to an input. >> >> To properly preserve the TPM state across suspend/resume and to make >> the chrome U-Boot happy, properly set the GPIO to mask the >> reset to the TPM. >> >> Signed-off-by: Doug Anderson <dianders@chromium.org> >> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> >> --- >> arch/arm/boot/dts/exynos5420-peach-pit.dts | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts >> index 7649982..8fd990a 100644 >> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts >> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts >> @@ -87,6 +87,18 @@ >> pinctrl-0 = <&usb301_vbus_en>; >> enable-active-high; >> }; >> + >> + /* We need GPX0_6 to be low at sleep time; just keep it low always */ >> + mask_tpm_reset_regulator: mask-tpm-reset-regulator { >> + compatible = "regulator-fixed"; >> + regulator-name = "mask-tpm-reset "; >> + gpio = <&gpx0 6 0>; >> + enable-active-low; >> + regulator-boot-on; >> + regulator-always-on; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&mask_tpm_reset>; >> + }; > > I don't think this pin is supposed to be a real regulator. If I'm right, > you should just add a hog for it, if you don't have a proper driver to > handle it. Yes, I agree that it shouldn't really be a regulator, but there's not a whole lot of choice. The pin needs to actually be driven low, not just pulled low. Without your proposed patch (pinctrl: samsung: Allow pin value to be initialized using pinfunc) I don't think it's possible to actually drive a pin low with a hog. I could be wrong, though. So I think our options are: * Begrudgingly use the regulator this way. * Re-propose your pinctrl patch and use hogs. * Implement a tiny driver that will just hold a GPIO in a certain state and instantiate that driver here. * Decide that really the firmware should have set this pin and that if someone wants suspend/resume to work then they'll just have to update their firmware. Queue the debate about what firmware vs. kernel's job is and commence yelling about the fact that it's hard to get an official RW firmware update approved. Note: I'm not quite sure why I didn't have the firmware init this GPIO to begin with (so that if the kernel didn't do anything it would just work). Probably just me being stupid. Ah, one other thing that could justify this being its own special driver (though you might be able to use the regulator framework for it too?). Technically you'll save a tiny amount of power in the system if you leave this GPIO high while the system is running and only drive it low when the system goes to suspend. That's because there's a pretty strong external pullup on this pin. The amount of power is probably not noticeable and the savings is only possible when the system is on (and in high power state) anyway, but... -Doug
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
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
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
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
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
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.
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
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
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
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.
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
On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote: > From: Doug Anderson <dianders@chromium.org> > > The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from > being reset across sleep/wake. If we don't set it to anything then > the TPM will be reset. U-Boot will detect this as invalid > and will reset the system on resume time. This GPIO can always be low > and not hurt anything. It will get pulled back high again during a > normal warm reset when it will default back to an input. > > To properly preserve the TPM state across suspend/resume and to make > the chrome U-Boot happy, properly set the GPIO to mask the > reset to the TPM. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> (...) > + /* We need GPX0_6 to be low at sleep time; just keep it low always */ > + mask_tpm_reset_regulator: mask-tpm-reset-regulator { > + compatible = "regulator-fixed"; No matter how the discussion ends up, regulator-fixed is wrong. Either folding it into the TPM driver or using a separate reset driver is fine with me. So what about the generic delayed reset GPIO thing? http://marc.info/?l=linux-kernel&m=140309916607115&w=2 Yours, Linus Walleij
Hi, On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote: > >> From: Doug Anderson <dianders@chromium.org> >> >> The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from >> being reset across sleep/wake. If we don't set it to anything then >> the TPM will be reset. U-Boot will detect this as invalid >> and will reset the system on resume time. This GPIO can always be low >> and not hurt anything. It will get pulled back high again during a >> normal warm reset when it will default back to an input. >> >> To properly preserve the TPM state across suspend/resume and to make >> the chrome U-Boot happy, properly set the GPIO to mask the >> reset to the TPM. >> >> Signed-off-by: Doug Anderson <dianders@chromium.org> >> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> > (...) >> + /* We need GPX0_6 to be low at sleep time; just keep it low always */ >> + mask_tpm_reset_regulator: mask-tpm-reset-regulator { >> + compatible = "regulator-fixed"; > > No matter how the discussion ends up, regulator-fixed is wrong. OK, fair enough. > Either folding it into the TPM driver or using a separate reset driver > is fine with me. OK, Vikas: do you want to code up the driver? > So what about the generic delayed reset GPIO thing? > http://marc.info/?l=linux-kernel&m=140309916607115&w=2 That's a neat concept and could be useful in other cases, but I think it's just as much of a hack as using a regulator. This is not a reset signal for the TPM. This is a signal that will mask the CPU's reset signal (using a special bit of board-specific logic). Personally I think Tomasz's idea of using hogs (after his patches allowing a default output level) is the cleanest, but I think Stephen didn't like that. -Doug
On 08.07.2014 17:27, Doug Anderson wrote: > Hi, > > On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote: >> >>> From: Doug Anderson <dianders@chromium.org> >>> >>> The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from >>> being reset across sleep/wake. If we don't set it to anything then >>> the TPM will be reset. U-Boot will detect this as invalid >>> and will reset the system on resume time. This GPIO can always be low >>> and not hurt anything. It will get pulled back high again during a >>> normal warm reset when it will default back to an input. >>> >>> To properly preserve the TPM state across suspend/resume and to make >>> the chrome U-Boot happy, properly set the GPIO to mask the >>> reset to the TPM. >>> >>> Signed-off-by: Doug Anderson <dianders@chromium.org> >>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> >> (...) >>> + /* We need GPX0_6 to be low at sleep time; just keep it low always */ >>> + mask_tpm_reset_regulator: mask-tpm-reset-regulator { >>> + compatible = "regulator-fixed"; >> >> No matter how the discussion ends up, regulator-fixed is wrong. > > OK, fair enough. > > >> Either folding it into the TPM driver or using a separate reset driver >> is fine with me. > > OK, Vikas: do you want to code up the driver? > > >> So what about the generic delayed reset GPIO thing? >> http://marc.info/?l=linux-kernel&m=140309916607115&w=2 > > That's a neat concept and could be useful in other cases, but I think > it's just as much of a hack as using a regulator. This is not a reset > signal for the TPM. This is a signal that will mask the CPU's reset > signal (using a special bit of board-specific logic). > > > Personally I think Tomasz's idea of using hogs (after his patches > allowing a default output level) is the cleanest, but I think Stephen > didn't like that. I don't see any benefits of using complex interfaces over simply initializing the pin to the right value once at boot-up or at suspend/resume. As far as I understand Doug's explanation of the problem, nothing else is expected from the OS with respect to this pin. Moreover it doesn't affect operation of any drivers. So I'm still for the simplest and effective solution, i.e. hogs. Best regards, Tomasz
Vikas, On Tue, Jul 8, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote: > On 08.07.2014 17:27, Doug Anderson wrote: >> Hi, >> >> On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote: >>> >>>> From: Doug Anderson <dianders@chromium.org> >>>> >>>> The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from >>>> being reset across sleep/wake. If we don't set it to anything then >>>> the TPM will be reset. U-Boot will detect this as invalid >>>> and will reset the system on resume time. This GPIO can always be low >>>> and not hurt anything. It will get pulled back high again during a >>>> normal warm reset when it will default back to an input. >>>> >>>> To properly preserve the TPM state across suspend/resume and to make >>>> the chrome U-Boot happy, properly set the GPIO to mask the >>>> reset to the TPM. >>>> >>>> Signed-off-by: Doug Anderson <dianders@chromium.org> >>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> >>> (...) >>>> + /* We need GPX0_6 to be low at sleep time; just keep it low always */ >>>> + mask_tpm_reset_regulator: mask-tpm-reset-regulator { >>>> + compatible = "regulator-fixed"; >>> >>> No matter how the discussion ends up, regulator-fixed is wrong. >> >> OK, fair enough. >> >> >>> Either folding it into the TPM driver or using a separate reset driver >>> is fine with me. >> >> OK, Vikas: do you want to code up the driver? >> >> >>> So what about the generic delayed reset GPIO thing? >>> http://marc.info/?l=linux-kernel&m=140309916607115&w=2 >> >> That's a neat concept and could be useful in other cases, but I think >> it's just as much of a hack as using a regulator. This is not a reset >> signal for the TPM. This is a signal that will mask the CPU's reset >> signal (using a special bit of board-specific logic). >> >> >> Personally I think Tomasz's idea of using hogs (after his patches >> allowing a default output level) is the cleanest, but I think Stephen >> didn't like that. > > I don't see any benefits of using complex interfaces over simply > initializing the pin to the right value once at boot-up or at > suspend/resume. As far as I understand Doug's explanation of the > problem, nothing else is expected from the OS with respect to this pin. > Moreover it doesn't affect operation of any drivers. > > So I'm still for the simplest and effective solution, i.e. hogs. It looks as if Tomasz's patch has landed, so perhaps you could try to code it up his way. It should be very simple. Please make sure to CC Stephen Warren on the patch so that he is included in the discussion (and of course include Tomasz, Linus W, etc). -Doug
Doug, On Wed, Jul 9, 2014 at 8:52 PM, Doug Anderson <dianders@chromium.org> wrote: > Vikas, > > On Tue, Jul 8, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote: >> On 08.07.2014 17:27, Doug Anderson wrote: >>> Hi, >>> >>> On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote: >>>> >>>>> From: Doug Anderson <dianders@chromium.org> >>>>> >>>>> The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from >>>>> being reset across sleep/wake. If we don't set it to anything then >>>>> the TPM will be reset. U-Boot will detect this as invalid >>>>> and will reset the system on resume time. This GPIO can always be low >>>>> and not hurt anything. It will get pulled back high again during a >>>>> normal warm reset when it will default back to an input. >>>>> >>>>> To properly preserve the TPM state across suspend/resume and to make >>>>> the chrome U-Boot happy, properly set the GPIO to mask the >>>>> reset to the TPM. >>>>> >>>>> Signed-off-by: Doug Anderson <dianders@chromium.org> >>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> >>>> (...) >>>>> + /* We need GPX0_6 to be low at sleep time; just keep it low always */ >>>>> + mask_tpm_reset_regulator: mask-tpm-reset-regulator { >>>>> + compatible = "regulator-fixed"; >>>> >>>> No matter how the discussion ends up, regulator-fixed is wrong. >>> >>> OK, fair enough. >>> >>> >>>> Either folding it into the TPM driver or using a separate reset driver >>>> is fine with me. >>> >>> OK, Vikas: do you want to code up the driver? >>> >>> >>>> So what about the generic delayed reset GPIO thing? >>>> http://marc.info/?l=linux-kernel&m=140309916607115&w=2 >>> >>> That's a neat concept and could be useful in other cases, but I think >>> it's just as much of a hack as using a regulator. This is not a reset >>> signal for the TPM. This is a signal that will mask the CPU's reset >>> signal (using a special bit of board-specific logic). >>> >>> >>> Personally I think Tomasz's idea of using hogs (after his patches >>> allowing a default output level) is the cleanest, but I think Stephen >>> didn't like that. >> >> I don't see any benefits of using complex interfaces over simply >> initializing the pin to the right value once at boot-up or at >> suspend/resume. As far as I understand Doug's explanation of the >> problem, nothing else is expected from the OS with respect to this pin. >> Moreover it doesn't affect operation of any drivers. >> >> So I'm still for the simplest and effective solution, i.e. hogs. > > It looks as if Tomasz's patch has landed, so perhaps you could try to > code it up his way. It should be very simple. Please make sure to CC > Stephen Warren on the patch so that he is included in the discussion > (and of course include Tomasz, Linus W, etc). Can you point me to Tomasz's patchset on which I should rebase this patch on. > > -Doug
Vikas, On Wed, Jul 9, 2014 at 9:35 PM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote: > Doug, > > On Wed, Jul 9, 2014 at 8:52 PM, Doug Anderson <dianders@chromium.org> wrote: >> Vikas, >> >> On Tue, Jul 8, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote: >>> On 08.07.2014 17:27, Doug Anderson wrote: >>>> Hi, >>>> >>>> On Tue, Jul 8, 2014 at 12:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>>> On Thu, Jun 26, 2014 at 11:15 AM, Vikas Sajjan <vikas.sajjan@samsung.com> wrote: >>>>> >>>>>> From: Doug Anderson <dianders@chromium.org> >>>>>> >>>>>> The mask-tpm-reset GPIO is used by the kernel to prevent the TPM from >>>>>> being reset across sleep/wake. If we don't set it to anything then >>>>>> the TPM will be reset. U-Boot will detect this as invalid >>>>>> and will reset the system on resume time. This GPIO can always be low >>>>>> and not hurt anything. It will get pulled back high again during a >>>>>> normal warm reset when it will default back to an input. >>>>>> >>>>>> To properly preserve the TPM state across suspend/resume and to make >>>>>> the chrome U-Boot happy, properly set the GPIO to mask the >>>>>> reset to the TPM. >>>>>> >>>>>> Signed-off-by: Doug Anderson <dianders@chromium.org> >>>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com> >>>>> (...) >>>>>> + /* We need GPX0_6 to be low at sleep time; just keep it low always */ >>>>>> + mask_tpm_reset_regulator: mask-tpm-reset-regulator { >>>>>> + compatible = "regulator-fixed"; >>>>> >>>>> No matter how the discussion ends up, regulator-fixed is wrong. >>>> >>>> OK, fair enough. >>>> >>>> >>>>> Either folding it into the TPM driver or using a separate reset driver >>>>> is fine with me. >>>> >>>> OK, Vikas: do you want to code up the driver? >>>> >>>> >>>>> So what about the generic delayed reset GPIO thing? >>>>> http://marc.info/?l=linux-kernel&m=140309916607115&w=2 >>>> >>>> That's a neat concept and could be useful in other cases, but I think >>>> it's just as much of a hack as using a regulator. This is not a reset >>>> signal for the TPM. This is a signal that will mask the CPU's reset >>>> signal (using a special bit of board-specific logic). >>>> >>>> >>>> Personally I think Tomasz's idea of using hogs (after his patches >>>> allowing a default output level) is the cleanest, but I think Stephen >>>> didn't like that. >>> >>> I don't see any benefits of using complex interfaces over simply >>> initializing the pin to the right value once at boot-up or at >>> suspend/resume. As far as I understand Doug's explanation of the >>> problem, nothing else is expected from the OS with respect to this pin. >>> Moreover it doesn't affect operation of any drivers. >>> >>> So I'm still for the simplest and effective solution, i.e. hogs. >> >> It looks as if Tomasz's patch has landed, so perhaps you could try to >> code it up his way. It should be very simple. Please make sure to CC >> Stephen Warren on the patch so that he is included in the discussion >> (and of course include Tomasz, Linus W, etc). > > Can you point me to Tomasz's patchset on which I should rebase this patch on. You can base it atop (0635c88 pinctrl: samsung: Allow pin value to be initialized using pinfunc), which appears to be in linux-next. That gives you "samsung,pin-val". You can use that together with a pinctrl "hog" to specify the state of this pin for the board. -Doug
diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts index 7649982..8fd990a 100644 --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts @@ -87,6 +87,18 @@ pinctrl-0 = <&usb301_vbus_en>; enable-active-high; }; + + /* We need GPX0_6 to be low at sleep time; just keep it low always */ + mask_tpm_reset_regulator: mask-tpm-reset-regulator { + compatible = "regulator-fixed"; + regulator-name = "mask-tpm-reset "; + gpio = <&gpx0 6 0>; + enable-active-low; + regulator-boot-on; + regulator-always-on; + pinctrl-names = "default"; + pinctrl-0 = <&mask_tpm_reset>; + }; }; &dp { @@ -210,6 +222,14 @@ &pinctrl_0 { + + mask_tpm_reset: mask-tpm-reset { + samsung,pins = "gpx0-6"; + samsung,pin-function = <1>; + samsung,pin-pud = <0>; + samsung,pin-drv = <0>; + }; + max98090_irq: max98090-irq { samsung,pins = "gpx0-2"; samsung,pin-function = <0>;