diff mbox series

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

Message ID 20250128081731.2284457-2-patrice.chotard@foss.st.com (mailing list archive)
State New
Headers show
Series Add STM32MP25 SPI NOR support | expand

Commit Message

Patrice CHOTARD Jan. 28, 2025, 8:17 a.m. UTC
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

Comments

Conor Dooley Jan. 28, 2025, 6:02 p.m. UTC | #1
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.

> +
> +  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
>
Krzysztof Kozlowski Jan. 29, 2025, 7:40 a.m. UTC | #2
On Tue, Jan 28, 2025 at 06:02:27PM +0000, Conor Dooley wrote:
> > +  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.

This needs double items: and listing them with description, instead of
free form text.

Best regards,
Krzysztof
Krzysztof Kozlowski Jan. 29, 2025, 7:53 a.m. UTC | #3
On 29/01/2025 08:40, Krzysztof Kozlowski wrote:
> On Tue, Jan 28, 2025 at 06:02:27PM +0000, Conor Dooley wrote:
>>> +  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.
> 
> This needs double items: and listing them with description, instead of
> free form text.

And I asked for this last time and I told you how to find examples doing
this. In other binding patch you did not implement all the comments either.

Best regards,
Krzysztof
Patrice CHOTARD Jan. 29, 2025, 5:40 p.m. UTC | #4
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
>>
Conor Dooley Jan. 29, 2025, 5:53 p.m. UTC | #5
On Wed, Jan 29, 2025 at 06:40:23PM +0100, Patrice CHOTARD wrote:
> 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:
> >> +  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: |

The | isn't needed here.

>       Memory region to be used for memory-map read access.

I don't think that's a good explanation, sorry. Why's a memory-region
required for 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.

:+1: 

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

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

These updates look fine to me.
Patrice CHOTARD Jan. 30, 2025, 8:51 a.m. UTC | #6
On 1/29/25 18:53, Conor Dooley wrote:
> On Wed, Jan 29, 2025 at 06:40:23PM +0100, Patrice CHOTARD wrote:
>> 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:
>>>> +  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: |
> 
> The | isn't needed here.
ok

> 
>>       Memory region to be used for memory-map read access.
> 
> I don't think that's a good explanation, sorry. Why's a memory-region
> required for read access?

The OCTOSPI interface support 3 functional modes: 
  _ indirect
  _ automatic polling status
  _ memory-mapped

256MB are reserved in the CPU memory map for the 2 OCTOSPI instance.
This area is used when OCTOSPI1 and/or OCTOSPI2 operate in memory-mapped
mode. In this mode, read access are performed from the memory device using
the direct mapping.

Thanks
Patrice

> 
>>>> +
>>>> +  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.
> 
> :+1: 
> 
>>> 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
> 
> :+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
> 
> These updates look fine to me.
Patrice CHOTARD Jan. 30, 2025, 9:48 a.m. UTC | #7
On 1/29/25 08:40, Krzysztof Kozlowski wrote:
> On Tue, Jan 28, 2025 at 06:02:27PM +0000, Conor Dooley wrote:
>>> +  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.
> 
> This needs double items: and listing them with description, instead of
> free form text.

ok, i will remove most of ths text description and update as following :

  st,syscfg-dlyb:
    description: configure OCTOSPI delay block.
    $ref: /schemas/types.yaml#/definitions/phandle-array
    items:
      - description: phandle to syscfg
      - description: register offset within syscfg

Thanks
Patrice
> 
> Best regards,
> Krzysztof
>
Patrice CHOTARD Jan. 30, 2025, 10:28 a.m. UTC | #8
On 1/29/25 18:53, Conor Dooley wrote:
> On Wed, Jan 29, 2025 at 06:40:23PM +0100, Patrice CHOTARD wrote:
>> 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:
>>>> +  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: |
> 
> The | isn't needed here.
> 
>>       Memory region to be used for memory-map read access.
> 
> I don't think that's a good explanation, sorry. Why's a memory-region
> required for 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.
> 
> :+1: 
> 
>>> 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
> 
> :+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
> 
> These updates look fine to me.

I got an issue with access-controllers property.

i can't list the 2 items (the second one is optional) and use minItems and maxItems.

For example:

 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
      - description: phandle to clock controller
    minItems: 1
    maxItems: 2


make dt_binding_check DT_SCHEMA_FILES=st,stm32mp25-ospi.yaml

Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml: properties: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'}, {'description': 'phandle to clock controller'}], 'minItems': 1, 'maxItems': 2} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
  DTC [C] Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.example.dtb

How can i indicate that at least one items is mandatory, the second one is optional and in the same
time describing the both items as required without getting the above error ? 


On other yaml files, for examples 
/usb/dwc2.yaml 
spi/st,stm32-qspi.yaml
spi/st,stm32-spi.yaml
sound/st,stm32-i2s.yaml
st,stm32-spdifrx.yaml
sound/st,stm32-sai.yam
serial/st,stm32-uart.yaml
rng/st,stm32-rng.yaml
regulator/st,stm32-vrefbuf.yaml
mfd/st,stm32-timers.yaml
.....

The only yaml given description is :

access-controllers:
  minItems: 1
  maxItems: 2

Thanks
Patrice
Krzysztof Kozlowski Jan. 30, 2025, 12:26 p.m. UTC | #9
On 30/01/2025 11:28, Patrice CHOTARD wrote:
> For example:
> 
>  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
>       - description: phandle to clock controller
>     minItems: 1
>     maxItems: 2
> 
> 
> make dt_binding_check DT_SCHEMA_FILES=st,stm32mp25-ospi.yaml
> 
> Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml: properties: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'}, {'description': 'phandle to clock controller'}], 'minItems': 1, 'maxItems': 2} should not be valid under {'required': ['maxItems']}
> 	hint: "maxItems" is not needed with an "items" list
> 	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
>   DTC [C] Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.example.dtb
> 
> How can i indicate that at least one items is mandatory, the second one is optional and in the same
> time describing the both items as required without getting the above error ? 

maxItems is redundant.

Best regards,
Krzysztof
Patrice CHOTARD Jan. 30, 2025, 12:39 p.m. UTC | #10
On 1/30/25 13:26, Krzysztof Kozlowski wrote:
> On 30/01/2025 11:28, Patrice CHOTARD wrote:
>> For example:
>>
>>  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
>>       - description: phandle to clock controller
>>     minItems: 1
>>     maxItems: 2
>>
>>
>> make dt_binding_check DT_SCHEMA_FILES=st,stm32mp25-ospi.yaml
>>
>> Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml: properties: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'}, {'description': 'phandle to clock controller'}], 'minItems': 1, 'maxItems': 2} should not be valid under {'required': ['maxItems']}
>> 	hint: "maxItems" is not needed with an "items" list
>> 	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
>>   DTC [C] Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.example.dtb
>>
>> How can i indicate that at least one items is mandatory, the second one is optional and in the same
>> time describing the both items as required without getting the above error ? 
> 
> maxItems is redundant.

ok, it solves the issue

Thanks
Patrice


> 
> Best regards,
> Krzysztof
diff mbox series

Patch

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
+
+  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
+    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
+    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 {
+      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>;
+      };
+    };