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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





在 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@xxxxxxxxxxx>

...

+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.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux