[PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx jpeg encoder/decoder

[PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

There are total 4 slots available in the IP, and we only need to use one
slot in one os, assign a single slot for imx jpeg device node, configure
interrupt and power domain only for 1 slot, not for the all 4 slots.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
v4
- improve the commit message
v3
- add vender prefix, change property slot to nxp,slot

 .../arm64/boot/dts/freescale/imx8-ss-img.dtsi | 22 +++++--------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
index a90654155a88..3043c416c43e 100644
--- a/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
@@ -18,10 +18,7 @@ img_ipg_clk: clock-img-ipg {
 
 	jpegdec: jpegdec@58400000 {
 		reg = <0x58400000 0x00050000>;
-		interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&img_jpeg_dec_lpcg IMX_LPCG_CLK_0>,
 			 <&img_jpeg_dec_lpcg IMX_LPCG_CLK_4>;
 		clock-names = "per", "ipg";
@@ -29,18 +26,13 @@ jpegdec: jpegdec@58400000 {
 				  <&img_jpeg_dec_lpcg IMX_LPCG_CLK_4>;
 		assigned-clock-rates = <200000000>, <200000000>;
 		power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>,
-				<&pd IMX_SC_R_MJPEG_DEC_S0>,
-				<&pd IMX_SC_R_MJPEG_DEC_S1>,
-				<&pd IMX_SC_R_MJPEG_DEC_S2>,
-				<&pd IMX_SC_R_MJPEG_DEC_S3>;
+				<&pd IMX_SC_R_MJPEG_DEC_S0>;
+		nxp,slot = <0>;
 	};
 
 	jpegenc: jpegenc@58450000 {
 		reg = <0x58450000 0x00050000>;
-		interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&img_jpeg_enc_lpcg IMX_LPCG_CLK_0>,
 			 <&img_jpeg_enc_lpcg IMX_LPCG_CLK_4>;
 		clock-names = "per", "ipg";
@@ -48,10 +40,8 @@ jpegenc: jpegenc@58450000 {
 				  <&img_jpeg_enc_lpcg IMX_LPCG_CLK_4>;
 		assigned-clock-rates = <200000000>, <200000000>;
 		power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>,
-				<&pd IMX_SC_R_MJPEG_ENC_S0>,
-				<&pd IMX_SC_R_MJPEG_ENC_S1>,
-				<&pd IMX_SC_R_MJPEG_ENC_S2>,
-				<&pd IMX_SC_R_MJPEG_ENC_S3>;
+				<&pd IMX_SC_R_MJPEG_ENC_S0>;
+		nxp,slot = <0>;
 	};
 
 	img_jpeg_dec_lpcg: clock-controller@585d0000 {
-- 
2.38.1

[PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

There are total 4 slots available in the IP, and we only need to use one
slot in one os, assign a single slot, configure interrupt and power
domain only for 1 slot, not for the all 4 slots.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
v4
- improve commit message
- don't make an ABI break
v3
- add vender prefix, change property slot to nxp,slot
- add type for property slot

 .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
index 3d9d1db37040..4bcfc815c894 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
@@ -32,19 +32,26 @@ properties:
     maxItems: 1
 
   interrupts:
-    description: |
-      There are 4 slots available in the IP, which the driver may use
-      If a certain slot is used, it should have an associated interrupt
-      The interrupt with index i is assumed to be for slot i
-    minItems: 1               # At least one slot is needed by the driver
-    maxItems: 4               # The IP has 4 slots available for use
+    description:
+      Interrupt number for slot
+    maxItems: 1
 
   power-domains:
     description:
       List of phandle and PM domain specifier as documented in
       Documentation/devicetree/bindings/power/power_domain.txt
-    minItems: 2               # Wrapper and 1 slot
-    maxItems: 5               # Wrapper and 4 slots
+    minItems: 1               # VPUMIX
+    maxItems: 2               # Wrapper and 1 slot
+
+  nxp,slot:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Integer number of slot index used. There are 4 slots available in the IP,
+      and driver can use a certain slot, it should have an associated interrupt
+      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
+      is used by default.
+    minimum: 0
+    maximum: 3
 
 required:
   - compatible
@@ -62,28 +69,18 @@ examples:
     jpegdec: jpegdec@58400000 {
         compatible = "nxp,imx8qxp-jpgdec";
         reg = <0x58400000 0x00050000 >;
-        interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
-                     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
-                     <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>,
-                     <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
+        interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>;
         power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>,
-                        <&pd IMX_SC_R_MJPEG_DEC_S0>,
-                        <&pd IMX_SC_R_MJPEG_DEC_S1>,
-                        <&pd IMX_SC_R_MJPEG_DEC_S2>,
-                        <&pd IMX_SC_R_MJPEG_DEC_S3>;
+                        <&pd IMX_SC_R_MJPEG_DEC_S0>;
+        nxp,slot = <0>;
     };
 
     jpegenc: jpegenc@58450000 {
         compatible = "nxp,imx8qm-jpgenc", "nxp,imx8qxp-jpgenc";
         reg = <0x58450000 0x00050000 >;
-        interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>,
-                     <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>,
-                     <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
-                     <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>;
+        interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
         power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>,
-                        <&pd IMX_SC_R_MJPEG_ENC_S0>,
-                        <&pd IMX_SC_R_MJPEG_ENC_S1>,
-                        <&pd IMX_SC_R_MJPEG_ENC_S2>,
-                        <&pd IMX_SC_R_MJPEG_ENC_S3>;
+                        <&pd IMX_SC_R_MJPEG_ENC_S0>;
+        nxp,slot = <0>;
     };
 ...
-- 
2.38.1

Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4807 bytes --]

Hi,

On Tue, Sep 26, 2023 at 06:10:00PM +0800, Ming Qian wrote:
> There are total 4 slots available in the IP, and we only need to use one
> slot in one os, assign a single slot, configure interrupt and power
> domain only for 1 slot, not for the all 4 slots.
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
> v4
> - improve commit message

> - don't make an ABI break

What does this mean? Can you please try to explain things a bit more
clearly in your changelogs?

Also, where is the code that actually makes use of these properties?

> v3
> - add vender prefix, change property slot to nxp,slot
> - add type for property slot
> 
>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> index 3d9d1db37040..4bcfc815c894 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> @@ -32,19 +32,26 @@ properties:
>      maxItems: 1
>  
>    interrupts:
> -    description: |
> -      There are 4 slots available in the IP, which the driver may use
> -      If a certain slot is used, it should have an associated interrupt
> -      The interrupt with index i is assumed to be for slot i
> -    minItems: 1               # At least one slot is needed by the driver
> -    maxItems: 4               # The IP has 4 slots available for use
> +    description:
> +      Interrupt number for slot
> +    maxItems: 1

So, what happens to users that already exist that have 2, 3 or 4
interrupts? Your commit message says:
> There are total 4 slots available in the IP, and we only need to use one
> slot in one os, assign a single slot, configure interrupt and power
> domain only for 1 slot, not for the all 4 slots.

Note that "only need to use one" doesn't mean that a user might not
want to use more than one. Is it possible to use more than one slot?

Also, if the interrupt numbers map 1:1 to slots, as the previous binding
states, why do you even need this new "nxp,slot" property?

Thanks,
Conor.

>  
>    power-domains:
>      description:
>        List of phandle and PM domain specifier as documented in
>        Documentation/devicetree/bindings/power/power_domain.txt
> -    minItems: 2               # Wrapper and 1 slot
> -    maxItems: 5               # Wrapper and 4 slots
> +    minItems: 1               # VPUMIX
> +    maxItems: 2               # Wrapper and 1 slot
> +
> +  nxp,slot:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Integer number of slot index used. There are 4 slots available in the IP,
> +      and driver can use a certain slot, it should have an associated interrupt
> +      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
> +      is used by default.
> +    minimum: 0
> +    maximum: 3
>  
>  required:
>    - compatible
> @@ -62,28 +69,18 @@ examples:
>      jpegdec: jpegdec@58400000 {
>          compatible = "nxp,imx8qxp-jpgdec";
>          reg = <0x58400000 0x00050000 >;
> -        interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
> -                     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
> -                     <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>,
> -                     <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>;
>          power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>,
> -                        <&pd IMX_SC_R_MJPEG_DEC_S0>,
> -                        <&pd IMX_SC_R_MJPEG_DEC_S1>,
> -                        <&pd IMX_SC_R_MJPEG_DEC_S2>,
> -                        <&pd IMX_SC_R_MJPEG_DEC_S3>;
> +                        <&pd IMX_SC_R_MJPEG_DEC_S0>;
> +        nxp,slot = <0>;
>      };
>  
>      jpegenc: jpegenc@58450000 {
>          compatible = "nxp,imx8qm-jpgenc", "nxp,imx8qxp-jpgenc";
>          reg = <0x58450000 0x00050000 >;
> -        interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>,
> -                     <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>,
> -                     <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
> -                     <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
>          power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>,
> -                        <&pd IMX_SC_R_MJPEG_ENC_S0>,
> -                        <&pd IMX_SC_R_MJPEG_ENC_S1>,
> -                        <&pd IMX_SC_R_MJPEG_ENC_S2>,
> -                        <&pd IMX_SC_R_MJPEG_ENC_S3>;
> +                        <&pd IMX_SC_R_MJPEG_ENC_S0>;
> +        nxp,slot = <0>;
>      };
>  ...
> -- 
> 2.38.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Conor Dooley <conor@kernel.org>
>Sent: 2023年9月26日 21:26
>To: Ming Qian <ming.qian@nxp.com>
>Cc: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>;
>robh+dt@kernel.org; shawnguo@kernel.org;
>krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for
>imx jpeg encoder/decoder
>
>Hi,
>
>On Tue, Sep 26, 2023 at 06:10:00PM +0800, Ming Qian wrote:
>> There are total 4 slots available in the IP, and we only need to use
>> one slot in one os, assign a single slot, configure interrupt and
>> power domain only for 1 slot, not for the all 4 slots.
>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>> v4
>> - improve commit message
>
>> - don't make an ABI break
>
>What does this mean? Can you please try to explain things a bit more clearly
>in your changelogs?
>
>Also, where is the code that actually makes use of these properties?

In v3 patch, I make this property required, make it an ABI break, so in v4, I remove it from required, and default to the previous behavior if it is missing.

The code patch is sent before, but the dts change is not applicable, so I send it separately. The code patch link is as below:
https://patchwork.linuxtv.org/project/linux-media/patch/cdadb4a23697fdc97def958c69b12cd00f547212.1685430841.git.ming.qian@nxp.com/

But in the patch, the property name is "slot", not "nxp,slot", I will make another patch to fix the property name after this patch is reviewed.

>
>> v3
>> - add vender prefix, change property slot to nxp,slot
>> - add type for property slot
>>
>>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>>  1 file changed, 21 insertions(+), 24 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> index 3d9d1db37040..4bcfc815c894 100644
>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> @@ -32,19 +32,26 @@ properties:
>>      maxItems: 1
>>
>>    interrupts:
>> -    description: |
>> -      There are 4 slots available in the IP, which the driver may use
>> -      If a certain slot is used, it should have an associated interrupt
>> -      The interrupt with index i is assumed to be for slot i
>> -    minItems: 1               # At least one slot is needed by the driver
>> -    maxItems: 4               # The IP has 4 slots available for use
>> +    description:
>> +      Interrupt number for slot
>> +    maxItems: 1
>
>So, what happens to users that already exist that have 2, 3 or 4 interrupts?
>Your commit message says:

And currently the answer is that is inappropriate behavior,  but there has been no real negative impact. As we now have no practical application to use other slots, now we just want to make all the 4 slots configurable and workable.

>> There are total 4 slots available in the IP, and we only need to use
>> one slot in one os, assign a single slot, configure interrupt and
>> power domain only for 1 slot, not for the all 4 slots.
>
>Note that "only need to use one" doesn't mean that a user might not want to
>use more than one. Is it possible to use more than one slot?
>
>Also, if the interrupt numbers map 1:1 to slots, as the previous binding states,
>why do you even need this new "nxp,slot" property?
>
>Thanks,
>Conor.
>

The purpose of 4 slots is to support multi vm or os, Multiple slots are time-sharing the same jpeg engine, there is no benefit to enabling multiple slots in one os, it won't improve the performance, but it just adds complexity to the code.
So we'd like to only use 1 slot in linux, Then, theoretically, we can support up to 3 VMS.

In previous implementation, the dts configured all the interrupts and power-domains for 4 slots, but in driver code, only slot 0 is used, the result is the slot 1,2,3 are not usable, otherwise it will cause conflict

If someone has to enable 2 slots in linux, he can define 2 device node with different slot index in dts.  But this is not what we expect.

This IP defined separate interrupt number for each slot, but it doesn't define separate register region for each slot. It simply defines a set of registers for slots, so driver need the slot index to configure register correctly.
This IP is reused on our other platform, so the interrupt number is not a fixed value for a certain slot. So I think it's not a good way to convert the interrupt number to slot index.

Best regards,
Ming


>>
>>    power-domains:
>>      description:
>>        List of phandle and PM domain specifier as documented in
>>        Documentation/devicetree/bindings/power/power_domain.txt
>> -    minItems: 2               # Wrapper and 1 slot
>> -    maxItems: 5               # Wrapper and 4 slots
>> +    minItems: 1               # VPUMIX
>> +    maxItems: 2               # Wrapper and 1 slot
>> +
>> +  nxp,slot:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Integer number of slot index used. There are 4 slots available in the IP,
>> +      and driver can use a certain slot, it should have an associated interrupt
>> +      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
>> +      is used by default.
>> +    minimum: 0
>> +    maximum: 3
>>
>>  required:
>>    - compatible
>> @@ -62,28 +69,18 @@ examples:
>>      jpegdec: jpegdec@58400000 {
>>          compatible = "nxp,imx8qxp-jpgdec";
>>          reg = <0x58400000 0x00050000 >;
>> -        interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
>> -                     <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
>> -                     <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>,
>> -                     <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
>> +        interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>;
>>          power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>,
>> -                        <&pd IMX_SC_R_MJPEG_DEC_S0>,
>> -                        <&pd IMX_SC_R_MJPEG_DEC_S1>,
>> -                        <&pd IMX_SC_R_MJPEG_DEC_S2>,
>> -                        <&pd IMX_SC_R_MJPEG_DEC_S3>;
>> +                        <&pd IMX_SC_R_MJPEG_DEC_S0>;
>> +        nxp,slot = <0>;
>>      };
>>
>>      jpegenc: jpegenc@58450000 {
>>          compatible = "nxp,imx8qm-jpgenc", "nxp,imx8qxp-jpgenc";
>>          reg = <0x58450000 0x00050000 >;
>> -        interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>,
>> -                     <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>,
>> -                     <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
>> -                     <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>;
>> +        interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
>>          power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>,
>> -                        <&pd IMX_SC_R_MJPEG_ENC_S0>,
>> -                        <&pd IMX_SC_R_MJPEG_ENC_S1>,
>> -                        <&pd IMX_SC_R_MJPEG_ENC_S2>,
>> -                        <&pd IMX_SC_R_MJPEG_ENC_S3>;
>> +                        <&pd IMX_SC_R_MJPEG_ENC_S0>;
>> +        nxp,slot = <0>;
>>      };
>>  ...
>> --
>> 2.38.1
>>

Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 27/09/2023 04:10, Ming Qian wrote:
>> From: Conor Dooley <conor@kernel.org>
>> Sent: 2023年9月26日 21:26
>> To: Ming Qian <ming.qian@nxp.com>
>> Cc: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>;
>> robh+dt@kernel.org; shawnguo@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for
>> imx jpeg encoder/decoder
>>
>> Hi,
>>
>> On Tue, Sep 26, 2023 at 06:10:00PM +0800, Ming Qian wrote:
>>> There are total 4 slots available in the IP, and we only need to use
>>> one slot in one os, assign a single slot, configure interrupt and
>>> power domain only for 1 slot, not for the all 4 slots.
>>>
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>> v4
>>> - improve commit message
>>
>>> - don't make an ABI break
>>
>> What does this mean? Can you please try to explain things a bit more clearly
>> in your changelogs?
>>
>> Also, where is the code that actually makes use of these properties?
> 
> In v3 patch, I make this property required, make it an ABI break, so in v4, I remove it from required, and default to the previous behavior if it is missing.

So say that you dropped line making the property required.

> 
> The code patch is sent before, but the dts change is not applicable, so I send it separately. The code patch link is as below:
> https://patchwork.linuxtv.org/project/linux-media/patch/cdadb4a23697fdc97def958c69b12cd00f547212.1685430841.git.ming.qian@nxp.com/
> 
> But in the patch, the property name is "slot", not "nxp,slot", I will make another patch to fix the property name after this patch is reviewed.

Format your emails properly. It's difficult to read it.

I already NAKed it, I will be NAKing still. Don't embed OS specific into
the bindings nor into the DTS.


Best regards,
Krzysztof

Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

On Wed, Sep 27, 2023 at 10:10:40AM +0200, Krzysztof Kozlowski wrote:
> On 27/09/2023 04:10, Ming Qian wrote:
> >> From: Conor Dooley <conor@kernel.org>
> >> Sent: 2023年9月26日 21:26
> >> To: Ming Qian <ming.qian@nxp.com>
> >> Cc: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>;
> >> robh+dt@kernel.org; shawnguo@kernel.org;
> >> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> >> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
> >> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
> >> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
> >> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> >> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> >> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for
> >> imx jpeg encoder/decoder
> >>
> >> Hi,
> >>
> >> On Tue, Sep 26, 2023 at 06:10:00PM +0800, Ming Qian wrote:
> >>> There are total 4 slots available in the IP, and we only need to use
> >>> one slot in one os, assign a single slot, configure interrupt and
> >>> power domain only for 1 slot, not for the all 4 slots.
> >>>
> >>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> >>> ---
> >>> v4
> >>> - improve commit message
> >>
> >>> - don't make an ABI break
> >>
> >> What does this mean? Can you please try to explain things a bit more clearly
> >> in your changelogs?
> >>
> >> Also, where is the code that actually makes use of these properties?
> > 
> > In v3 patch, I make this property required, make it an ABI break, so in v4, I remove it from required, and default to the previous behavior if it is missing.
> 
> So say that you dropped line making the property required.
> 
> > 
> > The code patch is sent before, but the dts change is not applicable, so I send it separately. The code patch link is as below:
> > https://patchwork.linuxtv.org/project/linux-media/patch/cdadb4a23697fdc97def958c69b12cd00f547212.1685430841.git.ming.qian@nxp.com/
> > 
> > But in the patch, the property name is "slot", not "nxp,slot", I will make another patch to fix the property name after this patch is reviewed.
> 
> Format your emails properly. It's difficult to read it.
> 
> I already NAKed it, I will be NAKing still. Don't embed OS specific into
> the bindings nor into the DTS.

Additionally, sending the binding and dts patch split out from the
proposed driver change is very unhelpful, as often (as is the case here)
binding patches are not very well explained and it is required to read
the driver to reverse-engineer the binding patch author's real intent.

Particularly when you mention an ABI break it is important to do so, so
that we can check the code changes to make sure that the ABI is upheld.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>Sent: 2023年9月27日 16:11
>To: Ming Qian <ming.qian@nxp.com>; Conor Dooley <conor@kernel.org>
>Cc: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>;
>robh+dt@kernel.org; shawnguo@kernel.org;
>krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot
>for imx jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On 27/09/2023 04:10, Ming Qian wrote:
>>> From: Conor Dooley <conor@kernel.org>
>>> Sent: 2023年9月26日 21:26
>>> To: Ming Qian <ming.qian@nxp.com>
>>> Cc: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>;
>>> robh+dt@kernel.org; shawnguo@kernel.org;
>>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg:
>>> Assign slot for imx jpeg encoder/decoder
>>>
>>> Hi,
>>>
>>> On Tue, Sep 26, 2023 at 06:10:00PM +0800, Ming Qian wrote:
>>>> There are total 4 slots available in the IP, and we only need to use
>>>> one slot in one os, assign a single slot, configure interrupt and
>>>> power domain only for 1 slot, not for the all 4 slots.
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>> ---
>>>> v4
>>>> - improve commit message
>>>
>>>> - don't make an ABI break
>>>
>>> What does this mean? Can you please try to explain things a bit more
>>> clearly in your changelogs?
>>>
>>> Also, where is the code that actually makes use of these properties?
>>
>> In v3 patch, I make this property required, make it an ABI break, so in v4, I
>remove it from required, and default to the previous behavior if it is missing.
>
>So say that you dropped line making the property required.
>

Got it, thanks

>>
>> The code patch is sent before, but the dts change is not applicable, so I send
>it separately. The code patch link is as below:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
>> hwork.linuxtv.org%2Fproject%2Flinux-
>media%2Fpatch%2Fcdadb4a23697fdc97d
>>
>ef958c69b12cd00f547212.1685430841.git.ming.qian%40nxp.com%2F&data=0
>5%7
>>
>C01%7Cming.qian%40nxp.com%7C8ea6dd7c90564f331d9108dbbf313eca%7C
>686ea1d
>>
>3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638313990445947012%7CUnkno
>wn%7CTWF
>>
>pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
>I6M
>>
>n0%3D%7C3000%7C%7C%7C&sdata=Hl6F1Th8FvfoGul5nhnvsf%2FKsXqpjZFhp
>r2y3U3C
>> hhw%3D&reserved=0
>>
>> But in the patch, the property name is "slot", not "nxp,slot", I will make
>another patch to fix the property name after this patch is reviewed.
>
>Format your emails properly. It's difficult to read it.
>
>I already NAKed it, I will be NAKing still. Don't embed OS specific into the
>bindings nor into the DTS.
>
>
>Best regards,
>Krzysztof

Hi Krzysztof,
    Do you mean the link is inappropriate or something else? I will avoid links in my emails in the future. I'm sorry to bother you.

Best regards,
Ming Qian

Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 27/09/2023 11:25, Ming Qian wrote:
>> Format your emails properly. It's difficult to read it.
>>
>> I already NAKed it, I will be NAKing still. Don't embed OS specific into the
>> bindings nor into the DTS.
>>
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
>     Do you mean the link is inappropriate or something else? I will avoid links in my emails in the future. I'm sorry to bother you.

Your emails are not wrapped according to email guidelines. You have
simply misconfigured email app.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>Sent: 2023年9月28日 13:24
>To: Ming Qian <ming.qian@nxp.com>; Conor Dooley <conor@kernel.org>
>Cc: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>;
>robh+dt@kernel.org; shawnguo@kernel.org;
>krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot
>for imx jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On 27/09/2023 11:25, Ming Qian wrote:
>>> Format your emails properly. It's difficult to read it.
>>>
>>> I already NAKed it, I will be NAKing still. Don't embed OS specific
>>> into the bindings nor into the DTS.
>>>
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Hi Krzysztof,
>>     Do you mean the link is inappropriate or something else? I will avoid links
>in my emails in the future. I'm sorry to bother you.
>
>Your emails are not wrapped according to email guidelines. You have simply
>misconfigured email app.
>
>Best regards,
>Krzysztof

Hi Krzysztof,

I have made my outlook content-type text/plain, and I checked Documentation/process/email-clients.rst, it just say Thunderbird is an Outlook clone that likes to mangle text, then give some thunderbird extensions to use of an external editor. No more advice on outlook, maybe outlook is unsuited. But I only have outlook email. So I will only use plain text and avoid things like links.

Best regards,
Ming

Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 26/09/2023 12:10, Ming Qian wrote:
> There are total 4 slots available in the IP, and we only need to use one
> slot in one os, assign a single slot, configure interrupt and power
> domain only for 1 slot, not for the all 4 slots.
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
> v4
> - improve commit message
> - don't make an ABI break
> v3
> - add vender prefix, change property slot to nxp,slot
> - add type for property slot
> 
>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> index 3d9d1db37040..4bcfc815c894 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> @@ -32,19 +32,26 @@ properties:
>      maxItems: 1
>  
>    interrupts:
> -    description: |
> -      There are 4 slots available in the IP, which the driver may use
> -      If a certain slot is used, it should have an associated interrupt
> -      The interrupt with index i is assumed to be for slot i
> -    minItems: 1               # At least one slot is needed by the driver
> -    maxItems: 4               # The IP has 4 slots available for use
> +    description:
> +      Interrupt number for slot
> +    maxItems: 1
>  
>    power-domains:
>      description:
>        List of phandle and PM domain specifier as documented in
>        Documentation/devicetree/bindings/power/power_domain.txt
> -    minItems: 2               # Wrapper and 1 slot
> -    maxItems: 5               # Wrapper and 4 slots
> +    minItems: 1               # VPUMIX
> +    maxItems: 2               # Wrapper and 1 slot
> +
> +  nxp,slot:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Integer number of slot index used. There are 4 slots available in the IP,
> +      and driver can use a certain slot, it should have an associated interrupt
> +      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
> +      is used by default.
> +    minimum: 0
> +    maximum: 3

NAK, you still did not answer concerns why this is needed and justified.
I keep asking and you keep ignoring...

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>Sent: 2023年9月27日 16:06
>To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
><mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for
>imx jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On 26/09/2023 12:10, Ming Qian wrote:
>> There are total 4 slots available in the IP, and we only need to use
>> one slot in one os, assign a single slot, configure interrupt and
>> power domain only for 1 slot, not for the all 4 slots.
>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>> v4
>> - improve commit message
>> - don't make an ABI break
>> v3
>> - add vender prefix, change property slot to nxp,slot
>> - add type for property slot
>>
>>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>>  1 file changed, 21 insertions(+), 24 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> index 3d9d1db37040..4bcfc815c894 100644
>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> @@ -32,19 +32,26 @@ properties:
>>      maxItems: 1
>>
>>    interrupts:
>> -    description: |
>> -      There are 4 slots available in the IP, which the driver may use
>> -      If a certain slot is used, it should have an associated interrupt
>> -      The interrupt with index i is assumed to be for slot i
>> -    minItems: 1               # At least one slot is needed by the driver
>> -    maxItems: 4               # The IP has 4 slots available for use
>> +    description:
>> +      Interrupt number for slot
>> +    maxItems: 1
>>
>>    power-domains:
>>      description:
>>        List of phandle and PM domain specifier as documented in
>>        Documentation/devicetree/bindings/power/power_domain.txt
>> -    minItems: 2               # Wrapper and 1 slot
>> -    maxItems: 5               # Wrapper and 4 slots
>> +    minItems: 1               # VPUMIX
>> +    maxItems: 2               # Wrapper and 1 slot
>> +
>> +  nxp,slot:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Integer number of slot index used. There are 4 slots available in the IP,
>> +      and driver can use a certain slot, it should have an associated interrupt
>> +      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
>> +      is used by default.
>> +    minimum: 0
>> +    maximum: 3
>
>NAK, you still did not answer concerns why this is needed and justified.
>I keep asking and you keep ignoring...
>
>Best regards,
>Krzysztof

Hi Krzysztof,

> Nothing explains what is a slot and nothing explains why do you need this property.

I thought I had answered that question, but seems you don't agree with that explanation. 
Would the following description be any better?

The slot is the IP used to share the jpeg engine across multiple VMS and os, there are 4 slots available in the IP. We can use one slot in linux. The slot index indicates which slot we choose. If not specified, 0 is used by default.

Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 27/09/2023 11:10, Ming Qian wrote:
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 2023年9月27日 16:06
>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for
>> imx jpeg encoder/decoder
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report
>> this email' button
>>
>>
>> On 26/09/2023 12:10, Ming Qian wrote:
>>> There are total 4 slots available in the IP, and we only need to use
>>> one slot in one os, assign a single slot, configure interrupt and
>>> power domain only for 1 slot, not for the all 4 slots.
>>>
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>> v4
>>> - improve commit message
>>> - don't make an ABI break
>>> v3
>>> - add vender prefix, change property slot to nxp,slot
>>> - add type for property slot
>>>
>>>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>>>  1 file changed, 21 insertions(+), 24 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> index 3d9d1db37040..4bcfc815c894 100644
>>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> @@ -32,19 +32,26 @@ properties:
>>>      maxItems: 1
>>>
>>>    interrupts:
>>> -    description: |
>>> -      There are 4 slots available in the IP, which the driver may use
>>> -      If a certain slot is used, it should have an associated interrupt
>>> -      The interrupt with index i is assumed to be for slot i
>>> -    minItems: 1               # At least one slot is needed by the driver
>>> -    maxItems: 4               # The IP has 4 slots available for use
>>> +    description:
>>> +      Interrupt number for slot
>>> +    maxItems: 1
>>>
>>>    power-domains:
>>>      description:
>>>        List of phandle and PM domain specifier as documented in
>>>        Documentation/devicetree/bindings/power/power_domain.txt
>>> -    minItems: 2               # Wrapper and 1 slot
>>> -    maxItems: 5               # Wrapper and 4 slots
>>> +    minItems: 1               # VPUMIX
>>> +    maxItems: 2               # Wrapper and 1 slot
>>> +
>>> +  nxp,slot:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Integer number of slot index used. There are 4 slots available in the IP,
>>> +      and driver can use a certain slot, it should have an associated interrupt
>>> +      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
>>> +      is used by default.
>>> +    minimum: 0
>>> +    maximum: 3
>>
>> NAK, you still did not answer concerns why this is needed and justified.
>> I keep asking and you keep ignoring...
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
> 
>> Nothing explains what is a slot and nothing explains why do you need this property.
> 
> I thought I had answered that question, but seems you don't agree with that explanation. 
> Would the following description be any better?

No, you didn't. I don't see any explanation in the commit msg. You just
say you have "4 slots".

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>Sent: 2023年9月28日 13:07
>To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
><mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot
>for imx jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On 27/09/2023 11:10, Ming Qian wrote:
>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Sent: 2023年9月27日 16:06
>>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org;
>shawnguo@kernel.org
>>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg:
>>> Assign slot for imx jpeg encoder/decoder
>>>
>>> Caution: This is an external email. Please take care when clicking
>>> links or opening attachments. When in doubt, report the message using
>>> the 'Report this email' button
>>>
>>>
>>> On 26/09/2023 12:10, Ming Qian wrote:
>>>> There are total 4 slots available in the IP, and we only need to use
>>>> one slot in one os, assign a single slot, configure interrupt and
>>>> power domain only for 1 slot, not for the all 4 slots.
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>> ---
>>>> v4
>>>> - improve commit message
>>>> - don't make an ABI break
>>>> v3
>>>> - add vender prefix, change property slot to nxp,slot
>>>> - add type for property slot
>>>>
>>>>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>>>>  1 file changed, 21 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>> index 3d9d1db37040..4bcfc815c894 100644
>>>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>> @@ -32,19 +32,26 @@ properties:
>>>>      maxItems: 1
>>>>
>>>>    interrupts:
>>>> -    description: |
>>>> -      There are 4 slots available in the IP, which the driver may use
>>>> -      If a certain slot is used, it should have an associated interrupt
>>>> -      The interrupt with index i is assumed to be for slot i
>>>> -    minItems: 1               # At least one slot is needed by the driver
>>>> -    maxItems: 4               # The IP has 4 slots available for use
>>>> +    description:
>>>> +      Interrupt number for slot
>>>> +    maxItems: 1
>>>>
>>>>    power-domains:
>>>>      description:
>>>>        List of phandle and PM domain specifier as documented in
>>>>        Documentation/devicetree/bindings/power/power_domain.txt
>>>> -    minItems: 2               # Wrapper and 1 slot
>>>> -    maxItems: 5               # Wrapper and 4 slots
>>>> +    minItems: 1               # VPUMIX
>>>> +    maxItems: 2               # Wrapper and 1 slot
>>>> +
>>>> +  nxp,slot:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      Integer number of slot index used. There are 4 slots available in the IP,
>>>> +      and driver can use a certain slot, it should have an associated
>interrupt
>>>> +      and power-domain. In theory, it supports 4 os or vm. If not specified,
>0
>>>> +      is used by default.
>>>> +    minimum: 0
>>>> +    maximum: 3
>>>
>>> NAK, you still did not answer concerns why this is needed and justified.
>>> I keep asking and you keep ignoring...
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Hi Krzysztof,
>>
>>> Nothing explains what is a slot and nothing explains why do you need this
>property.
>>
>> I thought I had answered that question, but seems you don't agree with that
>explanation.
>> Would the following description be any better?
>
>No, you didn't. I don't see any explanation in the commit msg. You just say
>you have "4 slots".
>
>Best regards,
>Krzysztof

Hi Krzysztof,

I check the our document again, it does not give a standard definition too. I'll explain it as I understand it. This IP includes a jpeg wrapper, the wrapper is working on descriptor based manner. It support up to 4 slots, each slots can have its own chained descriptors. The purpose is to share the jpeg engine across multiple VMS and os, It's a bit like vm partitioning. We just want to assign 1 slot to linux, Then, theoretically, it can support up to 3 VMS.
The slot index indicates which slot we choose, driver will use the slot index to configure. If not specified, 0 is used by default.
Currently driver only use the slot 0 by default, but it enable the power-domain and request irq to all the 4 slots.  there has been no real negative impact, as we have no practical application to use other slots yet. But it does cause trouble when we want to use another slot in a vm.

Best regards,
Ming

Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 28/09/2023 11:42, Ming Qian wrote:
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 2023年9月28日 13:07
>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot
>> for imx jpeg encoder/decoder
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report
>> this email' button
>>
>>
>> On 27/09/2023 11:10, Ming Qian wrote:
>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Sent: 2023年9月27日 16:06
>>>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>>>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org;
>> shawnguo@kernel.org
>>>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>>>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>>>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>>>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>>>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg:
>>>> Assign slot for imx jpeg encoder/decoder
>>>>
>>>> Caution: This is an external email. Please take care when clicking
>>>> links or opening attachments. When in doubt, report the message using
>>>> the 'Report this email' button
>>>>
>>>>
>>>> On 26/09/2023 12:10, Ming Qian wrote:
>>>>> There are total 4 slots available in the IP, and we only need to use
>>>>> one slot in one os, assign a single slot, configure interrupt and
>>>>> power domain only for 1 slot, not for the all 4 slots.
>>>>>
>>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>>> ---
>>>>> v4
>>>>> - improve commit message
>>>>> - don't make an ABI break
>>>>> v3
>>>>> - add vender prefix, change property slot to nxp,slot
>>>>> - add type for property slot
>>>>>
>>>>>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>>>>>  1 file changed, 21 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>> index 3d9d1db37040..4bcfc815c894 100644
>>>>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>> @@ -32,19 +32,26 @@ properties:
>>>>>      maxItems: 1
>>>>>
>>>>>    interrupts:
>>>>> -    description: |
>>>>> -      There are 4 slots available in the IP, which the driver may use
>>>>> -      If a certain slot is used, it should have an associated interrupt
>>>>> -      The interrupt with index i is assumed to be for slot i
>>>>> -    minItems: 1               # At least one slot is needed by the driver
>>>>> -    maxItems: 4               # The IP has 4 slots available for use
>>>>> +    description:
>>>>> +      Interrupt number for slot
>>>>> +    maxItems: 1
>>>>>
>>>>>    power-domains:
>>>>>      description:
>>>>>        List of phandle and PM domain specifier as documented in
>>>>>        Documentation/devicetree/bindings/power/power_domain.txt
>>>>> -    minItems: 2               # Wrapper and 1 slot
>>>>> -    maxItems: 5               # Wrapper and 4 slots
>>>>> +    minItems: 1               # VPUMIX
>>>>> +    maxItems: 2               # Wrapper and 1 slot
>>>>> +
>>>>> +  nxp,slot:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description:
>>>>> +      Integer number of slot index used. There are 4 slots available in the IP,
>>>>> +      and driver can use a certain slot, it should have an associated
>> interrupt
>>>>> +      and power-domain. In theory, it supports 4 os or vm. If not specified,
>> 0
>>>>> +      is used by default.
>>>>> +    minimum: 0
>>>>> +    maximum: 3
>>>>
>>>> NAK, you still did not answer concerns why this is needed and justified.
>>>> I keep asking and you keep ignoring...
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> Hi Krzysztof,
>>>
>>>> Nothing explains what is a slot and nothing explains why do you need this
>> property.
>>>
>>> I thought I had answered that question, but seems you don't agree with that
>> explanation.
>>> Would the following description be any better?
>>
>> No, you didn't. I don't see any explanation in the commit msg. You just say
>> you have "4 slots".
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
> 
> I check the our document again, it does not give a standard definition too. I'll explain it as I understand it. This IP includes a jpeg wrapper, the wrapper is working on descriptor based manner. It support up to 4 slots, each slots can have its own chained descriptors. The purpose is to share the jpeg engine across multiple VMS and os, It's a bit like vm partitioning. We just want to assign 1 slot to linux, Then, theoretically, it can support up to 3 VMS.
> The slot index indicates which slot we choose, driver will use the slot index to configure. If not specified, 0 is used by default.
> Currently driver only use the slot 0 by default, but it enable the power-domain and request irq to all the 4 slots.  there has been no real negative impact, as we have no practical application to use other slots yet. But it does cause trouble when we want to use another slot in a vm.

Which parts of hardware are common and which are shared?

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>Sent: 2023年9月30日 22:40
>To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
><mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot
>for imx jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On 28/09/2023 11:42, Ming Qian wrote:
>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Sent: 2023年9月28日 13:07
>>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org;
>shawnguo@kernel.org
>>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>> Subject: Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg:
>>> Assign slot for imx jpeg encoder/decoder
>>>
>>> Caution: This is an external email. Please take care when clicking
>>> links or opening attachments. When in doubt, report the message using
>>> the 'Report this email' button
>>>
>>>
>>> On 27/09/2023 11:10, Ming Qian wrote:
>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>> Sent: 2023年9月27日 16:06
>>>>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>>>>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org;
>>> shawnguo@kernel.org
>>>>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>>>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl;
>>>>> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
>>>>> X.H. Bao <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>;
>>>>> Tao Jiang <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>>>>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>>>>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>>> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg:
>>>>> Assign slot for imx jpeg encoder/decoder
>>>>>
>>>>> Caution: This is an external email. Please take care when clicking
>>>>> links or opening attachments. When in doubt, report the message
>>>>> using the 'Report this email' button
>>>>>
>>>>>
>>>>> On 26/09/2023 12:10, Ming Qian wrote:
>>>>>> There are total 4 slots available in the IP, and we only need to
>>>>>> use one slot in one os, assign a single slot, configure interrupt
>>>>>> and power domain only for 1 slot, not for the all 4 slots.
>>>>>>
>>>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>>>> ---
>>>>>> v4
>>>>>> - improve commit message
>>>>>> - don't make an ABI break
>>>>>> v3
>>>>>> - add vender prefix, change property slot to nxp,slot
>>>>>> - add type for property slot
>>>>>>
>>>>>>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>>>>>>  1 file changed, 21 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>>> index 3d9d1db37040..4bcfc815c894 100644
>>>>>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>>>>> @@ -32,19 +32,26 @@ properties:
>>>>>>      maxItems: 1
>>>>>>
>>>>>>    interrupts:
>>>>>> -    description: |
>>>>>> -      There are 4 slots available in the IP, which the driver may use
>>>>>> -      If a certain slot is used, it should have an associated interrupt
>>>>>> -      The interrupt with index i is assumed to be for slot i
>>>>>> -    minItems: 1               # At least one slot is needed by the driver
>>>>>> -    maxItems: 4               # The IP has 4 slots available for use
>>>>>> +    description:
>>>>>> +      Interrupt number for slot
>>>>>> +    maxItems: 1
>>>>>>
>>>>>>    power-domains:
>>>>>>      description:
>>>>>>        List of phandle and PM domain specifier as documented in
>>>>>>        Documentation/devicetree/bindings/power/power_domain.txt
>>>>>> -    minItems: 2               # Wrapper and 1 slot
>>>>>> -    maxItems: 5               # Wrapper and 4 slots
>>>>>> +    minItems: 1               # VPUMIX
>>>>>> +    maxItems: 2               # Wrapper and 1 slot
>>>>>> +
>>>>>> +  nxp,slot:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    description:
>>>>>> +      Integer number of slot index used. There are 4 slots available in the
>IP,
>>>>>> +      and driver can use a certain slot, it should have an
>>>>>> + associated
>>> interrupt
>>>>>> +      and power-domain. In theory, it supports 4 os or vm. If not
>>>>>> + specified,
>>> 0
>>>>>> +      is used by default.
>>>>>> +    minimum: 0
>>>>>> +    maximum: 3
>>>>>
>>>>> NAK, you still did not answer concerns why this is needed and justified.
>>>>> I keep asking and you keep ignoring...
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>
>>>> Hi Krzysztof,
>>>>
>>>>> Nothing explains what is a slot and nothing explains why do you
>>>>> need this
>>> property.
>>>>
>>>> I thought I had answered that question, but seems you don't agree
>>>> with that
>>> explanation.
>>>> Would the following description be any better?
>>>
>>> No, you didn't. I don't see any explanation in the commit msg. You
>>> just say you have "4 slots".
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Hi Krzysztof,
>>
>> I check the our document again, it does not give a standard definition too. I'll
>explain it as I understand it. This IP includes a jpeg wrapper, the wrapper is
>working on descriptor based manner. It support up to 4 slots, each slots can
>have its own chained descriptors. The purpose is to share the jpeg engine
>across multiple VMS and os, It's a bit like vm partitioning. We just want to
>assign 1 slot to linux, Then, theoretically, it can support up to 3 VMS.
>> The slot index indicates which slot we choose, driver will use the slot index
>to configure. If not specified, 0 is used by default.
>> Currently driver only use the slot 0 by default, but it enable the power-
>domain and request irq to all the 4 slots.  there has been no real negative
>impact, as we have no practical application to use other slots yet. But it does
>cause trouble when we want to use another slot in a vm.
>
>Which parts of hardware are common and which are shared?
>
>Best regards,
>Krzysztof

Hi Krzysztof,
    The jpeg codec engine are shared, but we don't configure the engine directly.
We can fill some descriptors to encode or decode one jpeg frame. Then configure the descriptors to certain slot register.
The jpeg wrapper will schedule between different slots. When some slot is finished, the slot interrupt will be triggered.

Best regards,
MIng

Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 27/09/2023 11:10, Ming Qian wrote:
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 2023年9月27日 16:06
>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for
>> imx jpeg encoder/decoder
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report
>> this email' button
>>
>>
>> On 26/09/2023 12:10, Ming Qian wrote:
>>> There are total 4 slots available in the IP, and we only need to use
>>> one slot in one os, assign a single slot, configure interrupt and
>>> power domain only for 1 slot, not for the all 4 slots.
>>>
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>> v4
>>> - improve commit message
>>> - don't make an ABI break
>>> v3
>>> - add vender prefix, change property slot to nxp,slot
>>> - add type for property slot
>>>
>>>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>>>  1 file changed, 21 insertions(+), 24 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> index 3d9d1db37040..4bcfc815c894 100644
>>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>>> @@ -32,19 +32,26 @@ properties:
>>>      maxItems: 1
>>>
>>>    interrupts:
>>> -    description: |
>>> -      There are 4 slots available in the IP, which the driver may use
>>> -      If a certain slot is used, it should have an associated interrupt
>>> -      The interrupt with index i is assumed to be for slot i
>>> -    minItems: 1               # At least one slot is needed by the driver
>>> -    maxItems: 4               # The IP has 4 slots available for use
>>> +    description:
>>> +      Interrupt number for slot
>>> +    maxItems: 1
>>>
>>>    power-domains:
>>>      description:
>>>        List of phandle and PM domain specifier as documented in
>>>        Documentation/devicetree/bindings/power/power_domain.txt
>>> -    minItems: 2               # Wrapper and 1 slot
>>> -    maxItems: 5               # Wrapper and 4 slots
>>> +    minItems: 1               # VPUMIX
>>> +    maxItems: 2               # Wrapper and 1 slot
>>> +
>>> +  nxp,slot:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Integer number of slot index used. There are 4 slots available in the IP,
>>> +      and driver can use a certain slot, it should have an associated interrupt
>>> +      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
>>> +      is used by default.
>>> +    minimum: 0
>>> +    maximum: 3
>>
>> NAK, you still did not answer concerns why this is needed and justified.
>> I keep asking and you keep ignoring...
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
> 
>> Nothing explains what is a slot and nothing explains why do you need this property.
> 
> I thought I had answered that question, but seems you don't agree with that explanation. 
> Would the following description be any better?

No, you didn't. I don't see any explanation in the commit msg. You just
say you have "4 slots".

Best regards,
Krzysztof

Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Rob Herring]

On Wed, Sep 27, 2023 at 09:10:52AM +0000, Ming Qian wrote:
> >From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >
> >On 26/09/2023 12:10, Ming Qian wrote:
> >> There are total 4 slots available in the IP, and we only need to use
> >> one slot in one os, assign a single slot, configure interrupt and
> >> power domain only for 1 slot, not for the all 4 slots.
> >>
> >> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> >> ---
> >> v4
> >> - improve commit message
> >> - don't make an ABI break
> >> v3
> >> - add vender prefix, change property slot to nxp,slot
> >> - add type for property slot
> >>
> >>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
> >>  1 file changed, 21 insertions(+), 24 deletions(-)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> >> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> >> index 3d9d1db37040..4bcfc815c894 100644
> >> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> >> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
> >> @@ -32,19 +32,26 @@ properties:
> >>      maxItems: 1
> >>
> >>    interrupts:
> >> -    description: |
> >> -      There are 4 slots available in the IP, which the driver may use
> >> -      If a certain slot is used, it should have an associated interrupt
> >> -      The interrupt with index i is assumed to be for slot i
> >> -    minItems: 1               # At least one slot is needed by the driver
> >> -    maxItems: 4               # The IP has 4 slots available for use
> >> +    description:
> >> +      Interrupt number for slot
> >> +    maxItems: 1
> >>
> >>    power-domains:
> >>      description:
> >>        List of phandle and PM domain specifier as documented in
> >>        Documentation/devicetree/bindings/power/power_domain.txt
> >> -    minItems: 2               # Wrapper and 1 slot
> >> -    maxItems: 5               # Wrapper and 4 slots
> >> +    minItems: 1               # VPUMIX
> >> +    maxItems: 2               # Wrapper and 1 slot
> >> +
> >> +  nxp,slot:
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    description:
> >> +      Integer number of slot index used. There are 4 slots available in the IP,
> >> +      and driver can use a certain slot, it should have an associated interrupt
> >> +      and power-domain. In theory, it supports 4 os or vm. If not specified, 0
> >> +      is used by default.
> >> +    minimum: 0
> >> +    maximum: 3
> >
> >NAK, you still did not answer concerns why this is needed and justified.
> >I keep asking and you keep ignoring...
> >
> >Best regards,
> >Krzysztof
> 
> Hi Krzysztof,
> 
> > Nothing explains what is a slot and nothing explains why do you need this property.
> 
> I thought I had answered that question, but seems you don't agree with that explanation. 
> Would the following description be any better?
> 
> The slot is the IP used to share the jpeg engine across multiple VMS 
> and os, there are 4 slots available in the IP. We can use one slot in 
> linux. The slot index indicates which slot we choose. If not 
> specified, 0 is used by default.

If you have to modify the DT to update 'nxp,slot', then you can modify 
the DT to just expose the parts of the h/w for the assigned slot. For 
example, only put slot 1's power-domains entry in the DT when slot 1 is 
assigned.

As far as formatting your emails, talk to your NXP colleagues. They've 
figured it out. If you've configured git-send-email successfully, then 
using mutt should be possible. You can use 'b4' to get the thread mbox, 
and read it in mutt.

Rob

RE: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Rob Herring <robh@kernel.org>
>Sent: 2023年9月29日 0:47
>To: Ming Qian <ming.qian@nxp.com>
>Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Mirela Rabulea (OSS)
><mirela.rabulea@oss.nxp.com>; shawnguo@kernel.org;
>krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [EXT] Re: [PATCH v4 2/2] dt-bindings: media: imx-jpeg: Assign slot
>for imx jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On Wed, Sep 27, 2023 at 09:10:52AM +0000, Ming Qian wrote:
>> >From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> >
>> >On 26/09/2023 12:10, Ming Qian wrote:
>> >> There are total 4 slots available in the IP, and we only need to
>> >> use one slot in one os, assign a single slot, configure interrupt
>> >> and power domain only for 1 slot, not for the all 4 slots.
>> >>
>> >> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> >> ---
>> >> v4
>> >> - improve commit message
>> >> - don't make an ABI break
>> >> v3
>> >> - add vender prefix, change property slot to nxp,slot
>> >> - add type for property slot
>> >>
>> >>  .../bindings/media/nxp,imx8-jpeg.yaml         | 45 +++++++++----------
>> >>  1 file changed, 21 insertions(+), 24 deletions(-)
>> >>
>> >> diff --git
>> >> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> >> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> >> index 3d9d1db37040..4bcfc815c894 100644
>> >> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> >> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>> >> @@ -32,19 +32,26 @@ properties:
>> >>      maxItems: 1
>> >>
>> >>    interrupts:
>> >> -    description: |
>> >> -      There are 4 slots available in the IP, which the driver may use
>> >> -      If a certain slot is used, it should have an associated interrupt
>> >> -      The interrupt with index i is assumed to be for slot i
>> >> -    minItems: 1               # At least one slot is needed by the driver
>> >> -    maxItems: 4               # The IP has 4 slots available for use
>> >> +    description:
>> >> +      Interrupt number for slot
>> >> +    maxItems: 1
>> >>
>> >>    power-domains:
>> >>      description:
>> >>        List of phandle and PM domain specifier as documented in
>> >>        Documentation/devicetree/bindings/power/power_domain.txt
>> >> -    minItems: 2               # Wrapper and 1 slot
>> >> -    maxItems: 5               # Wrapper and 4 slots
>> >> +    minItems: 1               # VPUMIX
>> >> +    maxItems: 2               # Wrapper and 1 slot
>> >> +
>> >> +  nxp,slot:
>> >> +    $ref: /schemas/types.yaml#/definitions/uint32
>> >> +    description:
>> >> +      Integer number of slot index used. There are 4 slots available in the
>IP,
>> >> +      and driver can use a certain slot, it should have an associated
>interrupt
>> >> +      and power-domain. In theory, it supports 4 os or vm. If not specified,
>0
>> >> +      is used by default.
>> >> +    minimum: 0
>> >> +    maximum: 3
>> >
>> >NAK, you still did not answer concerns why this is needed and justified.
>> >I keep asking and you keep ignoring...
>> >
>> >Best regards,
>> >Krzysztof
>>
>> Hi Krzysztof,
>>
>> > Nothing explains what is a slot and nothing explains why do you need this
>property.
>>
>> I thought I had answered that question, but seems you don't agree with that
>explanation.
>> Would the following description be any better?
>>
>> The slot is the IP used to share the jpeg engine across multiple VMS
>> and os, there are 4 slots available in the IP. We can use one slot in
>> linux. The slot index indicates which slot we choose. If not
>> specified, 0 is used by default.
>

Hi Rob

>If you have to modify the DT to update 'nxp,slot', then you can modify the DT
>to just expose the parts of the h/w for the assigned slot. For example, only put
>slot 1's power-domains entry in the DT when slot 1 is assigned.
>

That's what this patch is trying to do. 
What should I do next?
Prepare the next version patch and modify the commit message after reaching an agreement?

>As far as formatting your emails, talk to your NXP colleagues. They've figured it
>out. If you've configured git-send-email successfully, then using mutt should
>be possible. You can use 'b4' to get the thread mbox, and read it in mutt.
>
>Rob

I'll talk with someone in nxp, and I'll try to use mutt.
Thanks for your advice.

Best regards,
Ming

Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 26/09/2023 12:09, Ming Qian wrote:
> There are total 4 slots available in the IP, and we only need to use one
> slot in one os, assign a single slot for imx jpeg device node, configure
> interrupt and power domain only for 1 slot, not for the all 4 slots.

DTS is independent of OS, so same DTS will be used by two OSes. This
patch is not correct in that matter. Explanation is not proper
justification.

> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
> v4
> - improve the commit message
> v3
> - add vender prefix, change property slot to nxp,slot

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>Sent: 2023年9月27日 16:11
>To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
><mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: [EXT] Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx
>jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On 26/09/2023 12:09, Ming Qian wrote:
>> There are total 4 slots available in the IP, and we only need to use
>> one slot in one os, assign a single slot for imx jpeg device node,
>> configure interrupt and power domain only for 1 slot, not for the all 4 slots.
>
>DTS is independent of OS, so same DTS will be used by two OSes. This patch is
>not correct in that matter. Explanation is not proper justification.
>

Hi Krzysztof,

I agree that DTS is independent of OS, otherwise we need to choose the slot index by hard code in driver.
I think it's just the reason why we need this patch.
We have different platforms that use this IP, we need to configure the slot in dts.

Would you please give some guide about proper justification?

Thanks
Ming Qian

>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>> v4
>> - improve the commit message
>> v3
>> - add vender prefix, change property slot to nxp,slot
>
>Best regards,
>Krzysztof

Re: [EXT] Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx jpeg encoder/decoder

[Krzysztof Kozlowski]

On 27/09/2023 11:19, Ming Qian wrote:
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 2023年9月27日 16:11
>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: [EXT] Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx
>> jpeg encoder/decoder
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report
>> this email' button
>>
>>
>> On 26/09/2023 12:09, Ming Qian wrote:
>>> There are total 4 slots available in the IP, and we only need to use
>>> one slot in one os, assign a single slot for imx jpeg device node,
>>> configure interrupt and power domain only for 1 slot, not for the all 4 slots.
>>
>> DTS is independent of OS, so same DTS will be used by two OSes. This patch is
>> not correct in that matter. Explanation is not proper justification.
>>
> 
> Hi Krzysztof,
> 
> I agree that DTS is independent of OS, otherwise we need to choose the slot index by hard code in driver.
> I think it's just the reason why we need this patch.
> We have different platforms that use this IP, we need to configure the slot in dts.

I still do not know what is the "slot". VM partitioning? Some pinmux
configuration?

Other platforms will use the same DTS, so the same slot and you are back
at the same problem... DTS should be the full description of the
hardware, thus removing interrupts is not "full description" anymore.

> 
> Would you please give some guide about proper justification?
Describe architecture and then propose generic solution matching
multiple vendors and IP blocks, not something solving only this one problem.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for imx jpeg encoder/decoder

[Ming Qian]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>Sent: 2023年9月28日 13:03
>To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
><mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org; shawnguo@kernel.org
>Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
><xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
><tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [EXT] Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign slot for
>imx jpeg encoder/decoder
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>On 27/09/2023 11:19, Ming Qian wrote:
>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Sent: 2023年9月27日 16:11
>>> To: Ming Qian <ming.qian@nxp.com>; Mirela Rabulea (OSS)
>>> <mirela.rabulea@oss.nxp.com>; robh+dt@kernel.org;
>shawnguo@kernel.org
>>> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>> mchehab@kernel.org; hverkuil-cisco@xs4all.nl; s.hauer@pengutronix.de;
>>> kernel@pengutronix.de; festevam@gmail.com; X.H. Bao
>>> <xiahong.bao@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; Tao Jiang
>>> <tao.jiang_2@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>>> devicetree@vger.kernel.org; linux-media@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>> Subject: [EXT] Re: [PATCH v4 1/2] arm64: dts: imx8-ss-img: Assign
>>> slot for imx jpeg encoder/decoder
>>>
>>> Caution: This is an external email. Please take care when clicking
>>> links or opening attachments. When in doubt, report the message using
>>> the 'Report this email' button
>>>
>>>
>>> On 26/09/2023 12:09, Ming Qian wrote:
>>>> There are total 4 slots available in the IP, and we only need to use
>>>> one slot in one os, assign a single slot for imx jpeg device node,
>>>> configure interrupt and power domain only for 1 slot, not for the all 4
>slots.
>>>
>>> DTS is independent of OS, so same DTS will be used by two OSes. This
>>> patch is not correct in that matter. Explanation is not proper justification.
>>>
>>
>> Hi Krzysztof,
>>
>> I agree that DTS is independent of OS, otherwise we need to choose the slot
>index by hard code in driver.
>> I think it's just the reason why we need this patch.
>> We have different platforms that use this IP, we need to configure the slot in
>dts.
>
>I still do not know what is the "slot". VM partitioning? Some pinmux
>configuration?
>
>Other platforms will use the same DTS, so the same slot and you are back at
>the same problem... DTS should be the full description of the hardware, thus
>removing interrupts is not "full description" anymore.
>

Hi Krzysztof,
    I have tried to explain the slot in the previous mail, although I'm not sure I made that clear. I think I need an example here. In our imx8q platform, we just want to assign slot 0 to linux, but in imx9 platform, we may want to assign slot 0 to linux, but slot 1 to a vm. So we don't want to enable power of slot 1 in linux.

Best regards,
Ming


>>
>> Would you please give some guide about proper justification?
>Describe architecture and then propose generic solution matching multiple
>vendors and IP blocks, not something solving only this one problem.
>
>Best regards,
>Krzysztof