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

[Patrice CHOTARD <patrice.chotard@foss.st.com>]

On 1/28/25 19:02, Conor Dooley wrote:
> On Tue, Jan 28, 2025 at 09:17:23AM +0100, patrice.chotard@foss.st.com wrote:
>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>
>> 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@foss.st.com>
>> ---
>>  .../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@foss.st.com>
>> +
>> +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.

ok i will add :

    description: |
      Memory region to be used for memory-map read access.

> 
>> +
>> +  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

Yes, we got 2 OCTOSPI controller, each of them have a dedicated delay block
 syscfg register.

> 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.

I will add it as following:

  st,syscfg-dlyb:
    description:
      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
    items:
      - description: phandle to syscfg
      - description: register offset within syscfg


> 
>> +    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.

  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
    items:
      - description: phandle to bus controller or to clock controller
      - description: access controller specifier
     minItems: 1
     maxItems: 2

> 
>> +    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
>>