diff mbox series

[v3,09/11] ARM: dts: imx6ul-kontron-n6x1x-s: Disable the snvs-poweroff driver

Message ID 20191031142112.12431-10-frieder.schrempf@kontron.de (mailing list archive)
State New, archived
Headers show
Series Add support for more Kontron i.MX6UL/ULL SoMs and boards | expand

Commit Message

Frieder Schrempf Oct. 31, 2019, 2:24 p.m. UTC 
From: Frieder Schrempf <frieder.schrempf@kontron.de>

The snvs-poweroff driver can power off the system by pulling the
PMIC_ON_REQ signal low, to let the PMIC disable the power.
The Kontron SoMs do not have this signal connected, so let's remove
the node.

This seems to fix a real issue when the signal is asserted at
poweroff, but not actually causing the power to turn off. It was
observed, that in this case the system would not shut down properly.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron i.MX6UL N6310 SoM and boards")
---
 arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi | 4 ----
 1 file changed, 4 deletions(-)

Comments

Shawn Guo Nov. 4, 2019, 7:43 a.m. UTC | #1 
On Thu, Oct 31, 2019 at 02:24:27PM +0000, Schrempf Frieder wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The snvs-poweroff driver can power off the system by pulling the
> PMIC_ON_REQ signal low, to let the PMIC disable the power.
> The Kontron SoMs do not have this signal connected, so let's remove
> the node.
> 
> This seems to fix a real issue when the signal is asserted at
> poweroff, but not actually causing the power to turn off. It was
> observed, that in this case the system would not shut down properly.

I do not quite follow on this.  How does disabling snvs_poweroff fix the
issue?  The root cause of system not shut down properly seems to be that
PMIC doesn't shut down power.  This looks like a clean-up rather than
bug fix.

> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron i.MX6UL N6310 SoM and boards")

If you think this is really a bug fix, it should be applied to the file
before renaming rather than the one after renaming.

Shawn

> ---
>  arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
> index e18a8bd239be..4682a79f5b23 100644
> --- a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
> +++ b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
> @@ -158,10 +158,6 @@
>  	status = "okay";
>  };
>  
> -&snvs_poweroff {
> -	status = "okay";
> -};
> -
>  &uart1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_uart1>;
> -- 
> 2.17.1
Frieder Schrempf Nov. 4, 2019, 8:06 a.m. UTC | #2 
Hi Shawn,

On 04.11.19 08:43, Shawn Guo wrote:
> On Thu, Oct 31, 2019 at 02:24:27PM +0000, Schrempf Frieder wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> The snvs-poweroff driver can power off the system by pulling the
>> PMIC_ON_REQ signal low, to let the PMIC disable the power.
>> The Kontron SoMs do not have this signal connected, so let's remove
>> the node.
>>
>> This seems to fix a real issue when the signal is asserted at
>> poweroff, but not actually causing the power to turn off. It was
>> observed, that in this case the system would not shut down properly.
> 
> I do not quite follow on this.  How does disabling snvs_poweroff fix the
> issue?  The root cause of system not shut down properly seems to be that
> PMIC doesn't shut down power.  This looks like a clean-up rather than
> bug fix.

I don't know the exact reasons, but we had issues on these boards when 
doing a "poweroff". The kernel would print something like the log below.
Disabling the snvs-poweroff solved this.

But note that this has last been reproduced on v4.14. So I'm not sure if 
this is still a problem with the current kernel.

#######
reboot: Power down
Unable to poweroff system

====================================
WARNING: halt/675 still has locks held!
4.14.104-exceet #1 Not tainted
------------------------------------
1 lock held by halt/675:
  #0:  (reboot_mutex){+.+.}, at: [<c0145a98>] SyS_reboot+0x14c/0x1dc
#######

> 
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron i.MX6UL N6310 SoM and boards")
> 
> If you think this is really a bug fix, it should be applied to the file
> before renaming rather than the one after renaming.

I will try to reproduce the issue with the current kernel and depending 
on the results either drop the Fixes tag or move the patch before the 
renaming.

Thanks for reviewing,
Frieder

> 
> Shawn
> 
>> ---
>>   arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>> index e18a8bd239be..4682a79f5b23 100644
>> --- a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>> +++ b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>> @@ -158,10 +158,6 @@
>>   	status = "okay";
>>   };
>>   
>> -&snvs_poweroff {
>> -	status = "okay";
>> -};
>> -
>>   &uart1 {
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&pinctrl_uart1>;
>> -- 
>> 2.17.1
Frieder Schrempf Nov. 4, 2019, 11:18 a.m. UTC | #3 
On 04.11.19 09:06, Frieder Schrempf wrote:
> Hi Shawn,
> 
> On 04.11.19 08:43, Shawn Guo wrote:
>> On Thu, Oct 31, 2019 at 02:24:27PM +0000, Schrempf Frieder wrote:
>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>
>>> The snvs-poweroff driver can power off the system by pulling the
>>> PMIC_ON_REQ signal low, to let the PMIC disable the power.
>>> The Kontron SoMs do not have this signal connected, so let's remove
>>> the node.
>>>
>>> This seems to fix a real issue when the signal is asserted at
>>> poweroff, but not actually causing the power to turn off. It was
>>> observed, that in this case the system would not shut down properly.
>>
>> I do not quite follow on this.  How does disabling snvs_poweroff fix the
>> issue?  The root cause of system not shut down properly seems to be that
>> PMIC doesn't shut down power.  This looks like a clean-up rather than
>> bug fix.
> 
> I don't know the exact reasons, but we had issues on these boards when 
> doing a "poweroff". The kernel would print something like the log below.
> Disabling the snvs-poweroff solved this.
> 
> But note that this has last been reproduced on v4.14. So I'm not sure if 
> this is still a problem with the current kernel.
> 
> #######
> reboot: Power down
> Unable to poweroff system
> 
> ====================================
> WARNING: halt/675 still has locks held!
> 4.14.104-exceet #1 Not tainted
> ------------------------------------
> 1 lock held by halt/675:
>   #0:  (reboot_mutex){+.+.}, at: [<c0145a98>] SyS_reboot+0x14c/0x1dc
> #######
> 
>>
>>>
>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>> Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron 
>>> i.MX6UL N6310 SoM and boards")
>>
>> If you think this is really a bug fix, it should be applied to the file
>> before renaming rather than the one after renaming.
> 
> I will try to reproduce the issue with the current kernel and depending 
> on the results either drop the Fixes tag or move the patch before the 
> renaming.

I just tried this with 5.4-rc5 and the issue persists. When 
snvs-poweroff is enabled without the hardware actually being able to 
power down via PMIC I get a locking WARNING from the kernel. Probably 
because the system is still running, when it's already supposed to be 
shut down by the PMIC.

So this fixes a real problem and therefore I will keep the Fixes tag and 
move the patch to the proper place in this series.

> 
> Thanks for reviewing,
> Frieder
> 
>>
>> Shawn
>>
>>> ---
>>>   arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi | 4 ----
>>>   1 file changed, 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi 
>>> b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>>> index e18a8bd239be..4682a79f5b23 100644
>>> --- a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>>> +++ b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>>> @@ -158,10 +158,6 @@
>>>       status = "okay";
>>>   };
>>> -&snvs_poweroff {
>>> -    status = "okay";
>>> -};
>>> -
>>>   &uart1 {
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&pinctrl_uart1>;
>>> -- 
>>> 2.17.1
Shawn Guo Nov. 4, 2019, 12:45 p.m. UTC | #4 
On Mon, Nov 04, 2019 at 11:18:19AM +0000, Schrempf Frieder wrote:
> On 04.11.19 09:06, Frieder Schrempf wrote:
> > Hi Shawn,
> > 
> > On 04.11.19 08:43, Shawn Guo wrote:
> >> On Thu, Oct 31, 2019 at 02:24:27PM +0000, Schrempf Frieder wrote:
> >>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>>
> >>> The snvs-poweroff driver can power off the system by pulling the
> >>> PMIC_ON_REQ signal low, to let the PMIC disable the power.
> >>> The Kontron SoMs do not have this signal connected, so let's remove
> >>> the node.
> >>>
> >>> This seems to fix a real issue when the signal is asserted at
> >>> poweroff, but not actually causing the power to turn off. It was
> >>> observed, that in this case the system would not shut down properly.
> >>
> >> I do not quite follow on this.  How does disabling snvs_poweroff fix the
> >> issue?  The root cause of system not shut down properly seems to be that
> >> PMIC doesn't shut down power.  This looks like a clean-up rather than
> >> bug fix.
> > 
> > I don't know the exact reasons, but we had issues on these boards when 
> > doing a "poweroff". The kernel would print something like the log below.
> > Disabling the snvs-poweroff solved this.
> > 
> > But note that this has last been reproduced on v4.14. So I'm not sure if 
> > this is still a problem with the current kernel.
> > 
> > #######
> > reboot: Power down
> > Unable to poweroff system
> > 
> > ====================================
> > WARNING: halt/675 still has locks held!
> > 4.14.104-exceet #1 Not tainted
> > ------------------------------------
> > 1 lock held by halt/675:
> >   #0:  (reboot_mutex){+.+.}, at: [<c0145a98>] SyS_reboot+0x14c/0x1dc
> > #######
> > 
> >>
> >>>
> >>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>> Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron 
> >>> i.MX6UL N6310 SoM and boards")
> >>
> >> If you think this is really a bug fix, it should be applied to the file
> >> before renaming rather than the one after renaming.
> > 
> > I will try to reproduce the issue with the current kernel and depending 
> > on the results either drop the Fixes tag or move the patch before the 
> > renaming.
> 
> I just tried this with 5.4-rc5 and the issue persists. When 
> snvs-poweroff is enabled without the hardware actually being able to 
> power down via PMIC I get a locking WARNING from the kernel. Probably 
> because the system is still running, when it's already supposed to be 
> shut down by the PMIC.

I still do not get it.  Are you saying that with snvs-poweroff being
disabled, your system is now able to be shut down properly by PMIC?

Shawn

> 
> So this fixes a real problem and therefore I will keep the Fixes tag and 
> move the patch to the proper place in this series.
Frieder Schrempf Nov. 4, 2019, 1:14 p.m. UTC | #5 
On 04.11.19 13:45, Shawn Guo wrote:
> On Mon, Nov 04, 2019 at 11:18:19AM +0000, Schrempf Frieder wrote:
>> On 04.11.19 09:06, Frieder Schrempf wrote:
>>> Hi Shawn,
>>>
>>> On 04.11.19 08:43, Shawn Guo wrote:
>>>> On Thu, Oct 31, 2019 at 02:24:27PM +0000, Schrempf Frieder wrote:
>>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>>
>>>>> The snvs-poweroff driver can power off the system by pulling the
>>>>> PMIC_ON_REQ signal low, to let the PMIC disable the power.
>>>>> The Kontron SoMs do not have this signal connected, so let's remove
>>>>> the node.
>>>>>
>>>>> This seems to fix a real issue when the signal is asserted at
>>>>> poweroff, but not actually causing the power to turn off. It was
>>>>> observed, that in this case the system would not shut down properly.
>>>>
>>>> I do not quite follow on this.  How does disabling snvs_poweroff fix the
>>>> issue?  The root cause of system not shut down properly seems to be that
>>>> PMIC doesn't shut down power.  This looks like a clean-up rather than
>>>> bug fix.
>>>
>>> I don't know the exact reasons, but we had issues on these boards when
>>> doing a "poweroff". The kernel would print something like the log below.
>>> Disabling the snvs-poweroff solved this.
>>>
>>> But note that this has last been reproduced on v4.14. So I'm not sure if
>>> this is still a problem with the current kernel.
>>>
>>> #######
>>> reboot: Power down
>>> Unable to poweroff system
>>>
>>> ====================================
>>> WARNING: halt/675 still has locks held!
>>> 4.14.104-exceet #1 Not tainted
>>> ------------------------------------
>>> 1 lock held by halt/675:
>>>    #0:  (reboot_mutex){+.+.}, at: [<c0145a98>] SyS_reboot+0x14c/0x1dc
>>> #######
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>> Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron
>>>>> i.MX6UL N6310 SoM and boards")
>>>>
>>>> If you think this is really a bug fix, it should be applied to the file
>>>> before renaming rather than the one after renaming.
>>>
>>> I will try to reproduce the issue with the current kernel and depending
>>> on the results either drop the Fixes tag or move the patch before the
>>> renaming.
>>
>> I just tried this with 5.4-rc5 and the issue persists. When
>> snvs-poweroff is enabled without the hardware actually being able to
>> power down via PMIC I get a locking WARNING from the kernel. Probably
>> because the system is still running, when it's already supposed to be
>> shut down by the PMIC.
> 
> I still do not get it.  Are you saying that with snvs-poweroff being
> disabled, your system is now able to be shut down properly by PMIC?

Sorry for the confusion. The board does not have a PMIC and the 
PMIC_ON_REQ signal is not connected. So the snvs-poweroff driver should 
definitely be disabled because there is no reason for it to be enabled.

But indeed, with snvs-poweroff disabled the board does now at least halt 
correctly (it is still powered of course).

For some reason, probably by some copy and paste mistake, the 
snvs-poweroff driver is enabled in the dts which causes the issue 
described above when trying to halt/shutdown the system. Please also see 
the logs below for comparison of both cases.

With snvs-poweroff enabled:

############
~# poweroff
The system is going down for system halt NOW!ymxc3) (Thu Oct 31 18:57:55 
2019
[...]
reboot: Power down
Unable to poweroff system

====================================
WARNING: halt/869 still has locks held!
5.4.0-rc5-ktn #1 Not tainted
------------------------------------
1 lock held by halt/869:
  #0: c11138f8 (system_transition_mutex){+.+.}, at: 
__do_sys_reboot+0x174/0x20c

stack backtrace:
CPU: 0 PID: 869 Comm: halt Not tainted 5.4.0-rc5-ktn #1
Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[<c0112bb8>] (unwind_backtrace) from [<c010ce14>] (show_stack+0x10/0x14)
[<c010ce14>] (show_stack) from [<c0ae18dc>] (dump_stack+0xd8/0x110)
[<c0ae18dc>] (dump_stack) from [<c012a5c0>] (do_exit+0x7a4/0xb94)
[<c012a5c0>] (do_exit) from [<c014fe64>] (__do_sys_reboot+0x180/0x20c)
[<c014fe64>] (__do_sys_reboot) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
Exception stack(0xd9e15fa8 to 0xd9e15ff0)
5fa0:                   bed53e14 00000005 fee1dead 28121969 4321fedc 
fffffff6
5fc0: bed53e14 00000005 0040cf04 00000058 0041def8 00000000 00000001 
00000000
5fe0: 00000058 bed53c74 b6f03ad5 b6e89c46
############

With snvs-poweroff disabled:

############
The system is going down for system halt NOW!ymxc3) (Thu Oct 31 18:56:33 
2019
[...]
reboot: System halted
############

> 
> Shawn
> 
>>
>> So this fixes a real problem and therefore I will keep the Fixes tag and
>> move the patch to the proper place in this series.
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
index e18a8bd239be..4682a79f5b23 100644
--- a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
+++ b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
@@ -158,10 +158,6 @@ 
 	status = "okay";
 };
 
-&snvs_poweroff {
-	status = "okay";
-};
-
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1>;