Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp9664413imu; Wed, 5 Dec 2018 08:21:09 -0800 (PST) X-Google-Smtp-Source: AFSGD/WNtMV1fLcV107rSaLJ0+1bqMw7TjKmYQW8CVvrUdm0uEqyqPS/OyFLL/eYKTmFiBDVXdeF X-Received: by 2002:a63:2d82:: with SMTP id t124mr20866580pgt.260.1544026869756; Wed, 05 Dec 2018 08:21:09 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1544026869; cv=none; d=google.com; s=arc-20160816; b=dva5fd1bD4bhjeAsxNCIgJ0/GQpz3jyHBOO+CZW5WAfuU5L9LfPguDvZo33nf7ripl s2avzzZA9DO7HetgPUxAOTgGWFp//82Q5nexDnzAVYmtiW69SJOFzTlkYqzuxIdHYOY5 WyWupG8xehPXPYy+7nQ07LJm5XyiwN6UuO3DaHOOQmX96x+D1S/CF2TscY0zwV7rlJwD 1S3R1JyW5kiHOB9/WebAcP55dUEo0L8t6DubQAO1rV+jHVvno3IRwFHAJO0zZCrlNrrB EMP5xJWmxJr/nXP7j4iU0kWXGT12DJMZwaMuC5Lv81FRJ7b2qt7P/JfaFH9WVvCJ+EPK Rz7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=noS0fMWo8H79rOjZ7/mDcH1m5M0rBAFxVquY3hocat0=; b=NemwHAwtzmcdX57PG6YtKzEPbYmFuS4wwb10P6TLJLqjph4mv3+P+F2YzFj0BixCoX 3Aarb7MtdLjPZhs9l6xFykOtJuaNys074Vnnxe9aOBtpCLKaHrdLS9QKeepuI5t7Om6G OK1H75KEkh4cio4KCCLfRRrlUIwjC1sVdAZpRpVfTE3p9LlmgUFkgXHlPq2fKGDW97JT 2s7GH1hu3gp32N7e4zRE+ByD9LurHmGfmdxx8Jyek8jxi51787TGFt7dNVfuYVyVtPDp 9HZrIUQhT1u64JuoeuP6Tj6d/OHNDjvc+cHA9nyHUjfinj6nCoQaug6A9vrGPhvjnKUv t5gw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=VZnrblm5; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id x141si18007392pgx.266.2018.12.05.08.20.54; Wed, 05 Dec 2018 08:21:09 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=VZnrblm5; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728369AbeLEQTm (ORCPT + 99 others); Wed, 5 Dec 2018 11:19:42 -0500 Received: from mail.kernel.org ([198.145.29.99]:39524 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726918AbeLEQTm (ORCPT ); Wed, 5 Dec 2018 11:19:42 -0500 Received: from mail-lf1-f53.google.com (mail-lf1-f53.google.com [209.85.167.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8380C2146D; Wed, 5 Dec 2018 16:19:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1544026781; bh=ttjQTMKxQvSDqL8gME0wiXdDgWnR5tM+j3N9lxclCVs=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=VZnrblm5R+r/55KWnoca/t0eMM4ysKM4ry/QR+jBhu9ZEekWo+Afckw7wz8DGPiKY px067SYGt5nVmSDnDJUKj/8XwEhO7ant7dgOT9ad0VsFuUuIped+FbCBWa0b9skpyk nOU0I3qTTMIM16m6+oLSG9U7E6Mv7ohJEhYbfOD8= Received: by mail-lf1-f53.google.com with SMTP id c16so15159566lfj.8; Wed, 05 Dec 2018 08:19:40 -0800 (PST) X-Gm-Message-State: AA+aEWa98YYwWtCw4y2YTWPVo8VI3MfJZ7t40pRIcYGX4a94BVhkVkOM vXje1KrnF4MsJqAMdqPb0skP1twm2W29EjXkVU8= X-Received: by 2002:a19:2d16:: with SMTP id k22mr14105095lfj.12.1544026778560; Wed, 05 Dec 2018 08:19:38 -0800 (PST) MIME-Version: 1.0 References: <20181204194025.2719-1-linux.amoon@gmail.com> In-Reply-To: From: Krzysztof Kozlowski Date: Wed, 5 Dec 2018 17:19:27 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 To: linux.amoon@gmail.com Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "linux-samsung-soc@vger.kernel.org" , linux-kernel@vger.kernel.org, robh+dt@kernel.org, mark.rutland@arm.com, kgene@kernel.org, Marek Szyprowski , =?UTF-8?B?QmFydMWCb21pZWogxbtvxYJuaWVya2lld2ljeg==?= Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 5 Dec 2018 at 17:11, Anand Moon wrote: > > Hi Krzysztof, > > Thanks for your review. > . > On Wed, 5 Dec 2018 at 19:36, Krzysztof Kozlowski wrote: > > > > On Tue, 4 Dec 2018 at 20:40, Anand Moon wrote: > > > > > > Add suspend-to-mem node to regulator core to be enabled or disabled > > > during system suspend and also support changing the regulator operating > > > mode during runtime and when the system enter sleep mode. > > > > > > Signed-off-by: Anand Moon > > > --- > > > Tested on Odroid U3+ > > > --- > > > .../boot/dts/exynos4412-odroid-common.dtsi | 72 +++++++++++++++++++ > > > 1 file changed, 72 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > index 2caa3132f34e..837713a2ec3b 100644 > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > @@ -285,6 +285,9 @@ > > > regulator-min-microvolt = <1800000>; > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > > Driver does not support suspend_enable so this will be noop. We could > > add this for DT-correctness (and full description of HW) but then I > > need explanation why this regulator has to stay on during suspend. > > > > Well I tried to study the suspend/resume feature from > *arch/arm/boot/dts/exynos4412-midas.dtsi* > and them I tried to apply the same with this on Odroid-U3 boards and > test these changes. Midas DTSI clearly has bugs then. > > > > }; > > > > > > ldo3_reg: LDO3 { > > > @@ -292,6 +295,9 @@ > > > regulator-min-microvolt = <1800000>; > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > > The same but with suspend_disable. > > > > I am not saying that these are wrong but I would be happy to see > > explanations why you chosen these particular properties. > > > > Well I was not aware on the regulator-always-on cannot be set to off > in suspend mode. > Will drop this in the future patch where regulator-always-on; is set. > > > > }; > > > > > > ldo4_reg: LDO4 { > > > @@ -299,6 +305,9 @@ > > > regulator-min-microvolt = <2800000>; > > > regulator-max-microvolt = <2800000>; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > }; > > > > > > ldo5_reg: LDO5 { > > > @@ -307,6 +316,9 @@ > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > }; > > > > > > ldo6_reg: LDO6 { > > > @@ -314,6 +326,9 @@ > > > regulator-min-microvolt = <1000000>; > > > regulator-max-microvolt = <1000000>; > > > regulator-always-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > }; > > > > > > ldo7_reg: LDO7 { > > > @@ -321,18 +336,27 @@ > > > regulator-min-microvolt = <1000000>; > > > regulator-max-microvolt = <1000000>; > > > regulator-always-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > }; > > > > > > ldo8_reg: LDO8 { > > > regulator-name = "VDD10_HDMI_1.0V"; > > > regulator-min-microvolt = <1000000>; > > > regulator-max-microvolt = <1000000>; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo10_reg: LDO10 { > > > regulator-name = "VDDQ_MIPIHSI_1.8V"; > > > regulator-min-microvolt = <1800000>; > > > regulator-max-microvolt = <1800000>; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo11_reg: LDO11 { > > > @@ -340,6 +364,9 @@ > > > regulator-min-microvolt = <1800000>; > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo12_reg: LDO12 { > > > @@ -348,6 +375,9 @@ > > > regulator-max-microvolt = <3300000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo13_reg: LDO13 { > > > @@ -356,6 +386,9 @@ > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo14_reg: LDO14 { > > > @@ -364,6 +397,9 @@ > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo15_reg: LDO15 { > > > @@ -372,6 +408,9 @@ > > > regulator-max-microvolt = <1000000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo16_reg: LDO16 { > > > @@ -380,6 +419,9 @@ > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > }; > > > > > > ldo20_reg: LDO20 { > > > @@ -387,6 +429,9 @@ > > > regulator-min-microvolt = <1800000>; > > > regulator-max-microvolt = <1800000>; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > ldo21_reg: LDO21 { > > > @@ -394,6 +439,9 @@ > > > regulator-min-microvolt = <2800000>; > > > regulator-max-microvolt = <2800000>; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > > That's unusual setting... enabled in suspend but not necessarily > > during work (no always-on). > > > > I kept the regulator required for emmc/sd card in > regulator-on-in-suspend in suspend mode. > > > > + }; > > > }; > > > > > > ldo22_reg: LDO22 { > > > @@ -411,6 +459,9 @@ > > > regulator-max-microvolt = <1800000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > > > buck1_reg: BUCK1 { > > > @@ -419,6 +470,9 @@ > > > regulator-max-microvolt = <1100000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > > This is seriously wrong so I have doubts whether you tested the > > changes or you know what is happening with them. If you turn off > > memory bus regulator off, how the memory will work in > > Suspend-to-Memory mode? > > > > Well I did not find any issue in my testing but I might be wrong. > Ok I will drop this changes in the next version of the patch where > regulator-always-on is set. It worked fine because regulator-off-in-suspend is noop for buck1 but it is clearly wrong from logical point of view. Do you think that memory should be off in Suspend to RAM? > > > }; > > > > > > buck2_reg: BUCK2 { > > > @@ -427,6 +481,9 @@ > > > regulator-max-microvolt = <1350000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > }; > > > > > > buck3_reg: BUCK3 { > > > @@ -435,6 +492,9 @@ > > > regulator-max-microvolt = <1050000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > > Looks suspicious as well. > > Ok I will drop this changes in the next version of the patch where > regulator-always-on is set. > > > > > Best regards, > > Krzysztof > > > > Well I have tested this patch as following > with only one issue, before enable suspend number of On-line cpu is 4 > after resume number of On-line cpu is 1. If before your change number of resumed CPUs was 4, then you apparently break some things. Best regards, Krzysztof