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

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

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