Message-ID: <e1523562-0cf9-44a9-9cbe-23ace46ea997@linaro.org>
Date: Fri, 27 Sep 2024 09:57:00 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Abdellatif El Khlifi <abdellatif.elkhlifi@....com>
Cc: mathieu.poirier@...aro.org, Adam.Johnston@....com,
 Hugues.KambaMpiana@....com, Drew.Reed@....com, andersson@...nel.org,
 conor+dt@...nel.org, devicetree@...r.kernel.org,
 krzysztof.kozlowski+dt@...aro.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-remoteproc@...r.kernel.org,
 liviu.dudau@....com, lpieralisi@...nel.org, robh@...nel.org,
 sudeep.holla@....com, robin.murphy@....com
Subject: Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External
 Systems remote processors

On 23/09/2024 19:19, Abdellatif El Khlifi wrote:

>>>
>>> The Host Base System Control [3] is the big block containing various functionalities (MMIO registers).
>>> Among the functionalities, the two remote cores registers (aka External system 0 and 1).
>>> The remote cores have two registers each.
>>>
>>> 1/ In the v1 patchset, a valid point was made by the community:
>>>
>>>    Right now it seems somewhat tenuous to describe two consecutive
>>>    32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
>>
>> ARM is not special, neither this hardware is. Therefore:
>> 1. Each register as reg: nope, for obvious reasons.
>> 2. One device for entire syscon: quite common, why do you think it is
>> somehow odd?
>> 3. If you quote other person, please provide the lore link, so I won't
>> spend useless 5 minutes to find who said that or if it was even said...
> 
> Please have a look at this lore link [1]. The idea is to add syscon
> and regmap support which I did in the v2 patchset.
> 
> [1]: https://lore.kernel.org/all/ZfMVcQsmgQUXXcef@bogus/

There is nothing there about DT bindings. We do not talk here about
drivers. MFD and regmap are Linux driver stuff, not bindings.

I said nothing about not using MFD, regmap or whatever driver stuff you
want. We talk *only* about bindings. Syscon is mentioned there but I am
sorry - that quite a stretch to call a block syscon just because you
want regmap.

> 
>>
>>>    all there ever is. However if it's actually going to end up needing several
>>>    more additional MMIO and/or memory regions for other functionality, then
>>>    describing each register and location individually is liable to get
>>>    unmanageable really fast, and a higher-level functional grouping (e.g. these
>>>    reset-related registers together as a single 8-byte region) would likely be
>>>    a better design.
>>>
>>>    The Exernal system registers are part of a bigger block with other functionality in place.
>>>    MFD/syscon might be better way to use these registers. You never know in
>>>    future you might want to use another set of 2-4 registers with a different
>>>    functionality in another driver.
>>>
>>>    I would see if it makes sense to put together a single binding for
>>>    this "Host Base System Control" register (not sure what exactly that means).
>>>    Use MFD/regmap you access parts of this block. The remoteproc driver can
>>>    then be semi-generic (meaning applicable to group of similar platforms)
>>>    based on the platform compatible and use this regmap to provide the
>>>    functionality needed.
>>
>> I don't understand how this lengthy semi-quote answers my concerns.
>> Please write concise points as arguments to my questions.
>>
>> For example, I don't care what your remote proc driver does and it
>> should not matter in the terms of this binding.
> 
> I just wanted to show why we are using syscon based on the arguments
> of other reviewers.
> 
>>
>>>
>>> 2/ There are many examples in the kernel that use syscon as a parent node of
>>>    child nodes for devices located at an offset from the syscon base address.
>>>    Please see these two examples [1][2]. I'm trying to follow a similar design if that
>>>    makes sense.
>>
>> Yeah, for separate devices. If you have two words without any resources,
>> I claim you might not have here any separate devices or "not separate
>> enough", because all this is somehow fluid. Remote core sounds like
>> separate device, but all your arguments about need of extid which cannot
>> be used in reg does not support this case.
>>
>> The example in the binding is also not complete - missing rest of
>> devices - which does not help.
> 
> Here I would like to explain the current suggestion and suggest an alternative solution.
> 
> 1/ For more clarity, here is a complete example of use of both remote cores
> at the same time:
> 
>     syscon@...10000 {
>         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 8>;
>             firmware-name = "es0_flashfw.elf";
>             mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
>             mbox-names = "txes0", "rxes0";
>             memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
>         };
> 
>         remoteproc@318 {
>             compatible = "arm,sse710-extsys1";
>             reg = <0x318 8>;
>             firmware-name = "es1_flashfw.elf";
>             mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
>             mbox-names = "txes0", "rxes0";
>             memory-region = <&extsys1_vring0>, <&extsys1_vring1>;
>         };
> };
> 
> Here we have 2 cores, each one have 2 registers mapped respectively
> at 0x1a010310 and 0x1a010318.

All this looks fine, resources are indeed reasonable, except that I
still do not understand why do you need to call them 0 and 1 (now via
compatible).

Your driver code shows this nicely - it is entirely redundant! The 'reg'
- so the base - is already there! You just duplicate it with the
extsys_id, instead of relying on the base. So think what is the point of
'reg' property if you do not use it?

> 
> Definetly these cores have seperate HW resources associated with them
> which are the MHUs (mailboxes HW). There are 2 pairs of MHUs associated
> with each core. These mailbox peripherals are obviously seperate.
> Furthermore, the vring buffers used for communication are seperate.
> 
> Moreover, the remote cores are independent. They are not SMP cores of one processor.
> 
> They can have different default firmware to use. In this example es0_flashfw.elf
> and es1_flashfw.elf
> 
> The current HW implementation (Corstone-1000) provides one remote core only.
> However, the second core control registers are at 0x1a010318 do exist.
> 
> Support for a second core is taken into consideration in this work to help
> end users who want to add a second core to their HW.
> 
> 2/ Here I'm suggesting an alternative solution by using one remoteproc node for both cores as
> follows:
> 
>     syscon@...10000 {
>         compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
>         reg = <0x1a010000 0x1000>;
> 
>         remoteproc {
>             compatible = "arm,sse710-extsys";
>             firmware-name = "es0_flashfw.elf";
>             mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>, <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
>             mbox-names = "txes0", "rxes0", "txes1", "rxes1";
>             memory-region = <&extsys0_vring0>, <&extsys0_vring1>, <&extsys1_vring0>, <&extsys1_vring1>;
>         };
> };
> 
> Does this meet your expectations please ?
> 
>>
>>>
>>> 3/ Since there are two registers for each remote core. I'm suggesting to set the
>>>    size in the reg property to 8. 
>>
>> How is this related?
>>
>>> The driver will read the match data to get the right
>>>    offset to be used with regmap APIs.
>>
>> Sorry, no talks about driver. Don't care, at least in this context.
>>
>> You can completely omit address space from children in DT and everything
>> will work fine and look fine from bindings point of view.
>>
>>>
>>> Suggested nodes:
>>>
>>>
>>>     syscon@...10000 {
>>>         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 8>;
>>>             firmware-name = "es_flashfw.elf";
>>>             mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
>>>             mbox-names = "txes0", "rxes0";
>>>             memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
>>>         };
>>>
>>>         remoteproc@318 {
>>>             compatible = "arm,sse710-extsys1";
>>>             reg = <0x318 8>;
>>>             firmware-name = "es_flashfw.elf";
>>
>> Same firmware? Always or only depends?
> 
> The firmware of the second core depends on the end user choice.
> Currently the second core is not implemented in the HW.
> Users who want to tweak Corstone-1000 HW can add
> a second core.


Two cores make more sense in such case.

Best regards,
Krzysztof

Powered by blists - more mailing lists