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

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

On 1/22/25 16:22, Krzysztof Kozlowski wrote:
> On 22/01/2025 15:10, 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>
>> ---
>>  .../bindings/misc/st,stm32-omm.yaml           | 194 ++++++++++++++++++
>>  1 file changed, 194 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/misc/st,stm32-omm.yaml
> 
> 
> All my other comments apply.
> 
> Also:
> This cannot be misc. Depending what this is, either dedicated subsystem
> like memory or soc.
> 

Ok i will migrate it to memory

> 
>>
>> diff --git a/Documentation/devicetree/bindings/misc/st,stm32-omm.yaml b/Documentation/devicetree/bindings/misc/st,stm32-omm.yaml
>> new file mode 100644
>> index 000000000000..ef8f5d2c526c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/st,stm32-omm.yaml
> 
> 
>> @@ -0,0 +1,194 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/misc/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: registers
> 
> Well, why here is entirely different syntax? Anyway, useless
> description. Say something useful

ok

> 
>> +      - description: memory mapping
> 
> This is a bit better but still confusing. Memory mapping of what?
> Virtual memory? This is vague to me.

It's a memory map area, i will update to :

  reg:
    items:
      - description: OMM registers
      - description: OMM memory map area

> 
>> +
>> +  reg-names:
>> +    items:
>> +      - const: omm
>> +      - const: omm_mm
> 
> Not useful names. Drop prefixes and then you end up with empty first
> entry :/


Will replace by 
  reg-names:
    items:
      - const: regs
      - const: memory_map
> 
>> +
>> +  memory-region:
>> +    description: Phandle to a node describing memory-map region to be used.
> 
> Constraints.
> 
>> +
>> +  memory-region-names:
>> +    minItems: 1
> 
> Nope, you just said one phandle?

I made an error, it's not mandatory and can be up to 2 phandle.
will update as following: 

  memory-region:
    description: Phandle to a node describing memory-map region to be used.
    maxItems: 2



> 
>> +    items:
>> +      - const: mm_ospi1
>> +      - const: mm_ospi2
> 
> Drop redundant parts. If name is just 1 or 2, then just drop xxx-names

I will remove the memory-regions-names properties (a driver update is needed accordingly)

> 
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  access-controllers:
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  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,stm32-ospi.yaml#"
> 
> Drop quotes.
> 
> Look at your other $ref and keep things consistent.

ok

Thanks
Patrice

> 
> Best regards,
> Krzysztof