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 <[email protected]>
---
.../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
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,mdp-rdma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,mdp-rdma.yaml
deleted file mode 100644
index dd12e2ff685c..000000000000
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,mdp-rdma.yaml
+++ /dev/null
@@ -1,88 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/display/mediatek/mediatek,mdp-rdma.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: MediaTek MDP RDMA
-
-maintainers:
- - Chun-Kuang Hu <[email protected]>
- - Philipp Zabel <[email protected]>
-
-description:
- The MediaTek MDP RDMA stands for Read Direct Memory Access.
- It provides real time data to the back-end panel driver, such as DSI,
- DPI and DP_INTF.
- It contains one line buffer to store the sufficient pixel data.
- RDMA device node must be siblings to the central MMSYS_CONFIG node.
- For a description of the MMSYS_CONFIG binding, see
- Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml for details.
-
-properties:
- compatible:
- const: mediatek,mt8195-vdo1-rdma
-
- reg:
- maxItems: 1
-
- interrupts:
- maxItems: 1
-
- power-domains:
- maxItems: 1
-
- clocks:
- items:
- - description: RDMA Clock
-
- iommus:
- maxItems: 1
-
- mediatek,gce-client-reg:
- description:
- The register of display function block to be set by gce. There are 4 arguments,
- such as gce node, subsys id, offset and register size. The subsys id that is
- mapping to the register of display function blocks is defined in the gce header
- include/dt-bindings/gce/<chip>-gce.h of each chips.
- $ref: /schemas/types.yaml#/definitions/phandle-array
- items:
- items:
- - description: phandle of GCE
- - description: GCE subsys id
- - description: register offset
- - description: register size
- maxItems: 1
-
-required:
- - compatible
- - reg
- - power-domains
- - clocks
- - iommus
- - mediatek,gce-client-reg
-
-additionalProperties: false
-
-examples:
- - |
- #include <dt-bindings/interrupt-controller/arm-gic.h>
- #include <dt-bindings/clock/mt8195-clk.h>
- #include <dt-bindings/power/mt8195-power.h>
- #include <dt-bindings/gce/mt8195-gce.h>
- #include <dt-bindings/memory/mt8195-memory-port.h>
-
- soc {
- #address-cells = <2>;
- #size-cells = <2>;
-
- rdma@1c104000 {
- compatible = "mediatek,mt8195-vdo1-rdma";
- reg = <0 0x1c104000 0 0x1000>;
- interrupts = <GIC_SPI 495 IRQ_TYPE_LEVEL_HIGH 0>;
- clocks = <&vdosys1 CLK_VDO1_MDP_RDMA0>;
- power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS1>;
- iommus = <&iommu_vdo M4U_PORT_L2_MDP_RDMA0>;
- mediatek,gce-client-reg = <&gce0 SUBSYS_1c10XXXX 0x4000 0x1000>;
- };
- };
diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
index 3e128733ef53..c043204cf210 100644
--- a/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
@@ -20,8 +20,9 @@ description: |
properties:
compatible:
- items:
- - const: mediatek,mt8183-mdp3-rdma
+ enum:
+ - mediatek,mt8183-mdp3-rdma
+ - mediatek,mt8195-vdo1-rdma
reg:
maxItems: 1
@@ -49,17 +50,18 @@ properties:
maxItems: 1
clocks:
- items:
- - description: RDMA clock
- - description: RSZ clock
+ minItems: 1
+ maxItems: 2
iommus:
maxItems: 1
mboxes:
- items:
- - description: used for 1st data pipe from RDMA
- - description: used for 2nd data pipe from RDMA
+ minItems: 1
+ maxItems: 2
+
+ interrupts:
+ maxItems: 1
'#dma-cells':
const: 1
@@ -68,13 +70,46 @@ required:
- compatible
- reg
- mediatek,gce-client-reg
- - mediatek,gce-events
- power-domains
- clocks
- iommus
- - mboxes
- '#dma-cells'
+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
+
+ required:
+ - mboxes
+ - mediatek,gce-events
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: mediatek,mt8195-vdo1-rdma
+
+ then:
+ properties:
+ clocks:
+ items:
+ - description: RDMA clock
+
additionalProperties: false
examples:
--
2.18.0
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 <[email protected]>
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 <[email protected]>
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
>>