LinuxLists.cc - [PATCH v2] docs: dt-bindings: add DTS Coding Style document

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 11/20/23 09:40, Krzysztof Kozlowski wrote:
> Document preferred coding style for Devicetree sources (DTS and DTSI),
> to bring consistency among all (sub)architectures and ease in reviews.
>
> Cc: Andrew Davis <[email protected]>
> Cc: AngeloGioacchino Del Regno <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Heiko Stuebner <[email protected]>
> Cc: Konrad Dybcio <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Cc: Michal Simek <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Nishanth Menon <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>
> ---
>
> Merging idea: Rob/DT bindings
>
> Changes in v2
> =============
> 1. Hopefully incorporate entire feedback from comments:
> a. Fix \ { => / { (Rob)
> b. Name: dts-coding-style (Rob)
> c. Exceptions for ordering nodes by name for Renesas and pinctrl (Geert,
> Konrad)
> d. Ordering properties by common/vendor (Rob)
> e. Array entries in <> (Rob)
>
> 2. New chapter: Organizing DTSI and DTS
>
> 3. Several grammar fixes (missing articles)
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> .../devicetree/bindings/dts-coding-style.rst | 163 ++++++++++++++++++
> Documentation/devicetree/bindings/index.rst | 1 +
> 2 files changed, 164 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dts-coding-style.rst
>
> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst
> new file mode 100644
> index 000000000000..cc7e3b4d1b92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst
> @@ -0,0 +1,163 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. _dtscodingstyle:
> +
> +=====================================
> +Devicetree Sources (DTS) Coding Style
> +=====================================
> +
> +When writing Devicetree Sources (DTS) please observe below guidelines. They
> +should be considered complementary to any rules expressed already in Devicetree
> +Specification and dtc compiler (including W=1 and W=2 builds).
> +
> +Individual architectures and sub-architectures can add additional rules, making
> +the style stricter.
> +
> +Naming and Valid Characters
> +---------------------------
> +
> +1. Node and property names are allowed to use only:
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * dash: -

device-tree specification v0.4. Chapter 2.2.1/Table 2.1 is describing much more
valid characters for node names.
It means above description is not accurate or DT spec should be updated.

> +
> +2. Labels are allowed to use only:
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * underscore: _

based on dt spec uppercase is also valid char in label.

> +
> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
> +
> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
> + part can be padded with leading zeros.
> +
> +Example::
> +
> + gpi_dma2: dma-controller@800000 {
> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
> + reg = <0x0 0x00800000 0x0 0x60000>;

Is 0x0 recommended or 0 is enough?

> + }
> +
> +Order of Nodes
> +--------------
> +
> +1. Nodes within any bus, thus using unit addresses for children, shall be
> + ordered incrementally by unit address.
> + Alternatively for some sub-architectures, nodes of the same type can be
> + grouped together (e.g. all I2C controllers one after another even if this
> + breaks unit address ordering).
> +
> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
> + name. For a few types of nodes, they can be ordered by the main property
> + (e.g. pin configuration states ordered by value of "pins" property).
> +
> +3. When extending nodes in the board DTS via &label, the entries should be
> + ordered alpha-numerically.
> +
> +Example::
> +
> + // SoC DTSI

Same comment about /* */ as was mentioned in another thread.

> +
> + / {
> + cpus {
> + // ...
> + };
> +
> + psci {
> + // ...
> + };
> +
> + soc@ {
> + dma: dma-controller@10000 {
> + // ...
> + };
> +
> + clk: clock-controller@80000 {
> + // ...
> + };
> + };
> + };
> +
> + // Board DTS
> +
> + &clk {
> + // ...
> + };
> +
> + &dma {
> + // ...
> + };
> +
> +
> +Order of Properties in Device Node
> +----------------------------------
> +
> +Following order of properties in device nodes is preferred:
> +
> +1. compatible
> +2. reg
> +3. ranges
> +4. Standard/common properties (defined by common bindings, e.g. without
> + vendor-prefixes)
> +5. Vendor-specific properties
> +6. status (if applicable)
> +7. Child nodes, where each node is preceded with a blank line

Isn't the order already defined in DT spec in 2.3 in chapters?
compatible
model
status
#address/size cells
reg
virtual-reg
ranges
dma-ranges
dma-coherent
dma-non-coherent

Again I am fine with whatever order but I think we should reflect it in the spec
too. Especially status property is for my taste too low simply because you start
to read and then you will find that IP is disabled.

And are you describing all properties starting with # as standard properties?

> +
> +The "status" property is by default "okay", thus it can be omitted.
> +
> +Example::
> +
> + // SoC DTSI

/* */

> +
> + usb_1_hsphy: phy@88e3000 {
> + compatible = "qcom,sm8550-snps-eusb2-phy";
> + reg = <0x0 0x088e3000 0x0 0x154>;
> + #phy-cells = <0>;
> + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
> + status = "disabled";
> + };
> +
> + // Board DTS
> +
> + &usb_1_hsphy {
> + clocks = <&tcsr TCSR_USB2_CLKREF_EN>;
> + clock-names = "ref";
> + status = "okay";
> + };
> +
> +
> +Indentation
> +-----------
> +
> +1. Use indentation according to :ref:`codingstyle`.
> +2. For arrays spanning across lines, it is preferred to align the continued
> + entries with opening < from the first line.
> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses)
> + should be enclosed in <>.
> +
> +Example::
> +
> + thermal-sensor@c271000 {
> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2";
> + reg = <0x0 0x0c271000 0x0 0x1000>,
> + <0x0 0x0c222000 0x0 0x1000>;
> + };
> +
> +Organizing DTSI and DTS
> +-----------------------
> +
> +The DTSI and DTS files should be organized in a way representing the common
> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
> +and DTS files into several files:
> +
> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
> + on the SoC).
> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> + entire System-on-Module).

DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
there doesn't need to be DTS representing the board.

Thanks,
Michal

2023-11-20 14:54:56

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 20/11/2023 15:01, Michal Simek wrote:
>
>
> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>> to bring consistency among all (sub)architectures and ease in reviews.
>>
>> Cc: Andrew Davis <[email protected]>
>> Cc: AngeloGioacchino Del Regno <[email protected]>
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: Bjorn Andersson <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: Heiko Stuebner <[email protected]>
>> Cc: Konrad Dybcio <[email protected]>
>> Cc: Matthias Brugger <[email protected]>
>> Cc: Michal Simek <[email protected]>
>> Cc: Neil Armstrong <[email protected]>
>> Cc: Nishanth Menon <[email protected]>
>> Cc: Olof Johansson <[email protected]>
>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>
>> ---
>>
>> Merging idea: Rob/DT bindings
>>
>> Changes in v2
>> =============
>> 1. Hopefully incorporate entire feedback from comments:
>> a. Fix \ { => / { (Rob)
>> b. Name: dts-coding-style (Rob)
>> c. Exceptions for ordering nodes by name for Renesas and pinctrl (Geert,
>> Konrad)
>> d. Ordering properties by common/vendor (Rob)
>> e. Array entries in <> (Rob)
>>
>> 2. New chapter: Organizing DTSI and DTS
>>
>> 3. Several grammar fixes (missing articles)
>>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> .../devicetree/bindings/dts-coding-style.rst | 163 ++++++++++++++++++
>> Documentation/devicetree/bindings/index.rst | 1 +
>> 2 files changed, 164 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dts-coding-style.rst
>>
>> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst
>> new file mode 100644
>> index 000000000000..cc7e3b4d1b92
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst
>> @@ -0,0 +1,163 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +.. _dtscodingstyle:
>> +
>> +=====================================
>> +Devicetree Sources (DTS) Coding Style
>> +=====================================
>> +
>> +When writing Devicetree Sources (DTS) please observe below guidelines. They
>> +should be considered complementary to any rules expressed already in Devicetree
>> +Specification and dtc compiler (including W=1 and W=2 builds).
>> +
>> +Individual architectures and sub-architectures can add additional rules, making
>> +the style stricter.
>> +
>> +Naming and Valid Characters
>> +---------------------------
>> +
>> +1. Node and property names are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * dash: -
>
> device-tree specification v0.4. Chapter 2.2.1/Table 2.1 is describing much more
> valid characters for node names.
> It means above description is not accurate or DT spec should be updated.

Spec allows way to much. dtc doesn't. One thing is the spec, second
thing is coding style.

>
>
>> +
>> +2. Labels are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * underscore: _
>
> based on dt spec uppercase is also valid char in label.

Which we do not want in the DTS.

>
>
>> +
>> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
>> +
>> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
>> + part can be padded with leading zeros.
>> +
>> +Example::
>> +
>> + gpi_dma2: dma-controller@800000 {
>> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
>> + reg = <0x0 0x00800000 0x0 0x60000>;
>
> Is 0x0 recommended or 0 is enough?

I don't want to impose any rule on that, because someone would like to
argue that hex should be also in SPI chip-select reg.

>
>> + }
>> +
>> +Order of Nodes
>> +--------------
>> +
>> +1. Nodes within any bus, thus using unit addresses for children, shall be
>> + ordered incrementally by unit address.
>> + Alternatively for some sub-architectures, nodes of the same type can be
>> + grouped together (e.g. all I2C controllers one after another even if this
>> + breaks unit address ordering).
>> +
>> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
>> + name. For a few types of nodes, they can be ordered by the main property
>> + (e.g. pin configuration states ordered by value of "pins" property).
>> +
>> +3. When extending nodes in the board DTS via &label, the entries should be
>> + ordered alpha-numerically.
>> +
>> +Example::
>> +
>> + // SoC DTSI
>
> Same comment about /* */ as was mentioned in another thread.
>
>> +
>> + / {
>> + cpus {
>> + // ...
>> + };
>> +
>> + psci {
>> + // ...
>> + };
>> +
>> + soc@ {
>> + dma: dma-controller@10000 {
>> + // ...
>> + };
>> +
>> + clk: clock-controller@80000 {
>> + // ...
>> + };
>> + };
>> + };
>> +
>> + // Board DTS
>> +
>> + &clk {
>> + // ...
>> + };
>> +
>> + &dma {
>> + // ...
>> + };
>> +
>> +
>> +Order of Properties in Device Node
>> +----------------------------------
>> +
>> +Following order of properties in device nodes is preferred:
>> +
>> +1. compatible
>> +2. reg
>> +3. ranges
>> +4. Standard/common properties (defined by common bindings, e.g. without
>> + vendor-prefixes)
>> +5. Vendor-specific properties
>> +6. status (if applicable)
>> +7. Child nodes, where each node is preceded with a blank line
>
> Isn't the order already defined in DT spec in 2.3 in chapters?

Where is it defined as this is preferred order?

> compatible
> model
> status
> #address/size cells
> reg
> virtual-reg
> ranges
> dma-ranges
> dma-coherent
> dma-non-coherent
>
> Again I am fine with whatever order but I think we should reflect it in the spec

Spec is not a coding style.

> too. Especially status property is for my taste too low simply because you start
> to read and then you will find that IP is disabled.

Which is exactly what you want. status is irrelevant for hardware
description, so should be the last item.

>
> And are you describing all properties starting with # as standard properties?

Yes.

>
>
>> +
>> +The "status" property is by default "okay", thus it can be omitted.
>> +
>> +Example::
>> +
>> + // SoC DTSI
>
>
> /* */
>
>> +
>> + usb_1_hsphy: phy@88e3000 {
>> + compatible = "qcom,sm8550-snps-eusb2-phy";
>> + reg = <0x0 0x088e3000 0x0 0x154>;
>> + #phy-cells = <0>;
>> + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
>> + status = "disabled";
>> + };
>> +
>> + // Board DTS
>> +
>> + &usb_1_hsphy {
>> + clocks = <&tcsr TCSR_USB2_CLKREF_EN>;
>> + clock-names = "ref";
>> + status = "okay";
>> + };
>> +
>> +
>> +Indentation
>> +-----------
>> +
>> +1. Use indentation according to :ref:`codingstyle`.
>> +2. For arrays spanning across lines, it is preferred to align the continued
>> + entries with opening < from the first line.
>> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses)
>> + should be enclosed in <>.
>> +
>> +Example::
>> +
>> + thermal-sensor@c271000 {
>> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2";
>> + reg = <0x0 0x0c271000 0x0 0x1000>,
>> + <0x0 0x0c222000 0x0 0x1000>;
>> + };
>> +
>> +Organizing DTSI and DTS
>> +-----------------------
>> +
>> +The DTSI and DTS files should be organized in a way representing the common
>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>> +and DTS files into several files:
>> +
>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
>> + on the SoC).
>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>> + entire System-on-Module).
>
> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
> there doesn't need to be DTS representing the board.

I have never seen a SoM which can run without elaborate hardware-hacking
(e.g. connecting multiple wires to the SoM pins). The definition of the
SoM is that it is a module. Module can be re-used, just like SoC.

Best regards,
Krzysztof

2023-11-20 14:58:40

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 20/11/2023 12:43, AngeloGioacchino Del Regno wrote:
> Il 20/11/23 09:40, Krzysztof Kozlowski ha scritto:
>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>> to bring consistency among all (sub)architectures and ease in reviews.
>>
>> Cc: Andrew Davis <[email protected]>
>> Cc: AngeloGioacchino Del Regno <[email protected]>
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: Bjorn Andersson <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: Heiko Stuebner <[email protected]>
>> Cc: Konrad Dybcio <[email protected]>
>> Cc: Matthias Brugger <[email protected]>
>> Cc: Michal Simek <[email protected]>
>> Cc: Neil Armstrong <[email protected]>
>> Cc: Nishanth Menon <[email protected]>
>> Cc: Olof Johansson <[email protected]>
>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>
>> ---
>>
>> Merging idea: Rob/DT bindings
>>
>> Changes in v2
>> =============
>> 1. Hopefully incorporate entire feedback from comments:
>> a. Fix \ { => / { (Rob)
>> b. Name: dts-coding-style (Rob)
>> c. Exceptions for ordering nodes by name for Renesas and pinctrl (Geert,
>> Konrad)
>> d. Ordering properties by common/vendor (Rob)
>> e. Array entries in <> (Rob)
>>
>> 2. New chapter: Organizing DTSI and DTS
>>
>> 3. Several grammar fixes (missing articles)
>>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> .../devicetree/bindings/dts-coding-style.rst | 163 ++++++++++++++++++
>> Documentation/devicetree/bindings/index.rst | 1 +
>> 2 files changed, 164 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dts-coding-style.rst
>>
>> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst
>> new file mode 100644
>> index 000000000000..cc7e3b4d1b92
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst
>> @@ -0,0 +1,163 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +.. _dtscodingstyle:
>> +
>> +=====================================
>> +Devicetree Sources (DTS) Coding Style
>> +=====================================
>> +
>> +When writing Devicetree Sources (DTS) please observe below guidelines. They
>> +should be considered complementary to any rules expressed already in Devicetree
>> +Specification and dtc compiler (including W=1 and W=2 builds).
>> +
>> +Individual architectures and sub-architectures can add additional rules, making
>> +the style stricter.
>> +
>> +Naming and Valid Characters
>> +---------------------------
>> +
>> +1. Node and property names are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * dash: -
>> +
>> +2. Labels are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * underscore: _
>> +
>> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
>
> This is imperative, so: s/should/shall/g

Sure, fine.

>
>> +
>> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
>> + part can be padded with leading zeros.
>> +
>
> Same here, I'd say.... :-)
>
>> +Example::
>> +
>> + gpi_dma2: dma-controller@800000 {
>> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
>> + reg = <0x0 0x00800000 0x0 0x60000>;
>> + }
>> +
>> +Order of Nodes
>> +--------------
>> +
>> +1. Nodes within any bus, thus using unit addresses for children, shall be
>> + ordered incrementally by unit address.
>> + Alternatively for some sub-architectures, nodes of the same type can be
>> + grouped together (e.g. all I2C controllers one after another even if this
>> + breaks unit address ordering).
>> +
>> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
>> + name. For a few types of nodes, they can be ordered by the main property
>> + (e.g. pin configuration states ordered by value of "pins" property).
>> +
>> +3. When extending nodes in the board DTS via &label, the entries should be
>> + ordered alpha-numerically.
>> +
>> +Example::
>> +
>
> Hmm, comments!
>
>> + // SoC DTSI
>
> ....speaking of commenting, should we at least suggest to use C-style comments?
>
> /* SoC DTSI */

I can switch it to C-style in the example, but we are going with Linux
Coding Style which soon will allow // judging by Linus' statements.

>
>> +
>> + / {
>> + cpus {
>> + // ...
>> + };
>> +
>> + psci {
>> + // ...
>> + };
>> +
>> + soc@ {
>> + dma: dma-controller@10000 {
>> + // ...
>> + };
>> +
>> + clk: clock-controller@80000 {
>> + // ...
>> + };
>> + };
>> + };
>> +
>> + // Board DTS
>> +
>> + &clk {
>> + // ...
>> + };
>> +
>> + &dma {
>> + // ...
>> + };
>> +
>> +
>> +Order of Properties in Device Node
>> +----------------------------------
>> +
>> +Following order of properties in device nodes is preferred:
>> +
>> +1. compatible
>> +2. reg
>> +3. ranges
>> +4. Standard/common properties (defined by common bindings, e.g. without
>> + vendor-prefixes)
>> +5. Vendor-specific properties
>> +6. status (if applicable)
>> +7. Child nodes, where each node is preceded with a blank line
>> +
>> +The "status" property is by default "okay", thus it can be omitted.
>> +
>> +Example::
>> +
>> + // SoC DTSI
>> +
>> + usb_1_hsphy: phy@88e3000 {
>> + compatible = "qcom,sm8550-snps-eusb2-phy";
>> + reg = <0x0 0x088e3000 0x0 0x154>;
>> + #phy-cells = <0>;
>> + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
>> + status = "disabled";
>> + };
>
> Since this describes vendor-specific properties and vendor prefixes as well
> as standard properties, I think it would be clearer if we use something more
> complex that actually contains those as an example.
>
> There's a few. One is MediaTek:
>
> vdo1_rdma0: dma-controller@1c104000 {
> compatible = "mediatek,mt8195-vdo1-rdma";
> reg = <0 0x1c104000 0 0x1000>;
> #dma-cells = <1>;
> clocks = <&vdosys1 CLK_VDO1_MDP_RDMA0>;
> interrupts = <GIC_SPI 495 IRQ_TYPE_LEVEL_HIGH 0>;
> iommus = <&iommu_vdo M4U_PORT_L2_MDP_RDMA0>;
> power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS1>;
> mediatek,gce-client-reg = <&gce0 SUBSYS_1c10XXXX 0x4000 0x1000>;
> };
>
> ...or other one can be nVidia:
>
> mipi: mipi@700e3000 {
> compatible = "nvidia,tegra210-mipi";
> reg = <0x0 0x700e3000 0x0 0x100>;
> clocks = <&tegra_car TEGRA210_CLK_MIPI_CAL>;
> clock-names = "mipi-cal";
> power-domains = <&pd_sor>;
> #nvidia,mipi-calibrate-cells = <1>;
> };
>
> ...or we could make an example out of fantasy, which could work even better
> as far as describing goes.
>
> /* SoC DTSI */
>
> device_node: device-class@6789abc {
> compatible = "vendor,device";

Yep. I'll use this, unless checkpatch complains about undocumented
compatible. :) This allows to show the child node.

> reg = <0 0x06789abc 0 0xa123>;
> ranges = <0 0 0x6789abc 0x1000>;
> #dma-cells = <1>;
> clocks = <&clock_controller SOC_CLOCK>;
> clock-names = "dev-clk";
> #vendor,custom-cells = <2>;
> status = "disabled";
>
> child_node: child-class@100 {
> reg = <0x100 0x200>;
> /* ... */
> };
> };
>
> /* Board DTS */
>
> &device_node {
> device-supply = <&board_vreg1>;
> status = "okay";
> }
>
>> +
>> + // Board DTS
>> +
>> + &usb_1_hsphy {
>> + clocks = <&tcsr TCSR_USB2_CLKREF_EN>;
>> + clock-names = "ref";
>> + status = "okay";
>> + };
>> +
>> +
>> +Indentation
>> +-----------
>> +
>> +1. Use indentation according to :ref:`codingstyle`.
>> +2. For arrays spanning across lines, it is preferred to align the continued
>> + entries with opening < from the first line.
>> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses)
>> + should be enclosed in <>.
>> +
>> +Example::
>> +
>> + thermal-sensor@c271000 {
>> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2";
>> + reg = <0x0 0x0c271000 0x0 0x1000>,
>> + <0x0 0x0c222000 0x0 0x1000>;
>> + };
>> +
>> +Organizing DTSI and DTS
>> +-----------------------
>> +
>> +The DTSI and DTS files should be organized in a way representing the common
>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>
> ^^^^
> There's a double space here, it was probably unintentional.

I think I used everywhere double-spaces. At least this was my intention,
so I will fix single-spaces :)

Best regards,
Krzysztof

2023-11-20 19:19:26

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

Hi Krzysztof,

On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
<[email protected]> wrote:
> On 20/11/2023 15:01, Michal Simek wrote:> >
> > On 11/20/23 09:40, Krzysztof Kozlowski wrote:
> >> Document preferred coding style for Devicetree sources (DTS and DTSI),
> >> to bring consistency among all (sub)architectures and ease in reviews.

> >> +Organizing DTSI and DTS
> >> +-----------------------
> >> +
> >> +The DTSI and DTS files should be organized in a way representing the common
> >> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
> >> +and DTS files into several files:
> >> +
> >> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
> >> + on the SoC).
> >> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> >> + entire System-on-Module).
> >
> > DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
> > there doesn't need to be DTS representing the board.
>
> I have never seen a SoM which can run without elaborate hardware-hacking
> (e.g. connecting multiple wires to the SoM pins). The definition of the
> SoM is that it is a module. Module can be re-used, just like SoC.

/me looks at his board farm...

The Renesas White-Hawk CPU board can be used standalone, and has a
separate power input connector for this operation mode. As it has RAM,
Ethernet, serial console, eMMC, and even mini-DP, it can serve useful
purposes on its own.
I agree it's not a super-good example, as the board is not really a
"SoM", and we currently don't have r8a779g0-white-hawk-cpu.dts, only
r8a779g0-white-hawk-cpu.dtsi.

The RZ/A2M CPU Board is a real SoM, which can be powered over USB.
It has less standard connectors (microSD, USB, MIPI CSI-2), but still
sufficient features to be usable on its own.
Again, we're doing a bad job, as we only have a DTS for the full eval
board (r7s9210-rza2mevb.dts).

I guess there are (many) other examples...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-11-20 19:32:22

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 20/11/2023 20:18, Geert Uytterhoeven wrote:
> Hi Krzysztof,
>
> On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 20/11/2023 15:01, Michal Simek wrote:> >
>>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
>>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>>>> to bring consistency among all (sub)architectures and ease in reviews.
>
>>>> +Organizing DTSI and DTS
>>>> +-----------------------
>>>> +
>>>> +The DTSI and DTS files should be organized in a way representing the common
>>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>>>> +and DTS files into several files:
>>>> +
>>>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
>>>> + on the SoC).
>>>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>>> + entire System-on-Module).
>>>
>>> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
>>> there doesn't need to be DTS representing the board.
>>
>> I have never seen a SoM which can run without elaborate hardware-hacking
>> (e.g. connecting multiple wires to the SoM pins). The definition of the
>> SoM is that it is a module. Module can be re-used, just like SoC.
>
> /me looks at his board farm...
>
> The Renesas White-Hawk CPU board can be used standalone, and has a
> separate power input connector for this operation mode. As it has RAM,
> Ethernet, serial console, eMMC, and even mini-DP, it can serve useful
> purposes on its own.
> I agree it's not a super-good example, as the board is not really a
> "SoM", and we currently don't have r8a779g0-white-hawk-cpu.dts, only
> r8a779g0-white-hawk-cpu.dtsi.
>
> The RZ/A2M CPU Board is a real SoM, which can be powered over USB.
> It has less standard connectors (microSD, USB, MIPI CSI-2), but still
> sufficient features to be usable on its own.
> Again, we're doing a bad job, as we only have a DTS for the full eval
> board (r7s9210-rza2mevb.dts).
>
> I guess there are (many) other examples...

OK, I never had such in my hands. Anyway, the SoM which can run
standalone has a meaning of a board, so how exactly you want to
rephrase the paragraph?

Best regards,
Krzysztof

2023-11-20 20:16:31

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

> +Naming and Valid Characters
> +---------------------------
> +
> +1. Node and property names are allowed to use only:
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * dash: -
> +
> +2. Labels are allowed to use only:
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * underscore: _
> +
> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
> +
> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
> + part can be padded with leading zeros.
> +
> +Example::
> +
> + gpi_dma2: dma-controller@800000 {
> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
> + reg = <0x0 0x00800000 0x0 0x60000>;
> + }

Hi Krzysztof

What i like about the Linux Coding Style is that most sections have a
Rationale. I like the way it explains the 'Why?'. It makes it feel
less arbitrary. When it does not seem arbitrary, but reasoned, i find
it easier to follow.

Could you add rationale like the Coding Style?

Andrew

2023-11-21 07:33:57

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 11/20/23 20:31, Krzysztof Kozlowski wrote:
> On 20/11/2023 20:18, Geert Uytterhoeven wrote:
>> Hi Krzysztof,
>>
>> On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
>> <[email protected]> wrote:
>>> On 20/11/2023 15:01, Michal Simek wrote:> >
>>>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
>>>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>>>>> to bring consistency among all (sub)architectures and ease in reviews.
>>
>>>>> +Organizing DTSI and DTS
>>>>> +-----------------------
>>>>> +
>>>>> +The DTSI and DTS files should be organized in a way representing the common
>>>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>>>>> +and DTS files into several files:
>>>>> +
>>>>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
>>>>> + on the SoC).
>>>>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>>>> + entire System-on-Module).
>>>>
>>>> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
>>>> there doesn't need to be DTS representing the board.
>>>
>>> I have never seen a SoM which can run without elaborate hardware-hacking
>>> (e.g. connecting multiple wires to the SoM pins). The definition of the
>>> SoM is that it is a module. Module can be re-used, just like SoC.
>>
>> /me looks at his board farm...
>>
>> The Renesas White-Hawk CPU board can be used standalone, and has a
>> separate power input connector for this operation mode. As it has RAM,
>> Ethernet, serial console, eMMC, and even mini-DP, it can serve useful
>> purposes on its own.
>> I agree it's not a super-good example, as the board is not really a
>> "SoM", and we currently don't have r8a779g0-white-hawk-cpu.dts, only
>> r8a779g0-white-hawk-cpu.dtsi.
>>
>> The RZ/A2M CPU Board is a real SoM, which can be powered over USB.
>> It has less standard connectors (microSD, USB, MIPI CSI-2), but still
>> sufficient features to be usable on its own.
>> Again, we're doing a bad job, as we only have a DTS for the full eval
>> board (r7s9210-rza2mevb.dts).
>>
>> I guess there are (many) other examples...
>
> OK, I never had such in my hands. Anyway, the SoM which can run
> standalone has a meaning of a board, so how exactly you want to
> rephrase the paragraph?

What about?

2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
entire System-on-Module). DTS if runs standalone.

Thanks,
Michal

2023-11-21 07:37:25

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 11/20/23 21:15, Andrew Lunn wrote:
>> +Naming and Valid Characters
>> +---------------------------
>> +
>> +1. Node and property names are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * dash: -
>> +
>> +2. Labels are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * underscore: _
>> +
>> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
>> +
>> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
>> + part can be padded with leading zeros.
>> +
>> +Example::
>> +
>> + gpi_dma2: dma-controller@800000 {
>> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
>> + reg = <0x0 0x00800000 0x0 0x60000>;
>> + }
>
> Hi Krzysztof
>
> What i like about the Linux Coding Style is that most sections have a
> Rationale. I like the way it explains the 'Why?'. It makes it feel
> less arbitrary. When it does not seem arbitrary, but reasoned, i find
> it easier to follow.
>
> Could you add rationale like the Coding Style?

+1 on this.

Thanks,
Michal

2023-11-21 07:47:40

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 21/11/2023 08:33, Michal Simek wrote:
>
>
> On 11/20/23 20:31, Krzysztof Kozlowski wrote:
>> On 20/11/2023 20:18, Geert Uytterhoeven wrote:
>>> Hi Krzysztof,
>>>
>>> On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>> On 20/11/2023 15:01, Michal Simek wrote:> >
>>>>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
>>>>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>>>>>> to bring consistency among all (sub)architectures and ease in reviews.
>>>
>>>>>> +Organizing DTSI and DTS
>>>>>> +-----------------------
>>>>>> +
>>>>>> +The DTSI and DTS files should be organized in a way representing the common
>>>>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>>>>>> +and DTS files into several files:
>>>>>> +
>>>>>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
>>>>>> + on the SoC).
>>>>>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>>>>> + entire System-on-Module).
>>>>>
>>>>> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
>>>>> there doesn't need to be DTS representing the board.
>>>>
>>>> I have never seen a SoM which can run without elaborate hardware-hacking
>>>> (e.g. connecting multiple wires to the SoM pins). The definition of the
>>>> SoM is that it is a module. Module can be re-used, just like SoC.
>>>
>>> /me looks at his board farm...
>>>
>>> The Renesas White-Hawk CPU board can be used standalone, and has a
>>> separate power input connector for this operation mode. As it has RAM,
>>> Ethernet, serial console, eMMC, and even mini-DP, it can serve useful
>>> purposes on its own.
>>> I agree it's not a super-good example, as the board is not really a
>>> "SoM", and we currently don't have r8a779g0-white-hawk-cpu.dts, only
>>> r8a779g0-white-hawk-cpu.dtsi.
>>>
>>> The RZ/A2M CPU Board is a real SoM, which can be powered over USB.
>>> It has less standard connectors (microSD, USB, MIPI CSI-2), but still
>>> sufficient features to be usable on its own.
>>> Again, we're doing a bad job, as we only have a DTS for the full eval
>>> board (r7s9210-rza2mevb.dts).
>>>
>>> I guess there are (many) other examples...
>>
>> OK, I never had such in my hands. Anyway, the SoM which can run
>> standalone has a meaning of a board, so how exactly you want to
>> rephrase the paragraph?
>
> What about?
>
> 2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> entire System-on-Module). DTS if runs standalone.

OK, but then it's duplicating the option 3. It also suggests that SoM
should be a DTS, which is not what we want for such case. Such SoMs must
have DTSI+DTS.

Best regards,
Krzysztof

2023-11-21 07:49:09

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 20/11/2023 21:15, Andrew Lunn wrote:
>> +Naming and Valid Characters
>> +---------------------------
>> +
>> +1. Node and property names are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * dash: -
>> +
>> +2. Labels are allowed to use only:
>> +
>> + * lowercase characters: [a-z]
>> + * digits: [0-9]
>> + * underscore: _
>> +
>> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
>> +
>> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
>> + part can be padded with leading zeros.
>> +
>> +Example::
>> +
>> + gpi_dma2: dma-controller@800000 {
>> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
>> + reg = <0x0 0x00800000 0x0 0x60000>;
>> + }
>
> Hi Krzysztof
>
> What i like about the Linux Coding Style is that most sections have a
> Rationale. I like the way it explains the 'Why?'. It makes it feel
> less arbitrary. When it does not seem arbitrary, but reasoned, i find
> it easier to follow.
>
> Could you add rationale like the Coding Style?

I did not do it on purpose because it would grow too much.

Best regards,
Krzysztof

2023-11-21 08:09:02

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

Hi Krzysztof,

On Tue, Nov 21, 2023 at 8:47 AM Krzysztof Kozlowski
<[email protected]> wrote:
> On 21/11/2023 08:33, Michal Simek wrote:
> > On 11/20/23 20:31, Krzysztof Kozlowski wrote:
> >> On 20/11/2023 20:18, Geert Uytterhoeven wrote:
> >>> On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
> >>> <[email protected]> wrote:
> >>>> On 20/11/2023 15:01, Michal Simek wrote:> >
> >>>>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
> >>>>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
> >>>>>> to bring consistency among all (sub)architectures and ease in reviews.
> >>>
> >>>>>> +Organizing DTSI and DTS
> >>>>>> +-----------------------
> >>>>>> +
> >>>>>> +The DTSI and DTS files should be organized in a way representing the common
> >>>>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
> >>>>>> +and DTS files into several files:
> >>>>>> +
> >>>>>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
> >>>>>> + on the SoC).
> >>>>>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> >>>>>> + entire System-on-Module).
> >>>>>
> >>>>> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
> >>>>> there doesn't need to be DTS representing the board.
> >>>>
> >>>> I have never seen a SoM which can run without elaborate hardware-hacking
> >>>> (e.g. connecting multiple wires to the SoM pins). The definition of the
> >>>> SoM is that it is a module. Module can be re-used, just like SoC.
> >>>
> >>> /me looks at his board farm...

> >>> I guess there are (many) other examples...
> >>
> >> OK, I never had such in my hands. Anyway, the SoM which can run
> >> standalone has a meaning of a board, so how exactly you want to
> >> rephrase the paragraph?
> >
> > What about?
> >
> > 2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> > entire System-on-Module). DTS if runs standalone.
>
> OK, but then it's duplicating the option 3. It also suggests that SoM
> should be a DTS, which is not what we want for such case. Such SoMs must
> have DTSI+DTS.

So you want us to have a one-line <SoM>.dts, which just includes <SoM>.dtsi?
IMHO that adds more files for no much gain.
Users of a SoM can easily include <SoM>.dts.
'git grep "#include .*dts\>"' tells you we have plenty of users of that scheme.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-11-21 08:37:42

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 21/11/2023 09:08, Geert Uytterhoeven wrote:
>>>>> I guess there are (many) other examples...
>>>>
>>>> OK, I never had such in my hands. Anyway, the SoM which can run
>>>> standalone has a meaning of a board, so how exactly you want to
>>>> rephrase the paragraph?
>>>
>>> What about?
>>>
>>> 2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>> entire System-on-Module). DTS if runs standalone.
>>
>> OK, but then it's duplicating the option 3. It also suggests that SoM
>> should be a DTS, which is not what we want for such case. Such SoMs must
>> have DTSI+DTS.
>
> So you want us to have a one-line <SoM>.dts, which just includes <SoM>.dtsi?
> IMHO that adds more files for no much gain.

Yes, if this is a real SoM, then yes. There is much gain - it clearly
represents the hardware like we in general expect. It allows re-usage by
in- and out-tree users, while documenting this possibility.

We structure DTS according to main components of the hardware, which
serves as self-documenting, re-usable and easy to grasp solution.

> Users of a SoM can easily include <SoM>.dts.

Which is confusing during review and not a welcomed pattern.

> 'git grep "#include .*dts\>"' tells you we have plenty of users of that scheme.

Yeah, you can put C functions inside header (included only once). You
can include C file in other C file. But just because you can do it, it
does not mean you should do it. It's not the way we want to make code
organized.

Best regards,
Krzysztof

2023-11-21 10:13:41

by Dmitry Baryshkov

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On Tue, 21 Nov 2023 at 10:09, Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Krzysztof,
>
> On Tue, Nov 21, 2023 at 8:47 AM Krzysztof Kozlowski
> <[email protected]> wrote:
> > On 21/11/2023 08:33, Michal Simek wrote:
> > > On 11/20/23 20:31, Krzysztof Kozlowski wrote:
> > >> On 20/11/2023 20:18, Geert Uytterhoeven wrote:
> > >>> On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
> > >>> <[email protected]> wrote:
> > >>>> On 20/11/2023 15:01, Michal Simek wrote:> >
> > >>>>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
> > >>>>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
> > >>>>>> to bring consistency among all (sub)architectures and ease in reviews.
> > >>>
> > >>>>>> +Organizing DTSI and DTS
> > >>>>>> +-----------------------
> > >>>>>> +
> > >>>>>> +The DTSI and DTS files should be organized in a way representing the common
> > >>>>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
> > >>>>>> +and DTS files into several files:
> > >>>>>> +
> > >>>>>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
> > >>>>>> + on the SoC).
> > >>>>>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> > >>>>>> + entire System-on-Module).
> > >>>>>
> > >>>>> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
> > >>>>> there doesn't need to be DTS representing the board.
> > >>>>
> > >>>> I have never seen a SoM which can run without elaborate hardware-hacking
> > >>>> (e.g. connecting multiple wires to the SoM pins). The definition of the
> > >>>> SoM is that it is a module. Module can be re-used, just like SoC.
> > >>>
> > >>> /me looks at his board farm...
>
> > >>> I guess there are (many) other examples...
> > >>
> > >> OK, I never had such in my hands. Anyway, the SoM which can run
> > >> standalone has a meaning of a board, so how exactly you want to
> > >> rephrase the paragraph?
> > >
> > > What about?
> > >
> > > 2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> > > entire System-on-Module). DTS if runs standalone.
> >
> > OK, but then it's duplicating the option 3. It also suggests that SoM
> > should be a DTS, which is not what we want for such case. Such SoMs must
> > have DTSI+DTS.
>
> So you want us to have a one-line <SoM>.dts, which just includes <SoM>.dtsi?

Well, I think it is impossible to run SoM directly. There is a carrier
board anyway, which includes at least regulators. So, I guess, the
SoM.dts will not be a oneline file.

> IMHO that adds more files for no much gain.
> Users of a SoM can easily include <SoM>.dts.
> 'git grep "#include .*dts\>"' tells you we have plenty of users of that scheme.

--
With best wishes
Dmitry

2023-11-21 10:27:23

by AngeloGioacchino Del Regno

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

Il 20/11/23 15:57, Krzysztof Kozlowski ha scritto:
> On 20/11/2023 12:43, AngeloGioacchino Del Regno wrote:
>> Il 20/11/23 09:40, Krzysztof Kozlowski ha scritto:
>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>>> to bring consistency among all (sub)architectures and ease in reviews.
>>>
>>> Cc: Andrew Davis <[email protected]>
>>> Cc: AngeloGioacchino Del Regno <[email protected]>
>>> Cc: Arnd Bergmann <[email protected]>
>>> Cc: Bjorn Andersson <[email protected]>
>>> Cc: Geert Uytterhoeven <[email protected]>
>>> Cc: Heiko Stuebner <[email protected]>
>>> Cc: Konrad Dybcio <[email protected]>
>>> Cc: Matthias Brugger <[email protected]>
>>> Cc: Michal Simek <[email protected]>
>>> Cc: Neil Armstrong <[email protected]>
>>> Cc: Nishanth Menon <[email protected]>
>>> Cc: Olof Johansson <[email protected]>
>>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>>
>>> ---
>>>
>>> Merging idea: Rob/DT bindings
>>>
>>> Changes in v2
>>> =============
>>> 1. Hopefully incorporate entire feedback from comments:
>>> a. Fix \ { => / { (Rob)
>>> b. Name: dts-coding-style (Rob)
>>> c. Exceptions for ordering nodes by name for Renesas and pinctrl (Geert,
>>> Konrad)
>>> d. Ordering properties by common/vendor (Rob)
>>> e. Array entries in <> (Rob)
>>>
>>> 2. New chapter: Organizing DTSI and DTS
>>>
>>> 3. Several grammar fixes (missing articles)
>>>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> .../devicetree/bindings/dts-coding-style.rst | 163 ++++++++++++++++++
>>> Documentation/devicetree/bindings/index.rst | 1 +
>>> 2 files changed, 164 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/dts-coding-style.rst
>>>
>>> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst
>>> new file mode 100644
>>> index 000000000000..cc7e3b4d1b92
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst
>>> @@ -0,0 +1,163 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +.. _dtscodingstyle:
>>> +
>>> +=====================================
>>> +Devicetree Sources (DTS) Coding Style
>>> +=====================================
>>> +
>>> +When writing Devicetree Sources (DTS) please observe below guidelines. They
>>> +should be considered complementary to any rules expressed already in Devicetree
>>> +Specification and dtc compiler (including W=1 and W=2 builds).
>>> +
>>> +Individual architectures and sub-architectures can add additional rules, making
>>> +the style stricter.
>>> +
>>> +Naming and Valid Characters
>>> +---------------------------
>>> +
>>> +1. Node and property names are allowed to use only:
>>> +
>>> + * lowercase characters: [a-z]
>>> + * digits: [0-9]
>>> + * dash: -
>>> +
>>> +2. Labels are allowed to use only:
>>> +
>>> + * lowercase characters: [a-z]
>>> + * digits: [0-9]
>>> + * underscore: _
>>> +
>>> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
>>
>> This is imperative, so: s/should/shall/g
>
> Sure, fine.
>
>>
>>> +
>>> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
>>> + part can be padded with leading zeros.
>>> +
>>
>> Same here, I'd say.... :-)
>>
>>> +Example::
>>> +
>>> + gpi_dma2: dma-controller@800000 {
>>> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
>>> + reg = <0x0 0x00800000 0x0 0x60000>;
>>> + }
>>> +
>>> +Order of Nodes
>>> +--------------
>>> +
>>> +1. Nodes within any bus, thus using unit addresses for children, shall be
>>> + ordered incrementally by unit address.
>>> + Alternatively for some sub-architectures, nodes of the same type can be
>>> + grouped together (e.g. all I2C controllers one after another even if this
>>> + breaks unit address ordering).
>>> +
>>> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
>>> + name. For a few types of nodes, they can be ordered by the main property
>>> + (e.g. pin configuration states ordered by value of "pins" property).
>>> +
>>> +3. When extending nodes in the board DTS via &label, the entries should be
>>> + ordered alpha-numerically.
>>> +
>>> +Example::
>>> +
>>
>> Hmm, comments!
>>
>>> + // SoC DTSI
>>
>> ....speaking of commenting, should we at least suggest to use C-style comments?
>>
>> /* SoC DTSI */
>
> I can switch it to C-style in the example, but we are going with Linux
> Coding Style which soon will allow // judging by Linus' statements.
>

Right. That wasn't a strong opinion anyway, so it's totally okay as well.

>>
>>> +
>>> + / {
>>> + cpus {
>>> + // ...
>>> + };
>>> +
>>> + psci {
>>> + // ...
>>> + };
>>> +
>>> + soc@ {
>>> + dma: dma-controller@10000 {
>>> + // ...
>>> + };
>>> +
>>> + clk: clock-controller@80000 {
>>> + // ...
>>> + };
>>> + };
>>> + };
>>> +
>>> + // Board DTS
>>> +
>>> + &clk {
>>> + // ...
>>> + };
>>> +
>>> + &dma {
>>> + // ...
>>> + };
>>> +
>>> +
>>> +Order of Properties in Device Node
>>> +----------------------------------
>>> +
>>> +Following order of properties in device nodes is preferred:
>>> +
>>> +1. compatible
>>> +2. reg
>>> +3. ranges
>>> +4. Standard/common properties (defined by common bindings, e.g. without
>>> + vendor-prefixes)
>>> +5. Vendor-specific properties
>>> +6. status (if applicable)
>>> +7. Child nodes, where each node is preceded with a blank line
>>> +
>>> +The "status" property is by default "okay", thus it can be omitted.
>>> +
>>> +Example::
>>> +
>>> + // SoC DTSI
>>> +
>>> + usb_1_hsphy: phy@88e3000 {
>>> + compatible = "qcom,sm8550-snps-eusb2-phy";
>>> + reg = <0x0 0x088e3000 0x0 0x154>;
>>> + #phy-cells = <0>;
>>> + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
>>> + status = "disabled";
>>> + };
>>
>> Since this describes vendor-specific properties and vendor prefixes as well
>> as standard properties, I think it would be clearer if we use something more
>> complex that actually contains those as an example.
>>
>> There's a few. One is MediaTek:
>>
>> vdo1_rdma0: dma-controller@1c104000 {
>> compatible = "mediatek,mt8195-vdo1-rdma";
>> reg = <0 0x1c104000 0 0x1000>;
>> #dma-cells = <1>;
>> clocks = <&vdosys1 CLK_VDO1_MDP_RDMA0>;
>> interrupts = <GIC_SPI 495 IRQ_TYPE_LEVEL_HIGH 0>;
>> iommus = <&iommu_vdo M4U_PORT_L2_MDP_RDMA0>;
>> power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS1>;
>> mediatek,gce-client-reg = <&gce0 SUBSYS_1c10XXXX 0x4000 0x1000>;
>> };
>>
>> ...or other one can be nVidia:
>>
>> mipi: mipi@700e3000 {
>> compatible = "nvidia,tegra210-mipi";
>> reg = <0x0 0x700e3000 0x0 0x100>;
>> clocks = <&tegra_car TEGRA210_CLK_MIPI_CAL>;
>> clock-names = "mipi-cal";
>> power-domains = <&pd_sor>;
>> #nvidia,mipi-calibrate-cells = <1>;
>> };
>>
>> ...or we could make an example out of fantasy, which could work even better
>> as far as describing goes.
>>
>> /* SoC DTSI */
>>
>> device_node: device-class@6789abc {
>> compatible = "vendor,device";
>
> Yep. I'll use this, unless checkpatch complains about undocumented
> compatible. :) This allows to show the child node.
>

If checkpatch complains about undocumented compatible, could we perhaps use one
that does actually exist, while still retaining the actual mockup examples?

I understand the eventual concern about somewhat wrongly documenting said device,
but it's also true that this is documentation about something else that is not
related to a specific device (so perhaps a "warning: this is for representation
purposes only, and may contain properties that the devices pointed by the currently
used compatible string may not accept" might work to avoid confusion?).

>> reg = <0 0x06789abc 0 0xa123>;
>> ranges = <0 0 0x6789abc 0x1000>;
>> #dma-cells = <1>;
>> clocks = <&clock_controller SOC_CLOCK>;
>> clock-names = "dev-clk";
>> #vendor,custom-cells = <2>;
>> status = "disabled";
>>
>> child_node: child-class@100 {
>> reg = <0x100 0x200>;
>> /* ... */
>> };
>> };
>>
>> /* Board DTS */
>>
>> &device_node {
>> device-supply = <&board_vreg1>;
>> status = "okay";
>> }
>>
>>> +
>>> + // Board DTS
>>> +
>>> + &usb_1_hsphy {
>>> + clocks = <&tcsr TCSR_USB2_CLKREF_EN>;
>>> + clock-names = "ref";
>>> + status = "okay";
>>> + };
>>> +
>>> +
>>> +Indentation
>>> +-----------
>>> +
>>> +1. Use indentation according to :ref:`codingstyle`.
>>> +2. For arrays spanning across lines, it is preferred to align the continued
>>> + entries with opening < from the first line.
>>> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses)
>>> + should be enclosed in <>.
>>> +
>>> +Example::
>>> +
>>> + thermal-sensor@c271000 {
>>> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2";
>>> + reg = <0x0 0x0c271000 0x0 0x1000>,
>>> + <0x0 0x0c222000 0x0 0x1000>;
>>> + };
>>> +
>>> +Organizing DTSI and DTSthat
>>> +-----------------------
>>> +
>>> +The DTSI and DTS files should be organized in a way representing the common
>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>>
>> ^^^^
>> There's a double space here, it was probably unintentional.
>
> I think I used everywhere double-spaces. At least this was my intention,
> so I will fix single-spaces :)
>

Oh! Okay, yeah, that also works :-D

Cheers,
Angelo

2023-11-21 10:28:53

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 21/11/2023 11:13, Dmitry Baryshkov wrote:
> On Tue, 21 Nov 2023 at 10:09, Geert Uytterhoeven <[email protected]> wrote:
>>
>> Hi Krzysztof,
>>
>> On Tue, Nov 21, 2023 at 8:47 AM Krzysztof Kozlowski
>> <[email protected]> wrote:
>>> On 21/11/2023 08:33, Michal Simek wrote:
>>>> On 11/20/23 20:31, Krzysztof Kozlowski wrote:
>>>>> On 20/11/2023 20:18, Geert Uytterhoeven wrote:
>>>>>> On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
>>>>>> <[email protected]> wrote:
>>>>>>> On 20/11/2023 15:01, Michal Simek wrote:> >
>>>>>>>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
>>>>>>>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>>>>>>>>> to bring consistency among all (sub)architectures and ease in reviews.
>>>>>>
>>>>>>>>> +Organizing DTSI and DTS
>>>>>>>>> +-----------------------
>>>>>>>>> +
>>>>>>>>> +The DTSI and DTS files should be organized in a way representing the common
>>>>>>>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>>>>>>>>> +and DTS files into several files:
>>>>>>>>> +
>>>>>>>>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
>>>>>>>>> + on the SoC).
>>>>>>>>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>>>>>>>> + entire System-on-Module).
>>>>>>>>
>>>>>>>> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
>>>>>>>> there doesn't need to be DTS representing the board.
>>>>>>>
>>>>>>> I have never seen a SoM which can run without elaborate hardware-hacking
>>>>>>> (e.g. connecting multiple wires to the SoM pins). The definition of the
>>>>>>> SoM is that it is a module. Module can be re-used, just like SoC.
>>>>>>
>>>>>> /me looks at his board farm...
>>
>>>>>> I guess there are (many) other examples...
>>>>>
>>>>> OK, I never had such in my hands. Anyway, the SoM which can run
>>>>> standalone has a meaning of a board, so how exactly you want to
>>>>> rephrase the paragraph?
>>>>
>>>> What about?
>>>>
>>>> 2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>>> entire System-on-Module). DTS if runs standalone.
>>>
>>> OK, but then it's duplicating the option 3. It also suggests that SoM
>>> should be a DTS, which is not what we want for such case. Such SoMs must
>>> have DTSI+DTS.
>>
>> So you want us to have a one-line <SoM>.dts, which just includes <SoM>.dtsi?
>
> Well, I think it is impossible to run SoM directly. There is a carrier
> board anyway, which includes at least regulators. So, I guess, the
> SoM.dts will not be a oneline file.

Geert claims he has SoM with an USB connector which can run when power
is supplied by that USB connector. I can imagine a CPU board (so a SoM
in format of a board, not small DIMM-card) which has connectors e.g. for
power and a slot for external motherboard for additional, optional
interfaces.

Look at picture on 14th page:
https://www.renesas.com/us/en/document/mat/rza2m-cpu-board-users-manual

This looks like some case of SoM, although maybe not that popular
outside of Renesas :)

Best regards,
Krzysztof

2023-11-21 11:54:44

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 11/21/23 11:28, Krzysztof Kozlowski wrote:
> On 21/11/2023 11:13, Dmitry Baryshkov wrote:
>> On Tue, 21 Nov 2023 at 10:09, Geert Uytterhoeven <[email protected]> wrote:
>>>
>>> Hi Krzysztof,
>>>
>>> On Tue, Nov 21, 2023 at 8:47 AM Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>> On 21/11/2023 08:33, Michal Simek wrote:
>>>>> On 11/20/23 20:31, Krzysztof Kozlowski wrote:
>>>>>> On 20/11/2023 20:18, Geert Uytterhoeven wrote:
>>>>>>> On Mon, Nov 20, 2023 at 3:53 PM Krzysztof Kozlowski
>>>>>>> <[email protected]> wrote:
>>>>>>>> On 20/11/2023 15:01, Michal Simek wrote:> >
>>>>>>>>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
>>>>>>>>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>>>>>>>>>> to bring consistency among all (sub)architectures and ease in reviews.
>>>>>>>
>>>>>>>>>> +Organizing DTSI and DTS
>>>>>>>>>> +-----------------------
>>>>>>>>>> +
>>>>>>>>>> +The DTSI and DTS files should be organized in a way representing the common
>>>>>>>>>> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
>>>>>>>>>> +and DTS files into several files:
>>>>>>>>>> +
>>>>>>>>>> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
>>>>>>>>>> + on the SoC).
>>>>>>>>>> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>>>>>>>>> + entire System-on-Module).
>>>>>>>>>
>>>>>>>>> DTS/DTSI - SOMs can actually run as they are that's why it is fair to say that
>>>>>>>>> there doesn't need to be DTS representing the board.
>>>>>>>>
>>>>>>>> I have never seen a SoM which can run without elaborate hardware-hacking
>>>>>>>> (e.g. connecting multiple wires to the SoM pins). The definition of the
>>>>>>>> SoM is that it is a module. Module can be re-used, just like SoC.
>>>>>>>
>>>>>>> /me looks at his board farm...
>>>
>>>>>>> I guess there are (many) other examples...
>>>>>>
>>>>>> OK, I never had such in my hands. Anyway, the SoM which can run
>>>>>> standalone has a meaning of a board, so how exactly you want to
>>>>>> rephrase the paragraph?
>>>>>
>>>>> What about?
>>>>>
>>>>> 2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
>>>>> entire System-on-Module). DTS if runs standalone.
>>>>
>>>> OK, but then it's duplicating the option 3. It also suggests that SoM
>>>> should be a DTS, which is not what we want for such case. Such SoMs must
>>>> have DTSI+DTS.
>>>
>>> So you want us to have a one-line <SoM>.dts, which just includes <SoM>.dtsi?
>>
>> Well, I think it is impossible to run SoM directly. There is a carrier
>> board anyway, which includes at least regulators. So, I guess, the
>> SoM.dts will not be a oneline file.
>
> Geert claims he has SoM with an USB connector which can run when power
> is supplied by that USB connector. I can imagine a CPU board (so a SoM
> in format of a board, not small DIMM-card) which has connectors e.g. for
> power and a slot for external motherboard for additional, optional
> interfaces.
>
> Look at picture on 14th page:
> https://www.renesas.com/us/en/document/mat/rza2m-cpu-board-users-manual
>
> This looks like some case of SoM, although maybe not that popular
> outside of Renesas :)

In our case we have SOMs
https://www.xilinx.com/products/som/kria.html#portfolio

and also carrier cards (CC) like this
https://www.xilinx.com/products/som/kria/kv260-vision-starter-kit.html

and we also have debug boards.

There must be a carrier card in our case and there is power connector (with also
non sw accessible regulators), jtag, boot pins and for example ttl to usb
connector for UART but SOM spec describe directly which peripherals are on
certain pins by default.
It means we have CC card but there is nothing on it to describe for SOM itself.

Our default boot process is to boot with SOM peripherals described by spec. And
then do CC identification and switching to SOM+CC dtb description at U-Boot.

Some combinations are described here.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/xilinx/Makefile?h=v6.7-rc2#n21

We can create dtsi for SOM and then dts which just one line which includes SOM dtsi.

Thanks,
Michal

2023-11-21 11:59:39

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 11/20/23 15:53, Krzysztof Kozlowski wrote:
> On 20/11/2023 15:01, Michal Simek wrote:
>>
>>
>> On 11/20/23 09:40, Krzysztof Kozlowski wrote:
>>> Document preferred coding style for Devicetree sources (DTS and DTSI),
>>> to bring consistency among all (sub)architectures and ease in reviews.
>>>
>>> Cc: Andrew Davis <[email protected]>
>>> Cc: AngeloGioacchino Del Regno <[email protected]>
>>> Cc: Arnd Bergmann <[email protected]>
>>> Cc: Bjorn Andersson <[email protected]>
>>> Cc: Geert Uytterhoeven <[email protected]>
>>> Cc: Heiko Stuebner <[email protected]>
>>> Cc: Konrad Dybcio <[email protected]>
>>> Cc: Matthias Brugger <[email protected]>
>>> Cc: Michal Simek <[email protected]>
>>> Cc: Neil Armstrong <[email protected]>
>>> Cc: Nishanth Menon <[email protected]>
>>> Cc: Olof Johansson <[email protected]>
>>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>>
>>> ---
>>>
>>> Merging idea: Rob/DT bindings
>>>
>>> Changes in v2
>>> =============
>>> 1. Hopefully incorporate entire feedback from comments:
>>> a. Fix \ { => / { (Rob)
>>> b. Name: dts-coding-style (Rob)
>>> c. Exceptions for ordering nodes by name for Renesas and pinctrl (Geert,
>>> Konrad)
>>> d. Ordering properties by common/vendor (Rob)
>>> e. Array entries in <> (Rob)
>>>
>>> 2. New chapter: Organizing DTSI and DTS
>>>
>>> 3. Several grammar fixes (missing articles)
>>>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> .../devicetree/bindings/dts-coding-style.rst | 163 ++++++++++++++++++
>>> Documentation/devicetree/bindings/index.rst | 1 +
>>> 2 files changed, 164 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/dts-coding-style.rst
>>>
>>> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst
>>> new file mode 100644
>>> index 000000000000..cc7e3b4d1b92
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst
>>> @@ -0,0 +1,163 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +.. _dtscodingstyle:
>>> +
>>> +=====================================
>>> +Devicetree Sources (DTS) Coding Style
>>> +=====================================
>>> +
>>> +When writing Devicetree Sources (DTS) please observe below guidelines. They
>>> +should be considered complementary to any rules expressed already in Devicetree
>>> +Specification and dtc compiler (including W=1 and W=2 builds).
>>> +
>>> +Individual architectures and sub-architectures can add additional rules, making
>>> +the style stricter.
>>> +
>>> +Naming and Valid Characters
>>> +---------------------------
>>> +
>>> +1. Node and property names are allowed to use only:
>>> +
>>> + * lowercase characters: [a-z]
>>> + * digits: [0-9]
>>> + * dash: -
>>
>> device-tree specification v0.4. Chapter 2.2.1/Table 2.1 is describing much more
>> valid characters for node names.
>> It means above description is not accurate or DT spec should be updated.
>
> Spec allows way to much. dtc doesn't.
> One thing is the spec, second
> thing is coding style.

From my point of view spec is primary source of truth. If spec is saying name
can use upper case then I can use it. If upper case is not
recommended/deprecated because of whatever reason spec should be updated to
reflect it.
I know that DTC is reporting other issues but isn't it the right way to reflect
it back to the spec?

No doubt that it is nice to see to have guide like this.

Thanks,
Michal

2023-11-21 12:37:37

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 21/11/2023 12:55, Michal Simek wrote:
>>> device-tree specification v0.4. Chapter 2.2.1/Table 2.1 is describing much more
>>> valid characters for node names.
>>> It means above description is not accurate or DT spec should be updated.
>>
>> Spec allows way to much. dtc doesn't.
>> One thing is the spec, second
>> thing is coding style.
>
> From my point of view spec is primary source of truth. If spec is saying name
> can use upper case then I can use it. If upper case is not
> recommended/deprecated because of whatever reason spec should be updated to
> reflect it.
> I know that DTC is reporting other issues but isn't it the right way to reflect
> it back to the spec?

Then why aren't you putting Linux Coding Style into C spec? I do not see
any relation between specification of the language and the coding style
chosen for given project.

Zephyr can go with upper-case. Why it should be disallowed by the spec?

Best regards,
Krzysztof

2023-11-21 13:52:03

by Rafał Miłecki

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 20.11.2023 09:40, Krzysztof Kozlowski wrote:
> Document preferred coding style for Devicetree sources (DTS and DTSI),
> to bring consistency among all (sub)architectures and ease in reviews.

I really like the overall idea. Thanks for coming up with that!

Two questions inline.

> Cc: Andrew Davis <[email protected]>
> Cc: AngeloGioacchino Del Regno <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Heiko Stuebner <[email protected]>
> Cc: Konrad Dybcio <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Cc: Michal Simek <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Nishanth Menon <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>
> ---
>
> Merging idea: Rob/DT bindings
>
> Changes in v2
> =============
> 1. Hopefully incorporate entire feedback from comments:
> a. Fix \ { => / { (Rob)
> b. Name: dts-coding-style (Rob)
> c. Exceptions for ordering nodes by name for Renesas and pinctrl (Geert,
> Konrad)
> d. Ordering properties by common/vendor (Rob)
> e. Array entries in <> (Rob)
>
> 2. New chapter: Organizing DTSI and DTS
>
> 3. Several grammar fixes (missing articles)
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> .../devicetree/bindings/dts-coding-style.rst | 163 ++++++++++++++++++
> Documentation/devicetree/bindings/index.rst | 1 +
> 2 files changed, 164 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dts-coding-style.rst
>
> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst
> new file mode 100644
> index 000000000000..cc7e3b4d1b92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst
> @@ -0,0 +1,163 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. _dtscodingstyle:
> +
> +=====================================
> +Devicetree Sources (DTS) Coding Style
> +=====================================
> +
> +When writing Devicetree Sources (DTS) please observe below guidelines. They
> +should be considered complementary to any rules expressed already in Devicetree
> +Specification and dtc compiler (including W=1 and W=2 builds).
> +
> +Individual architectures and sub-architectures can add additional rules, making
> +the style stricter.
> +
> +Naming and Valid Characters
> +---------------------------
> +
> +1. Node and property names are allowed to use only:
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * dash: -
> +
> +2. Labels are allowed to use only:
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * underscore: _
> +
> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
> +
> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
> + part can be padded with leading zeros.
> +
> +Example::
> +
> + gpi_dma2: dma-controller@800000 {
> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
> + reg = <0x0 0x00800000 0x0 0x60000>;
> + }
> +
> +Order of Nodes
> +--------------
> +
> +1. Nodes within any bus, thus using unit addresses for children, shall be
> + ordered incrementally by unit address.
> + Alternatively for some sub-architectures, nodes of the same type can be
> + grouped together (e.g. all I2C controllers one after another even if this
> + breaks unit address ordering).
> +
> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
> + name. For a few types of nodes, they can be ordered by the main property
> + (e.g. pin configuration states ordered by value of "pins" property).
> +
> +3. When extending nodes in the board DTS via &label, the entries should be
> + ordered alpha-numerically.

Just an idea. Would that make (more) sense to make &label-like entries
match order of nodes in included .dts(i)?

Adventages:
1. We keep unit address incremental order that is unlikely to change

Disadventages:
1. More difficult to verify

> +Example::
> +
> + // SoC DTSI
> +
> + / {
> + cpus {
> + // ...
> + };
> +
> + psci {
> + // ...
> + };
> +
> + soc@ {
> + dma: dma-controller@10000 {
> + // ...
> + };
> +
> + clk: clock-controller@80000 {
> + // ...
> + };
> + };
> + };
> +
> + // Board DTS
> +
> + &clk {
> + // ...
> + };
> +
> + &dma {
> + // ...
> + };
> +
> +
> +Order of Properties in Device Node
> +----------------------------------
> +
> +Following order of properties in device nodes is preferred:
> +
> +1. compatible
> +2. reg
> +3. ranges
> +4. Standard/common properties (defined by common bindings, e.g. without
> + vendor-prefixes)
> +5. Vendor-specific properties
> +6. status (if applicable)
> +7. Child nodes, where each node is preceded with a blank line
> +
> +The "status" property is by default "okay", thus it can be omitted.

I think it would really help to include position of #address-cells and
#size-cells here. In some files I saw them above "compatible" that seems
unintuitive. Some prefer putting them at end which I think makes sense
as they affect children nodes.

Whatever you choose it'd be just nice to have things consistent.

> +Example::
> +
> + // SoC DTSI
> +
> + usb_1_hsphy: phy@88e3000 {
> + compatible = "qcom,sm8550-snps-eusb2-phy";
> + reg = <0x0 0x088e3000 0x0 0x154>;
> + #phy-cells = <0>;
> + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
> + status = "disabled";
> + };
> +
> + // Board DTS
> +
> + &usb_1_hsphy {
> + clocks = <&tcsr TCSR_USB2_CLKREF_EN>;
> + clock-names = "ref";
> + status = "okay";
> + };
> +
> +
> +Indentation
> +-----------
> +
> +1. Use indentation according to :ref:`codingstyle`.
> +2. For arrays spanning across lines, it is preferred to align the continued
> + entries with opening < from the first line.
> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses)
> + should be enclosed in <>.
> +
> +Example::
> +
> + thermal-sensor@c271000 {
> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2";
> + reg = <0x0 0x0c271000 0x0 0x1000>,
> + <0x0 0x0c222000 0x0 0x1000>;
> + };
> +
> +Organizing DTSI and DTS
> +-----------------------
> +
> +The DTSI and DTS files should be organized in a way representing the common
> +(and re-usable) parts of the hardware. Typically this means organizing DTSI
> +and DTS files into several files:
> +
> +1. DTSI with contents of the entire SoC (without nodes for hardware not present
> + on the SoC).
> +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g.
> + entire System-on-Module).
> +3. DTS representing the board.
> +
> +Hardware components which are present on the board should be placed in the
> +board DTS, not in the SoC or SoM DTSI. A partial exception is a common
> +external reference SoC-input clock, which could be coded as a fixed-clock in
> +the SoC DTSI with its frequency provided by each board DTS.
> diff --git a/Documentation/devicetree/bindings/index.rst b/Documentation/devicetree/bindings/index.rst
> index d9002a3a0abb..cc1fbdc05657 100644
> --- a/Documentation/devicetree/bindings/index.rst
> +++ b/Documentation/devicetree/bindings/index.rst
> @@ -4,6 +4,7 @@
> :maxdepth: 1
>
> ABI
> + dts-coding-style
> writing-bindings
> writing-schema
> submitting-patches

2023-11-21 16:04:40

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

Hi Krzysztof,

On Tue, Nov 21, 2023 at 1:36 PM Krzysztof Kozlowski
<[email protected]> wrote:
> On 21/11/2023 12:55, Michal Simek wrote:
> >>> device-tree specification v0.4. Chapter 2.2.1/Table 2.1 is describing much more
> >>> valid characters for node names.
> >>> It means above description is not accurate or DT spec should be updated.
> >>
> >> Spec allows way to much. dtc doesn't.
> >> One thing is the spec, second
> >> thing is coding style.
> >
> > From my point of view spec is primary source of truth. If spec is saying name
> > can use upper case then I can use it. If upper case is not
> > recommended/deprecated because of whatever reason spec should be updated to
> > reflect it.
> > I know that DTC is reporting other issues but isn't it the right way to reflect
> > it back to the spec?
>
> Then why aren't you putting Linux Coding Style into C spec? I do not see
> any relation between specification of the language and the coding style
> chosen for given project.
>
> Zephyr can go with upper-case. Why it should be disallowed by the spec?

I thought there was only One DT to bind them all?
IMHO it would be better to align DT usage of Zephyr and Linux (and
anything else).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-11-22 08:02:19

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 21/11/2023 17:04, Geert Uytterhoeven wrote:
> Hi Krzysztof,
>
> On Tue, Nov 21, 2023 at 1:36 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 21/11/2023 12:55, Michal Simek wrote:
>>>>> device-tree specification v0.4. Chapter 2.2.1/Table 2.1 is describing much more
>>>>> valid characters for node names.
>>>>> It means above description is not accurate or DT spec should be updated.
>>>>
>>>> Spec allows way to much. dtc doesn't.
>>>> One thing is the spec, second
>>>> thing is coding style.
>>>
>>> From my point of view spec is primary source of truth. If spec is saying name
>>> can use upper case then I can use it. If upper case is not
>>> recommended/deprecated because of whatever reason spec should be updated to
>>> reflect it.
>>> I know that DTC is reporting other issues but isn't it the right way to reflect
>>> it back to the spec?
>>
>> Then why aren't you putting Linux Coding Style into C spec? I do not see
>> any relation between specification of the language and the coding style
>> chosen for given project.
>>
>> Zephyr can go with upper-case. Why it should be disallowed by the spec?
>
> I thought there was only One DT to bind them all?
> IMHO it would be better to align DT usage of Zephyr and Linux (and
> anything else).

I actually don't know what Zephyr decides, but used it as example that
it might want different coding style. Just like C standard allows to
have all variables (including local ones) upper-case, we do not have
such coding style. And no one proposes to update C spec to match Linux
coding style. :)

Best regards,
Krzysztof

2023-11-22 08:07:26

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 21/11/2023 14:50, Rafał Miłecki wrote:
>> +Order of Nodes
>> +--------------
>> +
>> +1. Nodes within any bus, thus using unit addresses for children, shall be
>> + ordered incrementally by unit address.
>> + Alternatively for some sub-architectures, nodes of the same type can be
>> + grouped together (e.g. all I2C controllers one after another even if this
>> + breaks unit address ordering).
>> +
>> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
>> + name. For a few types of nodes, they can be ordered by the main property
>> + (e.g. pin configuration states ordered by value of "pins" property).
>> +
>> +3. When extending nodes in the board DTS via &label, the entries should be
>> + ordered alpha-numerically.
>
> Just an idea. Would that make (more) sense to make &label-like entries
> match order of nodes in included .dts(i)?
>
> Adventages:
> 1. We keep unit address incremental order that is unlikely to change
>
> Disadventages:
> 1. More difficult to verify

Rob also proposed this and I believe above disadvantage here is crucial.
If you add new SoC with board DTS you are fine. But if you add only new
board, the order of entries look random in the diff hunk. Reviewer must
open SoC DTSI to be able to review the patch with board DTS.

If review is tricky and we do not have tool to perform it automatically,
I am sure submissions will have disordered board DTS.

>
>
>> +Example::
>> +
>> + // SoC DTSI
>> +
>> + / {
>> + cpus {
>> + // ...
>> + };
>> +
>> + psci {
>> + // ...
>> + };
>> +
>> + soc@ {
>> + dma: dma-controller@10000 {
>> + // ...
>> + };
>> +
>> + clk: clock-controller@80000 {
>> + // ...
>> + };
>> + };
>> + };
>> +
>> + // Board DTS
>> +
>> + &clk {
>> + // ...
>> + };
>> +
>> + &dma {
>> + // ...
>> + };
>> +
>> +
>> +Order of Properties in Device Node
>> +----------------------------------
>> +
>> +Following order of properties in device nodes is preferred:
>> +
>> +1. compatible
>> +2. reg
>> +3. ranges
>> +4. Standard/common properties (defined by common bindings, e.g. without
>> + vendor-prefixes)
>> +5. Vendor-specific properties
>> +6. status (if applicable)
>> +7. Child nodes, where each node is preceded with a blank line
>> +
>> +The "status" property is by default "okay", thus it can be omitted.
>
> I think it would really help to include position of #address-cells and
> #size-cells here. In some files I saw them above "compatible" that seems
> unintuitive. Some prefer putting them at end which I think makes sense
> as they affect children nodes.
>
> Whatever you choose it'd be just nice to have things consistent.

This is a standard/common property, thus it goes to (4) above.

Best regards,
Krzysztof

2023-11-22 08:09:43

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 21/11/2023 14:50, Rafał Miłecki wrote:
> >> +Order of Nodes
> >> +--------------
> >> +
> >> +1. Nodes within any bus, thus using unit addresses for children, shall be
> >> + ordered incrementally by unit address.
> >> + Alternatively for some sub-architectures, nodes of the same type can be
> >> + grouped together (e.g. all I2C controllers one after another even if this
> >> + breaks unit address ordering).
> >> +
> >> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
> >> + name. For a few types of nodes, they can be ordered by the main property
> >> + (e.g. pin configuration states ordered by value of "pins" property).
> >> +
> >> +3. When extending nodes in the board DTS via &label, the entries should be
> >> + ordered alpha-numerically.
> >
> > Just an idea. Would that make (more) sense to make &label-like entries
> > match order of nodes in included .dts(i)?
> >
> > Adventages:
> > 1. We keep unit address incremental order that is unlikely to change
> >
> > Disadventages:
> > 1. More difficult to verify
>
> Rob also proposed this and I believe above disadvantage here is crucial.
> If you add new SoC with board DTS you are fine. But if you add only new
> board, the order of entries look random in the diff hunk. Reviewer must
> open SoC DTSI to be able to review the patch with board DTS.
>
> If review is tricky and we do not have tool to perform it automatically,
> I am sure submissions will have disordered board DTS.
>
> >
> >
> >> +Example::
> >> +
> >> + // SoC DTSI
> >> +
> >> + / {
> >> + cpus {
> >> + // ...
> >> + };
> >> +
> >> + psci {
> >> + // ...
> >> + };
> >> +
> >> + soc@ {
> >> + dma: dma-controller@10000 {
> >> + // ...
> >> + };
> >> +
> >> + clk: clock-controller@80000 {
> >> + // ...
> >> + };
> >> + };
> >> + };
> >> +
> >> + // Board DTS
> >> +
> >> + &clk {
> >> + // ...
> >> + };
> >> +
> >> + &dma {
> >> + // ...
> >> + };
> >> +
> >> +
> >> +Order of Properties in Device Node
> >> +----------------------------------
> >> +
> >> +Following order of properties in device nodes is preferred:
> >> +
> >> +1. compatible
> >> +2. reg
> >> +3. ranges
> >> +4. Standard/common properties (defined by common bindings, e.g. without
> >> + vendor-prefixes)
> >> +5. Vendor-specific properties
> >> +6. status (if applicable)
> >> +7. Child nodes, where each node is preceded with a blank line
> >> +
> >> +The "status" property is by default "okay", thus it can be omitted.
> >
> > I think it would really help to include position of #address-cells and
> > #size-cells here. In some files I saw them above "compatible" that seems
> > unintuitive. Some prefer putting them at end which I think makes sense
> > as they affect children nodes.
> >
> > Whatever you choose it'd be just nice to have things consistent.
>
> This is a standard/common property, thus it goes to (4) above.

It's probably a mix, but AFAIK a lot of the device trees in tree have
#*-cells after "status". In some cases they are added in the board
.dts files, not the chip/module .dtsi files.

+1 that it makes sense at the end as they affect child nodes.

ChenYu

2023-11-22 08:19:17

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 21/11/2023 08:47, Krzysztof Kozlowski wrote:
> On 20/11/2023 21:15, Andrew Lunn wrote:
>>> +Naming and Valid Characters
>>> +---------------------------
>>> +
>>> +1. Node and property names are allowed to use only:
>>> +
>>> + * lowercase characters: [a-z]
>>> + * digits: [0-9]
>>> + * dash: -
>>> +
>>> +2. Labels are allowed to use only:
>>> +
>>> + * lowercase characters: [a-z]
>>> + * digits: [0-9]
>>> + * underscore: _
>>> +
>>> +3. Unit addresses should use lowercase hex, without leading zeros (padding).
>>> +
>>> +4. Hex values in properties, e.g. "reg", should use lowercase hex. The address
>>> + part can be padded with leading zeros.
>>> +
>>> +Example::
>>> +
>>> + gpi_dma2: dma-controller@800000 {
>>> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
>>> + reg = <0x0 0x00800000 0x0 0x60000>;
>>> + }
>>
>> Hi Krzysztof
>>
>> What i like about the Linux Coding Style is that most sections have a
>> Rationale. I like the way it explains the 'Why?'. It makes it feel
>> less arbitrary. When it does not seem arbitrary, but reasoned, i find
>> it easier to follow.
>>
>> Could you add rationale like the Coding Style?
>
> I did not do it on purpose because it would grow too much.

I added short rationale in coming v3.

Best regards,
Krzysztof

2023-11-22 08:21:53

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 22/11/2023 09:09, Chen-Yu Tsai wrote:
> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 21/11/2023 14:50, Rafał Miłecki wrote:
>>>> +Order of Nodes
>>>> +--------------
>>>> +
>>>> +1. Nodes within any bus, thus using unit addresses for children, shall be
>>>> + ordered incrementally by unit address.
>>>> + Alternatively for some sub-architectures, nodes of the same type can be
>>>> + grouped together (e.g. all I2C controllers one after another even if this
>>>> + breaks unit address ordering).
>>>> +
>>>> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
>>>> + name. For a few types of nodes, they can be ordered by the main property
>>>> + (e.g. pin configuration states ordered by value of "pins" property).
>>>> +
>>>> +3. When extending nodes in the board DTS via &label, the entries should be
>>>> + ordered alpha-numerically.
>>>
>>> Just an idea. Would that make (more) sense to make &label-like entries
>>> match order of nodes in included .dts(i)?
>>>
>>> Adventages:
>>> 1. We keep unit address incremental order that is unlikely to change
>>>
>>> Disadventages:
>>> 1. More difficult to verify
>>
>> Rob also proposed this and I believe above disadvantage here is crucial.
>> If you add new SoC with board DTS you are fine. But if you add only new
>> board, the order of entries look random in the diff hunk. Reviewer must
>> open SoC DTSI to be able to review the patch with board DTS.
>>
>> If review is tricky and we do not have tool to perform it automatically,
>> I am sure submissions will have disordered board DTS.
>>
>>>
>>>
>>>> +Example::
>>>> +
>>>> + // SoC DTSI
>>>> +
>>>> + / {
>>>> + cpus {
>>>> + // ...
>>>> + };
>>>> +
>>>> + psci {
>>>> + // ...
>>>> + };
>>>> +
>>>> + soc@ {
>>>> + dma: dma-controller@10000 {
>>>> + // ...
>>>> + };
>>>> +
>>>> + clk: clock-controller@80000 {
>>>> + // ...
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + // Board DTS
>>>> +
>>>> + &clk {
>>>> + // ...
>>>> + };
>>>> +
>>>> + &dma {
>>>> + // ...
>>>> + };
>>>> +
>>>> +
>>>> +Order of Properties in Device Node
>>>> +----------------------------------
>>>> +
>>>> +Following order of properties in device nodes is preferred:
>>>> +
>>>> +1. compatible
>>>> +2. reg
>>>> +3. ranges
>>>> +4. Standard/common properties (defined by common bindings, e.g. without
>>>> + vendor-prefixes)
>>>> +5. Vendor-specific properties
>>>> +6. status (if applicable)
>>>> +7. Child nodes, where each node is preceded with a blank line
>>>> +
>>>> +The "status" property is by default "okay", thus it can be omitted.
>>>
>>> I think it would really help to include position of #address-cells and
>>> #size-cells here. In some files I saw them above "compatible" that seems
>>> unintuitive. Some prefer putting them at end which I think makes sense
>>> as they affect children nodes.
>>>
>>> Whatever you choose it'd be just nice to have things consistent.
>>
>> This is a standard/common property, thus it goes to (4) above.
>
> It's probably a mix, but AFAIK a lot of the device trees in tree have
> #*-cells after "status". In some cases they are added in the board
> .dts files, not the chip/module .dtsi files.

Existing DTS is not a good example :)

>
> +1 that it makes sense at the end as they affect child nodes.

I still insist that status must be the last, because:
1. Many SoC nodes have address/size cells but do not have any children
(I2C, SPI), so we put useless information at the end.
2. Status should be the final information to say whether the node is
ready or is not. I read the node, check properties and then look at the end:
a. Lack of status means it is ready.
b. status=disabled means device still needs board resources/customization

Best regards,
Krzysztof

2023-11-22 08:25:01

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 2023-11-22 09:01, Krzysztof Kozlowski wrote:
> On 21/11/2023 17:04, Geert Uytterhoeven wrote:
>> Hi Krzysztof,
>>
>> On Tue, Nov 21, 2023 at 1:36 PM Krzysztof Kozlowski
>> <[email protected]> wrote:
>>> On 21/11/2023 12:55, Michal Simek wrote:
>>>>>> device-tree specification v0.4. Chapter 2.2.1/Table 2.1 is
>>>>>> describing much more
>>>>>> valid characters for node names.
>>>>>> It means above description is not accurate or DT spec should be
>>>>>> updated.
>>>>>
>>>>> Spec allows way to much. dtc doesn't.
>>>>> One thing is the spec, second
>>>>> thing is coding style.
>>>>
>>>> From my point of view spec is primary source of truth. If spec is
>>>> saying name
>>>> can use upper case then I can use it. If upper case is not
>>>> recommended/deprecated because of whatever reason spec should be
>>>> updated to
>>>> reflect it.
>>>> I know that DTC is reporting other issues but isn't it the right way
>>>> to reflect
>>>> it back to the spec?
>>>
>>> Then why aren't you putting Linux Coding Style into C spec? I do not
>>> see
>>> any relation between specification of the language and the coding
>>> style
>>> chosen for given project.
>>>
>>> Zephyr can go with upper-case. Why it should be disallowed by the
>>> spec?
>>
>> I thought there was only One DT to bind them all?
>> IMHO it would be better to align DT usage of Zephyr and Linux (and
>> anything else).
>
> I actually don't know what Zephyr decides, but used it as example that
> it might want different coding style. Just like C standard allows to
> have all variables (including local ones) upper-case, we do not have
> such coding style. And no one proposes to update C spec to match Linux
> coding style. :)

I also agree about the need to differentiate the coding styles from the
underlying specifications. Also, as we know, the C language is the
unifying factor between various projects, but with wildly differing
coding styles. Expecting all those projects to have their C coding
styles aligned wouldn't be reasonable, if you agree.

BTW, having this document as part of the kernel documentation will be
great, and it's in fact quite overdue, if you agree. Huge thanks to
everyone working on it!

2023-11-22 08:28:52

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On Wed, Nov 22, 2023 at 9:21 AM Krzysztof Kozlowski
<[email protected]> wrote:
> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
> > On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >> On 21/11/2023 14:50, Rafał Miłecki wrote:
> >>>> +Order of Nodes
> >>>> +--------------
> >>>> +
> >>>> +1. Nodes within any bus, thus using unit addresses for children, shall be
> >>>> + ordered incrementally by unit address.
> >>>> + Alternatively for some sub-architectures, nodes of the same type can be
> >>>> + grouped together (e.g. all I2C controllers one after another even if this
> >>>> + breaks unit address ordering).
> >>>> +
> >>>> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
> >>>> + name. For a few types of nodes, they can be ordered by the main property
> >>>> + (e.g. pin configuration states ordered by value of "pins" property).
> >>>> +
> >>>> +3. When extending nodes in the board DTS via &label, the entries should be
> >>>> + ordered alpha-numerically.
> >>>
> >>> Just an idea. Would that make (more) sense to make &label-like entries
> >>> match order of nodes in included .dts(i)?
> >>>
> >>> Adventages:
> >>> 1. We keep unit address incremental order that is unlikely to change
> >>>
> >>> Disadventages:
> >>> 1. More difficult to verify
> >>
> >> Rob also proposed this and I believe above disadvantage here is crucial.
> >> If you add new SoC with board DTS you are fine. But if you add only new
> >> board, the order of entries look random in the diff hunk. Reviewer must
> >> open SoC DTSI to be able to review the patch with board DTS.
> >>
> >> If review is tricky and we do not have tool to perform it automatically,
> >> I am sure submissions will have disordered board DTS.
> >>
> >>>
> >>>
> >>>> +Example::
> >>>> +
> >>>> + // SoC DTSI
> >>>> +
> >>>> + / {
> >>>> + cpus {
> >>>> + // ...
> >>>> + };
> >>>> +
> >>>> + psci {
> >>>> + // ...
> >>>> + };
> >>>> +
> >>>> + soc@ {
> >>>> + dma: dma-controller@10000 {
> >>>> + // ...
> >>>> + };
> >>>> +
> >>>> + clk: clock-controller@80000 {
> >>>> + // ...
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + // Board DTS
> >>>> +
> >>>> + &clk {
> >>>> + // ...
> >>>> + };
> >>>> +
> >>>> + &dma {
> >>>> + // ...
> >>>> + };
> >>>> +
> >>>> +
> >>>> +Order of Properties in Device Node
> >>>> +----------------------------------
> >>>> +
> >>>> +Following order of properties in device nodes is preferred:
> >>>> +
> >>>> +1. compatible
> >>>> +2. reg
> >>>> +3. ranges
> >>>> +4. Standard/common properties (defined by common bindings, e.g. without
> >>>> + vendor-prefixes)
> >>>> +5. Vendor-specific properties
> >>>> +6. status (if applicable)
> >>>> +7. Child nodes, where each node is preceded with a blank line
> >>>> +
> >>>> +The "status" property is by default "okay", thus it can be omitted.
> >>>
> >>> I think it would really help to include position of #address-cells and
> >>> #size-cells here. In some files I saw them above "compatible" that seems
> >>> unintuitive. Some prefer putting them at end which I think makes sense
> >>> as they affect children nodes.
> >>>
> >>> Whatever you choose it'd be just nice to have things consistent.
> >>
> >> This is a standard/common property, thus it goes to (4) above.
> >
> > It's probably a mix, but AFAIK a lot of the device trees in tree have
> > #*-cells after "status". In some cases they are added in the board
> > .dts files, not the chip/module .dtsi files.
>
> Existing DTS is not a good example :)
>
> >
> > +1 that it makes sense at the end as they affect child nodes.
>
> I still insist that status must be the last, because:
> 1. Many SoC nodes have address/size cells but do not have any children
> (I2C, SPI), so we put useless information at the end.
> 2. Status should be the final information to say whether the node is
> ready or is not. I read the node, check properties and then look at the end:
> a. Lack of status means it is ready.
> b. status=disabled means device still needs board resources/customization

+1 for status at the end (before children), so it's easy to find.
Also, reality can look like the example in the bindings, with an optional
status property appended.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-11-22 08:30:02

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
>> <[email protected]> wrote:
>>>
>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
>>>>> +Order of Properties in Device Node
>>>>> +----------------------------------
>>>>> +
>>>>> +Following order of properties in device nodes is preferred:
>>>>> +
>>>>> +1. compatible
>>>>> +2. reg
>>>>> +3. ranges
>>>>> +4. Standard/common properties (defined by common bindings, e.g.
>>>>> without
>>>>> + vendor-prefixes)
>>>>> +5. Vendor-specific properties
>>>>> +6. status (if applicable)
>>>>> +7. Child nodes, where each node is preceded with a blank line
>>>>> +
>>>>> +The "status" property is by default "okay", thus it can be
>>>>> omitted.
>>>>
>>>> I think it would really help to include position of #address-cells
>>>> and
>>>> #size-cells here. In some files I saw them above "compatible" that
>>>> seems
>>>> unintuitive. Some prefer putting them at end which I think makes
>>>> sense
>>>> as they affect children nodes.
>>>>
>>>> Whatever you choose it'd be just nice to have things consistent.
>>>
>>> This is a standard/common property, thus it goes to (4) above.
>>
>> It's probably a mix, but AFAIK a lot of the device trees in tree have
>> #*-cells after "status". In some cases they are added in the board
>> .dts files, not the chip/module .dtsi files.
>
> Existing DTS is not a good example :)
>
>>
>> +1 that it makes sense at the end as they affect child nodes.
>
> I still insist that status must be the last, because:
> 1. Many SoC nodes have address/size cells but do not have any children
> (I2C, SPI), so we put useless information at the end.
> 2. Status should be the final information to say whether the node is
> ready or is not. I read the node, check properties and then look at the
> end:
> a. Lack of status means it is ready.
> b. status=disabled means device still needs board
> resources/customization

I agree with the "status" belonging to the very end, because it's both
logical and much more readable. Also, "status" is expected to be
modified in the dependent DT files, which makes it kind of volatile and
even more deserving to be placed last.

2023-11-22 08:50:07

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 11/22/23 09:29, Dragan Simic wrote:
> On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
>> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
>>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>>
>>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
>>>>>> +Order of Properties in Device Node
>>>>>> +----------------------------------
>>>>>> +
>>>>>> +Following order of properties in device nodes is preferred:
>>>>>> +
>>>>>> +1. compatible
>>>>>> +2. reg
>>>>>> +3. ranges
>>>>>> +4. Standard/common properties (defined by common bindings, e.g. without
>>>>>> + vendor-prefixes)
>>>>>> +5. Vendor-specific properties
>>>>>> +6. status (if applicable)
>>>>>> +7. Child nodes, where each node is preceded with a blank line
>>>>>> +
>>>>>> +The "status" property is by default "okay", thus it can be omitted.
>>>>>
>>>>> I think it would really help to include position of #address-cells and
>>>>> #size-cells here. In some files I saw them above "compatible" that seems
>>>>> unintuitive. Some prefer putting them at end which I think makes sense
>>>>> as they affect children nodes.
>>>>>
>>>>> Whatever you choose it'd be just nice to have things consistent.
>>>>
>>>> This is a standard/common property, thus it goes to (4) above.
>>>
>>> It's probably a mix, but AFAIK a lot of the device trees in tree have
>>> #*-cells after "status". In some cases they are added in the board
>>> .dts files, not the chip/module .dtsi files.
>>
>> Existing DTS is not a good example :)
>>
>>>
>>> +1 that it makes sense at the end as they affect child nodes.
>>
>> I still insist that status must be the last, because:
>> 1. Many SoC nodes have address/size cells but do not have any children
>> (I2C, SPI), so we put useless information at the end.
>> 2. Status should be the final information to say whether the node is
>> ready or is not. I read the node, check properties and then look at the end:
>> a. Lack of status means it is ready.
>> b. status=disabled means device still needs board resources/customization
>
> I agree with the "status" belonging to the very end, because it's both logical
> and much more readable. Also, "status" is expected to be modified in the
> dependent DT files, which makes it kind of volatile and even more deserving to
> be placed last.

I am just curious if having status property at the end won't affect
execution/boot up time. Not sure how it is done in Linux but in U-Boot at least
(we want to have DTs in sync between Linux and U-Boot) of_find_property is
pretty much big loop over all properties. And status property defined at the end
means going over all of them to find it out to if device is present.
Not sure if Linux works in the same way but at least of_get_property is done in
the same way.

It is not big deal on high speed cpus but wanted to point it out.

Thanks,
Michal

2023-11-22 08:54:24

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

Hi Michal,

On Wed, Nov 22, 2023 at 9:50 AM Michal Simek <[email protected]> wrote:
> On 11/22/23 09:29, Dragan Simic wrote:
> > On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
> >> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
> >>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
> >>>>>> +Order of Properties in Device Node
> >>>>>> +----------------------------------
> >>>>>> +
> >>>>>> +Following order of properties in device nodes is preferred:
> >>>>>> +
> >>>>>> +1. compatible
> >>>>>> +2. reg
> >>>>>> +3. ranges
> >>>>>> +4. Standard/common properties (defined by common bindings, e.g. without
> >>>>>> + vendor-prefixes)
> >>>>>> +5. Vendor-specific properties
> >>>>>> +6. status (if applicable)
> >>>>>> +7. Child nodes, where each node is preceded with a blank line
> >>>>>> +
> >>>>>> +The "status" property is by default "okay", thus it can be omitted.
> >>>>>
> >>>>> I think it would really help to include position of #address-cells and
> >>>>> #size-cells here. In some files I saw them above "compatible" that seems
> >>>>> unintuitive. Some prefer putting them at end which I think makes sense
> >>>>> as they affect children nodes.
> >>>>>
> >>>>> Whatever you choose it'd be just nice to have things consistent.
> >>>>
> >>>> This is a standard/common property, thus it goes to (4) above.
> >>>
> >>> It's probably a mix, but AFAIK a lot of the device trees in tree have
> >>> #*-cells after "status". In some cases they are added in the board
> >>> .dts files, not the chip/module .dtsi files.
> >>
> >> Existing DTS is not a good example :)
> >>
> >>>
> >>> +1 that it makes sense at the end as they affect child nodes.
> >>
> >> I still insist that status must be the last, because:
> >> 1. Many SoC nodes have address/size cells but do not have any children
> >> (I2C, SPI), so we put useless information at the end.
> >> 2. Status should be the final information to say whether the node is
> >> ready or is not. I read the node, check properties and then look at the end:
> >> a. Lack of status means it is ready.
> >> b. status=disabled means device still needs board resources/customization
> >
> > I agree with the "status" belonging to the very end, because it's both logical
> > and much more readable. Also, "status" is expected to be modified in the
> > dependent DT files, which makes it kind of volatile and even more deserving to
> > be placed last.
>
> I am just curious if having status property at the end won't affect
> execution/boot up time. Not sure how it is done in Linux but in U-Boot at least
> (we want to have DTs in sync between Linux and U-Boot) of_find_property is
> pretty much big loop over all properties. And status property defined at the end
> means going over all of them to find it out to if device is present.
> Not sure if Linux works in the same way but at least of_get_property is done in
> the same way.

As the default is "okay", you have to loop over all properties anyway.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-11-22 08:58:28

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

Hi Geert,

On 11/22/23 09:53, Geert Uytterhoeven wrote:
> Hi Michal,
>
> On Wed, Nov 22, 2023 at 9:50 AM Michal Simek <[email protected]> wrote:
>> On 11/22/23 09:29, Dragan Simic wrote:
>>> On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
>>>> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
>>>>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
>>>>>>>> +Order of Properties in Device Node
>>>>>>>> +----------------------------------
>>>>>>>> +
>>>>>>>> +Following order of properties in device nodes is preferred:
>>>>>>>> +
>>>>>>>> +1. compatible
>>>>>>>> +2. reg
>>>>>>>> +3. ranges
>>>>>>>> +4. Standard/common properties (defined by common bindings, e.g. without
>>>>>>>> + vendor-prefixes)
>>>>>>>> +5. Vendor-specific properties
>>>>>>>> +6. status (if applicable)
>>>>>>>> +7. Child nodes, where each node is preceded with a blank line
>>>>>>>> +
>>>>>>>> +The "status" property is by default "okay", thus it can be omitted.
>>>>>>>
>>>>>>> I think it would really help to include position of #address-cells and
>>>>>>> #size-cells here. In some files I saw them above "compatible" that seems
>>>>>>> unintuitive. Some prefer putting them at end which I think makes sense
>>>>>>> as they affect children nodes.
>>>>>>>
>>>>>>> Whatever you choose it'd be just nice to have things consistent.
>>>>>>
>>>>>> This is a standard/common property, thus it goes to (4) above.
>>>>>
>>>>> It's probably a mix, but AFAIK a lot of the device trees in tree have
>>>>> #*-cells after "status". In some cases they are added in the board
>>>>> .dts files, not the chip/module .dtsi files.
>>>>
>>>> Existing DTS is not a good example :)
>>>>
>>>>>
>>>>> +1 that it makes sense at the end as they affect child nodes.
>>>>
>>>> I still insist that status must be the last, because:
>>>> 1. Many SoC nodes have address/size cells but do not have any children
>>>> (I2C, SPI), so we put useless information at the end.
>>>> 2. Status should be the final information to say whether the node is
>>>> ready or is not. I read the node, check properties and then look at the end:
>>>> a. Lack of status means it is ready.
>>>> b. status=disabled means device still needs board resources/customization
>>>
>>> I agree with the "status" belonging to the very end, because it's both logical
>>> and much more readable. Also, "status" is expected to be modified in the
>>> dependent DT files, which makes it kind of volatile and even more deserving to
>>> be placed last.
>>
>> I am just curious if having status property at the end won't affect
>> execution/boot up time. Not sure how it is done in Linux but in U-Boot at least
>> (we want to have DTs in sync between Linux and U-Boot) of_find_property is
>> pretty much big loop over all properties. And status property defined at the end
>> means going over all of them to find it out to if device is present.
>> Not sure if Linux works in the same way but at least of_get_property is done in
>> the same way.
>
> As the default is "okay", you have to loop over all properties anyway.

No doubt if you don't define status property that you need to loop over all of
them. We normally describe the whole SOC with pretty much all IPs status =
disabled and then in board file we are changing it to okay based on what it is
actually wired out.
It means on our systems all nodes have status properties. If you have it at
first you don't need to go over all.

Thanks,
Michal

2023-11-22 08:59:49

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 2023-11-22 09:49, Michal Simek wrote:
> On 11/22/23 09:29, Dragan Simic wrote:
>> On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
>>> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
>>>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
>>>> <[email protected]> wrote:
>>>>>
>>>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
>>>>>>> +Order of Properties in Device Node
>>>>>>> +----------------------------------
>>>>>>> +
>>>>>>> +Following order of properties in device nodes is preferred:
>>>>>>> +
>>>>>>> +1. compatible
>>>>>>> +2. reg
>>>>>>> +3. ranges
>>>>>>> +4. Standard/common properties (defined by common bindings, e.g.
>>>>>>> without
>>>>>>> + vendor-prefixes)
>>>>>>> +5. Vendor-specific properties
>>>>>>> +6. status (if applicable)
>>>>>>> +7. Child nodes, where each node is preceded with a blank line
>>>>>>> +
>>>>>>> +The "status" property is by default "okay", thus it can be
>>>>>>> omitted.
>>>>>>
>>>>>> I think it would really help to include position of #address-cells
>>>>>> and
>>>>>> #size-cells here. In some files I saw them above "compatible" that
>>>>>> seems
>>>>>> unintuitive. Some prefer putting them at end which I think makes
>>>>>> sense
>>>>>> as they affect children nodes.
>>>>>>
>>>>>> Whatever you choose it'd be just nice to have things consistent.
>>>>>
>>>>> This is a standard/common property, thus it goes to (4) above.
>>>>
>>>> It's probably a mix, but AFAIK a lot of the device trees in tree
>>>> have
>>>> #*-cells after "status". In some cases they are added in the board
>>>> .dts files, not the chip/module .dtsi files.
>>>
>>> Existing DTS is not a good example :)
>>>
>>>>
>>>> +1 that it makes sense at the end as they affect child nodes.
>>>
>>> I still insist that status must be the last, because:
>>> 1. Many SoC nodes have address/size cells but do not have any
>>> children
>>> (I2C, SPI), so we put useless information at the end.
>>> 2. Status should be the final information to say whether the node is
>>> ready or is not. I read the node, check properties and then look at
>>> the end:
>>> a. Lack of status means it is ready.
>>> b. status=disabled means device still needs board
>>> resources/customization
>>
>> I agree with the "status" belonging to the very end, because it's both
>> logical and much more readable. Also, "status" is expected to be
>> modified in the dependent DT files, which makes it kind of volatile
>> and even more deserving to be placed last.
>
> I am just curious if having status property at the end won't affect
> execution/boot up time. Not sure how it is done in Linux but in U-Boot
> at least (we want to have DTs in sync between Linux and U-Boot)
> of_find_property is pretty much big loop over all properties. And
> status property defined at the end means going over all of them to
> find it out to if device is present.
> Not sure if Linux works in the same way but at least of_get_property
> is done in the same way.
>
> It is not big deal on high speed cpus but wanted to point it out.

That's a good point, saving every possible CPU cycle counts, so if we
can exit early, why not. However, that's perhaps something to be
handled within the dtc utility, by having it rearrange the properties.
I'll investigate that in detail.

2023-11-22 09:11:35

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 22/11/2023 09:57, Michal Simek wrote:
> Hi Geert,
>
> On 11/22/23 09:53, Geert Uytterhoeven wrote:
>> Hi Michal,
>>
>> On Wed, Nov 22, 2023 at 9:50 AM Michal Simek <[email protected]> wrote:
>>> On 11/22/23 09:29, Dragan Simic wrote:
>>>> On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
>>>>> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
>>>>>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
>>>>>>>>> +Order of Properties in Device Node
>>>>>>>>> +----------------------------------
>>>>>>>>> +
>>>>>>>>> +Following order of properties in device nodes is preferred:
>>>>>>>>> +
>>>>>>>>> +1. compatible
>>>>>>>>> +2. reg
>>>>>>>>> +3. ranges
>>>>>>>>> +4. Standard/common properties (defined by common bindings, e.g. without
>>>>>>>>> + vendor-prefixes)
>>>>>>>>> +5. Vendor-specific properties
>>>>>>>>> +6. status (if applicable)
>>>>>>>>> +7. Child nodes, where each node is preceded with a blank line
>>>>>>>>> +
>>>>>>>>> +The "status" property is by default "okay", thus it can be omitted.
>>>>>>>>
>>>>>>>> I think it would really help to include position of #address-cells and
>>>>>>>> #size-cells here. In some files I saw them above "compatible" that seems
>>>>>>>> unintuitive. Some prefer putting them at end which I think makes sense
>>>>>>>> as they affect children nodes.
>>>>>>>>
>>>>>>>> Whatever you choose it'd be just nice to have things consistent.
>>>>>>>
>>>>>>> This is a standard/common property, thus it goes to (4) above.
>>>>>>
>>>>>> It's probably a mix, but AFAIK a lot of the device trees in tree have
>>>>>> #*-cells after "status". In some cases they are added in the board
>>>>>> .dts files, not the chip/module .dtsi files.
>>>>>
>>>>> Existing DTS is not a good example :)
>>>>>
>>>>>>
>>>>>> +1 that it makes sense at the end as they affect child nodes.
>>>>>
>>>>> I still insist that status must be the last, because:
>>>>> 1. Many SoC nodes have address/size cells but do not have any children
>>>>> (I2C, SPI), so we put useless information at the end.
>>>>> 2. Status should be the final information to say whether the node is
>>>>> ready or is not. I read the node, check properties and then look at the end:
>>>>> a. Lack of status means it is ready.
>>>>> b. status=disabled means device still needs board resources/customization
>>>>
>>>> I agree with the "status" belonging to the very end, because it's both logical
>>>> and much more readable. Also, "status" is expected to be modified in the
>>>> dependent DT files, which makes it kind of volatile and even more deserving to
>>>> be placed last.
>>>
>>> I am just curious if having status property at the end won't affect
>>> execution/boot up time. Not sure how it is done in Linux but in U-Boot at least
>>> (we want to have DTs in sync between Linux and U-Boot) of_find_property is
>>> pretty much big loop over all properties. And status property defined at the end
>>> means going over all of them to find it out to if device is present.
>>> Not sure if Linux works in the same way but at least of_get_property is done in
>>> the same way.
>>
>> As the default is "okay", you have to loop over all properties anyway.
>
> No doubt if you don't define status property that you need to loop over all of
> them. We normally describe the whole SOC with pretty much all IPs status =
> disabled and then in board file we are changing it to okay based on what it is
> actually wired out.
> It means on our systems all nodes have status properties. If you have it at
> first you don't need to go over all.

We never sacrificed code readability in favor of code execution speed,
so neither should we do it here.

If the speed is a problem, project can still add a flag to dtc to
re-shuffle properties in FDT depending on its needs.

Best regards,
Krzysztof

2023-11-22 14:36:30

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On Wed, Nov 22, 2023 at 1:57 AM Michal Simek <[email protected]> wrote:
>
> Hi Geert,
>
> On 11/22/23 09:53, Geert Uytterhoeven wrote:
> > Hi Michal,
> >
> > On Wed, Nov 22, 2023 at 9:50 AM Michal Simek <[email protected]> wrote:
> >> On 11/22/23 09:29, Dragan Simic wrote:
> >>> On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
> >>>> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
> >>>>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
> >>>>> <[email protected]> wrote:
> >>>>>>
> >>>>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
> >>>>>>>> +Order of Properties in Device Node
> >>>>>>>> +----------------------------------
> >>>>>>>> +
> >>>>>>>> +Following order of properties in device nodes is preferred:
> >>>>>>>> +
> >>>>>>>> +1. compatible
> >>>>>>>> +2. reg
> >>>>>>>> +3. ranges
> >>>>>>>> +4. Standard/common properties (defined by common bindings, e.g. without
> >>>>>>>> + vendor-prefixes)
> >>>>>>>> +5. Vendor-specific properties
> >>>>>>>> +6. status (if applicable)
> >>>>>>>> +7. Child nodes, where each node is preceded with a blank line
> >>>>>>>> +
> >>>>>>>> +The "status" property is by default "okay", thus it can be omitted.
> >>>>>>>
> >>>>>>> I think it would really help to include position of #address-cells and
> >>>>>>> #size-cells here. In some files I saw them above "compatible" that seems
> >>>>>>> unintuitive. Some prefer putting them at end which I think makes sense
> >>>>>>> as they affect children nodes.
> >>>>>>>
> >>>>>>> Whatever you choose it'd be just nice to have things consistent.
> >>>>>>
> >>>>>> This is a standard/common property, thus it goes to (4) above.
> >>>>>
> >>>>> It's probably a mix, but AFAIK a lot of the device trees in tree have
> >>>>> #*-cells after "status". In some cases they are added in the board
> >>>>> .dts files, not the chip/module .dtsi files.
> >>>>
> >>>> Existing DTS is not a good example :)
> >>>>
> >>>>>
> >>>>> +1 that it makes sense at the end as they affect child nodes.
> >>>>
> >>>> I still insist that status must be the last, because:
> >>>> 1. Many SoC nodes have address/size cells but do not have any children
> >>>> (I2C, SPI), so we put useless information at the end.
> >>>> 2. Status should be the final information to say whether the node is
> >>>> ready or is not. I read the node, check properties and then look at the end:
> >>>> a. Lack of status means it is ready.
> >>>> b. status=disabled means device still needs board resources/customization
> >>>
> >>> I agree with the "status" belonging to the very end, because it's both logical
> >>> and much more readable. Also, "status" is expected to be modified in the
> >>> dependent DT files, which makes it kind of volatile and even more deserving to
> >>> be placed last.
> >>
> >> I am just curious if having status property at the end won't affect
> >> execution/boot up time. Not sure how it is done in Linux but in U-Boot at least
> >> (we want to have DTs in sync between Linux and U-Boot) of_find_property is
> >> pretty much big loop over all properties. And status property defined at the end
> >> means going over all of them to find it out to if device is present.
> >> Not sure if Linux works in the same way but at least of_get_property is done in
> >> the same way.
> >
> > As the default is "okay", you have to loop over all properties anyway.
>
> No doubt if you don't define status property that you need to loop over all of
> them. We normally describe the whole SOC with pretty much all IPs status =
> disabled and then in board file we are changing it to okay based on what it is
> actually wired out.
> It means on our systems all nodes have status properties. If you have it at
> first you don't need to go over all.

Order in the source and order in the OS are independent. If checking
status needs to be optimized, then we could just put it first in the
property list or make the state a field in struct device_node. But
provide some data that it matters first.

I've had this idea to randomize the order nodes are processed so
there's no reliance on the DT order. Maybe I need the same on
properties...

Rob

2023-11-22 14:43:23

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On 2023-11-22 15:34, Rob Herring wrote:
> On Wed, Nov 22, 2023 at 1:57 AM Michal Simek <[email protected]>
> wrote:
>> On 11/22/23 09:53, Geert Uytterhoeven wrote:
>> > On Wed, Nov 22, 2023 at 9:50 AM Michal Simek <[email protected]> wrote:
>> >> On 11/22/23 09:29, Dragan Simic wrote:
>> >>> On 2023-11-22 09:21, Krzysztof Kozlowski wrote:
>> >>>> On 22/11/2023 09:09, Chen-Yu Tsai wrote:
>> >>>>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski
>> >>>>> <[email protected]> wrote:
>> >>>>>>
>> >>>>>> On 21/11/2023 14:50, Rafał Miłecki wrote:
>> >>>>>>>> +Order of Properties in Device Node
>> >>>>>>>> +----------------------------------
>> >>>>>>>> +
>> >>>>>>>> +Following order of properties in device nodes is preferred:
>> >>>>>>>> +
>> >>>>>>>> +1. compatible
>> >>>>>>>> +2. reg
>> >>>>>>>> +3. ranges
>> >>>>>>>> +4. Standard/common properties (defined by common bindings, e.g. without
>> >>>>>>>> + vendor-prefixes)
>> >>>>>>>> +5. Vendor-specific properties
>> >>>>>>>> +6. status (if applicable)
>> >>>>>>>> +7. Child nodes, where each node is preceded with a blank line
>> >>>>>>>> +
>> >>>>>>>> +The "status" property is by default "okay", thus it can be omitted.
>> >>>>>>>
>> >>>>>>> I think it would really help to include position of #address-cells and
>> >>>>>>> #size-cells here. In some files I saw them above "compatible" that seems
>> >>>>>>> unintuitive. Some prefer putting them at end which I think makes sense
>> >>>>>>> as they affect children nodes.
>> >>>>>>>
>> >>>>>>> Whatever you choose it'd be just nice to have things consistent.
>> >>>>>>
>> >>>>>> This is a standard/common property, thus it goes to (4) above.
>> >>>>>
>> >>>>> It's probably a mix, but AFAIK a lot of the device trees in tree have
>> >>>>> #*-cells after "status". In some cases they are added in the board
>> >>>>> .dts files, not the chip/module .dtsi files.
>> >>>>
>> >>>> Existing DTS is not a good example :)
>> >>>>
>> >>>>>
>> >>>>> +1 that it makes sense at the end as they affect child nodes.
>> >>>>
>> >>>> I still insist that status must be the last, because:
>> >>>> 1. Many SoC nodes have address/size cells but do not have any children
>> >>>> (I2C, SPI), so we put useless information at the end.
>> >>>> 2. Status should be the final information to say whether the node is
>> >>>> ready or is not. I read the node, check properties and then look at the end:
>> >>>> a. Lack of status means it is ready.
>> >>>> b. status=disabled means device still needs board resources/customization
>> >>>
>> >>> I agree with the "status" belonging to the very end, because it's both logical
>> >>> and much more readable. Also, "status" is expected to be modified in the
>> >>> dependent DT files, which makes it kind of volatile and even more deserving to
>> >>> be placed last.
>> >>
>> >> I am just curious if having status property at the end won't affect
>> >> execution/boot up time. Not sure how it is done in Linux but in U-Boot at least
>> >> (we want to have DTs in sync between Linux and U-Boot) of_find_property is
>> >> pretty much big loop over all properties. And status property defined at the end
>> >> means going over all of them to find it out to if device is present.
>> >> Not sure if Linux works in the same way but at least of_get_property is done in
>> >> the same way.
>> >
>> > As the default is "okay", you have to loop over all properties anyway.
>>
>> No doubt if you don't define status property that you need to loop
>> over all of
>> them. We normally describe the whole SOC with pretty much all IPs
>> status =
>> disabled and then in board file we are changing it to okay based on
>> what it is
>> actually wired out.
>> It means on our systems all nodes have status properties. If you have
>> it at
>> first you don't need to go over all.
>
> Order in the source and order in the OS are independent. If checking
> status needs to be optimized, then we could just put it first in the
> property list or make the state a field in struct device_node. But
> provide some data that it matters first.

That's exactly what I plan to do, i.e. to perform some benchmarks before
and after, to see does it actually matter to the point where introducing
the changes is worth it.

> I've had this idea to randomize the order nodes are processed so
> there's no reliance on the DT order. Maybe I need the same on
> properties...

2023-11-22 14:57:26

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document

On Wed, Nov 22, 2023 at 1:05 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 21/11/2023 14:50, Rafał Miłecki wrote:
> >> +Order of Nodes
> >> +--------------
> >> +
> >> +1. Nodes within any bus, thus using unit addresses for children, shall be
> >> + ordered incrementally by unit address.
> >> + Alternatively for some sub-architectures, nodes of the same type can be
> >> + grouped together (e.g. all I2C controllers one after another even if this
> >> + breaks unit address ordering).
> >> +
> >> +2. Nodes without unit addresses should be ordered alpha-numerically by the node
> >> + name. For a few types of nodes, they can be ordered by the main property
> >> + (e.g. pin configuration states ordered by value of "pins" property).
> >> +
> >> +3. When extending nodes in the board DTS via &label, the entries should be
> >> + ordered alpha-numerically.
> >
> > Just an idea. Would that make (more) sense to make &label-like entries
> > match order of nodes in included .dts(i)?
> >
> > Adventages:
> > 1. We keep unit address incremental order that is unlikely to change
> >
> > Disadventages:
> > 1. More difficult to verify
>
> Rob also proposed this and I believe above disadvantage here is crucial.
> If you add new SoC with board DTS you are fine. But if you add only new
> board, the order of entries look random in the diff hunk. Reviewer must
> open SoC DTSI to be able to review the patch with board DTS.
>
> If review is tricky and we do not have tool to perform it automatically,
> I am sure submissions will have disordered board DTS.

I'm certainly in favor of only (or mostly?) specifying things we can
check with tools. I don't need more to check manually...

It wouldn't be too hard to get path from labels. dtc generates that
with -@ already. So I don't buy "we don't have a tool" if a tool to
check seems feasible.

Rob

2023-11-25 18:45:41