On 24/05/2022 14:18, AngeloGioacchino Del Regno wrote: > Il 20/05/22 10:15, Krzysztof Kozlowski ha scritto: >> On 19/05/2022 12:18, AngeloGioacchino Del Regno wrote: >>> Convert the mtk-gce documentation from freeform text format to a >>> json-schema. >>> >>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> >>> --- >>> .../bindings/mailbox/mediatek,gce-mbox.yaml | 114 ++++++++++++++++++ >>> .../devicetree/bindings/mailbox/mtk-gce.txt | 82 ------------- >>> 2 files changed, 114 insertions(+), 82 deletions(-) >>> create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,gce-mbox.yaml >>> delete mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt >>> >>> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mbox.yaml >>> new file mode 100644 >>> index 000000000000..750391b4038c >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mbox.yaml >>> @@ -0,0 +1,114 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/mailbox/mediatek,gce-mbox.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: MediaTek Global Command Engine (GCE) mailbox >>> + >>> +maintainers: >>> + - Houlong Wei <houlong.wei@xxxxxxxxxxxx> >>> + >>> +description: | >>> + The Global Command Engine (GCE) is used to help read/write registers >>> + with critical time limitation, such as updating display configuration >>> + during the vblank. >>> + The GCE can be used to implement the Command Queue (CMDQ) driver. >> >> Mention the headers in description. >> > > Values for properties used by the GCE, such as sub-system IDs, thread > > priority and event IDs are defined in 'dt-bindings/gce/<chip>-gce.h'. > > Would that be enough, or should I list all of the headers? Yes. > >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - mediatek,mt6779-gce >>> + - mediatek,mt8173-gce >>> + - mediatek,mt8183-gce >>> + - mediatek,mt8186-gce >>> + - mediatek,mt8192-gce >>> + - mediatek,mt8195-gce >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + clock-names: >>> + items: >>> + - const: gce >>> + >>> + '#mbox-cells': >>> + description: | >>> + The first cell describes the mailbox channel, which is the GCE Thread ID; >>> + The second cell describes the priority of the GCE thread. >>> + const: 2 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - clocks >>> + - clock-names >>> + - '#mbox-cells' >>> + >>> +additionalProperties: false >>> + >>> +allOf: >>> + - if: >>> + properties: >>> + compatible: >>> + enum: >>> + - mediatek,mt8195-gce >>> + then: >>> + properties: >>> + clocks: >>> + maxItems: 2 >> >> Are you sure this works on mt8195-gce? >> > > Thanks for that, I've just rechecked the driver and.. no, that won't > work for MT8195: it's just one clock there (like the others) and the > clock names aren't even enforced, as the driver is always taking the > clock at index 0. I was not thinking about driver, although it's nice that my review helped in that. What I was mentioning, is your bindings behave correctly for mediatek,mt8195-gce DTS? You have maxItems:1 and maxItems:2, so usually it was failing, AFAIR. The same with clock-names - I think the schema should fail here. > > I got confused because the driver uses a slightly different kind of > logic when probing on SoCs with multiple mailboxes, specifically: > - For single mailbox, having a clock with name "gce" is enforced > as it's grabbing it with devm_clk_get(dev, clk_name), where the > clock name is declared in a string called "clk_name"; > - For multiple mailboxes, it's looking for an of_alias, declared > in an array of strings called "clk_names" and getting the clock > with of_clk_get(node, 0). > > > So there comes my confusion, recapping: > > static const char * const clk_name = "gce"; > <- this is a clock name > static const char * const clk_names[] = { "gce0", "gce1" }; <- OF alias names > > > At this point, I think that the best idea would be to fix this issue > first... luckily there's no MT8195 devicetree upstream yet, so I would > technically not be breaking any ABI by changing it to be the same as > the others. All this sounds a bit unrelated to the bindings. Anyway, gce for one case and gce0+gce1 for other, are okay, although schema looks a bit more complicated. See for example: https://lore.kernel.org/linux-devicetree/20220523181836.2019180-8-dmitry.baryshkov@xxxxxxxxxx/ Best regards, Krzysztof
