Re: [PATCH v1 01/15] dt-binding: remoteproc: mediatek: Support dual-core SCP

[Tinghan Shen <tinghan.shen@mediatek.com>]

Hi Krzysztof,

On Wed, 2022-06-01 at 13:50 +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 13:21, Tinghan Shen wrote:
> > The SCP co-processor is a dual-core RISC-V MCU on MT8195.
> > 
> > Add a new property to identify each core and helps to find drivers
> > through device tree API to cooperate with each other, e.g. boot flow and
> > watchdog timeout flow.
> > 
> > Add a new compatile for the driver of SCP 2nd core.
> > 
> > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > ---
> >  .../devicetree/bindings/remoteproc/mtk,scp.yaml      | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > index eec3b9c4c713..b181786d9575 100644
> > --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > @@ -20,6 +20,7 @@ properties:
> >        - mediatek,mt8186-scp
> >        - mediatek,mt8192-scp
> >        - mediatek,mt8195-scp
> > +      - mediatek,mt8195-scp-dual
> >  
> >    reg:
> >      description:
> > @@ -57,6 +58,16 @@ properties:
> >    memory-region:
> >      maxItems: 1
> >  
> > +  mediatek,scp-core:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description:
> > +      The property value is a list with 2 items, a core id and a phandle
> 
> uint32, not phandle.
> 
> > +      to the sibling SCP node. 
> 
> Skip this. First part is obvious from the schema, second part should be
> described via items.
> 
> The core id represents the id of the dts node contains
> > +      this property. The valid values of core id are 0 and 1 for dual-core SCP.
> > +      The phandle of sibling SCP node is used to find the register settings,
> > +      trigger core dependent callback, and invoke rproc API.
> 
> Entire description did not help me to understand what's this. So far it
> looks like it is not a hardware property but some programming help, so
> it does not look like properly described in bindings.
> 
> > +    maxItems: 1
> 
> In description you said - two items.
> 
> You need allOf:if:then disallowing this property for other variants.
> 
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -115,6 +126,7 @@ examples:
> >          reg-names = "sram", "cfg", "l1tcm";
> >          clocks = <&infracfg CLK_INFRA_SCPSYS>;
> >          clock-names = "main";
> > +        mediatek,scp-core = <0 &scp_dual>;
> 
> This looks like phandle, so wrong type.
> >  
> >          cros_ec {
> >              mediatek,rpmsg-name = "cros-ec-rpmsg";
> 

Thanks for your feedback.
After looking for a comparable uses case, I find out a different approach.

  mediatek,scp-core:
    $ref: "/schemas/types.yaml#/definitions/phandle-array"
    description:
      Enable the dual-core support in scp driver.
    items:
      - items:
          - description: Assign a core id for current scp node.
            enum: [0, 1]
          - description:
              Phandle of another SCP node. This helps to find
              the scp driver of another core to trigger core
              dependent callback, invoke rproc subdevice API, etc.

> 
> Best regards,
> Krzysztof