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