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 11:18:19 +0000	[thread overview]
Message-ID: <7da30a17-d16c-3cde-12a8-430ff3eec692@kontron.de> (raw)
In-Reply-To: <626ad87a-eb1d-4dca-7cd3-8c5f38025aec@kontron.de>

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