Message-ID: <5f5f66a7-92ff-4a5c-9ad5-d113814da0b0@foss.st.com>
Date: Tue, 4 Feb 2025 09:16:23 +0100
From: Patrice CHOTARD <patrice.chotard@...s.st.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof
 Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Alexandre
 Torgue <alexandre.torgue@...s.st.com>,
        Philipp Zabel
	<p.zabel@...gutronix.de>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, <linux-spi@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <christophe.kerello@...s.st.com>
Subject: Re: [PATCH v2 3/9] dt-bindings: memory-controllers: Add STM32 Octo
 Memory Manager controller



On 2/4/25 08:50, Krzysztof Kozlowski wrote:
> 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@...s.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.
> 
> 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.

On our SoCs, there are always physically 2 OSPI instances, but one or two
instance can be enabled in the DT.

So we always have 2 ranges :

  ranges:
    description: |
      Reflects the memory layout with four integer values per OSPI instance.
      Format:
      <chip-select> 0 <registers base address> <size>
    minItems: 2
    maxItems: 2

Thanks 
Patrice


> 
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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

Powered by blists - more mailing lists