Message-ID: <9c49fd36-2b87-b25a-c799-c5845718def6@loongson.cn>
Date:   Fri, 19 May 2023 14:52:45 +0800
From:   zhuyinbo <zhuyinbo@...ngson.cn>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        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, zhuyinbo@...ngson.cn
Subject: Re: [PATCH v1 2/3] dt-bindings: soc: add loongson-2 pm



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


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.

Powered by blists - more mailing lists