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 <[email protected]>
> ---
> 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
>
>From: Conor Dooley <[email protected]>
>Sent: 2023??9??26?? 21:26
>To: Ming Qian <[email protected]>
>Cc: Mirela Rabulea (OSS) <[email protected]>;
>[email protected]; [email protected];
>[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 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 <[email protected]>
>> ---
>> 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
>>
On 27/09/2023 04:10, Ming Qian wrote:
>> From: Conor Dooley <[email protected]>
>> Sent: 2023年9月26日 21:26
>> To: Ming Qian <[email protected]>
>> Cc: Mirela Rabulea (OSS) <[email protected]>;
>> [email protected]; [email protected];
>> [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 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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> >> Sent: 2023年9月26日 21:26
> >> To: Ming Qian <[email protected]>
> >> Cc: Mirela Rabulea (OSS) <[email protected]>;
> >> [email protected]; [email protected];
> >> [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 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 <[email protected]>
> >>> ---
> >>> 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.