Re: [PATCH] [v1] dt-bindings: timer: Add HPE GXP Timer binding

[Krzysztof Kozlowski]

On 15/02/2022 19:19, nick.hawkins@xxxxxxx wrote:
> From: Nick Hawkins <nick.hawkins@xxxxxxx>
> 
> Description: Creating binding for the GXP timer to be used in device tree.
>  GXP is the name of the HPE SoC.
>  This SoC is used to implement BMC features of HPE servers
>   (all ProLiant, Synergy, and many Apollo, and Superdome machines)
>    It does support many features including:
>         ARMv7 architecture, and it is based on a Cortex A9 core
>         Use an AXI bus to which
>                 a memory controller is attached, as well as
>                  multiple SPI interfaces to connect boot flash,
>                  and ROM flash, a 10/100/1000 Mac engine which
>                  supports SGMII (2 ports) and RMII
>                 Multiple I2C engines to drive connectivity with a
> 				 host infrastructure
>                 A video engine which support VGA and DP, as well as
>                  an hardware video encoder
>                 Multiple PCIe ports
>                 A PECI interface, and LPC eSPI
>                 Multiple UART for debug purpose, and Virtual UART for
>                  host connectivity
>                 A GPIO engine.

Same comment as for other patches.

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@xxxxxxx>
> ---
>  .../bindings/timer/hpe,gxp-timer.yaml         | 45 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> new file mode 100644
> index 000000000000..1f4e345c5fb8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/hpe,gxp-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP TIMER
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@xxxxxxx>
> +  - Jean-Marie Verdun <verdun@xxxxxxx>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-timer
> +
> +  reg:
> +    items:
> +      - description: T0CNT register
> +      - description: T0CS register
> +      - description: TIMELO register

That's a bit odd - you pass each register as device address space. Why
not entire address space of the timer block? Probably you have there
more registers, don't you? In such  case what if you need to access that
additional register - change bindings?

Or maybe all these registers can be put in entirely different address
ranges?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: The frequency of the clock that drives the counter, in Hz.

I would expect here also "clocks" and "clock-names" properties, since
you clearly have a clock that driver the counter.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clock-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    timer@10003000 {
> +        compatible = "hpe,gxp-timer";
> +        reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> +        interrupts = <0>;
> +        interrupt-parent = <&vic0>;
> +        clock-frequency = <400000000>;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f41088418aae..8c2c1e8e0934 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8385,6 +8385,12 @@ L:	linux-efi@xxxxxxxxxxxxxxx
>  S:	Maintained
>  F:	block/partitions/efi.*
>  
> +GXP TIMER
> +M:	Nick Hawkins <nick.hawkins@xxxxxxx>
> +M:	Jean-Marie Verdun <verdun@xxxxxxx>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> +
>  H8/300 ARCHITECTURE
>  M:	Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
>  L:	uclinux-h8-devel@xxxxxxxxxxxxxxxxxxxx (moderated for non-subscribers)


Best regards,
Krzysztof