diff mbox series

[v2,1/3] dt-bindings: dma: snps,dw-axi-dmac: Add reset items

Message ID 20230221140424.719-2-walker.chen@starfivetech.com
State New
Headers show
Series [v2,1/3] dt-bindings: dma: snps,dw-axi-dmac: Add reset items | expand

Commit Message

Walker Chen Feb. 21, 2023, 2:04 p.m. UTC
This DMA controller needs to be reset before being used on JH7110 SoC,
so add reset items to support this chip.

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 .../devicetree/bindings/dma/snps,dw-axi-dmac.yaml         | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Walker Chen Feb. 24, 2023, 10:14 a.m. UTC | #1
On 2023/2/22 17:17, Krzysztof Kozlowski wrote:
> On 21/02/2023 15:04, Walker Chen wrote:
>> This DMA controller needs to be reset before being used on JH7110 SoC,
>> so add reset items to support this chip.
> 
> There is reset already. The DMA controller is reset already. Your commit
> msg and commit subject do not match commit at all.

OK, the commit message will be changed to match the actual modification.

> 
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  .../devicetree/bindings/dma/snps,dw-axi-dmac.yaml         | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml
>> index ad107a4d3b33..c2247c65a22f 100644
>> --- a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml
>> +++ b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml
>> @@ -20,6 +20,7 @@ properties:
>>      enum:
>>        - snps,axi-dma-1.01a
>>        - intel,kmb-axi-dma
>> +      - starfive,jh7110-axi-dma
>>  
>>    reg:
>>      minItems: 1
>> @@ -58,7 +59,12 @@ properties:
>>      maximum: 8
>>  
>>    resets:
>> -    maxItems: 1
>> +    maxItems: 2
> 
> This breaks ABI and all other users. Test your changes before sending.

I think 'minItems' should be added here. So like this:
resets:
  minItems: 1
  maxItems: 2

Other platform/users will not be affected by this.

Thanks.

Best regards,
Walker
Krzysztof Kozlowski Feb. 24, 2023, 10:51 a.m. UTC | #2
On 24/02/2023 11:14, Walker Chen wrote:
>>>    resets:
>>> -    maxItems: 1
>>> +    maxItems: 2
>>
>> This breaks ABI and all other users. Test your changes before sending.
> 
> I think 'minItems' should be added here. So like this:
> resets:
>   minItems: 1
>   maxItems: 2
> 
> Other platform/users will not be affected by this.

Which will allow two resets on all platforms. Is this correct for these
platforms? Do they have two resets?


Best regards,
Krzysztof
Walker Chen Feb. 24, 2023, 1:09 p.m. UTC | #3
On 2023/2/24 18:51, Krzysztof Kozlowski wrote:
> On 24/02/2023 11:14, Walker Chen wrote:
>>>>    resets:
>>>> -    maxItems: 1
>>>> +    maxItems: 2
>>>
>>> This breaks ABI and all other users. Test your changes before sending.
>> 
>> I think 'minItems' should be added here. So like this:
>> resets:
>>   minItems: 1
>>   maxItems: 2
>> 
>> Other platform/users will not be affected by this.
> 
> Which will allow two resets on all platforms. Is this correct for these
> platforms? Do they have two resets?
> 
In kernel 6.2, only two platforms use this DMA controller (see 'arch/arc/boot/dts/hsdk.dts' and 'arch/riscv/boot/dts/canaan/k210.dtsi').
There is one reset on k210, while there is no reset of DMA on hsdk at all.
If here minItems with value 1 is added and the value of maxItems is changed to 2, after my testing,
whether it is one reset or two resets, even no reset, there is no errors occur when doing dtbs_check,
the DMA initialization shall not be affected either on their platforms.
Krzysztof Kozlowski Feb. 24, 2023, 1:21 p.m. UTC | #4
On 24/02/2023 14:09, Walker Chen wrote:
> 
> 
> On 2023/2/24 18:51, Krzysztof Kozlowski wrote:
>> On 24/02/2023 11:14, Walker Chen wrote:
>>>>>    resets:
>>>>> -    maxItems: 1
>>>>> +    maxItems: 2
>>>>
>>>> This breaks ABI and all other users. Test your changes before sending.
>>>
>>> I think 'minItems' should be added here. So like this:
>>> resets:
>>>   minItems: 1
>>>   maxItems: 2
>>>
>>> Other platform/users will not be affected by this.
>>
>> Which will allow two resets on all platforms. Is this correct for these
>> platforms? Do they have two resets?
>>
> In kernel 6.2, only two platforms use this DMA controller (see 'arch/arc/boot/dts/hsdk.dts' and 'arch/riscv/boot/dts/canaan/k210.dtsi').

What about all out-of-tree platforms, bootloaders and FW?

> There is one reset on k210, while there is no reset of DMA on hsdk at all.
> If here minItems with value 1 is added and the value of maxItems is changed to 2, after my testing,
> whether it is one reset or two resets, even no reset, there is no errors occur when doing dtbs_check,

Yeah, I know how this works.

> the DMA initialization shall not be affected either on their platforms.

I asked whether the hardware physically have two resets. dtbs_check
checks the DTS, not the hardware. You know allow two resets for each
other variant. What's more, you call first reset axi for all variants.
This does not look correct, e.g. k210.dtsi does not indicate this is axi
reset line but reset for entire block.

Thus your change should be constrained per your variant (min/max in
top-level, allOf:if:then allowing two lines for you, disallowing for
others).



Best regards,
Krzysztof
Walker Chen Feb. 24, 2023, 3:23 p.m. UTC | #5
On 2023/2/24 21:21, Krzysztof Kozlowski wrote:
> On 24/02/2023 14:09, Walker Chen wrote:
>> 
>> 
>> On 2023/2/24 18:51, Krzysztof Kozlowski wrote:
>>> On 24/02/2023 11:14, Walker Chen wrote:
>>>>>>    resets:
>>>>>> -    maxItems: 1
>>>>>> +    maxItems: 2
>>>>>
>>>>> This breaks ABI and all other users. Test your changes before sending.
>>>>
>>>> I think 'minItems' should be added here. So like this:
>>>> resets:
>>>>   minItems: 1
>>>>   maxItems: 2
>>>>
>>>> Other platform/users will not be affected by this.
>>>
>>> Which will allow two resets on all platforms. Is this correct for these
>>> platforms? Do they have two resets?
>>>
>> In kernel 6.2, only two platforms use this DMA controller (see 'arch/arc/boot/dts/hsdk.dts' and 'arch/riscv/boot/dts/canaan/k210.dtsi').
> 
> What about all out-of-tree platforms, bootloaders and FW?
> 
>> There is one reset on k210, while there is no reset of DMA on hsdk at all.
>> If here minItems with value 1 is added and the value of maxItems is changed to 2, after my testing,
>> whether it is one reset or two resets, even no reset, there is no errors occur when doing dtbs_check,
> 
> Yeah, I know how this works.
> 
>> the DMA initialization shall not be affected either on their platforms.
> 
> I asked whether the hardware physically have two resets. dtbs_check
> checks the DTS, not the hardware. You know allow two resets for each
> other variant. What's more, you call first reset axi for all variants.
> This does not look correct, e.g. k210.dtsi does not indicate this is axi
> reset line but reset for entire block.
> 
> Thus your change should be constrained per your variant (min/max in
> top-level, allOf:if:then allowing two lines for you, disallowing for
> others).
> 
I see what you mean. Thank you for your guidance!
I will use allOf:if:then structure to indicate different case for resets in next version of patch.

Best regards,
Walker
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml
index ad107a4d3b33..c2247c65a22f 100644
--- a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml
+++ b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.yaml
@@ -20,6 +20,7 @@  properties:
     enum:
       - snps,axi-dma-1.01a
       - intel,kmb-axi-dma
+      - starfive,jh7110-axi-dma
 
   reg:
     minItems: 1
@@ -58,7 +59,12 @@  properties:
     maximum: 8
 
   resets:
-    maxItems: 1
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: axi
+      - const: ahb
 
   snps,dma-masters:
     description: |