linux-kernel - Re: [PATCH] ARM: dts: microchip: at91-sam9x75_curiosity: Add PMIC suspend voltage configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1c92e4ad-3aef-4eec-8f90-989884262415@tuxon.dev>
Date: Sat, 18 Jan 2025 16:38:33 +0200
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Andrei Simion <andrei.simion@...rochip.com>, nicolas.ferre@...rochip.com,
 alexandre.belloni@...tlin.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ARM: dts: microchip: at91-sam9x75_curiosity: Add PMIC
 suspend voltage configuration

Hi, Andrei,

On 13.01.2025 09:17, Andrei Simion wrote:
> Add missing essential configuration for the PMIC rails, which is necessary
> for proper low-power mode operation. This patch adds the required settings
> to ensure that the regulators behave correctly during Suspend-to-RAM and
> Standby states. Otherwise, after resuming, it receives: "No configuration"
> message.
> 
> Our driver implements the set_suspend_voltage and set_suspend_mode
> callbacks, which require the `regulator-suspend-microvolt` property to be
> specified in the device tree for each regulator node. This property defines
> the voltage level that the regulator should maintain during suspend mode.
> 
> Additionally, according to the datasheet, some regulators need to be turned
> on or off during suspend mode. This patch addresses these requirements by
> adding the `regulator-on-in-suspend` and `regulator-off-in-suspend`
> properties where appropriate.
> 
> Fixes: 371a47c9a58a ("ARM: dts: microchip: sam9x75_curiosity: add sam9x75 curiosity board")
> Signed-off-by: Andrei Simion <andrei.simion@...rochip.com>
> ---
>  .../dts/microchip/at91-sam9x75_curiosity.dts   | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts b/arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts
> index 1a6a909a5043..5514ad10cda5 100644
> --- a/arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts
> +++ b/arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts
> @@ -110,10 +110,12 @@ vdd_3v3: VDD_IO {
>  
>  				regulator-state-standby {
>  					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <3300000>;
>  					regulator-mode = <4>;
>  				};
>  
>  				regulator-state-mem {
> +					regulator-off-in-suspend;
>  					regulator-mode = <4>;
>  				};
>  			};
> @@ -128,11 +130,13 @@ vddioddr: VDD_DDR {
>  
>  				regulator-state-standby {
>  					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1350000>;
>  					regulator-mode = <4>;
>  				};
>  
>  				regulator-state-mem {
>  					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1350000>;
>  					regulator-mode = <4>;
>  				};
>  			};
> @@ -147,10 +151,12 @@ vddcore: VDD_CORE {
>  
>  				regulator-state-standby {
>  					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1150000>;
>  					regulator-mode = <4>;
>  				};
>  
>  				regulator-state-mem {
> +					regulator-off-in-suspend;

Is this correct? In the upstream kernel the SAM9X7 supports only standby
and ULP0 (see sam9x7_pm_init()). I don't think it is correct to turn off
the CPU while in ULP0.

Moreover, AFAICT, the MCP16502 will not transition out of active state
according to the FIGURE 4-2: Finite State Machine (FSM) States Diagram for
MCP16502AC from [1] and the schematics from Figure 3-6. Power Management
Integrated Circuit described in [2]. The LPM, HPM are tied to the ground
and SHDN is currently controlled (in the upstream kernel) only for backup
and self-refresh mode (see at91_backup_mode). Am I wrong?

Thank you,
Claudiu

[1]
https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP16502-Data-Sheet-DS20006275.pdf
[2]
https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/UserGuides/SAM9X75-Curiosity-User-Guide-DS60001859.pdf

>  					regulator-mode = <4>;
>  				};
>  			};
> @@ -166,10 +172,12 @@ dcdc4: VDD_OTHER {
>  
>  				regulator-state-standby {
>  					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1150000>;
>  					regulator-mode = <4>;
>  				};
>  
>  				regulator-state-mem {
> +					regulator-off-in-suspend;
>  					regulator-mode = <4>;
>  				};
>  			};
> @@ -182,6 +190,11 @@ vldo1: LDO1 {
>  
>  				regulator-state-standby {
>  					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <3300000>;
> +				};
> +
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
>  				};
>  			};
>  
> @@ -192,6 +205,11 @@ vldo2: LDO2 {
>  
>  				regulator-state-standby {
>  					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1800000>;
> +				};
> +
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
>  				};
>  			};
>  		};
> 
> base-commit: 37136bf5c3a6f6b686d74f41837a6406bec6b7bc