2023-09-26 21:17:17

by Ming Qian

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

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 <[email protected]>
---
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


2023-09-26 22:26:18

by Ming Qian

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

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 <[email protected]>
---
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

2023-09-27 09:29:18

by Krzysztof Kozlowski

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

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 <[email protected]>
> ---
> v4
> - improve the commit message
> v3
> - add vender prefix, change property slot to nxp,slot

Best regards,
Krzysztof

2023-09-27 14:20:21

by Krzysztof Kozlowski

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

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 <[email protected]>
> ---
> 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

2023-09-27 18:28:00

by Ming Qian

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

>From: Krzysztof Kozlowski <[email protected]>
>Sent: 2023年9月27日 16:11
>To: Ming Qian <[email protected]>; Mirela Rabulea (OSS)
><[email protected]>; [email protected]; [email protected]
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; X.H. Bao
><[email protected]>; Eagle Zhou <[email protected]>; Tao Jiang
><[email protected]>; dl-linux-imx <[email protected]>;
>[email protected]; [email protected]; linux-
>[email protected]; [email protected]
>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 <[email protected]>
>> ---
>> v4
>> - improve the commit message
>> v3
>> - add vender prefix, change property slot to nxp,slot
>
>Best regards,
>Krzysztof

2023-09-28 10:08:55

by Krzysztof Kozlowski

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

On 27/09/2023 11:19, Ming Qian wrote:
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: 2023年9月27日 16:11
>> To: Ming Qian <[email protected]>; Mirela Rabulea (OSS)
>> <[email protected]>; [email protected]; [email protected]
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; X.H. Bao
>> <[email protected]>; Eagle Zhou <[email protected]>; Tao Jiang
>> <[email protected]>; dl-linux-imx <[email protected]>;
>> [email protected]; [email protected]; linux-
>> [email protected]; [email protected]
>> 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

2023-09-28 16:27:36

by Ming Qian

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

>From: Krzysztof Kozlowski <[email protected]>
>Sent: 2023年9月28日 13:03
>To: Ming Qian <[email protected]>; Mirela Rabulea (OSS)
><[email protected]>; [email protected]; [email protected]
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; X.H. Bao
><[email protected]>; Eagle Zhou <[email protected]>; Tao Jiang
><[email protected]>; dl-linux-imx <[email protected]>;
>[email protected]; [email protected]; linux-
>[email protected]; [email protected]
>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 <[email protected]>
>>> Sent: 2023年9月27日 16:11
>>> To: Ming Qian <[email protected]>; Mirela Rabulea (OSS)
>>> <[email protected]>; [email protected];
>[email protected]
>>> Cc: [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; X.H. Bao
>>> <[email protected]>; Eagle Zhou <[email protected]>; Tao Jiang
>>> <[email protected]>; dl-linux-imx <[email protected]>;
>>> [email protected]; [email protected]; linux-
>>> [email protected]; [email protected]
>>> 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