| 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 |
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
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
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.
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
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 --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: |
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(-)