Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/09/2024 18:38, Abdellatif El Khlifi wrote:
> Hi Krzysztof,
> 
>>>>>>>>> +  '#extsys-id':
>>>>>>>>
>>>>>>>> '#' is not correct for sure, that's not a cell specifier.
>>>>>>>>
>>>>>>>> But anyway, we do not accept in general instance IDs.
>>>>>>>
>>>>>>> I'm happy to replace the instance ID with  another solution.
>>>>>>> In our case the remoteproc instance does not have a base address
>>>>>>> to use. So, we can't put remoteproc@address
>>>>>>>
>>>>>>> What do you recommend in this case please ?
>>>>>>
>>>>>> Waiting one month to respond is a great way to drop all context from my
>>>>>> memory. The emails are not even available for me - gone from inbox.
>>>>>>
>>>>>> Bus addressing could note it. Or you have different devices, so
>>>>>> different compatibles. Tricky to say, because you did not describe the
>>>>>> hardware really and it's one month later...
>>>>>>
>>>>>
>>>>> Sorry for waiting. I was in holidays.
>>>>>
>>>>> I'll add more documentation about the external system for more clarity [1].
>>>>>
>>>>> Basically, Linux runs on the Cortex-A35. The External system is a
>>>>> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
>>>>> It can only control Cortex-M core using the reset control and status registers mapped
>>>>> in the memory space of the Cortex-A35.
>>>>
>>>> That's pretty standard.
>>>>
>>>> It does not explain me why bus addressing or different compatible are
>>>> not sufficient here.
>>>
>>> Using an instance ID was a design choice.
>>> I'm happy to replace it with the use of compatible and match data (WIP).
>>>
>>> The match data will be pointing to a data structure containing the right offsets
>>> to be used with regmap APIs.
>>>
>>> syscon node is used to represent the Host Base System Control register area [1]
>>> where the external system reset registers are mapped (EXT_SYS*).
>>>
>>> The nodes will look like this:
>>>
>>> syscon@1a010000 {
>>>         compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
>>>         reg = <0x1a010000 0x1000>;
>>>
>>>         #address-cells = <1>;
>>>         #size-cells = <1>;
>>>
>>>         remoteproc@310 {
>>>             compatible = "arm,sse710-extsys0";
>>>             reg = <0x310 4>;
>>
>> Uh, why do you create device nodes for one word? This really suggests it
>> is part of parent device and your split is artificial.
> 
> The external system registers (described by the remoteproc node) are part
> of the parent device (the Host Base System Control register area) described
> by syscon.
> 
> In case of the external system 0 , its registers are located at offset 0x310
> (physical address: 0x1a010310)
> 
> When instantiating the devices without @address, the DTC compiler
> detects 2 nodes with the same name (remoteproc).

There should be no children at all. DT is not for instantiating your
drivers. I claim you have only one device and that's
arm,sse710-host-base-sysctrl. If you create child node for one word,
that's not a device.

Best regards,
Krzysztof