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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <476baef8-0255-45ed-85f4-2b9d877c4af1@linaro.org>
Date:   Thu, 2 Jun 2022 08:55:23 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Tinghan Shen <tinghan.shen@...iatek.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        Daisuke Nojiri <dnojiri@...omium.org>,
        Sebastian Reichel <sebastian.reichel@...labora.com>,
        "Dustin L. Howett" <dustin@...ett.net>,
        Tzung-Bi Shih <tzungbi@...nel.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Prashant Malani <pmalani@...omium.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Brian Norris <briannorris@...omium.org>
Cc:     linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        chrome-platform@...ts.linux.dev,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        weishunc@...gle.com
Subject: Re: [PATCH v1 01/15] dt-binding: remoteproc: mediatek: Support
 dual-core SCP

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