Message-ID: <3f81816a-b7a8-cc3a-0052-a2177bfb58c4@linaro.org>
Date:   Thu, 18 May 2023 16:15:12 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     zhuyinbo <zhuyinbo@...ngson.cn>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Huacai Chen <chenhuacai@...nel.org>,
        WANG Xuerui <kernel@...0n.name>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Pavel Machek <pavel@....cz>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Marc Zyngier <maz@...nel.org>,
        Youling Tang <tangyouling@...ngson.cn>,
        Baoqi Zhang <zhangbaoqi@...ngson.cn>,
        Arnd Bergmann <arnd@...db.de>, Yun Liu <liuyun@...ngson.cn>,
        linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev
Cc:     Jianmin Lv <lvjianmin@...ngson.cn>, wanghongliang@...ngson.cn,
        Liu Peibao <liupeibao@...ngson.cn>,
        loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm

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@...ngson.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

Powered by blists - more mailing lists