diff mbox series

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

Message ID 20220601112201.15510-2-tinghan.shen@mediatek.com (mailing list archive)
State Superseded
Headers show
Series Add support for MT8195 SCP 2nd core | expand

Commit Message

Tinghan Shen June 1, 2022, 11:21 a.m. UTC
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(+)

Comments

Krzysztof Kozlowski June 1, 2022, 11:50 a.m. UTC | #1
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";


Best regards,
Krzysztof
Tinghan Shen June 2, 2022, 5:21 a.m. UTC | #2
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
Krzysztof Kozlowski June 2, 2022, 6:55 a.m. UTC | #3
On 02/06/2022 07:21, Tinghan Shen wrote:
> 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.

You describe desired functional behavior, not the hardware. What is the
property about? If you just want to indicate this is two-core processor,
then it could be:
	mediatek,cores = <2>; /* number of cores */


However it seems you want to achieve here something different and as I
raised last time - it does not look like DT property.

Or maybe this is for first core and you want to indicate the sibling?
Something like that was mentioned in previous description.


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

Items should be rather reversed, as [0,1] being the argument to phandle
for a provider (see examples with syscon)...

Best regards,
Krzysztof
Tinghan Shen June 2, 2022, 8:58 a.m. UTC | #4
Hi Krzysztof,

On Thu, 2022-06-02 at 08:55 +0200, Krzysztof Kozlowski wrote:
> On 02/06/2022 07:21, Tinghan Shen wrote:
> > 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.
> 
> You describe desired functional behavior, not the hardware. What is the
> property about? If you just want to indicate this is two-core processor,
> then it could be:
> 	mediatek,cores = <2>; /* number of cores */
> 
> 
> However it seems you want to achieve here something different and as I
> raised last time - it does not look like DT property.
> 
> Or maybe this is for first core and you want to indicate the sibling?
> Something like that was mentioned in previous description.

This property is mainly added for scp 1st core driver 
and scp 2nd core driver to find each other via DT API.

After reconsidering the use of core id in the scp driver, it 
is not necessary in the control flow. I'll remove the core id 
at next version.

How about change the description as following,

  This property enables the dual-core support in scp driver.
  By providing the phandle of SCP 2nd core node, the 1st SCP node
  can control the SCP 2nd core as the subdevice of remoteproc framework.


Thanks,
Tinghan

> 
> 
> >     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.
> 
> Items should be rather reversed, as [0,1] being the argument to phandle
> for a provider (see examples with syscon)...
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski June 2, 2022, 10:37 a.m. UTC | #5
On 02/06/2022 10:58, Tinghan Shen wrote:
> Hi Krzysztof,
> 
> On Thu, 2022-06-02 at 08:55 +0200, Krzysztof Kozlowski wrote:
>> On 02/06/2022 07:21, Tinghan Shen wrote:
>>> 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.
>>
>> You describe desired functional behavior, not the hardware. What is the
>> property about? If you just want to indicate this is two-core processor,
>> then it could be:
>> 	mediatek,cores = <2>; /* number of cores */
>>
>>
>> However it seems you want to achieve here something different and as I
>> raised last time - it does not look like DT property.
>>
>> Or maybe this is for first core and you want to indicate the sibling?
>> Something like that was mentioned in previous description.
> 
> This property is mainly added for scp 1st core driver 
> and scp 2nd core driver to find each other via DT API.
> 
> After reconsidering the use of core id in the scp driver, it 
> is not necessary in the control flow. I'll remove the core id 
> at next version.
> 
> How about change the description as following,
> 
>   This property enables the dual-core support in scp driver.
>   By providing the phandle of SCP 2nd core node, the 1st SCP node
>   can control the SCP 2nd core as the subdevice of remoteproc framework.

Please, read it again:

>> You describe desired functional behavior, not the hardware.

Again, you describe Linux implementation (scp driver, remoteproc
framework). You need to describe the hardware, not Linux drivers.

Maybe the hardware property is that one core has its sibling and you
provide here that sibling?


Best regards,
Krzysztof
Tinghan Shen June 2, 2022, 11:29 a.m. UTC | #6
Hi Krzysztof,

On Thu, 2022-06-02 at 12:37 +0200, Krzysztof Kozlowski wrote:
> On 02/06/2022 10:58, Tinghan Shen wrote:
> > Hi Krzysztof,
> > 
> > On Thu, 2022-06-02 at 08:55 +0200, Krzysztof Kozlowski wrote:
> > > On 02/06/2022 07:21, Tinghan Shen wrote:
> > > > 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.
> > > 
> > > You describe desired functional behavior, not the hardware. What is the
> > > property about? If you just want to indicate this is two-core processor,
> > > then it could be:
> > > 	mediatek,cores = <2>; /* number of cores */
> > > 
> > > 
> > > However it seems you want to achieve here something different and as I
> > > raised last time - it does not look like DT property.
> > > 
> > > Or maybe this is for first core and you want to indicate the sibling?
> > > Something like that was mentioned in previous description.
> > 
> > This property is mainly added for scp 1st core driver 
> > and scp 2nd core driver to find each other via DT API.
> > 
> > After reconsidering the use of core id in the scp driver, it 
> > is not necessary in the control flow. I'll remove the core id 
> > at next version.
> > 
> > How about change the description as following,
> > 
> >   This property enables the dual-core support in scp driver.
> >   By providing the phandle of SCP 2nd core node, the 1st SCP node
> >   can control the SCP 2nd core as the subdevice of remoteproc framework.
> 
> Please, read it again:
> 
> > > You describe desired functional behavior, not the hardware.
> 
> Again, you describe Linux implementation (scp driver, remoteproc
> framework). You need to describe the hardware, not Linux drivers.
> 
> Maybe the hardware property is that one core has its sibling and you
> provide here that sibling?

Yes, exactly. I'm sorry for misreading your argument.

How about this one,
   
  Reference to the sibling SCP core. This is required when 
  dual-core SCP support is enabled.

Thanks,
TingHan
Krzysztof Kozlowski June 2, 2022, 12:02 p.m. UTC | #7
On 02/06/2022 13:29, Tinghan Shen wrote:
>> Maybe the hardware property is that one core has its sibling and you
>> provide here that sibling?
> 
> Yes, exactly. I'm sorry for misreading your argument.
> 
> How about this one,
>    
>   Reference to the sibling SCP core. This is required when 
>   dual-core SCP support is enabled.

Yes, sounds good. Thank you.


Best regards,
Krzysztof
diff mbox series

Patch

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
+      to the sibling SCP node. 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.
+    maxItems: 1
+
 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>;
 
         cros_ec {
             mediatek,rpmsg-name = "cros-ec-rpmsg";