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

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

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.

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

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.

3/ Since there are two registers for each remote core. I'm suggesting to set the
   size in the reg property to 8. The driver will read the match data to get the right
   offset to be used with regmap APIs.

Suggested nodes:


    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 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";
            mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
            mbox-names = "txes0", "rxes0";
            memory-region = <&extsys1_vring0>, <&extsys1_vring1>;
        };
};


[1]: Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml

    syscon@20e00000 {
      compatible = "sprd,sc9863a-glbregs", "syscon", "simple-mfd";
      reg = <0x20e00000 0x4000>;
      #address-cells = <1>;
      #size-cells = <1>;
      ranges = <0 0x20e00000 0x4000>;

      apahb_gate: apahb-gate@0 {
        compatible = "sprd,sc9863a-apahb-gate";
        reg = <0x0 0x1020>;
        #clock-cells = <1>;
      };
    };


[2]: Documentation/devicetree/bindings/arm/arm,juno-fpga-apb-regs.yaml:

    syscon@10000 {
        compatible = "arm,juno-fpga-apb-regs", "syscon", "simple-mfd";
        reg = <0x010000 0x1000>;
        ranges = <0x0 0x10000 0x1000>;
        #address-cells = <1>;
        #size-cells = <1>;

        led@8,0 {
            compatible = "register-bit-led";
            reg = <0x08 0x04>;
            offset = <0x08>;
            mask = <0x01>;
            label = "vexpress:0";
            linux,default-trigger = "heartbeat";
            default-state = "on";
        };
    };

[3]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary

Cheers,
Abdellatif