Re: [PATCH 1/6] dt-bindings: loongarch: Add CPU bindings for LoongArch

[Binbin Zhou <zhoubb.aaron@xxxxxxxxx>]

On Fri, Jun 16, 2023 at 5:51 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 16/06/2023 08:10, Binbin Zhou wrote:
> > Add the available CPUs in LoongArch binding with DT schema format using
> > json-schema.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/loongarch/cpus.yaml   | 65 +++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/loongarch/cpus.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/loongarch/cpus.yaml b/Documentation/devicetree/bindings/loongarch/cpus.yaml
> > new file mode 100644
> > index 000000000000..c3e2dba42c81
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/loongarch/cpus.yaml
> > @@ -0,0 +1,65 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/loongarch/cpus.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LoongArch CPUs
> > +
> > +maintainers:
> > +  - Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > +
> > +description:
> > +  The device tree allows to describe the layout of CPUs in a system through
> > +  the "cpus" node, which in turn contains a number of subnodes (ie "cpu")
> > +  defining properties for every CPU.
>
> I understand you copied it from ARM, but I would prefer to have here
> something meaningful. Bindings description does not explain what is DTS,
> but explains what the hardware is.

Hi Krzysztof:

I am very sorry, this is my problem and I will rewrite this part.

>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - loongson,la264
> > +      - loongson,la364
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  device_type: true
> > +
> > +  clock-frequency:
> > +    description: The frequency of cpu in Hz.
> > +
> > +  model:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: User-visible cpu name in /proc/cpuinfo.
>
> First, aren't you mixing nodes?
> Second, it is derived from compatible, so no need for such property.

Well, this attribute is an attempt to tweak it.

As the description says, this attribute was added to show the model
name in /proc/cpuinfo. here, we will show the custom name instead of
using the cpu core name directly.

For example, on a Loongson-3A5000 machine, although its cpu core is
la464, we can see:
[root@fedora ~]# cat /proc/cpuinfo
system type : generic-loongson-machine
..............
Model Name : Loongson-3A5000-HV
............
CPU MHz : 2500.00
...........

Unfortunately, some Loongson-2K chips are not designed with
corresponding CPUNAME registers, so we expect to add them in the DTS.

At first, we considered writing it directly into cpu compatible, but
it seems that dts compatible is all lower case, while our desired
model name contains upper case letters.

What do you think if we repositioned this attribute under cpu?

                cpu0: cpu@0 {
                        compatible = "loongson,la264".
                        model = "Loongson-2K1000".
                        device_type = "cpu".
                        reg= <0x0>.
                        .....
                }.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clock-frequency
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    cpus {
> > +        #size-cells = <0>;
> > +        #address-cells = <1>;
> > +
> > +        model = "Loongson-2K1000";
>
> Drop, not related.
>
> Best regards,
> Krzysztof
>