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 08:06:09 +0000	[thread overview]
Message-ID: <626ad87a-eb1d-4dca-7cd3-8c5f38025aec@kontron.de> (raw)
In-Reply-To: <20191104074346.GT24620@dragon>

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