[v7,02/16] dt-bindings: media: mediatek: mdp3: merge the indentical RDMA under display

  To simplify maintenance and avoid branches, the identical component
should be merged and placed in the path belonging to the MDP
(from display/* to media/*).

In addition, currently only MDP utilizes RDMA through CMDQ, and the
necessary properties for "mediatek,gce-events", and "mboxes" have been
set up for this purpose.
Within DISP, it directly receives component interrupt signals.

Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
---
 .../display/mediatek/mediatek,mdp-rdma.yaml   | 88 -------------------
 .../bindings/media/mediatek,mdp3-rdma.yaml    | 55 +++++++++---
 2 files changed, 45 insertions(+), 98 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,mdp-rdma.yaml
  

Il 12/10/23 10:40, Moudy Ho ha scritto:
> To simplify maintenance and avoid branches, the identical component
> should be merged and placed in the path belonging to the MDP
> (from display/* to media/*).
> 
> In addition, currently only MDP utilizes RDMA through CMDQ, and the
> necessary properties for "mediatek,gce-events", and "mboxes" have been
> set up for this purpose.
> Within DISP, it directly receives component interrupt signals.
> 
> Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>

I agree in that this belongs to bindings/media and not bindings/display, as the
display-specific RDMA component is display/mediatek/mediatek,rdma.yaml.

The merge looks good to me, so...

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
  

On 12/10/2023 10:40, Moudy Ho wrote:

>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mediatek,mt8183-mdp3-rdma
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: RDMA clock
> +            - description: RSZ clock (shared SRAM with RDMA)
> +
> +        mboxes:
> +          items:
> +            - description: used for 1st data pipe from RDMA
> +            - description: used for 2nd data pipe from RDMA

interrupts:
  false

> +
> +      required:
> +        - mboxes
> +        - mediatek,gce-events
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mediatek,mt8195-vdo1-rdma
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: RDMA clock

mboxes: false
mediatek,gce-events: false

I am not so sure it is actually "simpler" to merge these. They are quite
different. You will end up with unmanageable allOf  with a lot of
branches (which supposedly you want to remove).


> +
>  additionalProperties: false
>  
>  examples:

Best regards,
Krzysztof
  

On Fri, 2023-10-13 at 08:46 +0200, Krzysztof Kozlowski wrote:
>  	 

Hi Krzysztof,

Thank you for assisting with the review.

> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 12/10/2023 10:40, Moudy Ho wrote:
> 
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: mediatek,mt8183-mdp3-rdma
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: RDMA clock
> > +            - description: RSZ clock (shared SRAM with RDMA)
> > +
> > +        mboxes:
> > +          items:
> > +            - description: used for 1st data pipe from RDMA
> > +            - description: used for 2nd data pipe from RDMA
> 
> interrupts:
>   false
> 

As Angelo provided additional clarification in [15/16], explaining that
certain conditions in [2/16] and [3/16] were intentionally omitted due
to the need to integrate the same IP with different operations.
Apologies for any inconvenience this has caused you.

> > +
> > +      required:
> > +        - mboxes
> > +        - mediatek,gce-events
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: mediatek,mt8195-vdo1-rdma
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: RDMA clock
> 
> mboxes: false
> mediatek,gce-events: false
> 
> I am not so sure it is actually "simpler" to merge these. They are
> quite
> different. You will end up with unmanageable allOf  with a lot of
> branches (which supposedly you want to remove).
> 
> 

Upon examining the minor hardware changes in MDP for MT8183 and MT8195
RDMA ([3/16]), it appears that branching cannot be avoided. However,
consolidating these changes has the additional advantage of addressing
Rob's concerns from v4. Perhaps we can consider the current changes as
a form of progress.

Sincerely,
Moudy

> > +
> >  additionalProperties: false
> >  
> >  examples:
> 
> Best regards,
> Krzysztof
>
  

Il 18/10/23 05:06, Moudy Ho (何宗原) ha scritto:
> On Fri, 2023-10-13 at 08:46 +0200, Krzysztof Kozlowski wrote:
>>   	
> 
> Hi Krzysztof,
> 
> Thank you for assisting with the review.
> 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>   On 12/10/2023 10:40, Moudy Ho wrote:
>>
>>>   
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: mediatek,mt8183-mdp3-rdma
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          items:
>>> +            - description: RDMA clock
>>> +            - description: RSZ clock (shared SRAM with RDMA)
>>> +
>>> +        mboxes:
>>> +          items:
>>> +            - description: used for 1st data pipe from RDMA
>>> +            - description: used for 2nd data pipe from RDMA
>>
>> interrupts:
>>    false
>>
> 
> As Angelo provided additional clarification in [15/16], explaining that
> certain conditions in [2/16] and [3/16] were intentionally omitted due
> to the need to integrate the same IP with different operations.
> Apologies for any inconvenience this has caused you.
> 

MT8183's MDP3 RDMA interrupt property was omitted in the devicetree that we
have upstream because it was either unused in the driver, or MTK didn't want
to actually use it for reasons, but that SoC *definitely does* have a mdp_rdma0
IRQ and a mdp_rdma1 IRQ.

That's the same for MT8186 and MT8188... and it's probably the same for all
MediaTek SoCs, so interrupts shouldn't be disallowed in this binding.

>>> +
>>> +      required:
>>> +        - mboxes
>>> +        - mediatek,gce-events
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: mediatek,mt8195-vdo1-rdma
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          items:
>>> +            - description: RDMA clock
>>
>> mboxes: false
>> mediatek,gce-events: false
>>
>> I am not so sure it is actually "simpler" to merge these. They are
>> quite
>> different. You will end up with unmanageable allOf  with a lot of
>> branches (which supposedly you want to remove).
>>

It's the same thing as "split"... All of the display and mdp/mdp3 components of
MediaTek SoC do support GCE mailboxes by HW, so it's not limited to "split", but
literally all of them.

Disallowing mboxes and/or mediatek,gce-events on *any* of those is actually wrong.

Cheers,
Angelo

>>
> 
> Upon examining the minor hardware changes in MDP for MT8183 and MT8195
> RDMA ([3/16]), it appears that branching cannot be avoided. However,
> consolidating these changes has the additional advantage of addressing
> Rob's concerns from v4. Perhaps we can consider the current changes as
> a form of progress.
> 
> Sincerely,
> Moudy
> 
>>> +
>>>   additionalProperties: false
>>>   
>>>   examples:
>>
>> Best regards,
>> Krzysztof
>>