[PATCH v1 0/3] soc: loongson2_pm: add power management support

[PATCH v1 0/3] soc: loongson2_pm: add power management support

[Yinbo Zhu]

Loongson-2 platform support Power Management Controller (ACPI) and this
series patch was to add PM driver that base on dts and PM binding support.

Yinbo Zhu (3):
  loongarch: export loongarch pm interface
  dt-bindings: soc: add loongson-2 pm
  soc: loongson2_pm: add power management support

 .../soc/loongson/loongson,ls2k-pmc.yaml       |  47 ++++
 MAINTAINERS                                   |   7 +
 arch/loongarch/include/asm/acpi.h             |   5 +-
 arch/loongarch/include/asm/suspend.h          |  10 +
 arch/loongarch/power/suspend.c                |  10 +-
 arch/loongarch/power/suspend_asm.S            |   8 +-
 drivers/soc/loongson/Kconfig                  |  10 +
 drivers/soc/loongson/Makefile                 |   1 +
 drivers/soc/loongson/loongson2_pm.c           | 237 ++++++++++++++++++
 9 files changed, 323 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH v1 1/3] loongarch: export loongarch pm interface

[Yinbo Zhu]

Some Power Management Controllers need to support DTS and will use
the suspend interface thus this patch was to export such interface
for their use.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 arch/loongarch/include/asm/acpi.h    |  5 ++---
 arch/loongarch/include/asm/suspend.h | 10 ++++++++++
 arch/loongarch/power/suspend.c       | 10 +++++-----
 arch/loongarch/power/suspend_asm.S   |  8 ++++----
 4 files changed, 21 insertions(+), 12 deletions(-)
 create mode 100644 arch/loongarch/include/asm/suspend.h

diff --git a/arch/loongarch/include/asm/acpi.h b/arch/loongarch/include/asm/acpi.h
index 976a810352c6..d63507cc705f 100644
--- a/arch/loongarch/include/asm/acpi.h
+++ b/arch/loongarch/include/asm/acpi.h
@@ -8,6 +8,7 @@
 #ifndef _ASM_LOONGARCH_ACPI_H
 #define _ASM_LOONGARCH_ACPI_H
 
+#include <asm/suspend.h>
 #ifdef CONFIG_ACPI
 extern int acpi_strict;
 extern int acpi_disabled;
@@ -37,13 +38,11 @@ extern struct list_head acpi_wakeup_device_list;
 
 extern int loongarch_acpi_suspend(void);
 extern int (*acpi_suspend_lowlevel)(void);
-extern void loongarch_suspend_enter(void);
 
 static inline unsigned long acpi_get_wakeup_address(void)
 {
 #ifdef CONFIG_SUSPEND
-	extern void loongarch_wakeup_start(void);
-	return (unsigned long)loongarch_wakeup_start;
+	return (unsigned long)loongson_wakeup_start;
 #endif
 	return 0UL;
 }
diff --git a/arch/loongarch/include/asm/suspend.h b/arch/loongarch/include/asm/suspend.h
new file mode 100644
index 000000000000..a40b42f4f7f3
--- /dev/null
+++ b/arch/loongarch/include/asm/suspend.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_SUSPEND_H
+#define __ASM_SUSPEND_H
+
+void loongson_common_resume(void);
+void loongson_common_suspend(void);
+void loongson_suspend_enter(void);
+void loongson_wakeup_start(void);
+
+#endif
diff --git a/arch/loongarch/power/suspend.c b/arch/loongarch/power/suspend.c
index 5e19733e5e05..0587681b33ce 100644
--- a/arch/loongarch/power/suspend.c
+++ b/arch/loongarch/power/suspend.c
@@ -27,7 +27,7 @@ struct saved_registers {
 };
 static struct saved_registers saved_regs;
 
-static void arch_common_suspend(void)
+void loongson_common_suspend(void)
 {
 	save_counter();
 	saved_regs.pgd = csr_read64(LOONGARCH_CSR_PGDL);
@@ -40,7 +40,7 @@ static void arch_common_suspend(void)
 	loongarch_suspend_addr = loongson_sysconf.suspend_addr;
 }
 
-static void arch_common_resume(void)
+void loongson_common_resume(void)
 {
 	sync_counter();
 	local_flush_tlb_all();
@@ -62,12 +62,12 @@ int loongarch_acpi_suspend(void)
 	enable_gpe_wakeup();
 	enable_pci_wakeup();
 
-	arch_common_suspend();
+	loongson_common_suspend();
 
 	/* processor specific suspend */
-	loongarch_suspend_enter();
+	loongson_suspend_enter();
 
-	arch_common_resume();
+	loongson_common_resume();
 
 	return 0;
 }
diff --git a/arch/loongarch/power/suspend_asm.S b/arch/loongarch/power/suspend_asm.S
index e2fc3b4e31f0..809abd3b119d 100644
--- a/arch/loongarch/power/suspend_asm.S
+++ b/arch/loongarch/power/suspend_asm.S
@@ -57,13 +57,13 @@
 	.align 12
 
 /* Sleep/wakeup code for Loongson-3 */
-SYM_FUNC_START(loongarch_suspend_enter)
+SYM_FUNC_START(loongson_suspend_enter)
 	SETUP_SLEEP
 	bl		__flush_cache_all
 
 	/* Pass RA and SP to BIOS */
 	addi.d		a1, sp, 0
-	la.pcrel	a0, loongarch_wakeup_start
+	la.pcrel	a0, loongson_wakeup_start
 	la.pcrel	t0, loongarch_suspend_addr
 	ld.d		t0, t0, 0
 	jirl		a0, t0, 0 /* Call BIOS's STR sleep routine */
@@ -72,7 +72,7 @@ SYM_FUNC_START(loongarch_suspend_enter)
 	 * This is where we return upon wakeup.
 	 * Reload all of the registers and return.
 	 */
-SYM_INNER_LABEL(loongarch_wakeup_start, SYM_L_GLOBAL)
+SYM_INNER_LABEL(loongson_wakeup_start, SYM_L_GLOBAL)
 	li.d		t0, CSR_DMW0_INIT	# UC, PLV0
 	csrwr		t0, LOONGARCH_CSR_DMWIN0
 	li.d		t0, CSR_DMW1_INIT	# CA, PLV0
@@ -89,4 +89,4 @@ SYM_INNER_LABEL(loongarch_wakeup_start, SYM_L_GLOBAL)
 	SETUP_WAKEUP
 	addi.d		sp, sp, PT_SIZE
 	jr		ra
-SYM_FUNC_END(loongarch_suspend_enter)
+SYM_FUNC_END(loongson_suspend_enter)
-- 
2.20.1

[PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

[Yinbo Zhu]

Add the Loongson-2 SoC Power Management Controller binding with DT
schema format using json-schema.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 .../soc/loongson/loongson,ls2k-pmc.yaml       | 47 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml

diff --git a/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
new file mode 100644
index 000000000000..a52bfa5e3eb6
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/loongson/loongson,ls2k-pmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson-2 Power Manager controller
+
+maintainers:
+  - Yinbo Zhu <zhuyinbo@loongson.cn>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - loongson,ls2k-pmc
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  suspend-address:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      This option indicate this PM suspend address.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    pmc: pm@1fe27000 {
+        compatible = "loongson,ls2k-pmc", "syscon";
+        reg = <0x1fe27000 0x58>;
+        interrupt-parent = <&liointc1>;
+        interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
+        suspend-address = <0x1c000500>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 7a91f14cad2e..bcd05f1fa5c1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12190,6 +12190,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml
 F:	drivers/soc/loongson/loongson2_guts.c
 
+LOONGSON-2 SOC SERIES PM DRIVER
+M:	Yinbo Zhu <zhuyinbo@loongson.cn>
+L:	linux-pm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
+
 LOONGSON-2 SOC SERIES PINCTRL DRIVER
 M:	zhanghongchen <zhanghongchen@loongson.cn>
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
-- 
2.20.1

Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

[Krzysztof Kozlowski]

On 17/05/2023 09:31, Yinbo Zhu wrote:
> Add the Loongson-2 SoC Power Management Controller binding with DT
> schema format using json-schema.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>

...

> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - loongson,ls2k-pmc
> +      - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  suspend-address:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      This option indicate this PM suspend address.

This tells me nothing. Drop "This option indicate this" and rephrase
everything to actually describe this property. Why would the address
differ on given, specific SoC? It looks like you just miss compatibles.
Anyway this needs much more explanation so we can judge whether it fits DT.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    pmc: pm@1fe27000 {
> +        compatible = "loongson,ls2k-pmc", "syscon";
> +        reg = <0x1fe27000 0x58>;
> +        interrupt-parent = <&liointc1>;
> +        interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> +        suspend-address = <0x1c000500>;


Best regards,
Krzysztof

Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

[zhuyinbo]

在 2023/5/17 下午11:00, Krzysztof Kozlowski 写道:
> On 17/05/2023 09:31, Yinbo Zhu wrote:
>> Add the Loongson-2 SoC Power Management Controller binding with DT
>> schema format using json-schema.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> 
> ...
> 
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - loongson,ls2k-pmc
>> +      - const: syscon
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  suspend-address:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      This option indicate this PM suspend address.
> 
> This tells me nothing. Drop "This option indicate this" and rephrase
> everything to actually describe this property. Why would the address
> differ on given, specific SoC? It looks like you just miss compatibles.
> Anyway this needs much more explanation so we can judge whether it fits DT.

Hi Krzysztof,

I will add following description about "suspend-address", please review.

The "suspend-address" is a ACPI S3 (Suspend To RAM) firmware entry
address which was jumped from kernel and it's value was dependent on 
specific platform firmware code. In addition, the PM driver need 
according to it to indicate that current SoC whether support ACPI S3.

Thanks.

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    pmc: pm@1fe27000 {
>> +        compatible = "loongson,ls2k-pmc", "syscon";
>> +        reg = <0x1fe27000 0x58>;
>> +        interrupt-parent = <&liointc1>;
>> +        interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
>> +        suspend-address = <0x1c000500>;
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

[Krzysztof Kozlowski]

On 18/05/2023 05:23, zhuyinbo wrote:
> 
> 
> 在 2023/5/17 下午11:00, Krzysztof Kozlowski 写道:
>> On 17/05/2023 09:31, Yinbo Zhu wrote:
>>> Add the Loongson-2 SoC Power Management Controller binding with DT
>>> schema format using json-schema.
>>>
>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>
>> ...
>>
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - loongson,ls2k-pmc
>>> +      - const: syscon
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  suspend-address:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      This option indicate this PM suspend address.
>>
>> This tells me nothing. Drop "This option indicate this" and rephrase
>> everything to actually describe this property. Why would the address
>> differ on given, specific SoC? It looks like you just miss compatibles.
>> Anyway this needs much more explanation so we can judge whether it fits DT.
> 
> Hi Krzysztof,
> 
> I will add following description about "suspend-address", please review.

Thanks.

> 
> The "suspend-address" is a ACPI S3 (Suspend To RAM) firmware entry

Why do we add properties for ACPI? This does not seem right. Please
reword to skip ACPI stuff, e.g. deep sleep states (Suspend to RAM).


> address which was jumped from kernel and it's value was dependent on 
> specific platform firmware code. 

"entry address which was jumped" <- the address cannot jump. Please
explain who is jumping here - boot CPU? each suspended CPU? I guess the
first as CPUs are offlined, right?

> In addition, the PM driver need 
> according to it to indicate that current SoC whether support ACPI S3.

Skip references to driver.

> 

Best regards,
Krzysztof

Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

[zhuyinbo]

在 2023/5/18 下午3:15, Krzysztof Kozlowski 写道:
> On 18/05/2023 05:23, zhuyinbo wrote:
>>
>>
>> 在 2023/5/17 下午11:00, Krzysztof Kozlowski 写道:
>>> On 17/05/2023 09:31, Yinbo Zhu wrote:
>>>> Add the Loongson-2 SoC Power Management Controller binding with DT
>>>> schema format using json-schema.
>>>>
>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>
>>> ...
>>>
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - enum:
>>>> +          - loongson,ls2k-pmc
>>>> +      - const: syscon
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  suspend-address:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      This option indicate this PM suspend address.
>>>
>>> This tells me nothing. Drop "This option indicate this" and rephrase
>>> everything to actually describe this property. Why would the address
>>> differ on given, specific SoC? It looks like you just miss compatibles.
>>> Anyway this needs much more explanation so we can judge whether it fits DT.
>>
>> Hi Krzysztof,
>>
>> I will add following description about "suspend-address", please review.
> 
> Thanks.
> 
>>
>> The "suspend-address" is a ACPI S3 (Suspend To RAM) firmware entry
> 
> Why do we add properties for ACPI? This does not seem right. 


1.  The suspend-address value was dependent on specific platform
     firmware code and it tends to be confiurable. if it is a fixed value
     that seems not friendly or the ACPI S3 will not work.
2. the PM driver need according to it to indicate that current SoC
    whether support ACPI S3, because some Loongson-2 SoC doesn't support
    ACPI S3 but support other ACPI mode, so the PM driver need has a
    check. if no this check and other ACPI mode will not work.

Base on the above two points, this property was necessary.
Using this property "suspend-address" can make the firmware entry
address configurable, and then the kernel can also indicate whether
the current SoC supports S3

In addition, from kernel code perspective, the property
"suspend-address" was to initialize "loongarch_suspend_addr"

S3 call flow:
enter_state -> loongson_suspend_enter -> bios's loongarch_suspend_addr

SYM_FUNC_START(loongson_suspend_enter)
         SETUP_SLEEP
         bl              __flush_cache_all

         /* Pass RA and SP to BIOS */
         addi.d          a1, sp, 0
         la.pcrel        a0, loongson_wakeup_start
         la.pcrel        t0, loongarch_suspend_addr
         ld.d            t0, t0, 0
         jirl            a0, t0, 0 /* Call BIOS's STR sleep routine */


Please
> reword to skip ACPI stuff, e.g. deep sleep states (Suspend to RAM).


Sorry, I don't got your point.

> 
> 
>> address which was jumped from kernel and it's value was dependent on
>> specific platform firmware code.
> 
> "entry address which was jumped" <- the address cannot jump. Please
> explain who is jumping here - boot CPU? each suspended CPU? I guess the
> first as CPUs are offlined, right?

The boot CPU was jumping to firmware and finish remaining process in
firmware that was what ACPI S3 required and other CPUs (No-boot CPU)
have been offline before entering firmware.

> 
>> In addition, the PM driver need
>> according to it to indicate that current SoC whether support ACPI S3.
> 
> Skip references to driver.


Sorry, I don't got your point.  Could you elaborate on it?

Thanks
Yinbo.

Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

[Krzysztof Kozlowski]

On 18/05/2023 14:15, zhuyinbo wrote:
> 
> 
> 在 2023/5/18 下午3:15, Krzysztof Kozlowski 写道:
>> On 18/05/2023 05:23, zhuyinbo wrote:
>>>
>>>
>>> 在 2023/5/17 下午11:00, Krzysztof Kozlowski 写道:
>>>> On 17/05/2023 09:31, Yinbo Zhu wrote:
>>>>> Add the Loongson-2 SoC Power Management Controller binding with DT
>>>>> schema format using json-schema.
>>>>>
>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>
>>>> ...
>>>>
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - loongson,ls2k-pmc
>>>>> +      - const: syscon
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  suspend-address:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description:
>>>>> +      This option indicate this PM suspend address.
>>>>
>>>> This tells me nothing. Drop "This option indicate this" and rephrase
>>>> everything to actually describe this property. Why would the address
>>>> differ on given, specific SoC? It looks like you just miss compatibles.
>>>> Anyway this needs much more explanation so we can judge whether it fits DT.
>>>
>>> Hi Krzysztof,
>>>
>>> I will add following description about "suspend-address", please review.
>>
>> Thanks.
>>
>>>
>>> The "suspend-address" is a ACPI S3 (Suspend To RAM) firmware entry
>>
>> Why do we add properties for ACPI? This does not seem right. 
> 
> 
> 1.  The suspend-address value was dependent on specific platform
>      firmware code and it tends to be confiurable. if it is a fixed value
>      that seems not friendly or the ACPI S3 will not work.

> 2. the PM driver need according to it to indicate that current SoC
>     whether support ACPI S3, because some Loongson-2 SoC doesn't support

For this you have dedicated compatibles. Which points to the fact that
you missed them here.

>     ACPI S3 but support other ACPI mode, so the PM driver need has a
>     check. if no this check and other ACPI mode will not work.

Sure, but it is not really relevant to the bindings... or rather: should
not be relevant. Bindings are for hardware or in this case also for
firmware, but not for driver.

> 
> Base on the above two points, this property was necessary.

I did not object in my last response...

> Using this property "suspend-address" can make the firmware entry
> address configurable, and then the kernel can also indicate whether
> the current SoC supports S3
> 
> In addition, from kernel code perspective, the property
> "suspend-address" was to initialize "loongarch_suspend_addr"

Again, how does it matter what kernel does?

> 
> S3 call flow:
> enter_state -> loongson_suspend_enter -> bios's loongarch_suspend_addr
> 
> SYM_FUNC_START(loongson_suspend_enter)
>          SETUP_SLEEP
>          bl              __flush_cache_all
> 
>          /* Pass RA and SP to BIOS */
>          addi.d          a1, sp, 0
>          la.pcrel        a0, loongson_wakeup_start
>          la.pcrel        t0, loongarch_suspend_addr
>          ld.d            t0, t0, 0
>          jirl            a0, t0, 0 /* Call BIOS's STR sleep routine */
> 
> 
> Please
>> reword to skip ACPI stuff, e.g. deep sleep states (Suspend to RAM).
> 
> 
> Sorry, I don't got your point.

You have DT platform, so why do you use it with ACPI in the first place?
If you have ACPI, then please drop all this and make your life easier.

If this is booted without ACPI, which would justify DT, drop the
references to ACPI. I gave you example what to use instead. If you don't
like it, no problem, reword in different way.

> 
>>
>>
>>> address which was jumped from kernel and it's value was dependent on
>>> specific platform firmware code.
>>
>> "entry address which was jumped" <- the address cannot jump. Please
>> explain who is jumping here - boot CPU? each suspended CPU? I guess the
>> first as CPUs are offlined, right?
> 
> The boot CPU was jumping to firmware and finish remaining process in
> firmware that was what ACPI S3 required and other CPUs (No-boot CPU)
> have been offline before entering firmware.

Then fix the description.

> 
>>
>>> In addition, the PM driver need
>>> according to it to indicate that current SoC whether support ACPI S3.
>>
>> Skip references to driver.
> 
> 
> Sorry, I don't got your point.  Could you elaborate on it?

If you change driver, you change bindings? No.

Bindings are for hardware, not for driver. Whatever your driver is doing
usually does not matter for the bindings and should not be included.

Best regards,
Krzysztof

Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

[zhuyinbo]

在 2023/5/18 下午10:15, Krzysztof Kozlowski 写道:
> On 18/05/2023 14:15, zhuyinbo wrote:
>>
>>
>> 在 2023/5/18 下午3:15, Krzysztof Kozlowski 写道:
>>> On 18/05/2023 05:23, zhuyinbo wrote:
>>>>
>>>>
>>>> 在 2023/5/17 下午11:00, Krzysztof Kozlowski 写道:
>>>>> On 17/05/2023 09:31, Yinbo Zhu wrote:
>>>>>> Add the Loongson-2 SoC Power Management Controller binding with DT
>>>>>> schema format using json-schema.
>>>>>>
>>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>>
>>>>> ...
>>>>>
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    items:
>>>>>> +      - enum:
>>>>>> +          - loongson,ls2k-pmc
>>>>>> +      - const: syscon
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  interrupts:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  suspend-address:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    description:
>>>>>> +      This option indicate this PM suspend address.
>>>>>
>>>>> This tells me nothing. Drop "This option indicate this" and rephrase
>>>>> everything to actually describe this property. Why would the address
>>>>> differ on given, specific SoC? It looks like you just miss compatibles.
>>>>> Anyway this needs much more explanation so we can judge whether it fits DT.
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> I will add following description about "suspend-address", please review.
>>>
>>> Thanks.
>>>
>>>>
>>>> The "suspend-address" is a ACPI S3 (Suspend To RAM) firmware entry
>>>
>>> Why do we add properties for ACPI? This does not seem right.
>>
>>
>> 1.  The suspend-address value was dependent on specific platform
>>       firmware code and it tends to be confiurable. if it is a fixed value
>>       that seems not friendly or the ACPI S3 will not work.
> 
>> 2. the PM driver need according to it to indicate that current SoC
>>      whether support ACPI S3, because some Loongson-2 SoC doesn't support
> 
> For this you have dedicated compatibles. Which points to the fact that
> you missed them here.


Sorry, I may not have explained it clearly before. In fact, this is a
consideration for the future, and currently all SoC supports s3.
Add corresponding compatibles as needed in the future.

> 
>>      ACPI S3 but support other ACPI mode, so the PM driver need has a
>>      check. if no this check and other ACPI mode will not work.
> 
> Sure, but it is not really relevant to the bindings... or rather: should
> not be relevant. Bindings are for hardware or in this case also for
> firmware, but not for driver.

okay, I got it.

> 
>>
>> Base on the above two points, this property was necessary.
> 
> I did not object in my last response...


Yes, but I misunderstood your meaning before.

> 
>> Using this property "suspend-address" can make the firmware entry
>> address configurable, and then the kernel can also indicate whether
>> the current SoC supports S3
>>
>> In addition, from kernel code perspective, the property
>> "suspend-address" was to initialize "loongarch_suspend_addr"
> 
> Again, how does it matter what kernel does?


okay, I got it.

> 
>>
>> S3 call flow:
>> enter_state -> loongson_suspend_enter -> bios's loongarch_suspend_addr
>>
>> SYM_FUNC_START(loongson_suspend_enter)
>>           SETUP_SLEEP
>>           bl              __flush_cache_all
>>
>>           /* Pass RA and SP to BIOS */
>>           addi.d          a1, sp, 0
>>           la.pcrel        a0, loongson_wakeup_start
>>           la.pcrel        t0, loongarch_suspend_addr
>>           ld.d            t0, t0, 0
>>           jirl            a0, t0, 0 /* Call BIOS's STR sleep routine */
>>
>>
>> Please
>>> reword to skip ACPI stuff, e.g. deep sleep states (Suspend to RAM).
>>
>>
>> Sorry, I don't got your point.
> 
> You have DT platform, so why do you use it with ACPI in the first place?
> If you have ACPI, then please drop all this and make your life easier.


okay, I got it, I will reword to skip ACPI stuff in bindings for 
"suspend-address" property description.

> 
> If this is booted without ACPI, which would justify DT, drop the
> references to ACPI. I gave you example what to use instead. If you don't
> like it, no problem, reword in different way.

okay, I got it.

> 
>>
>>>
>>>
>>>> address which was jumped from kernel and it's value was dependent on
>>>> specific platform firmware code.
>>>
>>> "entry address which was jumped" <- the address cannot jump. Please
>>> explain who is jumping here - boot CPU? each suspended CPU? I guess the
>>> first as CPUs are offlined, right?
>>
>> The boot CPU was jumping to firmware and finish remaining process in
>> firmware that was what ACPI S3 required and other CPUs (No-boot CPU)
>> have been offline before entering firmware.
> 
> Then fix the description.

okay, I got it.

> 
>>
>>>
>>>> In addition, the PM driver need
>>>> according to it to indicate that current SoC whether support ACPI S3.
>>>
>>> Skip references to driver.
>>
>>
>> Sorry, I don't got your point.  Could you elaborate on it?
> 
> If you change driver, you change bindings? No.
> 
> Bindings are for hardware, not for driver. Whatever your driver is doing
> usually does not matter for the bindings and should not be included.


okay, I got it. I will skip references to driver in bindings for 
"suspend-address" property description.

Thanks.