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