[PATCH v2 0/9] Add STM32MP25 SPI NOR support

[PATCH v2 0/9] Add STM32MP25 SPI NOR support

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

This series adds SPI NOR support for STM32MP25 SoCs from STMicroelectronics,
for that it adds support for:
  - Octo Memory Manager driver.
  - Octo SPI driver.
  - yaml schema for Octo Memory Manager and Octo SPI drivers.

The device tree files adds Octo Memory Manager and associated Octo SPI instances
in stm32mp251.dtsi and adds SPI NOR support in stm32mp257f-ev1 board.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>

Changes in v2:
  - Move STM32 Octo Memory Manager controller driver and bindings from
    misc to memory-controllers.
  - Update STM32 OSPI controller bindings.
  - Update STM32 Octo Memory Manager controller bindings.
  - Update STM32 Octo Memory Manager driver to match bindings update.
  - Update DT to match bindings update.

Patrice Chotard (9):
  dt-bindings: spi: Add STM32 OSPI controller
  spi: stm32: Add OSPI driver
  dt-bindings: memory-controllers: Add STM32 Octo Memory Manager
    controller
  memory: Add STM32 Octo Memory Manager driver
  arm64: dts: st: Add OMM node on stm32mp251
  arm64: dts: st: Add ospi port1 pinctrl entries in
    stm32mp25-pinctrl.dtsi
  arm64: dts: st: Add SPI NOR flash support on stm32mp257f-ev1 board
  arm64: defconfig: Enable STM32 Octo Memory Manager driver
  arm64: defconfig: Enable STM32 OctoSPI driver

 .../memory-controllers/st,stm32-omm.yaml      |  190 +++
 .../bindings/spi/st,stm32mp25-ospi.yaml       |  102 ++
 arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi |   51 +
 arch/arm64/boot/dts/st/stm32mp251.dtsi        |   48 +
 arch/arm64/boot/dts/st/stm32mp257f-ev1.dts    |   32 +
 arch/arm64/configs/defconfig                  |    2 +
 drivers/memory/Kconfig                        |   17 +
 drivers/memory/Makefile                       |    1 +
 drivers/memory/stm32_omm.c                    |  509 ++++++++
 drivers/spi/Kconfig                           |   10 +
 drivers/spi/Makefile                          |    1 +
 drivers/spi/spi-stm32-ospi.c                  | 1064 +++++++++++++++++
 12 files changed, 2027 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
 create mode 100644 Documentation/devicetree/bindings/spi/st,stm32mp25-ospi.yaml
 create mode 100644 drivers/memory/stm32_omm.c
 create mode 100644 drivers/spi/spi-stm32-ospi.c

-- 
2.25.1

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

[patrice.chotard]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5137 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

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

[Patrice CHOTARD]

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
> 

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

[Patrice CHOTARD]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3248 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Patrice CHOTARD]

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.

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

[Patrice CHOTARD]

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

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

[Krzysztof Kozlowski]

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

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

[Patrice CHOTARD]

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

[PATCH v2 2/9] spi: stm32: Add OSPI driver

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Add STM32 OSPI driver, it supports :
  - support sNOR / sNAND devices.
  - Three functional modes: indirect, automatic-status polling,
    memory-mapped.
  - Single-, dual-, quad-, and octal-SPI communication.
  - Dual-quad communication.
  - Single data rate (SDR).
  - DMA channel for indirect mode.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 drivers/spi/Kconfig          |   10 +
 drivers/spi/Makefile         |    1 +
 drivers/spi/spi-stm32-ospi.c | 1064 ++++++++++++++++++++++++++++++++++
 3 files changed, 1075 insertions(+)
 create mode 100644 drivers/spi/spi-stm32-ospi.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ea8a31032927..256a3e23639a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1045,6 +1045,16 @@ config SPI_STM32
 	  is not available, the driver automatically falls back to
 	  PIO mode.
 
+config SPI_STM32_OSPI
+	tristate "STMicroelectronics STM32 OCTO SPI controller"
+	depends on ARCH_STM32 || COMPILE_TEST
+	depends on OF
+	depends on SPI_MEM
+	help
+	  This enables support for the Octo SPI controller in master mode.
+	  This driver does not support generic SPI. The implementation only
+	  supports spi-mem interface.
+
 config SPI_STM32_QSPI
 	tristate "STMicroelectronics STM32 QUAD SPI controller"
 	depends on ARCH_STM32 || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 9db7554c1864..65e0804ebb3a 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -138,6 +138,7 @@ obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
 obj-$(CONFIG_SPI_SPRD_ADI)		+= spi-sprd-adi.o
 obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
 obj-$(CONFIG_SPI_STM32_QSPI) 		+= spi-stm32-qspi.o
+obj-$(CONFIG_SPI_STM32_OSPI) 		+= spi-stm32-ospi.o
 obj-$(CONFIG_SPI_ST_SSC4)		+= spi-st-ssc4.o
 obj-$(CONFIG_SPI_SUN4I)			+= spi-sun4i.o
 obj-$(CONFIG_SPI_SUN6I)			+= spi-sun6i.o
diff --git a/drivers/spi/spi-stm32-ospi.c b/drivers/spi/spi-stm32-ospi.c
new file mode 100644
index 000000000000..131266a7c5fe
--- /dev/null
+++ b/drivers/spi/spi-stm32-ospi.c
@@ -0,0 +1,1064 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2025 - All Rights Reserved
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/sizes.h>
+#include <linux/spi/spi-mem.h>
+#include <linux/types.h>
+
+#define OSPI_CR			0x00
+#define CR_EN			BIT(0)
+#define CR_ABORT		BIT(1)
+#define CR_DMAEN		BIT(2)
+#define CR_FTHRES_SHIFT		8
+#define CR_TEIE			BIT(16)
+#define CR_TCIE			BIT(17)
+#define CR_SMIE			BIT(19)
+#define CR_APMS			BIT(22)
+#define CR_CSSEL		BIT(24)
+#define CR_FMODE_MASK		GENMASK(29, 28)
+#define CR_FMODE_INDW		(0U)
+#define CR_FMODE_INDR		(1U)
+#define CR_FMODE_APM		(2U)
+#define CR_FMODE_MM		(3U)
+
+#define OSPI_DCR1		0x08
+#define DCR1_DLYBYP		BIT(3)
+#define DCR1_DEVSIZE_MASK	GENMASK(20, 16)
+#define DCR1_MTYP_MASK		GENMASK(26, 24)
+#define DCR1_MTYP_MX_MODE	1
+#define DCR1_MTYP_HP_MEMMODE	4
+
+#define OSPI_DCR2		0x0c
+#define DCR2_PRESC_MASK		GENMASK(7, 0)
+
+#define OSPI_SR			0x20
+#define SR_TEF			BIT(0)
+#define SR_TCF			BIT(1)
+#define SR_FTF			BIT(2)
+#define SR_SMF			BIT(3)
+#define SR_BUSY			BIT(5)
+
+#define OSPI_FCR		0x24
+#define FCR_CTEF		BIT(0)
+#define FCR_CTCF		BIT(1)
+#define FCR_CSMF		BIT(3)
+
+#define OSPI_DLR		0x40
+#define OSPI_AR			0x48
+#define OSPI_DR			0x50
+#define OSPI_PSMKR		0x80
+#define OSPI_PSMAR		0x88
+
+#define OSPI_CCR		0x100
+#define CCR_IMODE_MASK		GENMASK(2, 0)
+#define CCR_IDTR		BIT(3)
+#define CCR_ISIZE_MASK		GENMASK(5, 4)
+#define CCR_ADMODE_MASK		GENMASK(10, 8)
+#define CCR_ADMODE_8LINES	4
+#define CCR_ADDTR		BIT(11)
+#define CCR_ADSIZE_MASK		GENMASK(13, 12)
+#define CCR_ADSIZE_32BITS	3
+#define CCR_DMODE_MASK		GENMASK(26, 24)
+#define CCR_DMODE_8LINES	4
+#define CCR_DQSE		BIT(29)
+#define CCR_DDTR		BIT(27)
+#define CCR_BUSWIDTH_0		0x0
+#define CCR_BUSWIDTH_1		0x1
+#define CCR_BUSWIDTH_2		0x2
+#define CCR_BUSWIDTH_4		0x3
+#define CCR_BUSWIDTH_8		0x4
+
+#define OSPI_TCR		0x108
+#define TCR_DCYC_MASK		GENMASK(4, 0)
+#define TCR_DHQC		BIT(28)
+#define TCR_SSHIFT		BIT(30)
+
+#define OSPI_IR			0x110
+
+#define STM32_OSPI_MAX_MMAP_SZ	SZ_256M
+#define STM32_OSPI_MAX_NORCHIP	2
+
+#define STM32_FIFO_TIMEOUT_US		30000
+#define STM32_ABT_TIMEOUT_US		100000
+#define STM32_COMP_TIMEOUT_MS		5000
+#define STM32_BUSY_TIMEOUT_US		100000
+
+
+#define STM32_AUTOSUSPEND_DELAY -1
+
+struct stm32_ospi {
+	struct device *dev;
+	struct spi_controller *ctrl;
+	struct clk *clk;
+	struct reset_control *rstc;
+
+	struct completion data_completion;
+	struct completion match_completion;
+
+	struct dma_chan *dma_chtx;
+	struct dma_chan *dma_chrx;
+	struct completion dma_completion;
+
+	void __iomem *regs_base;
+	void __iomem *mm_base;
+	phys_addr_t regs_phys_base;
+	resource_size_t mm_size;
+	u32 clk_rate;
+	u32 fmode;
+	u32 cr_reg;
+	u32 dcr_reg;
+	u32 flash_presc[STM32_OSPI_MAX_NORCHIP];
+	int irq;
+	unsigned long status_timeout;
+
+	/*
+	 * To protect device configuration, could be different between
+	 * 2 flash access
+	 */
+	struct mutex lock;
+};
+
+static void stm32_ospi_read_fifo(u8 *val, void __iomem *addr)
+{
+	*val = readb_relaxed(addr);
+}
+
+static void stm32_ospi_write_fifo(u8 *val, void __iomem *addr)
+{
+	writeb_relaxed(*val, addr);
+}
+
+static int stm32_ospi_abort(struct stm32_ospi *ospi)
+{
+	void __iomem *regs_base = ospi->regs_base;
+	u32 cr;
+	int timeout;
+
+	cr = readl_relaxed(regs_base + OSPI_CR) | CR_ABORT;
+	writel_relaxed(cr, regs_base + OSPI_CR);
+
+	/* wait clear of abort bit by hw */
+	timeout = readl_relaxed_poll_timeout_atomic(regs_base + OSPI_CR,
+						    cr, !(cr & CR_ABORT), 1,
+						    STM32_ABT_TIMEOUT_US);
+
+	if (timeout)
+		dev_err(ospi->dev, "%s abort timeout:%d\n", __func__, timeout);
+
+	return timeout;
+}
+
+static int stm32_ospi_tx_poll(struct stm32_ospi *ospi, u8 *buf, u32 len, bool read)
+{
+	void __iomem *regs_base = ospi->regs_base;
+	void (*tx_fifo)(u8 *val, void __iomem *addr);
+	u32 sr;
+	int ret;
+
+	if (read)
+		tx_fifo = stm32_ospi_read_fifo;
+	else
+		tx_fifo = stm32_ospi_write_fifo;
+
+	while (len--) {
+		ret = readl_relaxed_poll_timeout_atomic(regs_base + OSPI_SR,
+							sr, sr & SR_FTF, 1,
+							STM32_FIFO_TIMEOUT_US);
+		if (ret) {
+			dev_err(ospi->dev, "fifo timeout (len:%d stat:%#x)\n",
+				len, sr);
+			return ret;
+		}
+		tx_fifo(buf++, regs_base + OSPI_DR);
+	}
+
+	return 0;
+}
+
+static int stm32_ospi_wait_nobusy(struct stm32_ospi *ospi)
+{
+	u32 sr;
+
+	return readl_relaxed_poll_timeout_atomic(ospi->regs_base + OSPI_SR,
+						 sr, !(sr & SR_BUSY), 1,
+						 STM32_BUSY_TIMEOUT_US);
+}
+
+static int stm32_ospi_wait_cmd(struct stm32_ospi *ospi)
+{
+	void __iomem *regs_base = ospi->regs_base;
+	u32 cr, sr;
+	int err = 0;
+
+	if ((readl_relaxed(regs_base + OSPI_SR) & SR_TCF) ||
+	    ospi->fmode == CR_FMODE_APM)
+		goto out;
+
+	reinit_completion(&ospi->data_completion);
+	cr = readl_relaxed(regs_base + OSPI_CR);
+	writel_relaxed(cr | CR_TCIE | CR_TEIE, regs_base + OSPI_CR);
+
+	if (!wait_for_completion_timeout(&ospi->data_completion,
+				msecs_to_jiffies(STM32_COMP_TIMEOUT_MS)))
+		err = -ETIMEDOUT;
+
+	sr = readl_relaxed(regs_base + OSPI_SR);
+	if (sr & SR_TCF)
+		/* avoid false timeout */
+		err = 0;
+	if (sr & SR_TEF)
+		err = -EIO;
+
+out:
+	/* clear flags */
+	writel_relaxed(FCR_CTCF | FCR_CTEF, regs_base + OSPI_FCR);
+
+	if (!err)
+		err = stm32_ospi_wait_nobusy(ospi);
+
+	return err;
+}
+
+static void stm32_ospi_dma_callback(void *arg)
+{
+	struct completion *dma_completion = arg;
+
+	complete(dma_completion);
+}
+
+static irqreturn_t stm32_ospi_irq(int irq, void *dev_id)
+{
+	struct stm32_ospi *ospi = (struct stm32_ospi *)dev_id;
+	void __iomem *regs_base = ospi->regs_base;
+	u32 cr, sr;
+
+	cr = readl_relaxed(regs_base + OSPI_CR);
+	sr = readl_relaxed(regs_base + OSPI_SR);
+
+	if (cr & CR_SMIE && sr & SR_SMF) {
+		/* disable irq */
+		cr &= ~CR_SMIE;
+		writel_relaxed(cr, regs_base + OSPI_CR);
+		complete(&ospi->match_completion);
+
+		return IRQ_HANDLED;
+	}
+
+	if (sr & (SR_TEF | SR_TCF)) {
+		/* disable irq */
+		cr &= ~CR_TCIE & ~CR_TEIE;
+		writel_relaxed(cr, regs_base + OSPI_CR);
+		complete(&ospi->data_completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void stm32_ospi_dma_setup(struct stm32_ospi *ospi,
+			 struct dma_slave_config *dma_cfg)
+{
+	if (dma_cfg && ospi->dma_chrx) {
+		if (dmaengine_slave_config(ospi->dma_chrx, dma_cfg)) {
+			dev_err(ospi->dev, "dma rx config failed\n");
+			dma_release_channel(ospi->dma_chrx);
+			ospi->dma_chrx = NULL;
+		}
+	}
+
+	if (dma_cfg && ospi->dma_chtx) {
+		if (dmaengine_slave_config(ospi->dma_chtx, dma_cfg)) {
+			dev_err(ospi->dev, "dma tx config failed\n");
+			dma_release_channel(ospi->dma_chtx);
+			ospi->dma_chtx = NULL;
+		}
+	}
+
+	init_completion(&ospi->dma_completion);
+}
+
+static int stm32_ospi_tx_mm(struct stm32_ospi *ospi,
+			    const struct spi_mem_op *op)
+{
+	memcpy_fromio(op->data.buf.in, ospi->mm_base + op->addr.val,
+		      op->data.nbytes);
+	return 0;
+}
+
+static int stm32_ospi_tx_dma(struct stm32_ospi *ospi,
+			     const struct spi_mem_op *op)
+{
+	struct dma_async_tx_descriptor *desc;
+	void __iomem *regs_base = ospi->regs_base;
+	enum dma_transfer_direction dma_dir;
+	struct dma_chan *dma_ch;
+	struct sg_table sgt;
+	dma_cookie_t cookie;
+	u32 cr, t_out;
+	int err;
+
+	if (op->data.dir == SPI_MEM_DATA_IN) {
+		dma_dir = DMA_DEV_TO_MEM;
+		dma_ch = ospi->dma_chrx;
+	} else {
+		dma_dir = DMA_MEM_TO_DEV;
+		dma_ch = ospi->dma_chtx;
+	}
+
+	/*
+	 * Spi_map_buf return -EINVAL if the buffer is not DMA-able
+	 * (DMA-able: in vmalloc | kmap | virt_addr_valid)
+	 */
+	err = spi_controller_dma_map_mem_op_data(ospi->ctrl, op, &sgt);
+	if (err)
+		return err;
+
+	desc = dmaengine_prep_slave_sg(dma_ch, sgt.sgl, sgt.nents,
+				       dma_dir, DMA_PREP_INTERRUPT);
+	if (!desc) {
+		err = -ENOMEM;
+		goto out_unmap;
+	}
+
+	cr = readl_relaxed(regs_base + OSPI_CR);
+
+	reinit_completion(&ospi->dma_completion);
+	desc->callback = stm32_ospi_dma_callback;
+	desc->callback_param = &ospi->dma_completion;
+	cookie = dmaengine_submit(desc);
+	err = dma_submit_error(cookie);
+	if (err)
+		goto out;
+
+	dma_async_issue_pending(dma_ch);
+
+	writel_relaxed(cr | CR_DMAEN, regs_base + OSPI_CR);
+
+	t_out = sgt.nents * STM32_COMP_TIMEOUT_MS;
+	if (!wait_for_completion_timeout(&ospi->dma_completion,
+					 msecs_to_jiffies(t_out)))
+		err = -ETIMEDOUT;
+
+	if (err)
+		dmaengine_terminate_all(dma_ch);
+
+out:
+	writel_relaxed(cr & ~CR_DMAEN, regs_base + OSPI_CR);
+out_unmap:
+	spi_controller_dma_unmap_mem_op_data(ospi->ctrl, op, &sgt);
+
+	return err;
+}
+
+static int stm32_ospi_tx(struct stm32_ospi *ospi, const struct spi_mem_op *op)
+{
+	u8 *buf;
+
+	if (!op->data.nbytes)
+		return 0;
+
+	if (ospi->fmode == CR_FMODE_MM)
+		return stm32_ospi_tx_mm(ospi, op);
+	else if (((op->data.dir == SPI_MEM_DATA_IN && ospi->dma_chrx) ||
+		 (op->data.dir == SPI_MEM_DATA_OUT && ospi->dma_chtx)) &&
+		  op->data.nbytes > 8)
+		if (!stm32_ospi_tx_dma(ospi, op))
+			return 0;
+
+	if (op->data.dir == SPI_MEM_DATA_IN)
+		buf = op->data.buf.in;
+	else
+		buf = (u8 *)op->data.buf.out;
+
+	return stm32_ospi_tx_poll(ospi, buf, op->data.nbytes,
+				 op->data.dir == SPI_MEM_DATA_IN);
+}
+
+static int stm32_ospi_wait_poll_status(struct stm32_ospi *ospi,
+				       const struct spi_mem_op *op)
+{
+	void __iomem *regs_base = ospi->regs_base;
+	u32 cr;
+
+	reinit_completion(&ospi->match_completion);
+	cr = readl_relaxed(regs_base + OSPI_CR);
+	writel_relaxed(cr | CR_SMIE, regs_base + OSPI_CR);
+
+	if (!wait_for_completion_timeout(&ospi->match_completion,
+					 msecs_to_jiffies(ospi->status_timeout))) {
+		u32 sr = readl_relaxed(regs_base + OSPI_SR);
+
+		/* Avoid false timeout */
+		if (!(sr & SR_SMF))
+			return -ETIMEDOUT;
+	}
+
+	writel_relaxed(FCR_CSMF, regs_base + OSPI_FCR);
+
+	return 0;
+}
+
+static int stm32_ospi_get_mode(u8 buswidth)
+{
+	switch (buswidth) {
+	case 8:
+		return CCR_BUSWIDTH_8;
+	case 4:
+		return CCR_BUSWIDTH_4;
+	default:
+		return buswidth;
+	}
+}
+
+static int stm32_ospi_send(struct spi_device *spi, const struct spi_mem_op *op)
+{
+	struct stm32_ospi *ospi = spi_controller_get_devdata(spi->controller);
+	void __iomem *regs_base = ospi->regs_base;
+	u32 ccr, cr, dcr2, tcr;
+	int timeout, err = 0, err_poll_status = 0;
+	u8 cs = spi->chip_select[ffs(spi->cs_index_mask) - 1];
+
+	dev_dbg(ospi->dev, "cmd:%#x mode:%d.%d.%d.%d addr:%#llx len:%#x\n",
+		op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
+		op->dummy.buswidth, op->data.buswidth,
+		op->addr.val, op->data.nbytes);
+
+	cr = readl_relaxed(ospi->regs_base + OSPI_CR);
+	cr &= ~CR_CSSEL;
+	cr |= FIELD_PREP(CR_CSSEL, cs);
+	cr &= ~CR_FMODE_MASK;
+	cr |= FIELD_PREP(CR_FMODE_MASK, ospi->fmode);
+	writel_relaxed(cr, regs_base + OSPI_CR);
+
+	if (op->data.nbytes)
+		writel_relaxed(op->data.nbytes - 1, regs_base + OSPI_DLR);
+
+	/* set prescaler */
+	dcr2 = readl_relaxed(regs_base + OSPI_DCR2);
+	dcr2 |= FIELD_PREP(DCR2_PRESC_MASK, ospi->flash_presc[cs]);
+	writel_relaxed(dcr2, regs_base + OSPI_DCR2);
+
+	ccr = FIELD_PREP(CCR_IMODE_MASK, stm32_ospi_get_mode(op->cmd.buswidth));
+
+	if (op->addr.nbytes) {
+		ccr |= FIELD_PREP(CCR_ADMODE_MASK,
+				  stm32_ospi_get_mode(op->addr.buswidth));
+		ccr |= FIELD_PREP(CCR_ADSIZE_MASK, op->addr.nbytes - 1);
+	}
+
+	tcr = TCR_SSHIFT;
+	if (op->dummy.buswidth && op->dummy.nbytes) {
+		tcr |= FIELD_PREP(TCR_DCYC_MASK,
+				  op->dummy.nbytes * 8 / op->dummy.buswidth);
+	}
+	writel_relaxed(tcr, regs_base + OSPI_TCR);
+
+	if (op->data.nbytes) {
+		ccr |= FIELD_PREP(CCR_DMODE_MASK,
+				  stm32_ospi_get_mode(op->data.buswidth));
+	}
+
+	writel_relaxed(ccr, regs_base + OSPI_CCR);
+
+	/* set instruction, must be set after ccr register update */
+	writel_relaxed(op->cmd.opcode, regs_base + OSPI_IR);
+
+	if (op->addr.nbytes && ospi->fmode != CR_FMODE_MM)
+		writel_relaxed(op->addr.val, regs_base + OSPI_AR);
+
+	if (ospi->fmode == CR_FMODE_APM)
+		err_poll_status = stm32_ospi_wait_poll_status(ospi, op);
+
+	err = stm32_ospi_tx(ospi, op);
+
+	/*
+	 * Abort in:
+	 * -error case
+	 * -read memory map: prefetching must be stopped if we read the last
+	 *  byte of device (device size - fifo size). like device size is not
+	 *  knows, the prefetching is always stop.
+	 */
+	if (err || err_poll_status || ospi->fmode == CR_FMODE_MM)
+		goto abort;
+
+	/* Wait end of tx in indirect mode */
+	err = stm32_ospi_wait_cmd(ospi);
+	if (err)
+		goto abort;
+
+	return 0;
+
+abort:
+	timeout = stm32_ospi_abort(ospi);
+	writel_relaxed(FCR_CTCF | FCR_CSMF, regs_base + OSPI_FCR);
+
+	if (err || err_poll_status || timeout)
+		dev_err(ospi->dev, "%s err:%d err_poll_status:%d abort timeout:%d\n",
+			__func__, err, err_poll_status, timeout);
+
+	return err;
+}
+
+static int stm32_ospi_poll_status(struct spi_mem *mem,
+				  const struct spi_mem_op *op,
+				  u16 mask, u16 match,
+				  unsigned long initial_delay_us,
+				  unsigned long polling_rate_us,
+				  unsigned long timeout_ms)
+{
+	struct stm32_ospi *ospi = spi_controller_get_devdata(mem->spi->controller);
+	void __iomem *regs_base = ospi->regs_base;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&ospi->lock);
+
+	writel_relaxed(mask, regs_base + OSPI_PSMKR);
+	writel_relaxed(match, regs_base + OSPI_PSMAR);
+	ospi->fmode = CR_FMODE_APM;
+	ospi->status_timeout = timeout_ms;
+
+	ret = stm32_ospi_send(mem->spi, op);
+	mutex_unlock(&ospi->lock);
+
+	pm_runtime_mark_last_busy(ospi->dev);
+	pm_runtime_put_autosuspend(ospi->dev);
+
+	return ret;
+}
+
+static int stm32_ospi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	struct stm32_ospi *ospi = spi_controller_get_devdata(mem->spi->controller);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&ospi->lock);
+	if (op->data.dir == SPI_MEM_DATA_IN && op->data.nbytes)
+		ospi->fmode = CR_FMODE_INDR;
+	else
+		ospi->fmode = CR_FMODE_INDW;
+
+	ret = stm32_ospi_send(mem->spi, op);
+	mutex_unlock(&ospi->lock);
+
+	pm_runtime_mark_last_busy(ospi->dev);
+	pm_runtime_put_autosuspend(ospi->dev);
+
+	return ret;
+}
+
+static int stm32_ospi_dirmap_create(struct spi_mem_dirmap_desc *desc)
+{
+	struct stm32_ospi *ospi = spi_controller_get_devdata(desc->mem->spi->controller);
+
+	if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_OUT)
+		return -EOPNOTSUPP;
+
+	/* Should never happen, as mm_base == null is an error probe exit condition */
+	if (!ospi->mm_base && desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN)
+		return -EOPNOTSUPP;
+
+	if (!ospi->mm_size)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static ssize_t stm32_ospi_dirmap_read(struct spi_mem_dirmap_desc *desc,
+				      u64 offs, size_t len, void *buf)
+{
+	struct stm32_ospi *ospi = spi_controller_get_devdata(desc->mem->spi->controller);
+	struct spi_mem_op op;
+	u32 addr_max;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&ospi->lock);
+	/*
+	 * Make a local copy of desc op_tmpl and complete dirmap rdesc
+	 * spi_mem_op template with offs, len and *buf in  order to get
+	 * all needed transfer information into struct spi_mem_op
+	 */
+	memcpy(&op, &desc->info.op_tmpl, sizeof(struct spi_mem_op));
+	dev_dbg(ospi->dev, "%s len = 0x%zx offs = 0x%llx buf = 0x%p\n", __func__, len, offs, buf);
+
+	op.data.nbytes = len;
+	op.addr.val = desc->info.offset + offs;
+	op.data.buf.in = buf;
+
+	addr_max = op.addr.val + op.data.nbytes + 1;
+	if (addr_max < ospi->mm_size && op.addr.buswidth)
+		ospi->fmode = CR_FMODE_MM;
+	else
+		ospi->fmode = CR_FMODE_INDR;
+
+	ret = stm32_ospi_send(desc->mem->spi, &op);
+	mutex_unlock(&ospi->lock);
+
+	pm_runtime_mark_last_busy(ospi->dev);
+	pm_runtime_put_autosuspend(ospi->dev);
+
+	return ret ?: len;
+}
+
+static int stm32_ospi_transfer_one_message(struct spi_controller *ctrl,
+					   struct spi_message *msg)
+{
+	struct stm32_ospi *ospi = spi_controller_get_devdata(ctrl);
+	struct spi_transfer *transfer;
+	struct spi_device *spi = msg->spi;
+	struct spi_mem_op op;
+	struct gpio_desc *cs_gpiod = spi->cs_gpiod[ffs(spi->cs_index_mask) - 1];
+	int ret = 0;
+
+	if (!cs_gpiod)
+		return -EOPNOTSUPP;
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&ospi->lock);
+
+	gpiod_set_value_cansleep(cs_gpiod, true);
+
+	list_for_each_entry(transfer, &msg->transfers, transfer_list) {
+		u8 dummy_bytes = 0;
+
+		memset(&op, 0, sizeof(op));
+
+		dev_dbg(ospi->dev, "tx_buf:%p tx_nbits:%d rx_buf:%p rx_nbits:%d len:%d dummy_data:%d\n",
+			transfer->tx_buf, transfer->tx_nbits,
+			transfer->rx_buf, transfer->rx_nbits,
+			transfer->len, transfer->dummy_data);
+
+		/*
+		 * OSPI hardware supports dummy bytes transfer.
+		 * If current transfer is dummy byte, merge it with the next
+		 * transfer in order to take into account OSPI block constraint
+		 */
+		if (transfer->dummy_data) {
+			op.dummy.buswidth = transfer->tx_nbits;
+			op.dummy.nbytes = transfer->len;
+			dummy_bytes = transfer->len;
+
+			/* If happens, means that message is not correctly built */
+			if (list_is_last(&transfer->transfer_list, &msg->transfers)) {
+				ret = -EINVAL;
+				goto end_of_transfer;
+			}
+
+			transfer = list_next_entry(transfer, transfer_list);
+		}
+
+		op.data.nbytes = transfer->len;
+
+		if (transfer->rx_buf) {
+			ospi->fmode = CR_FMODE_INDR;
+			op.data.buswidth = transfer->rx_nbits;
+			op.data.dir = SPI_MEM_DATA_IN;
+			op.data.buf.in = transfer->rx_buf;
+		} else {
+			ospi->fmode = CR_FMODE_INDW;
+			op.data.buswidth = transfer->tx_nbits;
+			op.data.dir = SPI_MEM_DATA_OUT;
+			op.data.buf.out = transfer->tx_buf;
+		}
+
+		ret = stm32_ospi_send(spi, &op);
+		if (ret)
+			goto end_of_transfer;
+
+		msg->actual_length += transfer->len + dummy_bytes;
+	}
+
+end_of_transfer:
+	gpiod_set_value_cansleep(cs_gpiod, false);
+
+	mutex_unlock(&ospi->lock);
+
+	msg->status = ret;
+	spi_finalize_current_message(ctrl);
+
+	pm_runtime_mark_last_busy(ospi->dev);
+	pm_runtime_put_autosuspend(ospi->dev);
+
+	return ret;
+}
+
+static int stm32_ospi_setup(struct spi_device *spi)
+{
+	struct spi_controller *ctrl = spi->controller;
+	struct stm32_ospi *ospi = spi_controller_get_devdata(ctrl);
+	void __iomem *regs_base = ospi->regs_base;
+	int ret;
+	u8 cs = spi->chip_select[ffs(spi->cs_index_mask) - 1];
+
+	if (ctrl->busy)
+		return -EBUSY;
+
+	if (!spi->max_speed_hz)
+		return -EINVAL;
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		return ret;
+
+	ospi->flash_presc[cs] = DIV_ROUND_UP(ospi->clk_rate, spi->max_speed_hz) - 1;
+
+	mutex_lock(&ospi->lock);
+
+	ospi->cr_reg = CR_APMS | 3 << CR_FTHRES_SHIFT | CR_EN;
+	writel_relaxed(ospi->cr_reg, regs_base + OSPI_CR);
+
+	/* set dcr fsize to max address */
+	ospi->dcr_reg = DCR1_DEVSIZE_MASK | DCR1_DLYBYP;
+	writel_relaxed(ospi->dcr_reg, regs_base + OSPI_DCR1);
+
+	mutex_unlock(&ospi->lock);
+
+	pm_runtime_mark_last_busy(ospi->dev);
+	pm_runtime_put_autosuspend(ospi->dev);
+
+	return 0;
+}
+
+/*
+ * No special host constraint, so use default spi_mem_default_supports_op
+ * to check supported mode.
+ */
+static const struct spi_controller_mem_ops stm32_ospi_mem_ops = {
+	.exec_op	= stm32_ospi_exec_op,
+	.dirmap_create	= stm32_ospi_dirmap_create,
+	.dirmap_read	= stm32_ospi_dirmap_read,
+	.poll_status	= stm32_ospi_poll_status,
+};
+
+static int stm32_ospi_get_resources(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct stm32_ospi *ospi = platform_get_drvdata(pdev);
+	struct resource *res;
+	struct reserved_mem *rmem = NULL;
+	struct device_node *node;
+	int ret;
+
+	ospi->regs_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(ospi->regs_base))
+		return PTR_ERR(ospi->regs_base);
+
+	ospi->regs_phys_base = res->start;
+
+	ospi->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ospi->clk))
+		return dev_err_probe(dev, PTR_ERR(ospi->clk),
+				     "Can't get clock\n");
+
+	ospi->clk_rate = clk_get_rate(ospi->clk);
+	if (!ospi->clk_rate) {
+		dev_err(dev, "Invalid clock rate\n");
+		return -EINVAL;
+	}
+
+	ospi->irq = platform_get_irq(pdev, 0);
+	if (ospi->irq < 0) {
+		dev_err(dev, "Can't get irq %d\n", ospi->irq);
+		return ospi->irq;
+	}
+
+	ret = devm_request_irq(dev, ospi->irq, stm32_ospi_irq, 0,
+			       dev_name(dev), ospi);
+	if (ret) {
+		dev_err(dev, "Failed to request irq\n");
+		return ret;
+	}
+
+	ospi->rstc = devm_reset_control_array_get_optional_exclusive(dev);
+	if (IS_ERR(ospi->rstc))
+		return dev_err_probe(dev, PTR_ERR(ospi->rstc),
+				     "Can't get reset\n");
+
+	ospi->dma_chrx = dma_request_chan(dev, "rx");
+	if (IS_ERR(ospi->dma_chrx)) {
+		ret = PTR_ERR(ospi->dma_chrx);
+		ospi->dma_chrx = NULL;
+		if (ret == -EPROBE_DEFER)
+			goto err_dma;
+	}
+
+	ospi->dma_chtx = dma_request_chan(dev, "tx");
+	if (IS_ERR(ospi->dma_chtx)) {
+		ret = PTR_ERR(ospi->dma_chtx);
+		ospi->dma_chtx = NULL;
+		if (ret == -EPROBE_DEFER)
+			goto err_dma;
+	}
+
+	node = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (node)
+		rmem = of_reserved_mem_lookup(node);
+	of_node_put(node);
+
+	if (rmem) {
+		ospi->mm_size = rmem->size;
+		ospi->mm_base = devm_ioremap(dev, rmem->base, rmem->size);
+		if (IS_ERR(ospi->mm_base)) {
+			dev_err(dev, "unable to map memory region: %pa+%pa\n",
+				&rmem->base, &rmem->size);
+			ret = PTR_ERR(ospi->mm_base);
+			goto err_dma;
+		}
+
+		if (ospi->mm_size > STM32_OSPI_MAX_MMAP_SZ) {
+			dev_err(dev, "Memory map size outsize bounds\n");
+			ret = -EINVAL;
+			goto err_dma;
+		}
+	} else {
+		dev_info(dev, "No memory-map region found\n");
+	}
+
+	init_completion(&ospi->data_completion);
+	init_completion(&ospi->match_completion);
+
+	return 0;
+
+err_dma:
+	dev_info(dev, "Can't get all resources (%d)\n", ret);
+
+	if (ospi->dma_chtx)
+		dma_release_channel(ospi->dma_chtx);
+	if (ospi->dma_chrx)
+		dma_release_channel(ospi->dma_chrx);
+
+	return ret;
+};
+
+static int stm32_ospi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct spi_controller *ctrl;
+	struct stm32_ospi *ospi;
+	struct dma_slave_config dma_cfg;
+	struct device_node *child;
+	int ret;
+	u8 spi_flash_count = 0;
+
+	/*
+	 * Flash subnodes sanity check:
+	 *        1 or 2 spi-nand/spi-nor flashes		=> supported
+	 *	  All other flash node configuration		=> not supported
+	 */
+	for_each_available_child_of_node(dev->of_node, child) {
+		if (of_device_is_compatible(child, "jedec,spi-nor") ||
+		    of_device_is_compatible(child, "spi-nand"))
+			spi_flash_count++;
+	}
+
+	if (spi_flash_count == 0 || spi_flash_count > 2) {
+		dev_err(dev, "Incorrect DT flash node\n");
+		return -ENODEV;
+	}
+
+	ctrl = devm_spi_alloc_host(dev, sizeof(*ospi));
+	if (!ctrl)
+		return -ENOMEM;
+
+	ospi = spi_controller_get_devdata(ctrl);
+	ospi->ctrl = ctrl;
+
+	ospi->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ospi);
+
+	ret = stm32_ospi_get_resources(pdev);
+	if (ret)
+		return ret;
+
+	memset(&dma_cfg, 0, sizeof(dma_cfg));
+	dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_cfg.src_addr = ospi->regs_phys_base + OSPI_DR;
+	dma_cfg.dst_addr = ospi->regs_phys_base + OSPI_DR;
+	dma_cfg.src_maxburst = 4;
+	dma_cfg.dst_maxburst = 4;
+	stm32_ospi_dma_setup(ospi, &dma_cfg);
+
+	mutex_init(&ospi->lock);
+
+	ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD |
+			  SPI_TX_DUAL | SPI_TX_QUAD |
+			  SPI_TX_OCTAL | SPI_RX_OCTAL;
+	ctrl->setup = stm32_ospi_setup;
+	ctrl->bus_num = -1;
+	ctrl->mem_ops = &stm32_ospi_mem_ops;
+	ctrl->use_gpio_descriptors = true;
+	ctrl->transfer_one_message = stm32_ospi_transfer_one_message;
+	ctrl->num_chipselect = STM32_OSPI_MAX_NORCHIP;
+	ctrl->dev.of_node = dev->of_node;
+
+	pm_runtime_enable(ospi->dev);
+	pm_runtime_set_autosuspend_delay(ospi->dev, STM32_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(ospi->dev);
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		goto err_pm_enable;
+
+	if (ospi->rstc) {
+		reset_control_assert(ospi->rstc);
+		udelay(2);
+		reset_control_deassert(ospi->rstc);
+	}
+
+	ret = spi_register_controller(ctrl);
+	if (ret) {
+		/* Disable ospi */
+		writel_relaxed(0, ospi->regs_base + OSPI_CR);
+		goto err_pm_resume;
+	}
+
+	pm_runtime_mark_last_busy(ospi->dev);
+	pm_runtime_put_autosuspend(ospi->dev);
+
+	return 0;
+
+err_pm_resume:
+	pm_runtime_put_sync_suspend(ospi->dev);
+
+err_pm_enable:
+	pm_runtime_force_suspend(ospi->dev);
+	mutex_destroy(&ospi->lock);
+
+	return ret;
+}
+
+static void stm32_ospi_remove(struct platform_device *pdev)
+{
+	struct stm32_ospi *ospi = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		return;
+
+	spi_unregister_controller(ospi->ctrl);
+	/* Disable ospi */
+	writel_relaxed(0, ospi->regs_base + OSPI_CR);
+	mutex_destroy(&ospi->lock);
+
+	if (ospi->dma_chtx)
+		dma_release_channel(ospi->dma_chtx);
+	if (ospi->dma_chrx)
+		dma_release_channel(ospi->dma_chrx);
+
+	pm_runtime_put_sync_suspend(ospi->dev);
+	pm_runtime_force_suspend(ospi->dev);
+}
+
+static int __maybe_unused stm32_ospi_suspend(struct device *dev)
+{
+	struct stm32_ospi *ospi = dev_get_drvdata(dev);
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return pm_runtime_force_suspend(ospi->dev);
+}
+
+static int __maybe_unused stm32_ospi_resume(struct device *dev)
+{
+	struct stm32_ospi *ospi = dev_get_drvdata(dev);
+	void __iomem *regs_base = ospi->regs_base;
+	int ret;
+
+	ret = pm_runtime_force_resume(ospi->dev);
+	if (ret < 0)
+		return ret;
+
+	pinctrl_pm_select_default_state(dev);
+
+	ret = pm_runtime_resume_and_get(ospi->dev);
+	if (ret < 0)
+		return ret;
+
+	writel_relaxed(ospi->cr_reg, regs_base + OSPI_CR);
+	writel_relaxed(ospi->dcr_reg, regs_base + OSPI_DCR1);
+	pm_runtime_mark_last_busy(ospi->dev);
+	pm_runtime_put_autosuspend(ospi->dev);
+
+	return 0;
+}
+
+static int __maybe_unused stm32_ospi_runtime_suspend(struct device *dev)
+{
+	struct stm32_ospi *ospi = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(ospi->clk);
+
+	return 0;
+}
+
+static int __maybe_unused stm32_ospi_runtime_resume(struct device *dev)
+{
+	struct stm32_ospi *ospi = dev_get_drvdata(dev);
+
+	return clk_prepare_enable(ospi->clk);
+}
+
+static const struct dev_pm_ops stm32_ospi_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(stm32_ospi_suspend, stm32_ospi_resume)
+	SET_RUNTIME_PM_OPS(stm32_ospi_runtime_suspend,
+			   stm32_ospi_runtime_resume, NULL)
+};
+
+static const struct of_device_id stm32_ospi_of_match[] = {
+	{ .compatible = "st,stm32mp25-ospi" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stm32_ospi_of_match);
+
+static struct platform_driver stm32_ospi_driver = {
+	.probe	= stm32_ospi_probe,
+	.remove	= stm32_ospi_remove,
+	.driver	= {
+		.name = "stm32-ospi",
+		.pm = &stm32_ospi_pm_ops,
+		.of_match_table = stm32_ospi_of_match,
+	},
+};
+module_platform_driver(stm32_ospi_driver);
+
+MODULE_DESCRIPTION("STMicroelectronics STM32 OCTO SPI driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v2 2/9] spi: stm32: Add OSPI driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]

On Tue, Jan 28, 2025 at 09:17:24AM +0100, patrice.chotard@foss.st.com wrote:

> +static int stm32_ospi_tx_poll(struct stm32_ospi *ospi, u8 *buf, u32 len, bool read)
> +{

> +	if (read)
> +		tx_fifo = stm32_ospi_read_fifo;
> +	else
> +		tx_fifo = stm32_ospi_write_fifo;

> +		tx_fifo(buf++, regs_base + OSPI_DR);

It feels like the _tx_poll and tx_fifo naming is a landmine waiting to
surprise people in the future.  The code sharing makes sense but the
naming is just looking to cause surprises, especially with it just being
a bool selecting read or write.

> +static int stm32_ospi_tx(struct stm32_ospi *ospi, const struct spi_mem_op *op)
> +{

> +       return stm32_ospi_tx_poll(ospi, buf, op->data.nbytes,
> +                                op->data.dir == SPI_MEM_DATA_IN);

Though the one caller is also using _tx only naming, it's a bit more
tied in with the op sending though.

> +	ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD |
> +			  SPI_TX_DUAL | SPI_TX_QUAD |
> +			  SPI_TX_OCTAL | SPI_RX_OCTAL;
> +	ctrl->setup = stm32_ospi_setup;
> +	ctrl->bus_num = -1;
> +	ctrl->mem_ops = &stm32_ospi_mem_ops;
> +	ctrl->use_gpio_descriptors = true;
> +	ctrl->transfer_one_message = stm32_ospi_transfer_one_message;
> +	ctrl->num_chipselect = STM32_OSPI_MAX_NORCHIP;
> +	ctrl->dev.of_node = dev->of_node;

It looks like the controller only does half duplex as well so it should
set SPI_CONTROLLER_HALF_DUPLEX.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/9] spi: stm32: Add OSPI driver

[Patrice CHOTARD]

On 1/28/25 13:37, Mark Brown wrote:
> On Tue, Jan 28, 2025 at 09:17:24AM +0100, patrice.chotard@foss.st.com wrote:
> 
>> +static int stm32_ospi_tx_poll(struct stm32_ospi *ospi, u8 *buf, u32 len, bool read)
>> +{
> 
>> +	if (read)
>> +		tx_fifo = stm32_ospi_read_fifo;
>> +	else
>> +		tx_fifo = stm32_ospi_write_fifo;
> 
>> +		tx_fifo(buf++, regs_base + OSPI_DR);
> 
> It feels like the _tx_poll and tx_fifo naming is a landmine waiting to
> surprise people in the future.  The code sharing makes sense but the
> naming is just looking to cause surprises, especially with it just being
> a bool selecting read or write.

Agree, i will replace "tx_fifo" to a more neutral name as "fifo" for example

> 
>> +static int stm32_ospi_tx(struct stm32_ospi *ospi, const struct spi_mem_op *op)
>> +{
> 
>> +       return stm32_ospi_tx_poll(ospi, buf, op->data.nbytes,
>> +                                op->data.dir == SPI_MEM_DATA_IN);
> 
> Though the one caller is also using _tx only naming, it's a bit more
> tied in with the op sending though.

I will replace stm32_ospi_tx_poll() by stm32_ospi_poll()

> 
>> +	ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD |
>> +			  SPI_TX_DUAL | SPI_TX_QUAD |
>> +			  SPI_TX_OCTAL | SPI_RX_OCTAL;
>> +	ctrl->setup = stm32_ospi_setup;
>> +	ctrl->bus_num = -1;
>> +	ctrl->mem_ops = &stm32_ospi_mem_ops;
>> +	ctrl->use_gpio_descriptors = true;
>> +	ctrl->transfer_one_message = stm32_ospi_transfer_one_message;
>> +	ctrl->num_chipselect = STM32_OSPI_MAX_NORCHIP;
>> +	ctrl->dev.of_node = dev->of_node;
> 
> It looks like the controller only does half duplex as well so it should
> set SPI_CONTROLLER_HALF_DUPLEX.

Right, i will add it.

Thanks
Patrice

[PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Add bindings for STM32 Octo Memory Manager (OMM) controller.

OMM manages:
  - the muxing between 2 OSPI busses and 2 output ports.
    There are 4 possible muxing configurations:
      - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
        output is on port 2
      - OSPI1 and OSPI2 are multiplexed over the same output port 1
      - swapped mode (no multiplexing), OSPI1 output is on port 2,
        OSPI2 output is on port 1
      - OSPI1 and OSPI2 are multiplexed over the same output port 2
  - the split of the memory area shared between the 2 OSPI instances.
  - chip select selection override.
  - the time between 2 transactions in multiplexed mode.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
 1 file changed, 190 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
new file mode 100644
index 000000000000..7e0b150e0005
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
@@ -0,0 +1,190 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STM32 Octo Memory Manager (OMM)
+
+maintainers:
+  - Patrice Chotard <patrice.chotard@foss.st.com>
+
+description: |
+  The STM32 Octo Memory Manager is a low-level interface that enables an
+  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
+  function map) and multiplex of single/dual/quad/octal SPI interfaces over
+  the same bus. It Supports up to:
+    - Two single/dual/quad/octal SPI interfaces
+    - Two ports for pin assignment
+
+properties:
+  compatible:
+    const: st,stm32mp25-omm
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 1
+
+  ranges:
+    description: |
+      Reflects the memory layout with four integer values per OSPI instance.
+      Format:
+      <chip-select> 0 <registers base address> <size>
+
+  reg:
+    items:
+      - description: OMM registers
+      - description: OMM memory map area
+
+  reg-names:
+    items:
+      - const: regs
+      - const: memory_map
+
+  memory-region:
+    description: Phandle to node describing memory-map region to used.
+    minItems: 1
+    maxItems: 2
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  access-controllers:
+    maxItems: 1
+
+  st,syscfg-amcr:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      The Address Mapping Control Register (AMCR) is used to split the 256MB
+      memory map area shared between the 2 OSPI instance. The Octo Memory
+      Manager sets the AMCR depending of the memory-region configuration.
+      Format is phandle to syscfg / register offset within syscfg / memory split
+      bitmask.
+      The memory split bitmask description is:
+        - 000: OCTOSPI1 (256 Mbytes), OCTOSPI2 unmapped
+        - 001: OCTOSPI1 (192 Mbytes), OCTOSPI2 (64 Mbytes)
+        - 010: OCTOSPI1 (128 Mbytes), OCTOSPI2 (128 Mbytes)
+        - 011: OCTOSPI1 (64 Mbytes), OCTOSPI2 (192 Mbytes)
+        - 1xx: OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
+    items:
+      minItems: 3
+      maxItems: 3
+
+  st,omm-req2ack-ns:
+    description: |
+      In multiplexed mode (MUXEN = 1), this field defines the time in
+      nanoseconds between two transactions.
+
+  st,omm-cssel-ovr:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Configure the chip select selector override for the 2 OCTOSPIs.
+      The 2 bits mask muxing description is:
+        -bit 0: Chip select selector override setting for OCTOSPI1
+          0x0: the chip select signal from OCTOSPI1 is sent to NCS1
+          0x1: the chip select signal from OCTOSPI1 is sent to NCS2
+        -bit 1: Chip select selector override setting for OCTOSPI2
+          0x0: the chip select signal from OCTOSPI2 is sent to NCS1
+          0x1: the chip select signal from OCTOSPI2 is sent to NCS2
+
+  st,omm-mux:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Configure the muxing between the 2 OCTOSPIs busses and the 2 output ports.
+      The muxing 2 bits mask description is:
+        - 0x0: direct mode, default
+        - 0x1: mux OCTOSPI1 and OCTOSPI2 to port 1
+        - 0x2: swapped mode
+        - 0x3: mux OCTOSPI1 and OCTOSPI2 to port 2
+
+  power-domains:
+    maxItems: 1
+
+patternProperties:
+  ^spi@[a-f0-9]+$:
+    type: object
+    $ref: "/schemas/spi/st,stm32mp25-ospi.yaml#"
+    description: Required spi child node
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - clocks
+  - st,syscfg-amcr
+  - ranges
+
+additionalProperties: 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>
+    ommanager@40500000 {
+      compatible = "st,stm32mp25-omm";
+      reg = <0x40500000 0x400>, <0x60000000 0x10000000>;
+      reg-names = "regs", "memory_map";
+      memory-region = <&mm_ospi1>, <&mm_ospi2>;
+      pinctrl-names = "default", "sleep";
+      pinctrl-0 = <&ospi_port1_clk_pins_a
+                   &ospi_port1_io03_pins_a
+                   &ospi_port1_cs0_pins_a>;
+      pinctrl-1 = <&ospi_port1_clk_sleep_pins_a
+                   &ospi_port1_io03_sleep_pins_a
+                   &ospi_port1_cs0_sleep_pins_a>;
+      clocks = <&rcc CK_BUS_OSPIIOM>;
+      resets = <&rcc OSPIIOM_R>;
+      st,syscfg-amcr = <&syscfg 0x2c00 0x7>;
+      st,omm-req2ack-ns = <0x0>;
+      st,omm-mux = <0x0>;
+      st,omm-cssel-ovr = <0x0>;
+      access-controllers = <&rifsc 111>;
+      power-domains = <&CLUSTER_PD>;
+
+      #address-cells = <2>;
+      #size-cells = <1>;
+
+      ranges = <0 0 0x40430000 0x400>,
+               <1 0 0x40440000 0x400>;
+
+      spi@40430000 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        compatible = "st,stm32mp25-ospi";
+        reg = <0 0 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>;
+      };
+
+      spi@40440000 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        compatible = "st,stm32mp25-ospi";
+        reg = <1 0 0x400>;
+        memory-region = <&mm_ospi1>;
+        interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
+        dmas = <&hpdma 3 0x62 0x00003121 0x0>,
+               <&hpdma 3 0x42 0x00003112 0x0>;
+        dma-names = "tx", "rx";
+        clocks = <&scmi_clk CK_KER_OSPI2>;
+        resets = <&scmi_reset RST_SCMI_OSPI2>, <&scmi_reset RST_SCMI_OSPI1DLL>;
+        access-controllers = <&rifsc 75>;
+        power-domains = <&CLUSTER_PD>;
+        st,syscfg-dlyb = <&syscfg 0x1000>;
+      };
+    };
-- 
2.25.1

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Krzysztof Kozlowski]

On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
> From: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> Add bindings for STM32 Octo Memory Manager (OMM) controller.
> 
> OMM manages:
>   - the muxing between 2 OSPI busses and 2 output ports.
>     There are 4 possible muxing configurations:
>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>         output is on port 2
>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>         OSPI2 output is on port 1
>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>   - the split of the memory area shared between the 2 OSPI instances.
>   - chip select selection override.
>   - the time between 2 transactions in multiplexed mode.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> ---
>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>  1 file changed, 190 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
> new file mode 100644
> index 000000000000..7e0b150e0005
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml


Filename as compatible, so st,stm32mp25-omm.yaml

You already received this comment.

> @@ -0,0 +1,190 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STM32 Octo Memory Manager (OMM)
> +
> +maintainers:
> +  - Patrice Chotard <patrice.chotard@foss.st.com>
> +
> +description: |
> +  The STM32 Octo Memory Manager is a low-level interface that enables an
> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
> +  function map) and multiplex of single/dual/quad/octal SPI interfaces over
> +  the same bus. It Supports up to:
> +    - Two single/dual/quad/octal SPI interfaces
> +    - Two ports for pin assignment
> +
> +properties:
> +  compatible:
> +    const: st,stm32mp25-omm
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges:
> +    description: |
> +      Reflects the memory layout with four integer values per OSPI instance.
> +      Format:
> +      <chip-select> 0 <registers base address> <size>

Do you always have two children? If so, this should have maxItems.

> +
> +  reg:
> +    items:
> +      - description: OMM registers
> +      - description: OMM memory map area
> +
> +  reg-names:
> +    items:
> +      - const: regs
> +      - const: memory_map
> +
> +  memory-region:
> +    description: Phandle to node describing memory-map region to used.
> +    minItems: 1
> +    maxItems: 2

List the items with description instead with optional minItems. Why is
this flexible in number of items?

> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  access-controllers:
> +    maxItems: 1
> +
> +  st,syscfg-amcr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: |
> +      The Address Mapping Control Register (AMCR) is used to split the 256MB
> +      memory map area shared between the 2 OSPI instance. The Octo Memory
> +      Manager sets the AMCR depending of the memory-region configuration.
> +      Format is phandle to syscfg / register offset within syscfg / memory split
> +      bitmask.

Don't repeat constraints in free form text.

> +      The memory split bitmask description is:
> +        - 000: OCTOSPI1 (256 Mbytes), OCTOSPI2 unmapped
> +        - 001: OCTOSPI1 (192 Mbytes), OCTOSPI2 (64 Mbytes)
> +        - 010: OCTOSPI1 (128 Mbytes), OCTOSPI2 (128 Mbytes)
> +        - 011: OCTOSPI1 (64 Mbytes), OCTOSPI2 (192 Mbytes)
> +        - 1xx: OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
> +    items:
> +      minItems: 3
> +      maxItems: 3

You do not have there three phandles, but one. Look how other bindings
encode this.

> +
> +  st,omm-req2ack-ns:
> +    description: |
> +      In multiplexed mode (MUXEN = 1), this field defines the time in
> +      nanoseconds between two transactions.
> +
> +  st,omm-cssel-ovr:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Configure the chip select selector override for the 2 OCTOSPIs.
> +      The 2 bits mask muxing description is:

bit mask of size? 1? Then just enum string, no?

> +        -bit 0: Chip select selector override setting for OCTOSPI1
> +          0x0: the chip select signal from OCTOSPI1 is sent to NCS1
> +          0x1: the chip select signal from OCTOSPI1 is sent to NCS2
> +        -bit 1: Chip select selector override setting for OCTOSPI2
> +          0x0: the chip select signal from OCTOSPI2 is sent to NCS1
> +          0x1: the chip select signal from OCTOSPI2 is sent to NCS2

I don't understand why this is so complicated. First, can you even send
chip select OCTOSPI1 to NCS2 and use 0x1 as mux? or 0x3 as mux?

Second, your bitmask value of "0x0" means OCTOSPI1 and OCTOSPI2 are sent
to NCS1 (whateveer NCS is). This sounds wrong, but your binding says is
perfectly correct. Is that true? Is that correct binding?


> +
> +  st,omm-mux:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Configure the muxing between the 2 OCTOSPIs busses and the 2 output ports.
> +      The muxing 2 bits mask description is:
> +        - 0x0: direct mode, default
> +        - 0x1: mux OCTOSPI1 and OCTOSPI2 to port 1
> +        - 0x2: swapped mode
> +        - 0x3: mux OCTOSPI1 and OCTOSPI2 to port 2

So these are just 1-3, not hex, not bitmasks. Missing constraints or
enum.




> +
> +  power-domains:
> +    maxItems: 1
> +
> +patternProperties:
> +  ^spi@[a-f0-9]+$:
> +    type: object
> +    $ref: "/schemas/spi/st,stm32mp25-ospi.yaml#"

Not much improved. I think you got here comment to drop quotes. That's
the second comment you ignored.

> +    description: Required spi child node
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - clocks
> +  - st,syscfg-amcr
> +  - ranges
> +
> +additionalProperties: 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>
> +    ommanager@40500000 {
> +      compatible = "st,stm32mp25-omm";
> +      reg = <0x40500000 0x400>, <0x60000000 0x10000000>;
> +      reg-names = "regs", "memory_map";
> +      memory-region = <&mm_ospi1>, <&mm_ospi2>;
> +      pinctrl-names = "default", "sleep";

pinctrl-names after pinctrl-1

> +      pinctrl-0 = <&ospi_port1_clk_pins_a
> +                   &ospi_port1_io03_pins_a
> +                   &ospi_port1_cs0_pins_a>;
> +      pinctrl-1 = <&ospi_port1_clk_sleep_pins_a
> +                   &ospi_port1_io03_sleep_pins_a
> +                   &ospi_port1_cs0_sleep_pins_a>;
> +      clocks = <&rcc CK_BUS_OSPIIOM>;
> +      resets = <&rcc OSPIIOM_R>;
> +      st,syscfg-amcr = <&syscfg 0x2c00 0x7>;
> +      st,omm-req2ack-ns = <0x0>;

Time is never expressed in hex, we do not follow 0x18h clock in
continental Europe.

> +      st,omm-mux = <0x0>;
> +      st,omm-cssel-ovr = <0x0>;
> +      access-controllers = <&rifsc 111>;
> +      power-domains = <&CLUSTER_PD>;
> +
> +      #address-cells = <2>;
> +      #size-cells = <1>;
> +
> +      ranges = <0 0 0x40430000 0x400>,
> +               <1 0 0x40440000 0x400>;

ranges always go after reg/reg-names.

> +
> +      spi@40430000 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        compatible = "st,stm32mp25-ospi";

Please follow DTS coding style in ordering your properties.



Best regards,
Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Patrice CHOTARD]

On 1/29/25 08:52, Krzysztof Kozlowski wrote:
> On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>
>> Add bindings for STM32 Octo Memory Manager (OMM) controller.
>>
>> OMM manages:
>>   - the muxing between 2 OSPI busses and 2 output ports.
>>     There are 4 possible muxing configurations:
>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>         output is on port 2
>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>         OSPI2 output is on port 1
>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>   - the split of the memory area shared between the 2 OSPI instances.
>>   - chip select selection override.
>>   - the time between 2 transactions in multiplexed mode.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> ---
>>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>>  1 file changed, 190 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>> new file mode 100644
>> index 000000000000..7e0b150e0005
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
> 
> 
> Filename as compatible, so st,stm32mp25-omm.yaml
> 
> You already received this comment.

Sorry, i missed this update

> 
>> @@ -0,0 +1,190 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: STM32 Octo Memory Manager (OMM)
>> +
>> +maintainers:
>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>> +
>> +description: |
>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>> +  the same bus. It Supports up to:
>> +    - Two single/dual/quad/octal SPI interfaces
>> +    - Two ports for pin assignment
>> +
>> +properties:
>> +  compatible:
>> +    const: st,stm32mp25-omm
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 1
>> +
>> +  ranges:
>> +    description: |
>> +      Reflects the memory layout with four integer values per OSPI instance.
>> +      Format:
>> +      <chip-select> 0 <registers base address> <size>
> 
> Do you always have two children? If so, this should have maxItems.

No, we can have one child.

> 
>> +
>> +  reg:
>> +    items:
>> +      - description: OMM registers
>> +      - description: OMM memory map area
>> +
>> +  reg-names:
>> +    items:
>> +      - const: regs
>> +      - const: memory_map
>> +
>> +  memory-region:
>> +    description: Phandle to node describing memory-map region to used.
>> +    minItems: 1
>> +    maxItems: 2
> 
> List the items with description instead with optional minItems. Why is
> this flexible in number of items?

If only one child (OCTOSPI instance), only one memory-region is needed.

Another update, i will reintroduce "memory-region-names:" which was 
wrongly removed in V2, i have forgotten one particular case.

We need memory-region-names in case only one OCTOSPI instance is 
used. If it's OCTOCPI2 and the whole memory-map region
is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)

We need to know to which OCTOSPI instance the memory region is associated
with, in order to check "st,syscfg-amcr" 's value which must be coherent 
with memory region declared.

so i will add :

  memory-region-names:
    description: |
      OCTOSPI instance's name to which memory region is associated
    items:
      - const: ospi1
      - const: ospi2

> 
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  access-controllers:
>> +    maxItems: 1
>> +
>> +  st,syscfg-amcr:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: |
>> +      The Address Mapping Control Register (AMCR) is used to split the 256MB
>> +      memory map area shared between the 2 OSPI instance. The Octo Memory
>> +      Manager sets the AMCR depending of the memory-region configuration.
>> +      Format is phandle to syscfg / register offset within syscfg / memory split
>> +      bitmask.
> 
> Don't repeat constraints in free form text.

ok

> 
>> +      The memory split bitmask description is:
>> +        - 000: OCTOSPI1 (256 Mbytes), OCTOSPI2 unmapped
>> +        - 001: OCTOSPI1 (192 Mbytes), OCTOSPI2 (64 Mbytes)
>> +        - 010: OCTOSPI1 (128 Mbytes), OCTOSPI2 (128 Mbytes)
>> +        - 011: OCTOSPI1 (64 Mbytes), OCTOSPI2 (192 Mbytes)
>> +        - 1xx: OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>> +    items:
>> +      minItems: 3
>> +      maxItems: 3
> 
> You do not have there three phandles, but one. Look how other bindings
> encode this.

yes, i see, will update with 

    items:
      - description: phandle to syscfg
      - description: register offset within syscfg
      - description: register bitmask for memory split

> 
>> +
>> +  st,omm-req2ack-ns:
>> +    description: |
>> +      In multiplexed mode (MUXEN = 1), this field defines the time in
>> +      nanoseconds between two transactions.
>> +
>> +  st,omm-cssel-ovr:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      Configure the chip select selector override for the 2 OCTOSPIs.
>> +      The 2 bits mask muxing description is:
> 
> bit mask of size? 1? Then just enum string, no?

I didn't get your point ? the size of bitmask is 2 bits as indicated.
    -bit 0: Chip select selector override setting for OCTOSPI1
    -bit 1: Chip select selector override setting for OCTOSPI2


> 
>> +        -bit 0: Chip select selector override setting for OCTOSPI1
>> +          0x0: the chip select signal from OCTOSPI1 is sent to NCS1
>> +          0x1: the chip select signal from OCTOSPI1 is sent to NCS2
>> +        -bit 1: Chip select selector override setting for OCTOSPI2
>> +          0x0: the chip select signal from OCTOSPI2 is sent to NCS1
>> +          0x1: the chip select signal from OCTOSPI2 is sent to NCS2
> 
> I don't understand why this is so complicated. First, can you even send
> chip select OCTOSPI1 to NCS2 and use 0x1 as mux? or 0x3 as mux?


By default, if st,omm-cssel-ovr property is not present:
  _ chip select OCTOSPI1 is send to NCS1
  _ chip select OCTOSPI2 is send to NCS2

It's the default configuration.

If st,omm-cssel-ovr property is present, you can mux the chip select 
of both OCTOSPI instance on NCS1 or NCS2 as you want.

Yes you can send chip select OCTOSPI1 to NCS2 by using 0x1 as bitmask mux
(in this case chip select OCTOSPI2 is sent to NCS1).

If you use 0x3 as bitmask mux, you send  :
   _ chip select OCTOSPI1 is sent to NCS2
   _ chip select OCTOSPI2 is sent to NCS2

> 
> Second, your bitmask value of "0x0" means OCTOSPI1 and OCTOSPI2 are sent

i think the 0x0/0x1 in the description brings to confusion as it's only the 
bit value not the bitmask.

> to NCS1 (whateveer NCS is). This sounds wrong, but your binding says is
> perfectly correct. Is that true? Is that correct binding?

 4 bitmask possible choice :
   0x0 : the chip select signal from OCTOSPI1 is sent to NCS1
	 the chip select signal from OCTOSPI2 is sent to NCS1

   0x1 : the chip select signal from OCTOSPI1 is sent to NCS2
	 the chip select signal from OCTOSPI2 is sent to NCS1

   0x2 : the chip select signal from OCTOSPI1 is sent to NCS1
	 the chip select signal from OCTOSPI2 is sent to NCS2

   0x3 : the chip select signal from OCTOSPI1 is sent to NCS2
	 the chip select signal from OCTOSPI2 is sent to NCS2


I propose to update the st,omm-cssel-ovr description as following

  st,omm-cssel-ovr:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: |
      Configure the chip select selector override for the 2 OCTOSPIs.
        - 0: OCTOSPI1 chip select send to NCS1 OCTOSPI2 chip select send to NCS1
        - 1: OCTOSPI1 chip select send to NCS2 OCTOSPI2 chip select send to NCS1
        - 2: OCTOSPI1 chip select send to NCS1 OCTOSPI2 chip select send to NCS2
        - 3: OCTOSPI1 chip select send to NCS2 OCTOSPI2 chip select send to NCS2
    minimum: 0
    maximum: 3


> 
> 
>> +
>> +  st,omm-mux:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      Configure the muxing between the 2 OCTOSPIs busses and the 2 output ports.
>> +      The muxing 2 bits mask description is:
>> +        - 0x0: direct mode, default
>> +        - 0x1: mux OCTOSPI1 and OCTOSPI2 to port 1
>> +        - 0x2: swapped mode
>> +        - 0x3: mux OCTOSPI1 and OCTOSPI2 to port 2
> 
> So these are just 1-3, not hex, not bitmasks. Missing constraints or
> enum.


ok , i will update as following

  st,omm-mux:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: |
      Configure the muxing between the 2 OCTOSPIs busses and the 2 output ports.
        - 0: direct mode, default
        - 1: mux OCTOSPI1 and OCTOSPI2 to port 1
        - 2: swapped mode
        - 3: mux OCTOSPI1 and OCTOSPI2 to port 2
    minimum: 0
    maximum: 3


> 
> 
> 
> 
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  ^spi@[a-f0-9]+$:
>> +    type: object
>> +    $ref: "/schemas/spi/st,stm32mp25-ospi.yaml#"
> 
> Not much improved. I think you got here comment to drop quotes. That's
> the second comment you ignored.

sorry, i missed this comment too

> 
>> +    description: Required spi child node
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - clocks
>> +  - st,syscfg-amcr
>> +  - ranges
>> +
>> +additionalProperties: 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>
>> +    ommanager@40500000 {
>> +      compatible = "st,stm32mp25-omm";
>> +      reg = <0x40500000 0x400>, <0x60000000 0x10000000>;
>> +      reg-names = "regs", "memory_map";
>> +      memory-region = <&mm_ospi1>, <&mm_ospi2>;
>> +      pinctrl-names = "default", "sleep";
> 
> pinctrl-names after pinctrl-1

ok

> 
>> +      pinctrl-0 = <&ospi_port1_clk_pins_a
>> +                   &ospi_port1_io03_pins_a
>> +                   &ospi_port1_cs0_pins_a>;
>> +      pinctrl-1 = <&ospi_port1_clk_sleep_pins_a
>> +                   &ospi_port1_io03_sleep_pins_a
>> +                   &ospi_port1_cs0_sleep_pins_a>;
>> +      clocks = <&rcc CK_BUS_OSPIIOM>;
>> +      resets = <&rcc OSPIIOM_R>;
>> +      st,syscfg-amcr = <&syscfg 0x2c00 0x7>;
>> +      st,omm-req2ack-ns = <0x0>;
> 
> Time is never expressed in hex, we do not follow 0x18h clock in
> continental Europe.

ok

> 
>> +      st,omm-mux = <0x0>;
>> +      st,omm-cssel-ovr = <0x0>;
>> +      access-controllers = <&rifsc 111>;
>> +      power-domains = <&CLUSTER_PD>;
>> +
>> +      #address-cells = <2>;
>> +      #size-cells = <1>;
>> +
>> +      ranges = <0 0 0x40430000 0x400>,
>> +               <1 0 0x40440000 0x400>;
> 
> ranges always go after reg/reg-names.

ok

> 
>> +
>> +      spi@40430000 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        compatible = "st,stm32mp25-ospi";
> 
> Please follow DTS coding style in ordering your properties.
ok i will move address-cells and size-cells at the correct place.

Thanks 
Patrice

> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Krzysztof Kozlowski]

On 30/01/2025 09:57, Patrice CHOTARD wrote:
> 
> 
> On 1/29/25 08:52, Krzysztof Kozlowski wrote:
>> On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>
>>> Add bindings for STM32 Octo Memory Manager (OMM) controller.
>>>
>>> OMM manages:
>>>   - the muxing between 2 OSPI busses and 2 output ports.
>>>     There are 4 possible muxing configurations:
>>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>>         output is on port 2
>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>>         OSPI2 output is on port 1
>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>>   - the split of the memory area shared between the 2 OSPI instances.
>>>   - chip select selection override.
>>>   - the time between 2 transactions in multiplexed mode.
>>>
>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>>> ---
>>>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>>>  1 file changed, 190 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>> new file mode 100644
>>> index 000000000000..7e0b150e0005
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>
>>
>> Filename as compatible, so st,stm32mp25-omm.yaml
>>
>> You already received this comment.
> 
> Sorry, i missed this update
> 
>>
>>> @@ -0,0 +1,190 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: STM32 Octo Memory Manager (OMM)
>>> +
>>> +maintainers:
>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>> +
>>> +description: |
>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>> +  the same bus. It Supports up to:
>>> +    - Two single/dual/quad/octal SPI interfaces
>>> +    - Two ports for pin assignment
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: st,stm32mp25-omm
>>> +
>>> +  "#address-cells":
>>> +    const: 2
>>> +
>>> +  "#size-cells":
>>> +    const: 1
>>> +
>>> +  ranges:
>>> +    description: |
>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>> +      Format:
>>> +      <chip-select> 0 <registers base address> <size>
>>
>> Do you always have two children? If so, this should have maxItems.
> 
> No, we can have one child.

For the same SoC? How? You put the spi@ in the soc, so I don't
understand how one child is possible.

> 
>>
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: OMM registers
>>> +      - description: OMM memory map area
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: regs
>>> +      - const: memory_map
>>> +
>>> +  memory-region:
>>> +    description: Phandle to node describing memory-map region to used.
>>> +    minItems: 1
>>> +    maxItems: 2
>>
>> List the items with description instead with optional minItems. Why is
>> this flexible in number of items?
> 
> If only one child (OCTOSPI instance), only one memory-region is needed.

Which is not possible... look at your DTSI.

> 
> Another update, i will reintroduce "memory-region-names:" which was 
> wrongly removed in V2, i have forgotten one particular case.
> 
> We need memory-region-names in case only one OCTOSPI instance is 
> used. If it's OCTOCPI2 and the whole memory-map region
> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
> 
> We need to know to which OCTOSPI instance the memory region is associated
> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
> with memory region declared.
> 
> so i will add :
> 
>   memory-region-names:
>     description: |
>       OCTOSPI instance's name to which memory region is associated
>     items:
>       - const: ospi1
>       - const: ospi2
> 

I don't think this matches what you are saying to us. Let's talk about
the hardware which is directly represented by DTS/DTSI. You always have
two instances.


>>
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  resets:
>>> +    maxItems: 1
>>> +
>>> +  access-controllers:
>>> +    maxItems: 1
>>> +
>>> +  st,syscfg-amcr:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description: |
>>> +      The Address Mapping Control Register (AMCR) is used to split the 256MB
>>> +      memory map area shared between the 2 OSPI instance. The Octo Memory
>>> +      Manager sets the AMCR depending of the memory-region configuration.
>>> +      Format is phandle to syscfg / register offset within syscfg / memory split
>>> +      bitmask.
>>
>> Don't repeat constraints in free form text.
> 
> ok
> 
>>
>>> +      The memory split bitmask description is:
>>> +        - 000: OCTOSPI1 (256 Mbytes), OCTOSPI2 unmapped
>>> +        - 001: OCTOSPI1 (192 Mbytes), OCTOSPI2 (64 Mbytes)
>>> +        - 010: OCTOSPI1 (128 Mbytes), OCTOSPI2 (128 Mbytes)
>>> +        - 011: OCTOSPI1 (64 Mbytes), OCTOSPI2 (192 Mbytes)
>>> +        - 1xx: OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>> +    items:
>>> +      minItems: 3
>>> +      maxItems: 3
>>
>> You do not have there three phandles, but one. Look how other bindings
>> encode this.
> 
> yes, i see, will update with 
> 
>     items:
>       - description: phandle to syscfg
>       - description: register offset within syscfg
>       - description: register bitmask for memory split
> 
>>
>>> +
>>> +  st,omm-req2ack-ns:
>>> +    description: |
>>> +      In multiplexed mode (MUXEN = 1), this field defines the time in
>>> +      nanoseconds between two transactions.
>>> +
>>> +  st,omm-cssel-ovr:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      Configure the chip select selector override for the 2 OCTOSPIs.
>>> +      The 2 bits mask muxing description is:
>>
>> bit mask of size? 1? Then just enum string, no?
> 
> I didn't get your point ? the size of bitmask is 2 bits as indicated.
>     -bit 0: Chip select selector override setting for OCTOSPI1
>     -bit 1: Chip select selector override setting for OCTOSPI2
> 
> 
>>
>>> +        -bit 0: Chip select selector override setting for OCTOSPI1
>>> +          0x0: the chip select signal from OCTOSPI1 is sent to NCS1
>>> +          0x1: the chip select signal from OCTOSPI1 is sent to NCS2
>>> +        -bit 1: Chip select selector override setting for OCTOSPI2
>>> +          0x0: the chip select signal from OCTOSPI2 is sent to NCS1
>>> +          0x1: the chip select signal from OCTOSPI2 is sent to NCS2
>>
>> I don't understand why this is so complicated. First, can you even send
>> chip select OCTOSPI1 to NCS2 and use 0x1 as mux? or 0x3 as mux?
> 
> 
> By default, if st,omm-cssel-ovr property is not present:
>   _ chip select OCTOSPI1 is send to NCS1
>   _ chip select OCTOSPI2 is send to NCS2
> 
> It's the default configuration.
> 
> If st,omm-cssel-ovr property is present, you can mux the chip select 
> of both OCTOSPI instance on NCS1 or NCS2 as you want.
> 
> Yes you can send chip select OCTOSPI1 to NCS2 by using 0x1 as bitmask mux
> (in this case chip select OCTOSPI2 is sent to NCS1).
> 
> If you use 0x3 as bitmask mux, you send  :
>    _ chip select OCTOSPI1 is sent to NCS2
>    _ chip select OCTOSPI2 is sent to NCS2
> 
>>
>> Second, your bitmask value of "0x0" means OCTOSPI1 and OCTOSPI2 are sent
> 
> i think the 0x0/0x1 in the description brings to confusion as it's only the 
> bit value not the bitmask.
> 
>> to NCS1 (whateveer NCS is). This sounds wrong, but your binding says is
>> perfectly correct. Is that true? Is that correct binding?
> 
>  4 bitmask possible choice :
>    0x0 : the chip select signal from OCTOSPI1 is sent to NCS1
> 	 the chip select signal from OCTOSPI2 is sent to NCS1
> 
>    0x1 : the chip select signal from OCTOSPI1 is sent to NCS2
> 	 the chip select signal from OCTOSPI2 is sent to NCS1
> 
>    0x2 : the chip select signal from OCTOSPI1 is sent to NCS1
> 	 the chip select signal from OCTOSPI2 is sent to NCS2
> 
>    0x3 : the chip select signal from OCTOSPI1 is sent to NCS2
> 	 the chip select signal from OCTOSPI2 is sent to NCS2
> 
> 
> I propose to update the st,omm-cssel-ovr description as following
> 
>   st,omm-cssel-ovr:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description: |
>       Configure the chip select selector override for the 2 OCTOSPIs.
>         - 0: OCTOSPI1 chip select send to NCS1 OCTOSPI2 chip select send to NCS1
>         - 1: OCTOSPI1 chip select send to NCS2 OCTOSPI2 chip select send to NCS1
>         - 2: OCTOSPI1 chip select send to NCS1 OCTOSPI2 chip select send to NCS2
>         - 3: OCTOSPI1 chip select send to NCS2 OCTOSPI2 chip select send to NCS2
>     minimum: 0
>     maximum: 3
> 

My concerns were because I understood that this is not a real bitmask,
IOW you cannot set two of them to NCS2. But you said that setting of
0x3, so both going to NCS2, is perfectly correct setting, so it's fine.



Best regards,
Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Patrice CHOTARD]

On 1/30/25 13:12, Krzysztof Kozlowski wrote:
> On 30/01/2025 09:57, Patrice CHOTARD wrote:
>>
>>
>> On 1/29/25 08:52, Krzysztof Kozlowski wrote:
>>> On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
>>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>
>>>> Add bindings for STM32 Octo Memory Manager (OMM) controller.
>>>>
>>>> OMM manages:
>>>>   - the muxing between 2 OSPI busses and 2 output ports.
>>>>     There are 4 possible muxing configurations:
>>>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>>>         output is on port 2
>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>>>         OSPI2 output is on port 1
>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>>>   - the split of the memory area shared between the 2 OSPI instances.
>>>>   - chip select selection override.
>>>>   - the time between 2 transactions in multiplexed mode.
>>>>
>>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>>>> ---
>>>>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>>>>  1 file changed, 190 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>> new file mode 100644
>>>> index 000000000000..7e0b150e0005
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>
>>>
>>> Filename as compatible, so st,stm32mp25-omm.yaml
>>>
>>> You already received this comment.
>>
>> Sorry, i missed this update
>>
>>>
>>>> @@ -0,0 +1,190 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: STM32 Octo Memory Manager (OMM)
>>>> +
>>>> +maintainers:
>>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>>> +
>>>> +description: |
>>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>>> +  the same bus. It Supports up to:
>>>> +    - Two single/dual/quad/octal SPI interfaces
>>>> +    - Two ports for pin assignment
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: st,stm32mp25-omm
>>>> +
>>>> +  "#address-cells":
>>>> +    const: 2
>>>> +
>>>> +  "#size-cells":
>>>> +    const: 1
>>>> +
>>>> +  ranges:
>>>> +    description: |
>>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>>> +      Format:
>>>> +      <chip-select> 0 <registers base address> <size>
>>>
>>> Do you always have two children? If so, this should have maxItems.
>>
>> No, we can have one child.
> 
> For the same SoC? How? You put the spi@ in the soc, so I don't
> understand how one child is possible.

Yes for the same SoC, in DTSI file, the both OCTOSPI child are declared 
but are disabled by default.

In the DTS board file, 0,1 or 2 OCTOSPI instance can be enabled depending of the board design.

In our case, on stm32mp257f-ev1 board, one SPI-NOR is soldered on PCB, so only one OCTOSPI 
instance is needed and enabled.

Internally we got validation boards with several memory devices connected to OCTOSPI1 and 
OCTOSPI2, in this case, both OCTOSPI instance are needed and enabled.

> 
>>
>>>
>>>> +
>>>> +  reg:
>>>> +    items:
>>>> +      - description: OMM registers
>>>> +      - description: OMM memory map area
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: regs
>>>> +      - const: memory_map
>>>> +
>>>> +  memory-region:
>>>> +    description: Phandle to node describing memory-map region to used.
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>>
>>> List the items with description instead with optional minItems. Why is
>>> this flexible in number of items?
>>
>> If only one child (OCTOSPI instance), only one memory-region is needed.
> 
> Which is not possible... look at your DTSI.

It's possible. if one OCTOSPI is used (the second one is kept disabled), only
one memory-region is needed.

> 
>>
>> Another update, i will reintroduce "memory-region-names:" which was 
>> wrongly removed in V2, i have forgotten one particular case.
>>
>> We need memory-region-names in case only one OCTOSPI instance is 
>> used. If it's OCTOCPI2 and the whole memory-map region
>> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>
>> We need to know to which OCTOSPI instance the memory region is associated
>> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
>> with memory region declared.
>>
>> so i will add :
>>
>>   memory-region-names:
>>     description: |
>>       OCTOSPI instance's name to which memory region is associated
>>     items:
>>       - const: ospi1
>>       - const: ospi2
>>
> 
> I don't think this matches what you are saying to us. Let's talk about
> the hardware which is directly represented by DTS/DTSI. You always have
> two instances.
> 
> 

We have 2 instances, but both not always enabled.
In case only one is enabled, only one memory-region-names is needed.

We must know to which OCTCOSPI the memory-region makes reference to, in order
to configure and/or check the memory region split configuration. That' swhy 
the memory-regions-names must specify if it's the OCTOSPI1 or OCTOSPI2 instance.

>>>
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +
>>>> +  resets:
>>>> +    maxItems: 1
>>>> +
>>>> +  access-controllers:
>>>> +    maxItems: 1
>>>> +
>>>> +  st,syscfg-amcr:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>>> +    description: |
>>>> +      The Address Mapping Control Register (AMCR) is used to split the 256MB
>>>> +      memory map area shared between the 2 OSPI instance. The Octo Memory
>>>> +      Manager sets the AMCR depending of the memory-region configuration.
>>>> +      Format is phandle to syscfg / register offset within syscfg / memory split
>>>> +      bitmask.
>>>
>>> Don't repeat constraints in free form text.
>>
>> ok
>>
>>>
>>>> +      The memory split bitmask description is:
>>>> +        - 000: OCTOSPI1 (256 Mbytes), OCTOSPI2 unmapped
>>>> +        - 001: OCTOSPI1 (192 Mbytes), OCTOSPI2 (64 Mbytes)
>>>> +        - 010: OCTOSPI1 (128 Mbytes), OCTOSPI2 (128 Mbytes)
>>>> +        - 011: OCTOSPI1 (64 Mbytes), OCTOSPI2 (192 Mbytes)
>>>> +        - 1xx: OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>>> +    items:
>>>> +      minItems: 3
>>>> +      maxItems: 3
>>>
>>> You do not have there three phandles, but one. Look how other bindings
>>> encode this.
>>
>> yes, i see, will update with 
>>
>>     items:
>>       - description: phandle to syscfg
>>       - description: register offset within syscfg
>>       - description: register bitmask for memory split
>>
>>>
>>>> +
>>>> +  st,omm-req2ack-ns:
>>>> +    description: |
>>>> +      In multiplexed mode (MUXEN = 1), this field defines the time in
>>>> +      nanoseconds between two transactions.
>>>> +
>>>> +  st,omm-cssel-ovr:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: |
>>>> +      Configure the chip select selector override for the 2 OCTOSPIs.
>>>> +      The 2 bits mask muxing description is:
>>>
>>> bit mask of size? 1? Then just enum string, no?
>>
>> I didn't get your point ? the size of bitmask is 2 bits as indicated.
>>     -bit 0: Chip select selector override setting for OCTOSPI1
>>     -bit 1: Chip select selector override setting for OCTOSPI2
>>
>>
>>>
>>>> +        -bit 0: Chip select selector override setting for OCTOSPI1
>>>> +          0x0: the chip select signal from OCTOSPI1 is sent to NCS1
>>>> +          0x1: the chip select signal from OCTOSPI1 is sent to NCS2
>>>> +        -bit 1: Chip select selector override setting for OCTOSPI2
>>>> +          0x0: the chip select signal from OCTOSPI2 is sent to NCS1
>>>> +          0x1: the chip select signal from OCTOSPI2 is sent to NCS2
>>>
>>> I don't understand why this is so complicated. First, can you even send
>>> chip select OCTOSPI1 to NCS2 and use 0x1 as mux? or 0x3 as mux?
>>
>>
>> By default, if st,omm-cssel-ovr property is not present:
>>   _ chip select OCTOSPI1 is send to NCS1
>>   _ chip select OCTOSPI2 is send to NCS2
>>
>> It's the default configuration.
>>
>> If st,omm-cssel-ovr property is present, you can mux the chip select 
>> of both OCTOSPI instance on NCS1 or NCS2 as you want.
>>
>> Yes you can send chip select OCTOSPI1 to NCS2 by using 0x1 as bitmask mux
>> (in this case chip select OCTOSPI2 is sent to NCS1).
>>
>> If you use 0x3 as bitmask mux, you send  :
>>    _ chip select OCTOSPI1 is sent to NCS2
>>    _ chip select OCTOSPI2 is sent to NCS2
>>
>>>
>>> Second, your bitmask value of "0x0" means OCTOSPI1 and OCTOSPI2 are sent
>>
>> i think the 0x0/0x1 in the description brings to confusion as it's only the 
>> bit value not the bitmask.
>>
>>> to NCS1 (whateveer NCS is). This sounds wrong, but your binding says is
>>> perfectly correct. Is that true? Is that correct binding?
>>
>>  4 bitmask possible choice :
>>    0x0 : the chip select signal from OCTOSPI1 is sent to NCS1
>> 	 the chip select signal from OCTOSPI2 is sent to NCS1
>>
>>    0x1 : the chip select signal from OCTOSPI1 is sent to NCS2
>> 	 the chip select signal from OCTOSPI2 is sent to NCS1
>>
>>    0x2 : the chip select signal from OCTOSPI1 is sent to NCS1
>> 	 the chip select signal from OCTOSPI2 is sent to NCS2
>>
>>    0x3 : the chip select signal from OCTOSPI1 is sent to NCS2
>> 	 the chip select signal from OCTOSPI2 is sent to NCS2
>>
>>
>> I propose to update the st,omm-cssel-ovr description as following
>>
>>   st,omm-cssel-ovr:
>>     $ref: /schemas/types.yaml#/definitions/uint32
>>     description: |
>>       Configure the chip select selector override for the 2 OCTOSPIs.
>>         - 0: OCTOSPI1 chip select send to NCS1 OCTOSPI2 chip select send to NCS1
>>         - 1: OCTOSPI1 chip select send to NCS2 OCTOSPI2 chip select send to NCS1
>>         - 2: OCTOSPI1 chip select send to NCS1 OCTOSPI2 chip select send to NCS2
>>         - 3: OCTOSPI1 chip select send to NCS2 OCTOSPI2 chip select send to NCS2
>>     minimum: 0
>>     maximum: 3
>>
> 
> My concerns were because I understood that this is not a real bitmask,
> IOW you cannot set two of them to NCS2. But you said that setting of
> 0x3, so both going to NCS2, is perfectly correct setting, so it's fine.
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Krzysztof Kozlowski]

On 30/01/2025 14:32, Patrice CHOTARD wrote:
> 
> 
> On 1/30/25 13:12, Krzysztof Kozlowski wrote:
>> On 30/01/2025 09:57, Patrice CHOTARD wrote:
>>>
>>>
>>> On 1/29/25 08:52, Krzysztof Kozlowski wrote:
>>>> On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
>>>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>
>>>>> Add bindings for STM32 Octo Memory Manager (OMM) controller.
>>>>>
>>>>> OMM manages:
>>>>>   - the muxing between 2 OSPI busses and 2 output ports.
>>>>>     There are 4 possible muxing configurations:
>>>>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>>>>         output is on port 2
>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>>>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>>>>         OSPI2 output is on port 1
>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>>>>   - the split of the memory area shared between the 2 OSPI instances.
>>>>>   - chip select selection override.
>>>>>   - the time between 2 transactions in multiplexed mode.
>>>>>
>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>> ---
>>>>>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>>>>>  1 file changed, 190 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..7e0b150e0005
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>
>>>>
>>>> Filename as compatible, so st,stm32mp25-omm.yaml
>>>>
>>>> You already received this comment.
>>>
>>> Sorry, i missed this update
>>>
>>>>
>>>>> @@ -0,0 +1,190 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: STM32 Octo Memory Manager (OMM)
>>>>> +
>>>>> +maintainers:
>>>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>>>> +
>>>>> +description: |
>>>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>>>> +  the same bus. It Supports up to:
>>>>> +    - Two single/dual/quad/octal SPI interfaces
>>>>> +    - Two ports for pin assignment
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: st,stm32mp25-omm
>>>>> +
>>>>> +  "#address-cells":
>>>>> +    const: 2
>>>>> +
>>>>> +  "#size-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  ranges:
>>>>> +    description: |
>>>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>>>> +      Format:
>>>>> +      <chip-select> 0 <registers base address> <size>
>>>>
>>>> Do you always have two children? If so, this should have maxItems.
>>>
>>> No, we can have one child.
>>
>> For the same SoC? How? You put the spi@ in the soc, so I don't
>> understand how one child is possible.
> 
> Yes for the same SoC, in DTSI file, the both OCTOSPI child are declared 
> but are disabled by default.

But the child node is there anyway so are the ranges.

> 
> In the DTS board file, 0,1 or 2 OCTOSPI instance can be enabled depending of the board design.
> 
> In our case, on stm32mp257f-ev1 board, one SPI-NOR is soldered on PCB, so only one OCTOSPI 
> instance is needed and enabled.
> 
> Internally we got validation boards with several memory devices connected to OCTOSPI1 and 
> OCTOSPI2, in this case, both OCTOSPI instance are needed and enabled.

I could imagine that you would not want to have unused reserved ranges,
so that one indeed is flexible, I agree.

> 
>>
>>>
>>>>
>>>>> +
>>>>> +  reg:
>>>>> +    items:
>>>>> +      - description: OMM registers
>>>>> +      - description: OMM memory map area
>>>>> +
>>>>> +  reg-names:
>>>>> +    items:
>>>>> +      - const: regs
>>>>> +      - const: memory_map
>>>>> +
>>>>> +  memory-region:
>>>>> +    description: Phandle to node describing memory-map region to used.
>>>>> +    minItems: 1
>>>>> +    maxItems: 2
>>>>
>>>> List the items with description instead with optional minItems. Why is
>>>> this flexible in number of items?
>>>
>>> If only one child (OCTOSPI instance), only one memory-region is needed.
>>
>> Which is not possible... look at your DTSI.
> 
> It's possible. if one OCTOSPI is used (the second one is kept disabled), only
> one memory-region is needed.

Ack.

> 
>>
>>>
>>> Another update, i will reintroduce "memory-region-names:" which was 
>>> wrongly removed in V2, i have forgotten one particular case.
>>>
>>> We need memory-region-names in case only one OCTOSPI instance is 
>>> used. If it's OCTOCPI2 and the whole memory-map region
>>> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>>
>>> We need to know to which OCTOSPI instance the memory region is associated
>>> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
>>> with memory region declared.
>>>
>>> so i will add :
>>>
>>>   memory-region-names:
>>>     description: |
>>>       OCTOSPI instance's name to which memory region is associated
>>>     items:
>>>       - const: ospi1
>>>       - const: ospi2
>>>
>>
>> I don't think this matches what you are saying to us. Let's talk about
>> the hardware which is directly represented by DTS/DTSI. You always have
>> two instances.
>>
>>
> 
> We have 2 instances, but both not always enabled.
> In case only one is enabled, only one memory-region-names is needed.
> 
> We must know to which OCTCOSPI the memory-region makes reference to, in order
> to configure and/or check the memory region split configuration. That' swhy 
> the memory-regions-names must specify if it's the OCTOSPI1 or OCTOSPI2 instance.

Well, in that case two comments.
1. Above syntax does not allow you to skip one item. You would need:
items:
  enum: [ospi1, ospi2]
minItems: 1
maxItems: 2

2. But this points to other problem. From the omm-manager node point of
view, you should define all the resources regardless whether the child
is enabled or not. You do not skip some part of 'reg' if child is
missing. Do not skip interrupts, access controllers, clocks etc.
If some resource is to be skipped, it means that it belongs to the
child, not to the parent, IMO.
Therefore memory-region looks like child's property.

Imagine different case: runtime loaded overlay. In your setup, you probe
omm-manager with one memory-region and one child. Then someone loads
overlay enabling the second child, second SPI.

That's of course imaginary case, but shows the concept how the parent
would work.

It's the same with other buses in the kernel. You can load overlay with
any new child and the parent should not get new properties.



Best regards,
Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Patrice CHOTARD]

On 1/30/25 16:09, Krzysztof Kozlowski wrote:
> On 30/01/2025 14:32, Patrice CHOTARD wrote:
>>
>>
>> On 1/30/25 13:12, Krzysztof Kozlowski wrote:
>>> On 30/01/2025 09:57, Patrice CHOTARD wrote:
>>>>
>>>>
>>>> On 1/29/25 08:52, Krzysztof Kozlowski wrote:
>>>>> On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
>>>>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>
>>>>>> Add bindings for STM32 Octo Memory Manager (OMM) controller.
>>>>>>
>>>>>> OMM manages:
>>>>>>   - the muxing between 2 OSPI busses and 2 output ports.
>>>>>>     There are 4 possible muxing configurations:
>>>>>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>>>>>         output is on port 2
>>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>>>>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>>>>>         OSPI2 output is on port 1
>>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>>>>>   - the split of the memory area shared between the 2 OSPI instances.
>>>>>>   - chip select selection override.
>>>>>>   - the time between 2 transactions in multiplexed mode.
>>>>>>
>>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>> ---
>>>>>>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>>>>>>  1 file changed, 190 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..7e0b150e0005
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>
>>>>>
>>>>> Filename as compatible, so st,stm32mp25-omm.yaml
>>>>>
>>>>> You already received this comment.
>>>>
>>>> Sorry, i missed this update
>>>>
>>>>>
>>>>>> @@ -0,0 +1,190 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: STM32 Octo Memory Manager (OMM)
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>> +
>>>>>> +description: |
>>>>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>>>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>>>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>>>>> +  the same bus. It Supports up to:
>>>>>> +    - Two single/dual/quad/octal SPI interfaces
>>>>>> +    - Two ports for pin assignment
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: st,stm32mp25-omm
>>>>>> +
>>>>>> +  "#address-cells":
>>>>>> +    const: 2
>>>>>> +
>>>>>> +  "#size-cells":
>>>>>> +    const: 1
>>>>>> +
>>>>>> +  ranges:
>>>>>> +    description: |
>>>>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>>>>> +      Format:
>>>>>> +      <chip-select> 0 <registers base address> <size>
>>>>>
>>>>> Do you always have two children? If so, this should have maxItems.
>>>>
>>>> No, we can have one child.
>>>
>>> For the same SoC? How? You put the spi@ in the soc, so I don't
>>> understand how one child is possible.
>>
>> Yes for the same SoC, in DTSI file, the both OCTOSPI child are declared 
>> but are disabled by default.
> 
> But the child node is there anyway so are the ranges.

if both child are disabled, omm-manager should be disabled as well, 
omm-manager alone makes no sense.

> 
>>
>> In the DTS board file, 0,1 or 2 OCTOSPI instance can be enabled depending of the board design.
>>
>> In our case, on stm32mp257f-ev1 board, one SPI-NOR is soldered on PCB, so only one OCTOSPI 
>> instance is needed and enabled.
>>
>> Internally we got validation boards with several memory devices connected to OCTOSPI1 and 
>> OCTOSPI2, in this case, both OCTOSPI instance are needed and enabled.
> 
> I could imagine that you would not want to have unused reserved ranges,
> so that one indeed is flexible, I agree.
> 
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +  reg:
>>>>>> +    items:
>>>>>> +      - description: OMM registers
>>>>>> +      - description: OMM memory map area
>>>>>> +
>>>>>> +  reg-names:
>>>>>> +    items:
>>>>>> +      - const: regs
>>>>>> +      - const: memory_map
>>>>>> +
>>>>>> +  memory-region:
>>>>>> +    description: Phandle to node describing memory-map region to used.
>>>>>> +    minItems: 1
>>>>>> +    maxItems: 2
>>>>>
>>>>> List the items with description instead with optional minItems. Why is
>>>>> this flexible in number of items?
>>>>
>>>> If only one child (OCTOSPI instance), only one memory-region is needed.
>>>
>>> Which is not possible... look at your DTSI.
>>
>> It's possible. if one OCTOSPI is used (the second one is kept disabled), only
>> one memory-region is needed.
> 
> Ack.
> 
>>
>>>
>>>>
>>>> Another update, i will reintroduce "memory-region-names:" which was 
>>>> wrongly removed in V2, i have forgotten one particular case.
>>>>
>>>> We need memory-region-names in case only one OCTOSPI instance is 
>>>> used. If it's OCTOCPI2 and the whole memory-map region
>>>> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>>>
>>>> We need to know to which OCTOSPI instance the memory region is associated
>>>> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
>>>> with memory region declared.
>>>>
>>>> so i will add :
>>>>
>>>>   memory-region-names:
>>>>     description: |
>>>>       OCTOSPI instance's name to which memory region is associated
>>>>     items:
>>>>       - const: ospi1
>>>>       - const: ospi2
>>>>
>>>
>>> I don't think this matches what you are saying to us. Let's talk about
>>> the hardware which is directly represented by DTS/DTSI. You always have
>>> two instances.
>>>
>>>
>>
>> We have 2 instances, but both not always enabled.
>> In case only one is enabled, only one memory-region-names is needed.
>>
>> We must know to which OCTCOSPI the memory-region makes reference to, in order
>> to configure and/or check the memory region split configuration. That' swhy 
>> the memory-regions-names must specify if it's the OCTOSPI1 or OCTOSPI2 instance.
> 
> Well, in that case two comments.
> 1. Above syntax does not allow you to skip one item. You would need:
> items:
>   enum: [ospi1, ospi2]
> minItems: 1
> maxItems: 2
> 

ok

> 2. But this points to other problem. From the omm-manager node point of
> view, you should define all the resources regardless whether the child
> is enabled or not. You do not skip some part of 'reg' if child is
> missing. Do not skip interrupts, access controllers, clocks etc.
> If some resource is to be skipped, it means that it belongs to the
> child, not to the parent, IMO.

I didn't get your point. 

The resource declared in omm-manager's node pnly belongs to omm-manager
(reg/clocks/resets/access-controllers/st,syscfg-amcr/power-domains), regardless 
there are 1 or 2 children. None of them can be skipped.

If omm-manager has no child enabled, omm-manager must be disabled as well in DT.

> Therefore memory-region looks like child's property.
> 
> Imagine different case: runtime loaded overlay. In your setup, you probe
> omm-manager with one memory-region and one child. Then someone loads
> overlay enabling the second child, second SPI.
> 
> That's of course imaginary case, but shows the concept how the parent
> would work.
> 
> It's the same with other buses in the kernel. You can load overlay with
> any new child and the parent should not get new properties.
> 

In case of runtime loaded overlay, if a second child is added with an associated 
memory-region, omm-manager must be unbind/bind to :
  _ check the added child's access rights.
  _ take into account the added child's memory-region configuration (to set 
    the syscfg-amcr register accordingly) 

Thanks
Patrice

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Krzysztof Kozlowski]

On 03/02/2025 11:46, Patrice CHOTARD wrote:
> 
> 
> On 1/30/25 16:09, Krzysztof Kozlowski wrote:
>> On 30/01/2025 14:32, Patrice CHOTARD wrote:
>>>
>>>
>>> On 1/30/25 13:12, Krzysztof Kozlowski wrote:
>>>> On 30/01/2025 09:57, Patrice CHOTARD wrote:
>>>>>
>>>>>
>>>>> On 1/29/25 08:52, Krzysztof Kozlowski wrote:
>>>>>> On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
>>>>>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>>
>>>>>>> Add bindings for STM32 Octo Memory Manager (OMM) controller.
>>>>>>>
>>>>>>> OMM manages:
>>>>>>>   - the muxing between 2 OSPI busses and 2 output ports.
>>>>>>>     There are 4 possible muxing configurations:
>>>>>>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>>>>>>         output is on port 2
>>>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>>>>>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>>>>>>         OSPI2 output is on port 1
>>>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>>>>>>   - the split of the memory area shared between the 2 OSPI instances.
>>>>>>>   - chip select selection override.
>>>>>>>   - the time between 2 transactions in multiplexed mode.
>>>>>>>
>>>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>> ---
>>>>>>>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>>>>>>>  1 file changed, 190 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..7e0b150e0005
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>>
>>>>>>
>>>>>> Filename as compatible, so st,stm32mp25-omm.yaml
>>>>>>
>>>>>> You already received this comment.
>>>>>
>>>>> Sorry, i missed this update
>>>>>
>>>>>>
>>>>>>> @@ -0,0 +1,190 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: STM32 Octo Memory Manager (OMM)
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>> +
>>>>>>> +description: |
>>>>>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>>>>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>>>>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>>>>>> +  the same bus. It Supports up to:
>>>>>>> +    - Two single/dual/quad/octal SPI interfaces
>>>>>>> +    - Two ports for pin assignment
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: st,stm32mp25-omm
>>>>>>> +
>>>>>>> +  "#address-cells":
>>>>>>> +    const: 2
>>>>>>> +
>>>>>>> +  "#size-cells":
>>>>>>> +    const: 1
>>>>>>> +
>>>>>>> +  ranges:
>>>>>>> +    description: |
>>>>>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>>>>>> +      Format:
>>>>>>> +      <chip-select> 0 <registers base address> <size>
>>>>>>
>>>>>> Do you always have two children? If so, this should have maxItems.
>>>>>
>>>>> No, we can have one child.
>>>>
>>>> For the same SoC? How? You put the spi@ in the soc, so I don't
>>>> understand how one child is possible.
>>>
>>> Yes for the same SoC, in DTSI file, the both OCTOSPI child are declared 
>>> but are disabled by default.
>>
>> But the child node is there anyway so are the ranges.
> 
> if both child are disabled, omm-manager should be disabled as well, 
> omm-manager alone makes no sense.


Yes, it is obvious, but how is this related?

> 
>>
>>>
>>> In the DTS board file, 0,1 or 2 OCTOSPI instance can be enabled depending of the board design.
>>>
>>> In our case, on stm32mp257f-ev1 board, one SPI-NOR is soldered on PCB, so only one OCTOSPI 
>>> instance is needed and enabled.
>>>
>>> Internally we got validation boards with several memory devices connected to OCTOSPI1 and 
>>> OCTOSPI2, in this case, both OCTOSPI instance are needed and enabled.
>>
>> I could imagine that you would not want to have unused reserved ranges,
>> so that one indeed is flexible, I agree.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    items:
>>>>>>> +      - description: OMM registers
>>>>>>> +      - description: OMM memory map area
>>>>>>> +
>>>>>>> +  reg-names:
>>>>>>> +    items:
>>>>>>> +      - const: regs
>>>>>>> +      - const: memory_map
>>>>>>> +
>>>>>>> +  memory-region:
>>>>>>> +    description: Phandle to node describing memory-map region to used.
>>>>>>> +    minItems: 1
>>>>>>> +    maxItems: 2
>>>>>>
>>>>>> List the items with description instead with optional minItems. Why is
>>>>>> this flexible in number of items?
>>>>>
>>>>> If only one child (OCTOSPI instance), only one memory-region is needed.
>>>>
>>>> Which is not possible... look at your DTSI.
>>>
>>> It's possible. if one OCTOSPI is used (the second one is kept disabled), only
>>> one memory-region is needed.
>>
>> Ack.
>>
>>>
>>>>
>>>>>
>>>>> Another update, i will reintroduce "memory-region-names:" which was 
>>>>> wrongly removed in V2, i have forgotten one particular case.
>>>>>
>>>>> We need memory-region-names in case only one OCTOSPI instance is 
>>>>> used. If it's OCTOCPI2 and the whole memory-map region
>>>>> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>>>>
>>>>> We need to know to which OCTOSPI instance the memory region is associated
>>>>> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
>>>>> with memory region declared.
>>>>>
>>>>> so i will add :
>>>>>
>>>>>   memory-region-names:
>>>>>     description: |
>>>>>       OCTOSPI instance's name to which memory region is associated
>>>>>     items:
>>>>>       - const: ospi1
>>>>>       - const: ospi2
>>>>>
>>>>
>>>> I don't think this matches what you are saying to us. Let's talk about
>>>> the hardware which is directly represented by DTS/DTSI. You always have
>>>> two instances.
>>>>
>>>>
>>>
>>> We have 2 instances, but both not always enabled.
>>> In case only one is enabled, only one memory-region-names is needed.
>>>
>>> We must know to which OCTCOSPI the memory-region makes reference to, in order
>>> to configure and/or check the memory region split configuration. That' swhy 
>>> the memory-regions-names must specify if it's the OCTOSPI1 or OCTOSPI2 instance.
>>
>> Well, in that case two comments.
>> 1. Above syntax does not allow you to skip one item. You would need:
>> items:
>>   enum: [ospi1, ospi2]
>> minItems: 1
>> maxItems: 2
>>
> 
> ok
> 
>> 2. But this points to other problem. From the omm-manager node point of
>> view, you should define all the resources regardless whether the child
>> is enabled or not. You do not skip some part of 'reg' if child is
>> missing. Do not skip interrupts, access controllers, clocks etc.
>> If some resource is to be skipped, it means that it belongs to the
>> child, not to the parent, IMO.
> 
> I didn't get your point. 
> 
> The resource declared in omm-manager's node pnly belongs to omm-manager
> (reg/clocks/resets/access-controllers/st,syscfg-amcr/power-domains), regardless 
> there are 1 or 2 children. None of them can be skipped.

That's not true, you skip ranges and memory region.

> 
> If omm-manager has no child enabled, omm-manager must be disabled as well in DT.

That's not what we talk about. We do not talk about enabled or disabled.
We talk about being there in the first place.

> 
>> Therefore memory-region looks like child's property.
>>
>> Imagine different case: runtime loaded overlay. In your setup, you probe
>> omm-manager with one memory-region and one child. Then someone loads
>> overlay enabling the second child, second SPI.
>>
>> That's of course imaginary case, but shows the concept how the parent
>> would work.
>>
>> It's the same with other buses in the kernel. You can load overlay with
>> any new child and the parent should not get new properties.
>>
> 
> In case of runtime loaded overlay, if a second child is added with an associated 
> memory-region, omm-manager must be unbind/bind to :
>   _ check the added child's access rights.
>   _ take into account the added child's memory-region configuration (to set 
>     the syscfg-amcr register accordingly) 

That's driver part, we talk about bindings and DTS.



Best regards,
Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Patrice CHOTARD]

On 2/3/25 12:40, Krzysztof Kozlowski wrote:
> On 03/02/2025 11:46, Patrice CHOTARD wrote:
>>
>>
>> On 1/30/25 16:09, Krzysztof Kozlowski wrote:
>>> On 30/01/2025 14:32, Patrice CHOTARD wrote:
>>>>
>>>>
>>>> On 1/30/25 13:12, Krzysztof Kozlowski wrote:
>>>>> On 30/01/2025 09:57, Patrice CHOTARD wrote:
>>>>>>
>>>>>>
>>>>>> On 1/29/25 08:52, Krzysztof Kozlowski wrote:
>>>>>>> On Tue, Jan 28, 2025 at 09:17:25AM +0100, patrice.chotard@foss.st.com wrote:
>>>>>>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>>>
>>>>>>>> Add bindings for STM32 Octo Memory Manager (OMM) controller.
>>>>>>>>
>>>>>>>> OMM manages:
>>>>>>>>   - the muxing between 2 OSPI busses and 2 output ports.
>>>>>>>>     There are 4 possible muxing configurations:
>>>>>>>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>>>>>>>         output is on port 2
>>>>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>>>>>>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>>>>>>>         OSPI2 output is on port 1
>>>>>>>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>>>>>>>   - the split of the memory area shared between the 2 OSPI instances.
>>>>>>>>   - chip select selection override.
>>>>>>>>   - the time between 2 transactions in multiplexed mode.
>>>>>>>>
>>>>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>>> ---
>>>>>>>>  .../memory-controllers/st,stm32-omm.yaml      | 190 ++++++++++++++++++
>>>>>>>>  1 file changed, 190 insertions(+)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..7e0b150e0005
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-omm.yaml
>>>>>>>
>>>>>>>
>>>>>>> Filename as compatible, so st,stm32mp25-omm.yaml
>>>>>>>
>>>>>>> You already received this comment.
>>>>>>
>>>>>> Sorry, i missed this update
>>>>>>
>>>>>>>
>>>>>>>> @@ -0,0 +1,190 @@
>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>> +%YAML 1.2
>>>>>>>> +---
>>>>>>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>> +
>>>>>>>> +title: STM32 Octo Memory Manager (OMM)
>>>>>>>> +
>>>>>>>> +maintainers:
>>>>>>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>>> +
>>>>>>>> +description: |
>>>>>>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>>>>>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>>>>>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>>>>>>> +  the same bus. It Supports up to:
>>>>>>>> +    - Two single/dual/quad/octal SPI interfaces
>>>>>>>> +    - Two ports for pin assignment
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> +  compatible:
>>>>>>>> +    const: st,stm32mp25-omm
>>>>>>>> +
>>>>>>>> +  "#address-cells":
>>>>>>>> +    const: 2
>>>>>>>> +
>>>>>>>> +  "#size-cells":
>>>>>>>> +    const: 1
>>>>>>>> +
>>>>>>>> +  ranges:
>>>>>>>> +    description: |
>>>>>>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>>>>>>> +      Format:
>>>>>>>> +      <chip-select> 0 <registers base address> <size>
>>>>>>>
>>>>>>> Do you always have two children? If so, this should have maxItems.
>>>>>>
>>>>>> No, we can have one child.
>>>>>
>>>>> For the same SoC? How? You put the spi@ in the soc, so I don't
>>>>> understand how one child is possible.
>>>>
>>>> Yes for the same SoC, in DTSI file, the both OCTOSPI child are declared 
>>>> but are disabled by default.
>>>
>>> But the child node is there anyway so are the ranges.
>>
>> if both child are disabled, omm-manager should be disabled as well, 
>> omm-manager alone makes no sense.
> 
> 
> Yes, it is obvious, but how is this related?

As described in the commit message, OMM manages the muxing of the 2 OSPI buses 
(its 2 child), that's the relation.

Do you want i add this directly in yaml file ?

> 
>>
>>>
>>>>
>>>> In the DTS board file, 0,1 or 2 OCTOSPI instance can be enabled depending of the board design.
>>>>
>>>> In our case, on stm32mp257f-ev1 board, one SPI-NOR is soldered on PCB, so only one OCTOSPI 
>>>> instance is needed and enabled.
>>>>
>>>> Internally we got validation boards with several memory devices connected to OCTOSPI1 and 
>>>> OCTOSPI2, in this case, both OCTOSPI instance are needed and enabled.
>>>
>>> I could imagine that you would not want to have unused reserved ranges,
>>> so that one indeed is flexible, I agree.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +  reg:
>>>>>>>> +    items:
>>>>>>>> +      - description: OMM registers
>>>>>>>> +      - description: OMM memory map area
>>>>>>>> +
>>>>>>>> +  reg-names:
>>>>>>>> +    items:
>>>>>>>> +      - const: regs
>>>>>>>> +      - const: memory_map
>>>>>>>> +
>>>>>>>> +  memory-region:
>>>>>>>> +    description: Phandle to node describing memory-map region to used.
>>>>>>>> +    minItems: 1
>>>>>>>> +    maxItems: 2
>>>>>>>
>>>>>>> List the items with description instead with optional minItems. Why is
>>>>>>> this flexible in number of items?
>>>>>>
>>>>>> If only one child (OCTOSPI instance), only one memory-region is needed.
>>>>>
>>>>> Which is not possible... look at your DTSI.
>>>>
>>>> It's possible. if one OCTOSPI is used (the second one is kept disabled), only
>>>> one memory-region is needed.
>>>
>>> Ack.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Another update, i will reintroduce "memory-region-names:" which was 
>>>>>> wrongly removed in V2, i have forgotten one particular case.
>>>>>>
>>>>>> We need memory-region-names in case only one OCTOSPI instance is 
>>>>>> used. If it's OCTOCPI2 and the whole memory-map region
>>>>>> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>>>>>
>>>>>> We need to know to which OCTOSPI instance the memory region is associated
>>>>>> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
>>>>>> with memory region declared.
>>>>>>
>>>>>> so i will add :
>>>>>>
>>>>>>   memory-region-names:
>>>>>>     description: |
>>>>>>       OCTOSPI instance's name to which memory region is associated
>>>>>>     items:
>>>>>>       - const: ospi1
>>>>>>       - const: ospi2
>>>>>>
>>>>>
>>>>> I don't think this matches what you are saying to us. Let's talk about
>>>>> the hardware which is directly represented by DTS/DTSI. You always have
>>>>> two instances.
>>>>>
>>>>>
>>>>
>>>> We have 2 instances, but both not always enabled.
>>>> In case only one is enabled, only one memory-region-names is needed.
>>>>
>>>> We must know to which OCTCOSPI the memory-region makes reference to, in order
>>>> to configure and/or check the memory region split configuration. That' swhy 
>>>> the memory-regions-names must specify if it's the OCTOSPI1 or OCTOSPI2 instance.
>>>
>>> Well, in that case two comments.
>>> 1. Above syntax does not allow you to skip one item. You would need:
>>> items:
>>>   enum: [ospi1, ospi2]
>>> minItems: 1
>>> maxItems: 2
>>>
>>
>> ok
>>
>>> 2. But this points to other problem. From the omm-manager node point of
>>> view, you should define all the resources regardless whether the child
>>> is enabled or not. You do not skip some part of 'reg' if child is
>>> missing. Do not skip interrupts, access controllers, clocks etc.
>>> If some resource is to be skipped, it means that it belongs to the
>>> child, not to the parent, IMO.
>>
>> I didn't get your point. 
>>
>> The resource declared in omm-manager's node pnly belongs to omm-manager
>> (reg/clocks/resets/access-controllers/st,syscfg-amcr/power-domains), regardless 
>> there are 1 or 2 children. None of them can be skipped.
> 
> That's not true, you skip ranges and memory region.

If i have correctly understood, you want a constraint on range and memory-regions properties ?
Is it what you expect ?

  ranges:
    description: |
      Reflects the memory layout with four integer values per OSPI instance.
      Format:
      <chip-select> 0 <registers base address> <size>
    minItems: 1
    maxItems: 2

  memory-region-names:
    description: |
      OCTOSPI instance's name to which memory region is associated
    items:
      enum: [ospi1, ospi2]
    minItems: 1
    maxItems: 2


Thanks
Patrice


> 
>>
>> If omm-manager has no child enabled, omm-manager must be disabled as well in DT.
> 
> That's not what we talk about. We do not talk about enabled or disabled.
> We talk about being there in the first place.
> 
>>
>>> Therefore memory-region looks like child's property.
>>>
>>> Imagine different case: runtime loaded overlay. In your setup, you probe
>>> omm-manager with one memory-region and one child. Then someone loads
>>> overlay enabling the second child, second SPI.
>>>
>>> That's of course imaginary case, but shows the concept how the parent
>>> would work.
>>>
>>> It's the same with other buses in the kernel. You can load overlay with
>>> any new child and the parent should not get new properties.
>>>
>>
>> In case of runtime loaded overlay, if a second child is added with an associated 
>> memory-region, omm-manager must be unbind/bind to :
>>   _ check the added child's access rights.
>>   _ take into account the added child's memory-region configuration (to set 
>>     the syscfg-amcr register accordingly) 
> 
> That's driver part, we talk about bindings and DTS.
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Krzysztof Kozlowski]

On 04/02/2025 08:29, Patrice CHOTARD wrote:
>>>>>>>>> @@ -0,0 +1,190 @@
>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>> +%YAML 1.2
>>>>>>>>> +---
>>>>>>>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>> +
>>>>>>>>> +title: STM32 Octo Memory Manager (OMM)
>>>>>>>>> +
>>>>>>>>> +maintainers:
>>>>>>>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>>>> +
>>>>>>>>> +description: |
>>>>>>>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>>>>>>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>>>>>>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>>>>>>>> +  the same bus. It Supports up to:
>>>>>>>>> +    - Two single/dual/quad/octal SPI interfaces
>>>>>>>>> +    - Two ports for pin assignment
>>>>>>>>> +
>>>>>>>>> +properties:
>>>>>>>>> +  compatible:
>>>>>>>>> +    const: st,stm32mp25-omm
>>>>>>>>> +
>>>>>>>>> +  "#address-cells":
>>>>>>>>> +    const: 2
>>>>>>>>> +
>>>>>>>>> +  "#size-cells":
>>>>>>>>> +    const: 1
>>>>>>>>> +
>>>>>>>>> +  ranges:
>>>>>>>>> +    description: |
>>>>>>>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>>>>>>>> +      Format:
>>>>>>>>> +      <chip-select> 0 <registers base address> <size>
>>>>>>>>
>>>>>>>> Do you always have two children? If so, this should have maxItems.
>>>>>>>
>>>>>>> No, we can have one child.
>>>>>>
>>>>>> For the same SoC? How? You put the spi@ in the soc, so I don't
>>>>>> understand how one child is possible.
>>>>>
>>>>> Yes for the same SoC, in DTSI file, the both OCTOSPI child are declared 
>>>>> but are disabled by default.
>>>>
>>>> But the child node is there anyway so are the ranges.
>>>
>>> if both child are disabled, omm-manager should be disabled as well, 
>>> omm-manager alone makes no sense.
>>
>>
>> Yes, it is obvious, but how is this related?
> 
> As described in the commit message, OMM manages the muxing of the 2 OSPI buses 
> (its 2 child), that's the relation.

Again, nothing related to our discussion.

You claim you can have only one child and we do not talk about child
disabled status here. So show me the product which have second address
space removed from *the SoC*.

> 
> Do you want i add this directly in yaml file ?


No, I want the answer when is it possible to have only one ranges. What
such DTSI would represent - what real world hardware.

> 
>>
>>>
>>>>
>>>>>
>>>>> In the DTS board file, 0,1 or 2 OCTOSPI instance can be enabled depending of the board design.
>>>>>
>>>>> In our case, on stm32mp257f-ev1 board, one SPI-NOR is soldered on PCB, so only one OCTOSPI 
>>>>> instance is needed and enabled.
>>>>>
>>>>> Internally we got validation boards with several memory devices connected to OCTOSPI1 and 
>>>>> OCTOSPI2, in this case, both OCTOSPI instance are needed and enabled.
>>>>
>>>> I could imagine that you would not want to have unused reserved ranges,
>>>> so that one indeed is flexible, I agree.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +  reg:
>>>>>>>>> +    items:
>>>>>>>>> +      - description: OMM registers
>>>>>>>>> +      - description: OMM memory map area
>>>>>>>>> +
>>>>>>>>> +  reg-names:
>>>>>>>>> +    items:
>>>>>>>>> +      - const: regs
>>>>>>>>> +      - const: memory_map
>>>>>>>>> +
>>>>>>>>> +  memory-region:
>>>>>>>>> +    description: Phandle to node describing memory-map region to used.
>>>>>>>>> +    minItems: 1
>>>>>>>>> +    maxItems: 2
>>>>>>>>
>>>>>>>> List the items with description instead with optional minItems. Why is
>>>>>>>> this flexible in number of items?
>>>>>>>
>>>>>>> If only one child (OCTOSPI instance), only one memory-region is needed.
>>>>>>
>>>>>> Which is not possible... look at your DTSI.
>>>>>
>>>>> It's possible. if one OCTOSPI is used (the second one is kept disabled), only
>>>>> one memory-region is needed.
>>>>
>>>> Ack.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Another update, i will reintroduce "memory-region-names:" which was 
>>>>>>> wrongly removed in V2, i have forgotten one particular case.
>>>>>>>
>>>>>>> We need memory-region-names in case only one OCTOSPI instance is 
>>>>>>> used. If it's OCTOCPI2 and the whole memory-map region
>>>>>>> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>>>>>>
>>>>>>> We need to know to which OCTOSPI instance the memory region is associated
>>>>>>> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
>>>>>>> with memory region declared.
>>>>>>>
>>>>>>> so i will add :
>>>>>>>
>>>>>>>   memory-region-names:
>>>>>>>     description: |
>>>>>>>       OCTOSPI instance's name to which memory region is associated
>>>>>>>     items:
>>>>>>>       - const: ospi1
>>>>>>>       - const: ospi2
>>>>>>>
>>>>>>
>>>>>> I don't think this matches what you are saying to us. Let's talk about
>>>>>> the hardware which is directly represented by DTS/DTSI. You always have
>>>>>> two instances.
>>>>>>
>>>>>>
>>>>>
>>>>> We have 2 instances, but both not always enabled.
>>>>> In case only one is enabled, only one memory-region-names is needed.
>>>>>
>>>>> We must know to which OCTCOSPI the memory-region makes reference to, in order
>>>>> to configure and/or check the memory region split configuration. That' swhy 
>>>>> the memory-regions-names must specify if it's the OCTOSPI1 or OCTOSPI2 instance.
>>>>
>>>> Well, in that case two comments.
>>>> 1. Above syntax does not allow you to skip one item. You would need:
>>>> items:
>>>>   enum: [ospi1, ospi2]
>>>> minItems: 1
>>>> maxItems: 2
>>>>
>>>
>>> ok
>>>
>>>> 2. But this points to other problem. From the omm-manager node point of
>>>> view, you should define all the resources regardless whether the child
>>>> is enabled or not. You do not skip some part of 'reg' if child is
>>>> missing. Do not skip interrupts, access controllers, clocks etc.
>>>> If some resource is to be skipped, it means that it belongs to the
>>>> child, not to the parent, IMO.
>>>
>>> I didn't get your point. 
>>>
>>> The resource declared in omm-manager's node pnly belongs to omm-manager
>>> (reg/clocks/resets/access-controllers/st,syscfg-amcr/power-domains), regardless 
>>> there are 1 or 2 children. None of them can be skipped.
>>
>> That's not true, you skip ranges and memory region.
> 
> If i have correctly understood, you want a constraint on range and memory-regions properties ?
> Is it what you expect ?
> 
>   ranges:
>     description: |
>       Reflects the memory layout with four integer values per OSPI instance.
>       Format:
>       <chip-select> 0 <registers base address> <size>
>     minItems: 1
>     maxItems: 2
> 
>   memory-region-names:
>     description: |
>       OCTOSPI instance's name to which memory region is associated
>     items:
>       enum: [ospi1, ospi2]
>     minItems: 1
>     maxItems: 2
> 


Best regards,
Krzysztof

Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo Memory Manager controller

[Patrice CHOTARD]

On 2/4/25 08:50, Krzysztof Kozlowski wrote:
> On 04/02/2025 08:29, Patrice CHOTARD wrote:
>>>>>>>>>> @@ -0,0 +1,190 @@
>>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>>> +%YAML 1.2
>>>>>>>>>> +---
>>>>>>>>>> +$id: http://devicetree.org/schemas/memory-controllers/st,stm32-omm.yaml#
>>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>>> +
>>>>>>>>>> +title: STM32 Octo Memory Manager (OMM)
>>>>>>>>>> +
>>>>>>>>>> +maintainers:
>>>>>>>>>> +  - Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>>>>> +
>>>>>>>>>> +description: |
>>>>>>>>>> +  The STM32 Octo Memory Manager is a low-level interface that enables an
>>>>>>>>>> +  efficient OCTOSPI pin assignment with a full I/O matrix (before alternate
>>>>>>>>>> +  function map) and multiplex of single/dual/quad/octal 		SPI interfaces over
>>>>>>>>>> +  the same bus. It Supports up to:
>>>>>>>>>> +    - Two single/dual/quad/octal SPI interfaces
>>>>>>>>>> +    - Two ports for pin assignment
>>>>>>>>>> +
>>>>>>>>>> +properties:
>>>>>>>>>> +  compatible:
>>>>>>>>>> +    const: st,stm32mp25-omm
>>>>>>>>>> +
>>>>>>>>>> +  "#address-cells":
>>>>>>>>>> +    const: 2
>>>>>>>>>> +
>>>>>>>>>> +  "#size-cells":
>>>>>>>>>> +    const: 1
>>>>>>>>>> +
>>>>>>>>>> +  ranges:
>>>>>>>>>> +    description: |
>>>>>>>>>> +      Reflects the memory layout with four integer values per OSPI instance.
>>>>>>>>>> +      Format:
>>>>>>>>>> +      <chip-select> 0 <registers base address> <size>
>>>>>>>>>
>>>>>>>>> Do you always have two children? If so, this should have maxItems.
>>>>>>>>
>>>>>>>> No, we can have one child.
>>>>>>>
>>>>>>> For the same SoC? How? You put the spi@ in the soc, so I don't
>>>>>>> understand how one child is possible.
>>>>>>
>>>>>> Yes for the same SoC, in DTSI file, the both OCTOSPI child are declared 
>>>>>> but are disabled by default.
>>>>>
>>>>> But the child node is there anyway so are the ranges.
>>>>
>>>> if both child are disabled, omm-manager should be disabled as well, 
>>>> omm-manager alone makes no sense.
>>>
>>>
>>> Yes, it is obvious, but how is this related?
>>
>> As described in the commit message, OMM manages the muxing of the 2 OSPI buses 
>> (its 2 child), that's the relation.
> 
> Again, nothing related to our discussion.
> 
> You claim you can have only one child and we do not talk about child
> disabled status here. So show me the product which have second address
> space removed from *the SoC*.
> 
>>
>> Do you want i add this directly in yaml file ?
> 
> 
> No, I want the answer when is it possible to have only one ranges. What
> such DTSI would represent - what real world hardware.

On our SoCs, there are always physically 2 OSPI instances, but one or two
instance can be enabled in the DT.

So we always have 2 ranges :

  ranges:
    description: |
      Reflects the memory layout with four integer values per OSPI instance.
      Format:
      <chip-select> 0 <registers base address> <size>
    minItems: 2
    maxItems: 2

Thanks 
Patrice


> 
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> In the DTS board file, 0,1 or 2 OCTOSPI instance can be enabled depending of the board design.
>>>>>>
>>>>>> In our case, on stm32mp257f-ev1 board, one SPI-NOR is soldered on PCB, so only one OCTOSPI 
>>>>>> instance is needed and enabled.
>>>>>>
>>>>>> Internally we got validation boards with several memory devices connected to OCTOSPI1 and 
>>>>>> OCTOSPI2, in this case, both OCTOSPI instance are needed and enabled.
>>>>>
>>>>> I could imagine that you would not want to have unused reserved ranges,
>>>>> so that one indeed is flexible, I agree.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +  reg:
>>>>>>>>>> +    items:
>>>>>>>>>> +      - description: OMM registers
>>>>>>>>>> +      - description: OMM memory map area
>>>>>>>>>> +
>>>>>>>>>> +  reg-names:
>>>>>>>>>> +    items:
>>>>>>>>>> +      - const: regs
>>>>>>>>>> +      - const: memory_map
>>>>>>>>>> +
>>>>>>>>>> +  memory-region:
>>>>>>>>>> +    description: Phandle to node describing memory-map region to used.
>>>>>>>>>> +    minItems: 1
>>>>>>>>>> +    maxItems: 2
>>>>>>>>>
>>>>>>>>> List the items with description instead with optional minItems. Why is
>>>>>>>>> this flexible in number of items?
>>>>>>>>
>>>>>>>> If only one child (OCTOSPI instance), only one memory-region is needed.
>>>>>>>
>>>>>>> Which is not possible... look at your DTSI.
>>>>>>
>>>>>> It's possible. if one OCTOSPI is used (the second one is kept disabled), only
>>>>>> one memory-region is needed.
>>>>>
>>>>> Ack.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Another update, i will reintroduce "memory-region-names:" which was 
>>>>>>>> wrongly removed in V2, i have forgotten one particular case.
>>>>>>>>
>>>>>>>> We need memory-region-names in case only one OCTOSPI instance is 
>>>>>>>> used. If it's OCTOCPI2 and the whole memory-map region
>>>>>>>> is dedicated to OCTOSPI2 (OCTOSPI1 unmapped, OCTOSPI2 (256 Mbytes)
>>>>>>>>
>>>>>>>> We need to know to which OCTOSPI instance the memory region is associated
>>>>>>>> with, in order to check "st,syscfg-amcr" 's value which must be coherent 
>>>>>>>> with memory region declared.
>>>>>>>>
>>>>>>>> so i will add :
>>>>>>>>
>>>>>>>>   memory-region-names:
>>>>>>>>     description: |
>>>>>>>>       OCTOSPI instance's name to which memory region is associated
>>>>>>>>     items:
>>>>>>>>       - const: ospi1
>>>>>>>>       - const: ospi2
>>>>>>>>
>>>>>>>
>>>>>>> I don't think this matches what you are saying to us. Let's talk about
>>>>>>> the hardware which is directly represented by DTS/DTSI. You always have
>>>>>>> two instances.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> We have 2 instances, but both not always enabled.
>>>>>> In case only one is enabled, only one memory-region-names is needed.
>>>>>>
>>>>>> We must know to which OCTCOSPI the memory-region makes reference to, in order
>>>>>> to configure and/or check the memory region split configuration. That' swhy 
>>>>>> the memory-regions-names must specify if it's the OCTOSPI1 or OCTOSPI2 instance.
>>>>>
>>>>> Well, in that case two comments.
>>>>> 1. Above syntax does not allow you to skip one item. You would need:
>>>>> items:
>>>>>   enum: [ospi1, ospi2]
>>>>> minItems: 1
>>>>> maxItems: 2
>>>>>
>>>>
>>>> ok
>>>>
>>>>> 2. But this points to other problem. From the omm-manager node point of
>>>>> view, you should define all the resources regardless whether the child
>>>>> is enabled or not. You do not skip some part of 'reg' if child is
>>>>> missing. Do not skip interrupts, access controllers, clocks etc.
>>>>> If some resource is to be skipped, it means that it belongs to the
>>>>> child, not to the parent, IMO.
>>>>
>>>> I didn't get your point. 
>>>>
>>>> The resource declared in omm-manager's node pnly belongs to omm-manager
>>>> (reg/clocks/resets/access-controllers/st,syscfg-amcr/power-domains), regardless 
>>>> there are 1 or 2 children. None of them can be skipped.
>>>
>>> That's not true, you skip ranges and memory region.
>>
>> If i have correctly understood, you want a constraint on range and memory-regions properties ?
>> Is it what you expect ?
>>
>>   ranges:
>>     description: |
>>       Reflects the memory layout with four integer values per OSPI instance.
>>       Format:
>>       <chip-select> 0 <registers base address> <size>
>>     minItems: 1
>>     maxItems: 2
>>
>>   memory-region-names:
>>     description: |
>>       OCTOSPI instance's name to which memory region is associated
>>     items:
>>       enum: [ospi1, ospi2]
>>     minItems: 1
>>     maxItems: 2
>>
> 
> 
> Best regards,
> Krzysztof

[PATCH v2 4/9] memory: Add STM32 Octo Memory Manager driver

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Octo Memory Manager driver (OMM) manages:
  - the muxing between 2 OSPI busses and 2 output ports.
    There are 4 possible muxing configurations:
      - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
        output is on port 2
      - OSPI1 and OSPI2 are multiplexed over the same output port 1
      - swapped mode (no multiplexing), OSPI1 output is on port 2,
        OSPI2 output is on port 1
      - OSPI1 and OSPI2 are multiplexed over the same output port 2
  - the split of the memory area shared between the 2 OSPI instances.
  - chip select selection override.
  - the time between 2 transactions in multiplexed mode.
  - check firewall access.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/memory/Kconfig     |  17 ++
 drivers/memory/Makefile    |   1 +
 drivers/memory/stm32_omm.c | 509 +++++++++++++++++++++++++++++++++++++
 3 files changed, 527 insertions(+)
 create mode 100644 drivers/memory/stm32_omm.c

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index c82d8d8a16ea..80c44a6d5bdb 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -225,6 +225,23 @@ config STM32_FMC2_EBI
 	  devices (like SRAM, ethernet adapters, FPGAs, LCD displays, ...) on
 	  SOCs containing the FMC2 External Bus Interface.
 
+config STM32_OMM
+	tristate "STM32 Octo Memory Manager"
+	depends on SPI_STM32_OSPI || TEST_COMPILE
+	help
+	  This driver manages the muxing between the 2 OSPI busses and
+	  the 2 output ports. There are 4 possible muxing configurations:
+	  - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
+	       output is on port 2
+	  - OSPI1 and OSPI2 are multiplexed over the same output port 1
+	  - swapped mode (no multiplexing), OSPI1 output is on port 2,
+	       OSPI2 output is on port 1
+	  - OSPI1 and OSPI2 are multiplexed over the same output port 2
+	  It also manages :
+	    - the split of the memory area shared between the 2 OSPI instances.
+	    - chip select selection override.
+	    - the time between 2 transactions in multiplexed mode.
+
 source "drivers/memory/samsung/Kconfig"
 source "drivers/memory/tegra/Kconfig"
 
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index d2e6ca9abbe0..c1959661bf63 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_DA8XX_DDRCTL)	+= da8xx-ddrctl.o
 obj-$(CONFIG_PL353_SMC)		+= pl353-smc.o
 obj-$(CONFIG_RENESAS_RPCIF)	+= renesas-rpc-if.o
 obj-$(CONFIG_STM32_FMC2_EBI)	+= stm32-fmc2-ebi.o
+obj-$(CONFIG_STM32_OMM)		+= stm32_omm.o
 
 obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
 obj-$(CONFIG_TEGRA_MC)		+= tegra/
diff --git a/drivers/memory/stm32_omm.c b/drivers/memory/stm32_omm.c
new file mode 100644
index 000000000000..6f20fe0183ec
--- /dev/null
+++ b/drivers/memory/stm32_omm.c
@@ -0,0 +1,509 @@
+// SPDX-License-Identifier: GPL
+/*
+ * Copyright (C) STMicroelectronics 2024 - All Rights Reserved
+ * Author(s): Patrice Chotard <patrice.chotard@foss.st.com> for STMicroelectronics.
+ */
+
+#include <linux/bus/stm32_firewall_device.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define OMM_CR			0
+#define CR_MUXEN		BIT(0)
+#define CR_MUXENMODE_MASK	GENMASK(1, 0)
+#define CR_CSSEL_OVR_EN		BIT(4)
+#define CR_CSSEL_OVR_MASK	GENMASK(6, 5)
+#define CR_REQ2ACK_MASK		GENMASK(23, 16)
+
+#define OMM_CHILD_NB		2
+
+struct ospi_child {
+	struct device *dev;
+	struct device_node *node;
+	struct clk *clk;
+};
+
+struct stm32_omm {
+	struct ospi_child child[OMM_CHILD_NB];
+	struct resource *mm_res;
+	struct clk *clk;
+	void __iomem *io_base;
+	u32 cr;
+	u8 nb_child;
+	bool restore_omm;
+};
+
+static int stm32_omm_set_amcr(struct device *dev, bool set)
+{
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+	struct regmap *syscfg_regmap;
+	struct device_node *node;
+	struct resource res, res1;
+	resource_size_t mm_ospi2_size = 0;
+	u32 amcr_base, amcr_mask;
+	int ret, i;
+	unsigned int amcr, read_amcr;
+
+	for (i = 0; i < omm->nb_child; i++) {
+		/* res1 only used on second loop iteration */
+		res1.start = res.start;
+		res1.end = res.end;
+
+		node = of_parse_phandle(dev->of_node, "memory-region", i);
+		if (!node)
+			continue;
+
+		ret = of_address_to_resource(node, 0, &res);
+		if (ret) {
+			dev_err(dev, "unable to resolve memory region\n");
+			return ret;
+		}
+
+		/* check that memory region fits inside OMM memory map area */
+		if (!resource_contains(omm->mm_res, &res)) {
+			dev_err(dev, "[0x%llx-0x%llx] doesn't fit inside OMM memory map area [0x%llx-0x%llx]\n",
+				res.start, res.end,
+				omm->mm_res->start, omm->mm_res->end);
+
+			return -EFAULT;
+		}
+
+		if (i == 1) {
+			mm_ospi2_size = resource_size(&res);
+
+			/* check that OMM memory region 1 doesn't overlap memory region 2 */
+			if (resource_overlaps(&res, &res1)) {
+				dev_err(dev, "[0x%llx-0x%llx] overlaps OMM memory map area [0x%llx-0x%llx]\n",
+					res1.start, res1.end, res.start, res.end);
+
+				return -EFAULT;
+			}
+		}
+	}
+
+	syscfg_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "st,syscfg-amcr");
+	if (IS_ERR(syscfg_regmap)) {
+		dev_err(dev, "Failed to get st,syscfg-amcr property\n");
+		return PTR_ERR(syscfg_regmap);
+	}
+
+	ret = of_property_read_u32_index(dev->of_node, "st,syscfg-amcr", 1,
+					 &amcr_base);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32_index(dev->of_node, "st,syscfg-amcr", 2,
+					 &amcr_mask);
+	if (ret)
+		return ret;
+
+	amcr = mm_ospi2_size / SZ_64M;
+
+	if (set)
+		regmap_update_bits(syscfg_regmap, amcr_base, amcr_mask, amcr);
+
+	/* read AMCR and check coherency with memory-map areas defined in DT */
+	regmap_read(syscfg_regmap, amcr_base, &read_amcr);
+	read_amcr = read_amcr >> (ffs(amcr_mask) - 1);
+
+	if (amcr != read_amcr) {
+		dev_err(dev, "AMCR value not coherent with DT memory-map areas\n");
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int stm32_omm_enable_child_clock(struct device *dev, bool enable)
+{
+	/* As there is only 2 children, remember first child in case of error */
+	struct clk *first_child_clk = NULL;
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+	u8 i;
+	int ret;
+
+	for (i = 0; i < omm->nb_child; i++) {
+		if (enable) {
+			ret = clk_prepare_enable(omm->child[i].clk);
+			if (ret) {
+				if (first_child_clk)
+					clk_disable_unprepare(first_child_clk);
+
+				dev_err(dev, "Can not enable clock\n");
+				return ret;
+			}
+		} else {
+			clk_disable_unprepare(omm->child[i].clk);
+		}
+
+		first_child_clk = omm->child[i].clk;
+	}
+
+	return 0;
+}
+
+static int stm32_omm_configure(struct device *dev)
+{
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+	struct reset_control *rstc;
+	unsigned long clk_rate, clk_rate_max = 0;
+	int ret;
+	u8 i;
+	u32 mux = 0;
+	u32 cssel_ovr = 0;
+	u32 req2ack = 0;
+
+	omm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(omm->clk)) {
+		dev_err(dev, "Failed to get OMM clock (%ld)\n",
+			PTR_ERR(omm->clk));
+
+		return PTR_ERR(omm->clk);
+	}
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		return ret;
+
+	/* parse children's clock */
+	for (i = 0; i < omm->nb_child; i++) {
+		clk_rate = clk_get_rate(omm->child[i].clk);
+		if (!clk_rate) {
+			dev_err(dev, "Invalid clock rate\n");
+			goto err_clk_disable;
+		}
+
+		if (clk_rate > clk_rate_max)
+			clk_rate_max = clk_rate;
+	}
+
+	rstc = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(rstc)) {
+		ret = dev_err_probe(dev, PTR_ERR(rstc), "reset get failed\n");
+		goto err_clk_disable;
+	}
+
+	reset_control_assert(rstc);
+	udelay(2);
+	reset_control_deassert(rstc);
+
+	omm->cr = readl_relaxed(omm->io_base + OMM_CR);
+	/* optional */
+	ret = of_property_read_u32(dev->of_node, "st,omm-mux", &mux);
+	if (!ret) {
+		if (mux & CR_MUXEN) {
+			ret = of_property_read_u32(dev->of_node, "st,omm-req2ack-ns",
+						   &req2ack);
+			if (!ret && !req2ack) {
+				req2ack = DIV_ROUND_UP(req2ack, NSEC_PER_SEC / clk_rate_max) - 1;
+
+				if (req2ack > 256)
+					req2ack = 256;
+			}
+
+			req2ack = FIELD_PREP(CR_REQ2ACK_MASK, req2ack);
+
+			omm->cr &= ~CR_REQ2ACK_MASK;
+			omm->cr |= FIELD_PREP(CR_REQ2ACK_MASK, req2ack);
+
+			/*
+			 * If the mux is enabled, the 2 OSPI clocks have to be
+			 * always enabled
+			 */
+			ret = stm32_omm_enable_child_clock(dev, true);
+			if (ret)
+				goto err_clk_disable;
+		}
+
+		omm->cr &= ~CR_MUXENMODE_MASK;
+		omm->cr |= FIELD_PREP(CR_MUXENMODE_MASK, mux);
+	}
+
+	/* optional */
+	ret = of_property_read_u32(dev->of_node, "st,omm-cssel-ovr", &cssel_ovr);
+	if (!ret) {
+		omm->cr &= ~CR_CSSEL_OVR_MASK;
+		omm->cr |= FIELD_PREP(CR_CSSEL_OVR_MASK, cssel_ovr);
+		omm->cr |= CR_CSSEL_OVR_EN;
+	}
+
+	omm->restore_omm = true;
+	writel_relaxed(omm->cr, omm->io_base + OMM_CR);
+
+	ret = stm32_omm_set_amcr(dev, true);
+
+err_clk_disable:
+	pm_runtime_put_sync_suspend(dev);
+
+	return ret;
+}
+
+static int stm32_omm_check_access(struct device *dev, struct device_node *np)
+{
+	struct stm32_firewall firewall;
+	int ret;
+
+	ret = stm32_firewall_get_firewall(np, &firewall, 1);
+	if (ret)
+		return ret;
+
+	return stm32_firewall_grant_access(&firewall);
+}
+
+static int stm32_omm_disable_child(struct device *dev)
+{
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+	struct reset_control *reset;
+	int ret;
+	u8 i;
+
+	for (i = 0; i < omm->nb_child; i++) {
+		ret = clk_prepare_enable(omm->child[i].clk);
+		if (ret) {
+			dev_err(dev, "Can not enable clock\n");
+			return ret;
+		}
+
+		reset = of_reset_control_get_exclusive(omm->child[i].node, 0);
+		if (IS_ERR(reset)) {
+			dev_err(dev, "Can't get child reset\n");
+			return PTR_ERR(reset);
+		};
+
+		/* reset OSPI to ensure CR_EN bit is set to 0 */
+		reset_control_assert(reset);
+		udelay(2);
+		reset_control_deassert(reset);
+
+		reset_control_put(reset);
+		clk_disable_unprepare(omm->child[i].clk);
+	}
+
+	return 0;
+}
+
+static int stm32_omm_probe(struct platform_device *pdev)
+{
+	struct platform_device *vdev;
+	struct device *dev = &pdev->dev;
+	struct stm32_omm *omm;
+	struct clk *clk;
+	int ret;
+	u8 child_access_granted = 0;
+	u8 i, j;
+	bool child_access[OMM_CHILD_NB];
+
+	omm = devm_kzalloc(dev, sizeof(*omm), GFP_KERNEL);
+	if (!omm)
+		return -ENOMEM;
+
+	omm->io_base = devm_platform_ioremap_resource_byname(pdev, "regs");
+	if (IS_ERR(omm->io_base))
+		return PTR_ERR(omm->io_base);
+
+	omm->mm_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory_map");
+	if (IS_ERR(omm->mm_res))
+		return PTR_ERR(omm->mm_res);
+
+	/* check child's access */
+	for_each_child_of_node_scoped(dev->of_node, child) {
+		if (omm->nb_child >= OMM_CHILD_NB) {
+			dev_err(dev, "Bad DT, found too much children\n");
+			ret = -E2BIG;
+			goto err_clk_release;
+		}
+
+		if (!of_device_is_compatible(child, "st,stm32mp25-ospi")) {
+			ret = -EINVAL;
+			goto err_clk_release;
+		}
+
+		ret = stm32_omm_check_access(dev, child);
+		if (ret < 0 && ret != -EACCES)
+			goto err_clk_release;
+
+		child_access[omm->nb_child] = false;
+		if (!ret) {
+			child_access_granted++;
+			child_access[omm->nb_child] = true;
+		}
+
+		omm->child[omm->nb_child].node = child;
+
+		clk = of_clk_get(child, 0);
+		if (IS_ERR(clk)) {
+			dev_err(dev, "Can't get child clock\n");
+			ret = PTR_ERR(clk);
+			goto err_clk_release;
+		};
+
+		omm->child[omm->nb_child].clk = clk;
+		omm->nb_child++;
+	}
+
+	if (omm->nb_child != OMM_CHILD_NB) {
+		ret = -EINVAL;
+		goto err_clk_release;
+	}
+
+	platform_set_drvdata(pdev, omm);
+
+	pm_runtime_enable(dev);
+
+	/* check if OMM's resource access is granted */
+	ret = stm32_omm_check_access(dev, dev->of_node);
+	if (ret < 0 && ret != -EACCES)
+		goto err_clk_release;
+
+	if (!ret && child_access_granted == OMM_CHILD_NB) {
+		/* Ensure both OSPI instance are disabled before configuring OMM */
+		ret = stm32_omm_disable_child(dev);
+		if (ret)
+			goto err_clk_release;
+
+		ret = stm32_omm_configure(dev);
+		if (ret)
+			goto err_clk_release;
+	} else {
+		dev_dbg(dev, "Octo Memory Manager resource's access not granted\n");
+		/*
+		 * AMCR can't be set, so check if current value is coherent
+		 * with memory-map areas defined in DT
+		 */
+		ret = stm32_omm_set_amcr(dev, false);
+		if (ret)
+			goto err_clk_release;
+	}
+
+	/* for each child, if resource access is granted and status "okay", probe it */
+	for (i = 0; i < omm->nb_child; i++) {
+		if (!child_access[i] || !of_device_is_available(omm->child[i].node))
+			continue;
+
+		vdev = of_platform_device_create(omm->child[i].node, NULL, NULL);
+		if (!vdev) {
+			dev_err(dev, "Failed to create Octo Memory Manager child\n");
+			for (j = i; j > 0; --j) {
+				if (omm->child[j].dev)
+					of_platform_device_destroy(omm->child[j].dev, NULL);
+			}
+
+			ret = -EINVAL;
+			goto err_clk_release;
+		}
+		omm->child[i].dev = &vdev->dev;
+	}
+
+err_clk_release:
+	for (i = 0; i < omm->nb_child; i++)
+		clk_put(omm->child[i].clk);
+
+	return ret;
+}
+
+static void stm32_omm_remove(struct platform_device *pdev)
+{
+	struct stm32_omm *omm = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < omm->nb_child; i++)
+		if (omm->child[i].dev)
+			of_platform_device_destroy(omm->child[i].dev, NULL);
+
+	if (omm->cr & CR_MUXEN)
+		stm32_omm_enable_child_clock(&pdev->dev, false);
+
+	pm_runtime_disable(&pdev->dev);
+}
+
+static const struct of_device_id stm32_omm_of_match[] = {
+	{ .compatible = "st,stm32mp25-omm", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stm32_omm_of_match);
+
+static int __maybe_unused stm32_omm_runtime_suspend(struct device *dev)
+{
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(omm->clk);
+
+	return 0;
+}
+
+static int __maybe_unused stm32_omm_runtime_resume(struct device *dev)
+{
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+
+	return clk_prepare_enable(omm->clk);
+}
+
+static int __maybe_unused stm32_omm_suspend(struct device *dev)
+{
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+
+	if (omm->restore_omm && omm->cr & CR_MUXEN)
+		stm32_omm_enable_child_clock(dev, false);
+
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int __maybe_unused stm32_omm_resume(struct device *dev)
+{
+	struct stm32_omm *omm = dev_get_drvdata(dev);
+	int ret;
+
+	pinctrl_pm_select_default_state(dev);
+
+	if (!omm->restore_omm)
+		return 0;
+
+	/* Ensure both OSPI instance are disabled before configuring OMM */
+	ret = stm32_omm_disable_child(dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		return ret;
+
+	writel_relaxed(omm->cr, omm->io_base + OMM_CR);
+	ret = stm32_omm_set_amcr(dev, true);
+	pm_runtime_put_sync_suspend(dev);
+	if (ret)
+		return ret;
+
+	if (omm->cr & CR_MUXEN)
+		ret = stm32_omm_enable_child_clock(dev, true);
+
+	return ret;
+}
+
+static const struct dev_pm_ops stm32_omm_pm_ops = {
+	SET_RUNTIME_PM_OPS(stm32_omm_runtime_suspend,
+			   stm32_omm_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(stm32_omm_suspend, stm32_omm_resume)
+};
+
+static struct platform_driver stm32_omm_driver = {
+	.probe	= stm32_omm_probe,
+	.remove = stm32_omm_remove,
+	.driver	= {
+		.name = "stm32-omm",
+		.of_match_table = stm32_omm_of_match,
+		.pm = &stm32_omm_pm_ops,
+	},
+};
+module_platform_driver(stm32_omm_driver);
+
+MODULE_DESCRIPTION("STMicroelectronics Octo Memory Manager driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v2 4/9] memory: Add STM32 Octo Memory Manager driver

[Philipp Zabel]

On Di, 2025-01-28 at 09:17 +0100, patrice.chotard@foss.st.com wrote:
> From: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> Octo Memory Manager driver (OMM) manages:
>   - the muxing between 2 OSPI busses and 2 output ports.
>     There are 4 possible muxing configurations:
>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>         output is on port 2
>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>         OSPI2 output is on port 1
>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>   - the split of the memory area shared between the 2 OSPI instances.
>   - chip select selection override.
>   - the time between 2 transactions in multiplexed mode.
>   - check firewall access.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
>  drivers/memory/Kconfig     |  17 ++
>  drivers/memory/Makefile    |   1 +
>  drivers/memory/stm32_omm.c | 509 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 527 insertions(+)
>  create mode 100644 drivers/memory/stm32_omm.c
> 
[...]
> diff --git a/drivers/memory/stm32_omm.c b/drivers/memory/stm32_omm.c
> new file mode 100644
> index 000000000000..6f20fe0183ec
> --- /dev/null
> +++ b/drivers/memory/stm32_omm.c
> @@ -0,0 +1,509 @@
[...]
> +static int stm32_omm_configure(struct device *dev)
> +{
> +	struct stm32_omm *omm = dev_get_drvdata(dev);
> +	struct reset_control *rstc;
> +	unsigned long clk_rate, clk_rate_max = 0;
> +	int ret;
> +	u8 i;
> +	u32 mux = 0;
> +	u32 cssel_ovr = 0;
> +	u32 req2ack = 0;
> +
> +	omm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(omm->clk)) {
> +		dev_err(dev, "Failed to get OMM clock (%ld)\n",
> +			PTR_ERR(omm->clk));
> +
> +		return PTR_ERR(omm->clk);
> +	}
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* parse children's clock */
> +	for (i = 0; i < omm->nb_child; i++) {
> +		clk_rate = clk_get_rate(omm->child[i].clk);
> +		if (!clk_rate) {
> +			dev_err(dev, "Invalid clock rate\n");
> +			goto err_clk_disable;
> +		}
> +
> +		if (clk_rate > clk_rate_max)
> +			clk_rate_max = clk_rate;
> +	}
> +
> +	rstc = devm_reset_control_get_optional(dev, NULL);

Please use devm_reset_control_get_optional_exclusive() directly.

regards
Philipp

Re: [PATCH v2 4/9] memory: Add STM32 Octo Memory Manager driver

[Patrice CHOTARD]

On 1/28/25 10:17, Philipp Zabel wrote:
> On Di, 2025-01-28 at 09:17 +0100, patrice.chotard@foss.st.com wrote:
>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>
>> Octo Memory Manager driver (OMM) manages:
>>   - the muxing between 2 OSPI busses and 2 output ports.
>>     There are 4 possible muxing configurations:
>>       - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>         output is on port 2
>>       - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>       - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>         OSPI2 output is on port 1
>>       - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>   - the split of the memory area shared between the 2 OSPI instances.
>>   - chip select selection override.
>>   - the time between 2 transactions in multiplexed mode.
>>   - check firewall access.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> ---
>>  drivers/memory/Kconfig     |  17 ++
>>  drivers/memory/Makefile    |   1 +
>>  drivers/memory/stm32_omm.c | 509 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 527 insertions(+)
>>  create mode 100644 drivers/memory/stm32_omm.c
>>
> [...]
>> diff --git a/drivers/memory/stm32_omm.c b/drivers/memory/stm32_omm.c
>> new file mode 100644
>> index 000000000000..6f20fe0183ec
>> --- /dev/null
>> +++ b/drivers/memory/stm32_omm.c
>> @@ -0,0 +1,509 @@
> [...]
>> +static int stm32_omm_configure(struct device *dev)
>> +{
>> +	struct stm32_omm *omm = dev_get_drvdata(dev);
>> +	struct reset_control *rstc;
>> +	unsigned long clk_rate, clk_rate_max = 0;
>> +	int ret;
>> +	u8 i;
>> +	u32 mux = 0;
>> +	u32 cssel_ovr = 0;
>> +	u32 req2ack = 0;
>> +
>> +	omm->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(omm->clk)) {
>> +		dev_err(dev, "Failed to get OMM clock (%ld)\n",
>> +			PTR_ERR(omm->clk));
>> +
>> +		return PTR_ERR(omm->clk);
>> +	}
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* parse children's clock */
>> +	for (i = 0; i < omm->nb_child; i++) {
>> +		clk_rate = clk_get_rate(omm->child[i].clk);
>> +		if (!clk_rate) {
>> +			dev_err(dev, "Invalid clock rate\n");
>> +			goto err_clk_disable;
>> +		}
>> +
>> +		if (clk_rate > clk_rate_max)
>> +			clk_rate_max = clk_rate;
>> +	}
>> +
>> +	rstc = devm_reset_control_get_optional(dev, NULL);
> 
> Please use devm_reset_control_get_optional_exclusive() directly.

ok

Thanks
Patrice

> 
> regards
> Philipp

[PATCH v2 5/9] arm64: dts: st: Add OMM node on stm32mp251

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Add Octo Memory Manager (OMM) entry on stm32mp251 and its two
OSPI instance.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 arch/arm64/boot/dts/st/stm32mp251.dtsi | 48 ++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index f3c6cdfd7008..f364b9e87e96 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -768,6 +768,54 @@ rng: rng@42020000 {
 				status = "disabled";
 			};
 
+			ommanager: ommanager@40500000 {
+				compatible = "st,stm32mp25-omm";
+				reg = <0x40500000 0x400>, <0x60000000 0x10000000>;
+				reg-names = "regs", "memory_map";
+				ranges = <0 0 0x40430000 0x400>,
+					 <1 0 0x40440000 0x400>;
+				clocks = <&rcc CK_BUS_OSPIIOM>;
+				resets = <&rcc OSPIIOM_R>;
+				#address-cells = <2>;
+				#size-cells = <1>;
+				st,syscfg-amcr = <&syscfg 0x2c00 0x7>;
+				access-controllers = <&rifsc 111>;
+				power-domains = <&CLUSTER_PD>;
+				status = "disabled";
+
+				ospi1: spi@40430000 {
+					compatible = "st,stm32mp25-ospi";
+					reg = <0 0 0x400>;
+					interrupts = <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>;
+					dmas = <&hpdma 2 0x62 0x00003121>,
+					       <&hpdma 2 0x42 0x00003112>;
+					dma-names = "tx", "rx";
+					st,syscfg-dlyb = <&syscfg 0x1000>;
+					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>;
+					status = "disabled";
+				};
+
+				ospi2: spi@40440000 {
+					compatible = "st,stm32mp25-ospi";
+					reg = <1 0 0x400>;
+					interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
+					dmas = <&hpdma 3 0x62 0x00003121>,
+					       <&hpdma 3 0x42 0x00003112>;
+					dma-names = "tx", "rx";
+					st,syscfg-dlyb = <&syscfg 0x1400>;
+					clocks = <&scmi_clk CK_SCMI_OSPI2>;
+					resets = <&scmi_reset RST_SCMI_OSPI2>,
+						 <&scmi_reset RST_SCMI_OSPI2DLL>;
+					access-controllers = <&rifsc 75>;
+					power-domains = <&CLUSTER_PD>;
+					status = "disabled";
+				};
+			};
+
 			spi8: spi@46020000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
-- 
2.25.1

[PATCH v2 6/9] arm64: dts: st: Add ospi port1 pinctrl entries in stm32mp25-pinctrl.dtsi

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Add pinctrl entry related to OSPI's port1 in stm32mp25-pinctrl.dtsi

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
index 8fdd5f020425..cf5be316de26 100644
--- a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
@@ -101,6 +101,57 @@ pins2 {
 		};
 	};
 
+	ospi_port1_clk_pins_a: ospi-port1-clk-0 {
+		pins {
+			pinmux = <STM32_PINMUX('D', 0, AF10)>; /* OSPI1_CLK */
+			bias-disable;
+			drive-push-pull;
+			slew-rate = <2>;
+		};
+	};
+
+	ospi_port1_clk_sleep_pins_a: ospi-port1-clk-sleep-0 {
+		pins {
+			pinmux = <STM32_PINMUX('D', 0, ANALOG)>; /* OSPI1_CLK */
+		};
+	};
+
+	ospi_port1_cs0_pins_a: ospi-port1-cs0-0 {
+		pins {
+			pinmux = <STM32_PINMUX('D', 3, AF10)>; /* OSPI_NCS0 */
+			bias-pull-up;
+			drive-push-pull;
+			slew-rate = <0>;
+		};
+	};
+
+	ospi_port1_cs0_sleep_pins_a: ospi-port1-cs0-sleep-0 {
+		pins {
+			pinmux = <STM32_PINMUX('D', 3, ANALOG)>; /* OSPI_NCS0 */
+		};
+	};
+
+	ospi_port1_io03_pins_a: ospi-port1-io03-0 {
+		pins {
+			pinmux = <STM32_PINMUX('D', 4, AF10)>, /* OSPI_IO0 */
+				 <STM32_PINMUX('D', 5, AF10)>, /* OSPI_IO1 */
+				 <STM32_PINMUX('D', 6, AF10)>, /* OSPI_IO2 */
+				 <STM32_PINMUX('D', 7, AF10)>; /* OSPI_IO3 */
+			bias-disable;
+			drive-push-pull;
+			slew-rate = <0>;
+		};
+	};
+
+	ospi_port1_io03_sleep_pins_a: ospi-port1-io03-sleep-0 {
+		pins {
+			pinmux = <STM32_PINMUX('D', 4, ANALOG)>, /* OSPI_IO0 */
+				 <STM32_PINMUX('D', 5, ANALOG)>, /* OSPI_IO1 */
+				 <STM32_PINMUX('D', 6, ANALOG)>, /* OSPI_IO2 */
+				 <STM32_PINMUX('D', 7, ANALOG)>; /* OSPI_IO3 */
+		};
+	};
+
 	sdmmc1_b4_od_pins_a: sdmmc1-b4-od-0 {
 		pins1 {
 			pinmux = <STM32_PINMUX('E', 4, AF10)>, /* SDMMC1_D0 */
-- 
2.25.1

[PATCH v2 7/9] arm64: dts: st: Add SPI NOR flash support on stm32mp257f-ev1 board

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Add SPI NOR flash nor support on stm32mp257f-ev1 board.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 arch/arm64/boot/dts/st/stm32mp257f-ev1.dts | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
index 1b88485a62a1..b9d308ea2d21 100644
--- a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
+++ b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
@@ -80,6 +80,11 @@ fw@80000000 {
 			reg = <0x0 0x80000000 0x0 0x4000000>;
 			no-map;
 		};
+
+		mm_ospi1: mm-ospi@60000000 {
+			reg = <0x0 0x60000000 0x0 0x10000000>;
+			no-map;
+		};
 	};
 };
 
@@ -190,6 +195,33 @@ &i2c8 {
 	status = "disabled";
 };
 
+&ommanager {
+	memory-region = <&mm_ospi1>;
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&ospi_port1_clk_pins_a
+		     &ospi_port1_io03_pins_a
+		     &ospi_port1_cs0_pins_a>;
+	pinctrl-1 = <&ospi_port1_clk_sleep_pins_a
+		     &ospi_port1_io03_sleep_pins_a
+		     &ospi_port1_cs0_sleep_pins_a>;
+	status = "okay";
+
+	spi@40430000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		memory-region = <&mm_ospi1>;
+		status = "okay";
+
+		flash0: flash@0 {
+			compatible = "jedec,spi-nor";
+			reg = <0>;
+			spi-rx-bus-width = <4>;
+			spi-tx-bus-width = <4>;
+			spi-max-frequency = <50000000>;
+		};
+	};
+};
+
 &rtc {
 	status = "okay";
 };
-- 
2.25.1

[PATCH v2 8/9] arm64: defconfig: Enable STM32 Octo Memory Manager driver

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Enable STM32 Octo Memory Manager (OMM) driver which is needed
for OSPI usage on STM32MP257F-EV1 board.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 246a13412bf0..bbbaf9a91d6f 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1506,6 +1506,7 @@ CONFIG_EXTCON_USB_GPIO=y
 CONFIG_EXTCON_USBC_CROS_EC=y
 CONFIG_FSL_IFC=y
 CONFIG_RENESAS_RPCIF=m
+CONFIG_STM32_OMM=m
 CONFIG_IIO=y
 CONFIG_EXYNOS_ADC=y
 CONFIG_IMX8QXP_ADC=m
-- 
2.25.1

[PATCH v2 9/9] arm64: defconfig: Enable STM32 OctoSPI driver

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Enable the STM32 OctoSPI driver.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index bbbaf9a91d6f..b089cf4b90a1 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -577,6 +577,7 @@ CONFIG_SPI_QUP=y
 CONFIG_SPI_QCOM_GENI=m
 CONFIG_SPI_S3C64XX=y
 CONFIG_SPI_SH_MSIOF=m
+CONFIG_SPI_STM32_OSPI=m
 CONFIG_SPI_SUN6I=y
 CONFIG_SPI_TEGRA210_QUAD=m
 CONFIG_SPI_TEGRA114=m
-- 
2.25.1

Re: [PATCH v2 9/9] arm64: defconfig: Enable STM32 OctoSPI driver

[Krzysztof Kozlowski]

On 28/01/2025 09:17, patrice.chotard@foss.st.com wrote:
> From: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> Enable the STM32 OctoSPI driver.
> 
Please squash this patches. It's not one driver per one defconfig change.

Best regards,
Krzysztof

Re: [PATCH v2 9/9] arm64: defconfig: Enable STM32 OctoSPI driver

[Krzysztof Kozlowski]

On 29/01/2025 10:36, Krzysztof Kozlowski wrote:
> On 28/01/2025 09:17, patrice.chotard@foss.st.com wrote:
>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>
>> Enable the STM32 OctoSPI driver.
>>
> Please squash this patches. It's not one driver per one defconfig change.


s/this/these two/

So only one defconfig patch.

Best regards,
Krzysztof

Re: [PATCH v2 9/9] arm64: defconfig: Enable STM32 OctoSPI driver

[Patrice CHOTARD]

On 1/29/25 11:30, Krzysztof Kozlowski wrote:
> On 29/01/2025 10:36, Krzysztof Kozlowski wrote:
>> On 28/01/2025 09:17, patrice.chotard@foss.st.com wrote:
>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>
>>> Enable the STM32 OctoSPI driver.
>>>
>> Please squash this patches. It's not one driver per one defconfig change.
> 
> 
> s/this/these two/
> 
> So only one defconfig patch.

ok

Thanks
Patrice

> 
> Best regards,
> Krzysztof