Re: [PATCH 1/2] dt-bindings: remoteproc: Add bindings for new Ingenic SoCs.

[Zhou Yanjie <zhouyanjie@wanyeetech.com>]

Hi Paul,

On 2021/7/24 下午7:02, Paul Cercueil wrote:
> Hi Zhou,
>
> Le sam., juil. 24 2021 at 17:11:37 +0800, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Add the remoteproc bindings for the JZ4760 SoC, the JZ4760B SoC,
>> the JZ4775 SoC, and the JZ4780 SoC from Ingenic.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>  .../bindings/remoteproc/ingenic,vpu.yaml           | 74 
>> ++++++++++++++++------
>>  1 file changed, 56 insertions(+), 18 deletions(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml 
>> b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>> index d0aa91b..6154596 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>> @@ -17,31 +17,52 @@ maintainers:
>>
>>  properties:
>>    compatible:
>> -    const: ingenic,jz4770-vpu-rproc
>> +    enum:
>> +      - ingenic,jz4760-vpu-rproc
>> +      - ingenic,jz4760b-vpu-rproc
>> +      - ingenic,jz4770-vpu-rproc
>> +      - ingenic,jz4775-vpu-rproc
>> +      - ingenic,jz4780-vpu-rproc
>>
>>    reg:
>> -    items:
>> -      - description: aux registers
>> -      - description: tcsm0 registers
>> -      - description: tcsm1 registers
>> -      - description: sram registers
>> +    oneOf:
>> +      - items:
>> +          - description: aux registers
>> +          - description: tcsm0 registers
>> +          - description: tcsm1 registers
>> +          - description: sram registers
>> +      - items:
>> +          - description: aux registers
>> +          - description: tcsm registers
>> +          - description: sram registers
>
> Since we have "reg-names" already, we don't really need any 
> description, so you could just have:
>
> reg:
>  minItems: 3
>  maxItems: 4


Sure, I will do it in the next version.


>
>>
>>    reg-names:
>> -    items:
>> -      - const: aux
>> -      - const: tcsm0
>> -      - const: tcsm1
>> -      - const: sram
>> +    oneOf:
>> +      - items:
>> +          - const: aux
>> +          - const: tcsm0
>> +          - const: tcsm1
>> +          - const: sram
>> +      - items:
>> +          - const: aux
>> +          - const: tcsm
>> +          - const: sram
>
> You could just add "tcsm" to the items list, and add:
> minItems: 3
> maxItems: 4


Sure.


>
>>
>>    clocks:
>> -    items:
>> -      - description: aux clock
>> -      - description: vpu clock
>> +    oneOf:
>> +      - items:
>> +          - description: aux clock
>> +          - description: vpu clock
>> +      - items:
>> +          - description: vpu clock
>
> Same as above, since we already have clock-names, the descriptions 
> don't bring much.
>
> You can replace with:
>
> clocks:
>  minItems: 1
>  maxItems: 2


Sure.


>
>>
>>    clock-names:
>> -    items:
>> -      - const: aux
>> -      - const: vpu
>> +    oneOf:
>> +      - items:
>> +          - const: aux
>> +          - const: vpu
>> +      - items:
>> +          - const: vpu
>
> I think you could just add:
> minItems: 1


Sure.


Thanks and best regards!


>
> Cheers,
> -Paul
>
>>
>>    interrupts:
>>      maxItems: 1
>> @@ -60,7 +81,7 @@ examples:
>>    - |
>>      #include <dt-bindings/clock/jz4770-cgu.h>
>>
>> -    vpu: video-decoder@132a0000 {
>> +    video-decoder@132a0000 {
>>        compatible = "ingenic,jz4770-vpu-rproc";
>>
>>        reg = <0x132a0000 0x20>, /* AUX */
>> @@ -75,3 +96,20 @@ examples:
>>        interrupt-parent = <&cpuintc>;
>>        interrupts = <3>;
>>      };
>> +  - |
>> +    #include <dt-bindings/clock/jz4780-cgu.h>
>> +
>> +    video-decoder@132a0000 {
>> +      compatible = "ingenic,jz4780-vpu-rproc";
>> +
>> +      reg = <0x132a0000 0x20>, /* AUX */
>> +            <0x132c0000 0x8000>, /* TCSM */
>> +            <0x132f0000 0x4000>; /* SRAM */
>> +      reg-names = "aux", "tcsm", "sram";
>> +
>> +      clocks = <&cgu JZ4780_CLK_VPU>;
>> +      clock-names = "vpu";
>> +
>> +      interrupt-parent = <&intc>;
>> +      interrupts = <62>;
>> +    };
>> -- 
>> 2.7.4
>>
>