Re: [PATCH v5 1/3] dt-bindings: reset: microchip sparx5 reset driver bindings

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 10/02/2021 10:19:50+0100, Steen Hegelund wrote:
> Document the Sparx5 reset device driver bindings
> 
> The driver uses two IO ranges on sparx5 for access to
> the reset control and the reset status.
> 
> Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
> ---
>  .../bindings/reset/microchip,rst.yaml         | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/microchip,rst.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/microchip,rst.yaml b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> new file mode 100644
> index 000000000000..80046172c9f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/reset/microchip,rst.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Microchip Sparx5 Switch Reset Controller
> +
> +maintainers:
> +  - Steen Hegelund <steen.hegelund@microchip.com>
> +  - Lars Povlsen <lars.povlsen@microchip.com>
> +
> +description: |
> +  The Microchip Sparx5 Switch provides reset control and implements the following
> +  functions
> +    - One Time Switch Core Reset (Soft Reset)
> +
> +properties:
> +  $nodename:
> +    pattern: "^reset-controller@[0-9a-f]+$"
> +
> +  compatible:
> +    const: microchip,sparx5-switch-reset
> +
> +  reg:
> +    items:
> +      - description: cpu block registers
> +      - description: global control block registers
> +
> +  reg-names:
> +    items:
> +      - const: cpu
> +      - const: gcb
> +

I still think this is not right because then you will be mapping the
same set of register using multiple drivers without any form of
synchronisation which will work because you are mapping the region
without requesting it. But this may lead to issues later.

At least, you should make cpu start at 0x80 and of size 4. Else, you
won't be able to define and use the GPRs that are in front of
CPU_REGS:RESET.

I would still keep DEVCPU_GCB:CHIP_REGS as a syscon, especially since
you are mapping the whole set of registers.


> +  "#reset-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - "#reset-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    reset: reset-controller@0 {
> +        compatible = "microchip,sparx5-switch-reset";
> +        #reset-cells = <1>;
> +        reg = <0x0 0xd0>,
> +              <0x11010000 0x10000>;
> +        reg-names = "cpu", "gcb";
> +    };
> +
> -- 
> 2.30.0
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com