Message-ID: <899675e8-4c2e-4ff2-a6af-854e0ec29bb6@kernel.org>
Date: Thu, 30 Jan 2025 16:09:06 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Patrice CHOTARD <patrice.chotard@...s.st.com>
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 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@...s.st.com wrote:
>>>>> From: Patrice Chotard <patrice.chotard@...s.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@...s.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@...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.

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

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



Best regards,
Krzysztof

Powered by blists - more mailing lists