diff mbox series

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

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

Commit Message

Patrice CHOTARD Jan. 28, 2025, 8:17 a.m. UTC
From: Patrice Chotard <patrice.chotard@foss.st.com>

Add 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

Comments

Krzysztof Kozlowski Jan. 29, 2025, 7:52 a.m. UTC | #1
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
Patrice CHOTARD Jan. 30, 2025, 8:57 a.m. UTC | #2
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
>
Krzysztof Kozlowski Jan. 30, 2025, 12:12 p.m. UTC | #3
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
Patrice CHOTARD Jan. 30, 2025, 1:32 p.m. UTC | #4
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
Krzysztof Kozlowski Jan. 30, 2025, 3:09 p.m. UTC | #5
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
diff mbox series

Patch

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