Hi Krzysztof,
On Wed, Sep 21, 2022 at 10:11 AM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
>
> Hi Krzysztof,
>
> [cc: Hauke Mehrtens and Rafał Miłecki as maintainers for
> 'Documentation/devicetree/bindings/mips/brcm/']
>
> On Wed, Sep 21, 2022 at 9:51 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >
> > On 21/09/2022 09:18, Sergio Paracuellos wrote:
> > > Hi Krzysztof,
> > >
> > > On Wed, Sep 21, 2022 at 8:42 AM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > >>
> > >> On 20/09/2022 07:51, Sergio Paracuellos wrote:
> > >>> Hi Krzysztof,
> > >>>
> > >>> On Mon, Sep 19, 2022 at 6:08 PM Krzysztof Kozlowski
> > >>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > >>>>
> > >>>> On 19/09/2022 15:41, Sergio Paracuellos wrote:
> > >>>>> Hi Krzysztof,
> > >>>>>
> > >>>>> On Mon, Sep 19, 2022 at 2:48 PM Krzysztof Kozlowski
> > >>>>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > >>>>>>
> > >>>>>> On 19/09/2022 14:29, Sergio Paracuellos wrote:
> > >>>>>>>>
> > >>>>>>>> else mips-hpt-frequency: false
> > >>>>>>>>
> > >>>>>>>>> +
> > >>>>>>>>> +required:
> > >>>>>>>>> + - compatible
> > >>>>>>>>> +
> > >>>>>>>>> +additionalProperties: true
> > >>>>>>>>
> > >>>>>>>> and this is why you did not notice errors...
> > >>>>>>>
> > >>>>>>> Current arch/mips/boot/dts folder dts files are a mess for cpu nodes,
> > >>>>>>> so I set additionalProperties to true and only make required for
> > >>>>>>> 'compatible'. What should be the correct approach?
> > >>>>>>
> > >>>>>> This is okay, but it caused you did not notice errors...
> > >>>>>>
> > >>>>>>>
> > >>>>>>>>
> > >>>>>>>>> +
> > >>>>>>>>> +examples:
> > >>>>>>>>> + - |
> > >>>>>>>>> + cpus {
> > >>>>>>>>> + #size-cells = <0>;
> > >>>>>>>>> + #address-cells = <1>;
> > >>>>>>>>> +
> > >>>>>>>>> + cpu@0 {
> > >>>>>>>>> + device_type = "cpu";
> > >>>>>>>>> + compatible = "mips,mips1004Kc";
> > >>>>>>>>> + reg = <0>;
> > >>>>>>>>> + };
> > >>>>>>>>> +
> > >>>>>>>>> + cpu@1 {
> > >>>>>>>>> + device_type = "cpu";
> > >>>>>>>>> + compatible = "mips,mips1004Kc";
> > >>>>>>>>> + reg = <1>;
> > >>>>>>>>> + };
> > >>>>>>>>> + };
> > >>>>>>>>> +
> > >>>>>>>>> + - |
> > >>>>>>>>> + // Example 2 (BMIPS CPU)
> > >>>>>>>>> + cpus {
> > >>>>>>>>> + #address-cells = <1>;
> > >>>>>>>>> + #size-cells = <0>;
> > >>>>>>>>> +
> > >>>>>>>>> + mips-hpt-frequency = <150000000>;
> > >>>>>>>>
> > >>>>>>>> Does not match your bindings. Are you sure you tested the patches?
> > >>>>>>>
> > >>>>>>> Yes I did:
> > >>>>>>>
> > >>>>>>> $ make dt_binding_check
> > >>>>>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mips/cpus.yaml
> > >>>>>>> LINT Documentation/devicetree/bindings
> > >>>>>>> CHKDT Documentation/devicetree/bindings/processed-schema.json
> > >>>>>>> SCHEMA Documentation/devicetree/bindings/processed-schema.json
> > >>>>>>> DTEX Documentation/devicetree/bindings/mips/cpus.example.dts
> > >>>>>>> DTC Documentation/devicetree/bindings/mips/cpus.example.dtb
> > >>>>>>> ' CHECK Documentation/devicetree/bindings/mips/cpus.example.dtb
> > >>>>>>>
> > >>>>>>> Can you please point me to a sample of how to make required in a
> > >>>>>>> parent node of cpu@X property 'mips-hpt-frequency' only for some
> > >>>>>>> compatible strings inside the node? What can this be properly
> > >>>>>>> expressed using schema??
> > >>>>>>> I was looking and testing different things for a while without success at all.
> > >>>>>>
> > >>>>>> You either define new schema for /cpus node (and match by name, define
> > >>>>>> children etc) or include it in schema for top-level properties. The
> > >>>>>> first is tricky, because the cpus node does not have compatible (like
> > >>>>>> nvidia,tegra194-ccplex.yaml).
> > >>>
> > >>> Ok so if I am understanding correctly having two schemas is a way to go:
> > >>>
> > >>> One for brcm,bmips-cpus.yaml (since there is no compatible, should
> > >>> this be a valid name for this?) containing something like:
> > >>>
> > >>> properties:
> > >>> $nodename:
> > >>> const: cpus
> > >>>
> > >>> mips-hpt-frequency:
> > >>> $ref: /schemas/types.yaml#/definitions/uint32
> > >>> description: |
> > >>> This is common to all CPUs in the system so it lives
> > >>> under the "cpus" node.
> > >>>
> > >>> additionalProperties: true
> > >>
> > >> Almost. Such schema will allow mips-hpt-frequency in each cpus node,
> > >> everywhere. On every board and architecture.
> > >
> > > Yes, that is what I thought since no compatible to match this is
> > > included in current node.
> > >
> > >>
> > >> You need to limit it per top-level compatibles.
> > >
> > > Any sample of how to do this? So this bmips SoCs use compatible
> > > strings that are described in:
> > > https://elixir.bootlin.com/linux/v6.0-rc5/source/Documentation/devicetree/bindings/mips/brcm/soc.txt
> >
> > Could be something like this:
> > https://lore.kernel.org/all/20220830065744.161163-2-krzysztof.kozlowski@xxxxxxxxxx/
> > which is a part of top-level schema or add a new one. The new one will
> > duplicate the compatibles and parts from that one there, so maybe better
> > to keep it in top-level?
> >
> > I am not sure, any suggestions are welcome. Also platform/architecture
> > maintainers might have their preference to organize it.
>
> I am also not sure.
>
> >
> > Anyway, you did not Cc the actual platform maintainers (Rafał and Hauke).
>
> True, sorry for the inconvenience. Added now to CC list.
>
> >
> > >
> > > Can the top level compatible string be used in some way to filter this
> > > easily from this new 'brcm,bmips-cpus.yaml'
> >
> > Yes. If schema matches the top level compatible, then in allOf:if:then
> > you can add restriction to disallow it for other variants:
> >
> > For example:
> > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212
> >
> > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml#L152
>
> Thanks for the clue.
>
> I'll try to do some tests and come back here later :)
How about this?
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/mips/brcm/brcm,bmips-cpus.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
title: BMIPS CPUs bindings
maintainers:
- Hauke Mehrtens <hauke@xxxxxxxxxx>
- Rafał Miłecki <zajec5@xxxxxxxxx>
description: |
The device tree allows to describe the layout of BMIPS CPUs.
properties:
$nodename:
const: "/"
compatible:
enum:
- "brcm,bcm3368"
- "brcm,bcm3384"
- "brcm,bcm33843"
- "brcm,bcm3384-viper"
- "brcm,bcm33843-viper"
- "brcm,bcm6328"
- "brcm,bcm6358"
- "brcm,bcm6362"
- "brcm,bcm6368"
- "brcm,bcm63168"
- "brcm,bcm63268"
- "brcm,bcm7125"
- "brcm,bcm7346"
- "brcm,bcm7358"
- "brcm,bcm7360"
- "brcm,bcm7362"
- "brcm,bcm7420"
- "brcm,bcm7425"
cpus:
type: object
additionalProperties: true
properties:
'#address-cells':
const: 1
'#size-cells':
const: 0
mips-hpt-frequency:
description: This is common to all CPUs in the system so it lives
under the "cpus" node.
$ref: /schemas/types.yaml#/definitions/uint32
required:
- '#address-cells'
- '#size-cells'
allOf:
- if:
properties:
compatible:
contains:
enum:
- "brcm,bcm3368"
- "brcm,bcm3384"
- "brcm,bcm33843"
- "brcm,bcm3384-viper"
- "brcm,bcm33843-viper"
- "brcm,bcm6328"
- "brcm,bcm6358"
- "brcm,bcm6362"
- "brcm,bcm6368"
- "brcm,bcm63168"
- "brcm,bcm63268"
- "brcm,bcm7125"
- "brcm,bcm7346"
- "brcm,bcm7358"
- "brcm,bcm7360"
- "brcm,bcm7362"
- "brcm,bcm7420"
- "brcm,bcm7425"
then:
required:
- mips-hpt-frequency
additionalProperties: true
examples:
- |
/ {
#address-cells = <1>;
#size-cells = <1>;
compatible = "brcm,bcm3368";
cpus {
#address-cells = <1>;
#size-cells = <0>;
mips-hpt-frequency = <150000000>;
cpu@0 {
compatible = "brcm,bmips4350";
device_type = "cpu";
reg = <0>;
};
cpu@1 {
compatible = "brcm,bmips4350";
device_type = "cpu";
reg = <1>;
};
};
};
This seems to work as expected with this node:
make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.yaml
LINT Documentation/devicetree/bindings
CHKDT Documentation/devicetree/bindings/processed-schema.json
SCHEMA Documentation/devicetree/bindings/processed-schema.json
DTEX Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dts
DTC Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb
CHECK Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb
/home/sergio/GNUBEE-SERGIO-TEST/linux/Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb:
/: 'model' is a required property
>From schema: /home/sergio/.local/lib/python3.8/site-packages/dtschema/schemas/root-node.yaml
If I remove the property from the CPU nodes I get:
make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.yaml
LINT Documentation/devicetree/bindings
CHKDT Documentation/devicetree/bindings/processed-schema.json
SCHEMA Documentation/devicetree/bindings/processed-schema.json
DTEX Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dts
DTC Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb
CHECK Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb
/home/sergio/GNUBEE-SERGIO-TEST/linux/Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb:
/: cpus: 'mips-hpt-frequency' is a required property
>From schema: /home/sergio/GNUBEE-SERGIO-TEST/linux/Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.yaml
/home/sergio/GNUBEE-SERGIO-TEST/linux/Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb:
/: 'model' is a required property
>From schema: /home/sergio/.local/lib/python3.8/site-packages/dtschema/schemas/root-node.yaml
And if I change the top level compatible it does not complain also:
make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.yaml
LINT Documentation/devicetree/bindings
CHKDT Documentation/devicetree/bindings/processed-schema.json
SCHEMA Documentation/devicetree/bindings/processed-schema.json
DTEX Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dts
DTC Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb
CHECK Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb
/home/sergio/GNUBEE-SERGIO-TEST/linux/Documentation/devicetree/bindings/mips/brcm/brcm,bmips-cpus.example.dtb:
/: 'model' is a required property
However, the root-node schema requires 'model' as property and there
is no model at all in any real DTS file. I don't know if can be added
only in this sample to avoid the check fail.
Thanks,
Sergio Paracuellos
>
> Thanks,
> Sergio Paracuellos
> >
> >
> > Best regards,
> > Krzysztof
