[PATCH v2 0/2] arm64: allwinner: h6: Add second IOMMU reference to Cedrus

[PATCH v2 0/2] arm64: allwinner: h6: Add second IOMMU reference to Cedrus

[Jernej Skrabec]

It turns out that Cedrus on H6 actually uses two IOMMU channels. Manual
mentiones two IOMMU channels, but it doesn't specify which is used when.
Page faults were also observed from both.

This series updates binding to allow up to two IOMMU channels and fixes
H6 DTSI.

Please take a look.

Best regards,
Jernej

Changes from v1:
- add minItems to binding
- add reviewed-by to DT patch

Jernej Skrabec (2):
  media: dt-bindings: allwinner: video-engine: Fix number of IOMMU
    channels
  arm64: dts: allwinner: h6: Fix Cedrus IOMMU channels

 .../bindings/media/allwinner,sun4i-a10-video-engine.yaml       | 3 ++-
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi                   | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

--
2.38.1

[PATCH v2 1/2] media: dt-bindings: allwinner: video-engine: Fix number of IOMMU channels

[Jernej Skrabec]

Cedrus (video engine) on Allwinner H6 actually uses two IOMMU channel,
not just one. However, Cedrus on SoCs like D1 only uses one channel.

Allow up to 2 IOMMU channels.

Fixes: 62a8ccf3a248 ("arm64: dts: allwinner: h6: Fix Cedrus IOMMU usage")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 .../bindings/media/allwinner,sun4i-a10-video-engine.yaml       | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml
index 541325f900a1..6446004d59d9 100644
--- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml
+++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml
@@ -55,7 +55,8 @@ properties:
     description: Phandle to the device SRAM
 
   iommus:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   memory-region:
     maxItems: 1
-- 
2.38.1

Re: [PATCH v2 1/2] media: dt-bindings: allwinner: video-engine: Fix number of IOMMU channels

[Krzysztof Kozlowski]

On 17/11/2022 07:07, Jernej Skrabec wrote:
> Cedrus (video engine) on Allwinner H6 actually uses two IOMMU channel,
> not just one. However, Cedrus on SoCs like D1 only uses one channel.
> 
> Allow up to 2 IOMMU channels.
> 
> Fixes: 62a8ccf3a248 ("arm64: dts: allwinner: h6: Fix Cedrus IOMMU usage")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  .../bindings/media/allwinner,sun4i-a10-video-engine.yaml       | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml
> index 541325f900a1..6446004d59d9 100644
> --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml
> +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engine.yaml
> @@ -55,7 +55,8 @@ properties:
>      description: Phandle to the device SRAM
>  
>    iommus:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2

You have several compatibles in the file, so usually this is further
constrained per each variant in allOf:if:then:.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] media: dt-bindings: allwinner: video-engine: Fix number of IOMMU channels

[Jernej Škrabec]

Dne četrtek, 17. november 2022 ob 14:13:00 CET je Krzysztof Kozlowski 
napisal(a):
> On 17/11/2022 07:07, Jernej Skrabec wrote:
> > Cedrus (video engine) on Allwinner H6 actually uses two IOMMU channel,
> > not just one. However, Cedrus on SoCs like D1 only uses one channel.
> > 
> > Allow up to 2 IOMMU channels.
> > 
> > Fixes: 62a8ccf3a248 ("arm64: dts: allwinner: h6: Fix Cedrus IOMMU usage")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> > 
> >  .../bindings/media/allwinner,sun4i-a10-video-engine.yaml       | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
> > e.yaml
> > b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
> > e.yaml index 541325f900a1..6446004d59d9 100644
> > ---
> > a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
> > e.yaml +++
> > b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
> > e.yaml> 
> > @@ -55,7 +55,8 @@ properties:
> >      description: Phandle to the device SRAM
> >    
> >    iommus:
> > -    maxItems: 1
> > +    minItems: 1
> > +    maxItems: 2
> 
> You have several compatibles in the file, so usually this is further
> constrained per each variant in allOf:if:then:.

Usually, yes. But this whole binding would need update. It has a few optional 
properties and none of them is tied to any compatible. Additionally, if I do 
it as you suggest, then Robs automatic test will report the issue, because 
existing H6 based boards won't match this binding anymore. I would much rather 
send follow up patch which clears up all optional properties.

Best regards,
Jernej

Re: [PATCH v2 1/2] media: dt-bindings: allwinner: video-engine: Fix number of IOMMU channels

[Krzysztof Kozlowski]

On 17/11/2022 21:31, Jernej Škrabec wrote:
> Dne četrtek, 17. november 2022 ob 14:13:00 CET je Krzysztof Kozlowski 
> napisal(a):
>> On 17/11/2022 07:07, Jernej Skrabec wrote:
>>> Cedrus (video engine) on Allwinner H6 actually uses two IOMMU channel,
>>> not just one. However, Cedrus on SoCs like D1 only uses one channel.
>>>
>>> Allow up to 2 IOMMU channels.
>>>
>>> Fixes: 62a8ccf3a248 ("arm64: dts: allwinner: h6: Fix Cedrus IOMMU usage")
>>> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> ---
>>>
>>>  .../bindings/media/allwinner,sun4i-a10-video-engine.yaml       | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
>>> e.yaml
>>> b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
>>> e.yaml index 541325f900a1..6446004d59d9 100644
>>> ---
>>> a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
>>> e.yaml +++
>>> b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-video-engin
>>> e.yaml> 
>>> @@ -55,7 +55,8 @@ properties:
>>>      description: Phandle to the device SRAM
>>>    
>>>    iommus:
>>> -    maxItems: 1
>>> +    minItems: 1
>>> +    maxItems: 2
>>
>> You have several compatibles in the file, so usually this is further
>> constrained per each variant in allOf:if:then:.
> 
> Usually, yes. But this whole binding would need update. It has a few optional 
> properties and none of them is tied to any compatible. Additionally, if I do 
> it as you suggest, then Robs automatic test will report the issue, because 
> existing H6 based boards won't match this binding anymore. I would much rather 
> send follow up patch which clears up all optional properties.

I don't understand last argument. It's basically like saying - I keep
bindings not really correct, because otherwise I would see warnings and
I would need to fix them.

If this can be constrained per variant, constrain it and then fix the
boards.
Best regards,
Krzysztof

[PATCH v2 2/2] arm64: dts: allwinner: h6: Fix Cedrus IOMMU channels

[Jernej Skrabec]

Cedrus H6 actually uses two IOMMU channels. During development page
faults from both were observed. Documentation also lists both of them
to be connected to Cedrus, but it doesn't make clear which is used for
what.

Add second IOMMU channel.

Reviewed-by: Samuel Holland <samuel@sholland.org>
Fixes: 62a8ccf3a248 ("arm64: dts: allwinner: h6: Fix Cedrus IOMMU usage")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 53f6660656ac..7bff054a9bdf 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -172,7 +172,7 @@ video-codec@1c0e000 {
 			resets = <&ccu RST_BUS_VE>;
 			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
 			allwinner,sram = <&ve_sram 1>;
-			iommus = <&iommu 3>;
+			iommus = <&iommu 1>, <&iommu 3>;
 		};
 
 		gpu: gpu@1800000 {
-- 
2.38.1