Re: [PATCH v2 1/9] dt-bindings: spi: Add STM32 OSPI controller

[Conor Dooley]

On Tue, Jan 28, 2025 at 09:17:23AM +0100, patrice.chotard@xxxxxxxxxxx wrote:
> From: Patrice Chotard <patrice.chotard@xxxxxxxxxxx>
> 
> Add device tree bindings for the STM32 OSPI controller.
> 
> Main features of the Octo-SPI controller :
>   - support sNOR / sNAND / HyperRAM™ and HyperFlash™ devices.
>   - Three functional modes: indirect, automatic-status polling,
>     memory-mapped.
>   - Up to 4 Gbytes of external memory can be addressed in indirect
>     mode (per physical port and per CS), and up to 256 Mbytes in
>     memory-mapped mode (combined for both physical ports and per CS).
>   - Single-, dual-, quad-, and octal-SPI communication.
>   - Dual-quad communication.
>   - Single data rate (SDR) and double transfer rate (DTR).
>   - Maximum target frequency is 133 MHz for SDR and 133 MHz for DTR.
>   - Data strobe support.
>   - DMA channel for indirect mode.
>   - Double CS mapping that allows two external flash devices to be
>     addressed with a single OCTOSPI controller mapped on a single
>     OCTOSPI port.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@xxxxxxxxxxx>
> ---
>  .../bindings/spi/st,stm32mp25-ospi.yaml       | 102 ++++++++++++++++++
>  1 file changed, 102 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml b/Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml
> new file mode 100644
> index 000000000000..f1d539444673
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml
> @@ -0,0 +1,102 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/st,stm32mp25-ospi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectronics STM32 Octal Serial Peripheral Interface (OSPI)
> +
> +maintainers:
> +  - Patrice Chotard <patrice.chotard@xxxxxxxxxxx>
> +
> +allOf:
> +  - $ref: spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: st,stm32mp25-ospi
> +
> +  reg:
> +    maxItems: 1
> +
> +  memory-region:
> +    maxItems: 1

Whatever about not having descriptions for clocks or reg when there's
only one, I think a memory region should be explained.

> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  resets:
> +    items:
> +      - description: phandle to OSPI block reset
> +      - description: phandle to delay block reset
> +
> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  st,syscfg-dlyb:
> +    description: phandle to syscon block
> +      Use to set the OSPI delay block within syscon to
> +      tune the phase of the RX sampling clock (or DQS) in order
> +      to sample the data in their valid window and to
> +      tune the phase of the TX launch clock in order to meet setup
> +      and hold constraints of TX signals versus the memory clock.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

Why do you need a phandle here? I assume looking up by compatible ain't
possible because you have multiple controllers on the SoC? Also, I don't
think your copy-paste "phandle to" stuff here is accurate:
      st,syscfg-dlyb = <&syscfg 0x1000>;
There's an offset here that you don't mention in your description.

> +    items:
> +      maxItems: 1
> +
> +  access-controllers:
> +    description: phandle to the rifsc device to check access right
> +      and in some cases, an additional phandle to the rcc device for
> +      secure clock control

This should be described using items rather than a free-form list.

> +    minItems: 1
> +    maxItems: 2
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +  - st,syscfg-dlyb
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
> +    spi@40430000 {

nit: you missing a blank line here.

> +      compatible = "st,stm32mp25-ospi";
> +      reg = <0x40430000 0x400>;
> +      memory-region = <&mm_ospi1>;
> +      interrupts = <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>;
> +      dmas = <&hpdma 2 0x62 0x00003121 0x0>,
> +             <&hpdma 2 0x42 0x00003112 0x0>;
> +      dma-names = "tx", "rx";
> +      clocks = <&scmi_clk CK_SCMI_OSPI1>;
> +      resets = <&scmi_reset RST_SCMI_OSPI1>, <&scmi_reset RST_SCMI_OSPI1DLL>;
> +      access-controllers = <&rifsc 74>;
> +      power-domains = <&CLUSTER_PD>;
> +      st,syscfg-dlyb = <&syscfg 0x1000>;
> +
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      flash@0 {
> +        compatible = "jedec,spi-nor";
> +        reg = <0>;
> +        spi-rx-bus-width = <4>;
> +        spi-max-frequency = <108000000>;
> +      };
> +    };
> -- 
> 2.25.1
>

Attachment: signature.asc
Description: PGP signature