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

[Schrempf Frieder]

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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> 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