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

From: Schrempf Frieder <frieder.schrempf@kontron.de>
To: Shawn Guo <shawnguo@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>,
	Fabio Estevam <festevam@gmail.com>,
	Mark Rutland <mark.rutland@arm.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Rob Herring <robh+dt@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 09/11] ARM: dts: imx6ul-kontron-n6x1x-s: Disable the snvs-poweroff driver
Date: Mon, 4 Nov 2019 13:14:08 +0000	[thread overview]
Message-ID: <54e7efb9-bf18-f513-569b-817ff2e403f6@kontron.de> (raw)
In-Reply-To: <20191104124553.GY24620@dragon>

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.