[1/4] dt-bindings: reset: add generic bit reset controller

Commit Message

Inochi Amaoto Feb. 13, 2025, 2:08 a.m. UTC

Some SoCs from Aspeed, Allwinner, Sophgo and Synopsys have
a simple reset controller by toggling bit. It is a hard time
for each device to add its own compatible to the driver.
Since this device share a common design, it is possible to
add a common device to reduce these unnecessary change.

Add common binding for these kind generic reset controller.

Signed-off-by: Inochi Amaoto <[e-mail]>
---
 .../bindings/reset/reset-simple.yaml          | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/reset-simple.yaml

Comments

Krzysztof Kozlowski Feb. 13, 2025, 9:35 a.m. UTC | #1

On Thu, Feb 13, 2025 at 10:08:54AM +0800, Inochi Amaoto wrote:
> Some SoCs from Aspeed, Allwinner, Sophgo and Synopsys have
> a simple reset controller by toggling bit. It is a hard time
> for each device to add its own compatible to the driver.
> Since this device share a common design, it is possible to
> add a common device to reduce these unnecessary change.

SoC components are rarely that simple and even if it is just a bit,
usually it is part of one or few registers.

Anyway, there are already bindings for reset-simple and I do not
understand why this has to be duplicated.

> 
> Add common binding for these kind generic reset controller.
> 
> Signed-off-by: Inochi Amaoto <[e-mail]>
> ---
>  .../bindings/reset/reset-simple.yaml          | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/reset-simple.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/reset-simple.yaml b/Documentation/devicetree/bindings/reset/reset-simple.yaml
> new file mode 100644
> index 000000000000..77584e23e8e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/reset-simple.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/reset-simple.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic BIT Reset Controller
> +
> +maintainers:
> +  - Inochi Amaoto <[e-mail]>
> +
> +description:
> +  Some reset controller devices uses a simple method to perform
> +  assert/deassert by toggling bit. Some SoCs from Aspeed, Allwinner,
> +  Sophgo and Synopsys have this kind of reset controller instances.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - reset-simple-high
> +      - reset-simple-low

It would be one compatible and set of properties describing resets.

Best regards,
Krzysztof

Philipp Zabel Feb. 13, 2025, 10:08 a.m. UTC | #2

On Do, 2025-02-13 at 10:08 +0800, Inochi Amaoto wrote:
> Some SoCs from Aspeed, Allwinner, Sophgo and Synopsys have
> a simple reset controller by toggling bit. It is a hard time
> for each device to add its own compatible to the driver.
> Since this device share a common design, it is possible to
> add a common device to reduce these unnecessary change.
> 
> Add common binding for these kind generic reset controller.
> 
> Signed-off-by: Inochi Amaoto <[e-mail]>
> ---
>  .../bindings/reset/reset-simple.yaml          | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/reset-simple.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/reset-simple.yaml b/Documentation/devicetree/bindings/reset/reset-simple.yaml
> new file mode 100644
> index 000000000000..77584e23e8e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/reset-simple.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/reset-simple.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic BIT Reset Controller
> +
> +maintainers:
> +  - Inochi Amaoto <[e-mail]>
> +
> +description:
> +  Some reset controller devices uses a simple method to perform
> +  assert/deassert by toggling bit. Some SoCs from Aspeed, Allwinner,
> +  Sophgo and Synopsys have this kind of reset controller instances.

I think some properties should be documented that make reset
controllers "simple" according to this binding.

For example, right now, the reset-simple driver assumes the following:

  - There is a single, contiguous range of 32-bit registers.
  - All bits in each register directly control a reset line.
     - There are no self-deasserting resets.
     - There are no timing requirements.
     - The bits are exclusively resets, nothing else.
  - All bits behave the same, so all reset bits are either
    active-high or all are active-low.
  - The bits can be read back, but the read status may
    be active-low independently from the writes.

> +properties:
> +  compatible:
> +    enum:
> +      - reset-simple-high
> +      - reset-simple-low

I wonder if it would be better to have a single

  compatible:
    const: reset-simple

and a boolean property, e.g.

  active-low:
    type: boolean

like in leds/common.yaml. Also it should be documented clearly what
this means for reads and writes.

> +  reg:
> +    maxItems: 1
> +
> +  "#reset-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#reset-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    reset-controller@1000000 {
> +        compatible = "reset-simple-high";

The example should probably include a SoC specific compatible?

regards
Philipp

Philipp Zabel Feb. 13, 2025, 10:22 a.m. UTC | #3

On Do, 2025-02-13 at 10:35 +0100, Krzysztof Kozlowski wrote:
> On Thu, Feb 13, 2025 at 10:08:54AM +0800, Inochi Amaoto wrote:
> > Some SoCs from Aspeed, Allwinner, Sophgo and Synopsys have
> > a simple reset controller by toggling bit. It is a hard time
> > for each device to add its own compatible to the driver.
> > Since this device share a common design, it is possible to
> > add a common device to reduce these unnecessary change.
> 
> SoC components are rarely that simple and even if it is just a bit,
> usually it is part of one or few registers.

Yes, in those cases (which are probably most of them), I would argue
this binding doesn't really fit.

> Anyway, there are already bindings for reset-simple and I do not
> understand why this has to be duplicated.

I think the motivation is to not have to add a new binding document and
modify reset-simple.c every time there is a new SoC. I wonder if some
of this can be mitigated by adding just the binding document similar to
trivial-devices.yaml, without the actual "reset-simple" compatible.

> 
regards
Philipp

Inochi Amaoto Feb. 13, 2025, 11:18 a.m. UTC | #4

On Thu, Feb 13, 2025 at 11:22:07AM +0100, Philipp Zabel wrote:
> On Do, 2025-02-13 at 10:35 +0100, Krzysztof Kozlowski wrote:
> > On Thu, Feb 13, 2025 at 10:08:54AM +0800, Inochi Amaoto wrote:
> > > Some SoCs from Aspeed, Allwinner, Sophgo and Synopsys have
> > > a simple reset controller by toggling bit. It is a hard time
> > > for each device to add its own compatible to the driver.
> > > Since this device share a common design, it is possible to
> > > add a common device to reduce these unnecessary change.
> > 
> > SoC components are rarely that simple and even if it is just a bit,
> > usually it is part of one or few registers.
> 
> Yes, in those cases (which are probably most of them), I would argue
> this binding doesn't really fit.
> 

Yes, I agree. 

> > Anyway, there are already bindings for reset-simple and I do not
> > understand why this has to be duplicated.
> 
> I think the motivation is to not have to add a new binding document and
> modify reset-simple.c every time there is a new SoC. 

Yeah, this is the motivation for me to write this patch. It seems that I
describe it in a wrong way.

> I wonder if some of this can be mitigated by adding just the 
> binding document similar to trivial-devices.yaml, without the
> actual "reset-simple" compatible.
> 

It is better to keep the original compatible. Adding new base compatible
will break existing device and make the migration hard.

Regards,
Inochi

Inochi Amaoto Feb. 13, 2025, 11:26 a.m. UTC | #5

On Thu, Feb 13, 2025 at 11:08:59AM +0100, Philipp Zabel wrote:
> On Do, 2025-02-13 at 10:08 +0800, Inochi Amaoto wrote:
> > Some SoCs from Aspeed, Allwinner, Sophgo and Synopsys have
> > a simple reset controller by toggling bit. It is a hard time
> > for each device to add its own compatible to the driver.
> > Since this device share a common design, it is possible to
> > add a common device to reduce these unnecessary change.
> > 
> > Add common binding for these kind generic reset controller.
> > 
> > Signed-off-by: Inochi Amaoto <[e-mail]>
> > ---
> >  .../bindings/reset/reset-simple.yaml          | 42 +++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reset/reset-simple.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/reset/reset-simple.yaml b/Documentation/devicetree/bindings/reset/reset-simple.yaml
> > new file mode 100644
> > index 000000000000..77584e23e8e8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reset/reset-simple.yaml
> > @@ -0,0 +1,42 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reset/reset-simple.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic BIT Reset Controller
> > +
> > +maintainers:
> > +  - Inochi Amaoto <[e-mail]>
> > +
> > +description:
> > +  Some reset controller devices uses a simple method to perform
> > +  assert/deassert by toggling bit. Some SoCs from Aspeed, Allwinner,
> > +  Sophgo and Synopsys have this kind of reset controller instances.
> 
> I think some properties should be documented that make reset
> controllers "simple" according to this binding.
> 
> For example, right now, the reset-simple driver assumes the following:
> 
>   - There is a single, contiguous range of 32-bit registers.
>   - All bits in each register directly control a reset line.
>      - There are no self-deasserting resets.
>      - There are no timing requirements.
>      - The bits are exclusively resets, nothing else.
>   - All bits behave the same, so all reset bits are either
>     active-high or all are active-low.
>   - The bits can be read back, but the read status may
>     be active-low independently from the writes.
> 

Thanks, I will add these assumes in to the binding.

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - reset-simple-high
> > +      - reset-simple-low
> 
> I wonder if it would be better to have a single
> 
>   compatible:
>     const: reset-simple
> 
> and a boolean property, e.g.
> 
>   active-low:
>     type: boolean
> 
> like in leds/common.yaml. Also it should be documented clearly what
> this means for reads and writes.
> 

Yeah, it is better to have a property instead of defining a base
compatible. With this property, there are two ways to process this
property with existing device:

1. If device has defined the data, the active-low is ignored
2. If device has defined the data, the active-low will overwrite
   the device data.

I wonder which one is better?

> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#reset-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#reset-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    reset-controller@1000000 {
> > +        compatible = "reset-simple-high";
> 
> The example should probably include a SoC specific compatible?
> 
> regards
> Philipp

I think it is OK. But I think it should be added when a
specific compatible is coming.

Regards,
Inochi

Patch

diff --git a/Documentation/devicetree/bindings/reset/reset-simple.yaml b/Documentation/devicetree/bindings/reset/reset-simple.yaml
new file mode 100644
index 000000000000..77584e23e8e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/reset-simple.yaml
@@ -0,0 +1,42 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/reset-simple.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic BIT Reset Controller
+
+maintainers:
+  - Inochi Amaoto <[e-mail]>
+
+description:
+  Some reset controller devices uses a simple method to perform
+  assert/deassert by toggling bit. Some SoCs from Aspeed, Allwinner,
+  Sophgo and Synopsys have this kind of reset controller instances.
+
+properties:
+  compatible:
+    enum:
+      - reset-simple-high
+      - reset-simple-low
+
+  reg:
+    maxItems: 1
+
+  "#reset-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    reset-controller@1000000 {
+        compatible = "reset-simple-high";
+        reg = <0x1000000 0x1000>;
+        #reset-cells = <1>;
+    };