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

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

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