[PATCH 0/4] MediaTek CMDQ - drop clock-names and refactor clock probe

[PATCH 0/4] MediaTek CMDQ - drop clock-names and refactor clock probe

[AngeloGioacchino Del Regno]

All CMDQ Mailboxes have only one GCE clock per hardware instance:
remove the clock-names requirement and, while at it, also partially
refactor the clock probe code to improve both readability and its
flexibility.

This was tested on MT8192, MT8195 Chromebooks, MT8395 Radxa NIO 12L
and on the MT6795 Xperia M5 smartphone.

AngeloGioacchino Del Regno (4):
  mailbox: mtk-cmdq: Stop requiring name for GCE clock
  mailbox: mtk-cmdq: Move and partially refactor clocks probe
  mailbox: mtk-cmdq: Dynamically allocate clk_bulk_data structure
  dt-bindings: mailbox: mediatek,gce-mailbox: Stop requiring clock-names

 .../mailbox/mediatek,gce-mailbox.yaml         | 11 ---
 drivers/mailbox/mtk-cmdq-mailbox.c            | 87 ++++++++++++-------
 2 files changed, 57 insertions(+), 41 deletions(-)

-- 
2.45.2

[PATCH 1/4] mailbox: mtk-cmdq: Stop requiring name for GCE clock

[AngeloGioacchino Del Regno]

The Global Command Engine mailbox has only one clock hence
requiring clock-names is useless.

Get the first (and only) clock instead, without name checks.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index 4aa394e91109..66dbc8e846d8 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -629,7 +629,7 @@ static int cmdq_probe(struct platform_device *pdev)
 		}
 	} else {
 		cmdq->clocks[alias_id].id = clk_name;
-		cmdq->clocks[alias_id].clk = devm_clk_get(&pdev->dev, clk_name);
+		cmdq->clocks[alias_id].clk = devm_clk_get(&pdev->dev, NULL);
 		if (IS_ERR(cmdq->clocks[alias_id].clk)) {
 			return dev_err_probe(dev, PTR_ERR(cmdq->clocks[alias_id].clk),
 					     "failed to get gce clk\n");
-- 
2.45.2

[PATCH 2/4] mailbox: mtk-cmdq: Move and partially refactor clocks probe

[AngeloGioacchino Del Regno]

Move the clocks probe to a new cmdq_get_clocks() function; while
at it, partially refactor the code:

Drop the clk_names[] array and assign clock names to the array
of clk_bulk_data with devm_kasprintf() instead, slightly reduce
the indentation for the multi-gce clock probe path and add a
comment describing the reason why we get clocks of other GCE
instance instead of just the clock from the one that it is
getting probed.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c | 79 +++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 28 deletions(-)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index 66dbc8e846d8..a3f57151a221 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -578,16 +578,59 @@ static struct mbox_chan *cmdq_xlate(struct mbox_controller *mbox,
 	return &mbox->chans[ind];
 }
 
+static int cmdq_get_clocks(struct device *dev, struct cmdq *cmdq)
+{
+	static const char * const gce_name = "gce";
+	struct device_node *node, *parent = dev->of_node->parent;
+	struct clk_bulk_data *clks;
+
+	if (cmdq->pdata->gce_num == 1) {
+		clks = &cmdq->clocks[0];
+
+		clks->id = gce_name;
+		clks->clk = devm_clk_get(dev, NULL);
+		if (IS_ERR(clks->clk))
+			return dev_err_probe(dev, PTR_ERR(clks->clk),
+					     "failed to get gce clock\n");
+
+		return 0;
+	}
+
+	/*
+	 * If there is more than one GCE, get the clocks for the others too,
+	 * as the clock of the main GCE must be enabled for additional IPs
+	 * to be reachable.
+	 */
+	for_each_child_of_node(parent, node) {
+		int alias_id = of_alias_get_id(node, gce_name);
+
+		if (alias_id < 0 || alias_id >= cmdq->pdata->gce_num)
+			continue;
+
+		clks = &cmdq->clocks[alias_id];
+
+		clks->id = devm_kasprintf(dev, GFP_KERNEL, "gce%d", alias_id);
+		if (!clks->id) {
+			of_node_put(node);
+			return -ENOMEM;
+		}
+
+		clks->clk = of_clk_get(node, 0);
+		if (IS_ERR(clks->clk)) {
+			of_node_put(node);
+			return dev_err_probe(dev, PTR_ERR(clks->clk),
+					     "failed to get gce%d clock\n", alias_id);
+		}
+	}
+
+	return 0;
+}
+
 static int cmdq_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct cmdq *cmdq;
 	int err, i;
-	struct device_node *phandle = dev->of_node;
-	struct device_node *node;
-	int alias_id = 0;
-	static const char * const clk_name = "gce";
-	static const char * const clk_names[] = { "gce0", "gce1" };
 
 	cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
 	if (!cmdq)
@@ -612,29 +655,9 @@ static int cmdq_probe(struct platform_device *pdev)
 	dev_dbg(dev, "cmdq device: addr:0x%p, va:0x%p, irq:%d\n",
 		dev, cmdq->base, cmdq->irq);
 
-	if (cmdq->pdata->gce_num > 1) {
-		for_each_child_of_node(phandle->parent, node) {
-			alias_id = of_alias_get_id(node, clk_name);
-			if (alias_id >= 0 && alias_id < cmdq->pdata->gce_num) {
-				cmdq->clocks[alias_id].id = clk_names[alias_id];
-				cmdq->clocks[alias_id].clk = of_clk_get(node, 0);
-				if (IS_ERR(cmdq->clocks[alias_id].clk)) {
-					of_node_put(node);
-					return dev_err_probe(dev,
-							     PTR_ERR(cmdq->clocks[alias_id].clk),
-							     "failed to get gce clk: %d\n",
-							     alias_id);
-				}
-			}
-		}
-	} else {
-		cmdq->clocks[alias_id].id = clk_name;
-		cmdq->clocks[alias_id].clk = devm_clk_get(&pdev->dev, NULL);
-		if (IS_ERR(cmdq->clocks[alias_id].clk)) {
-			return dev_err_probe(dev, PTR_ERR(cmdq->clocks[alias_id].clk),
-					     "failed to get gce clk\n");
-		}
-	}
+	err = cmdq_get_clocks(dev, cmdq);
+	if (err)
+		return err;
 
 	cmdq->mbox.dev = dev;
 	cmdq->mbox.chans = devm_kcalloc(dev, cmdq->pdata->thread_nr,
-- 
2.45.2

[PATCH 3/4] mailbox: mtk-cmdq: Dynamically allocate clk_bulk_data structure

[AngeloGioacchino Del Regno]

Now that the clock probing code uses devm_kasprintf(), there is
no more restriction on the number of GCEs: dynamically allocate
the clk_bulk_data clocks array to improve flexibility and also
to get a slight memory saving on platforms featuring only one
CMDQ mailbox (and consequently only one Global Command Engine).

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index a3f57151a221..2a71a5400088 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -22,7 +22,6 @@
 
 #define CMDQ_OP_CODE_MASK		(0xff << CMDQ_OP_CODE_SHIFT)
 #define CMDQ_NUM_CMD(t)			(t->cmd_buf_size / CMDQ_INST_SIZE)
-#define CMDQ_GCE_NUM_MAX		(2)
 
 #define CMDQ_CURR_IRQ_STATUS		0x10
 #define CMDQ_SYNC_TOKEN_UPDATE		0x68
@@ -81,7 +80,7 @@ struct cmdq {
 	u32			irq_mask;
 	const struct gce_plat	*pdata;
 	struct cmdq_thread	*thread;
-	struct clk_bulk_data	clocks[CMDQ_GCE_NUM_MAX];
+	struct clk_bulk_data	*clocks;
 	bool			suspended;
 };
 
@@ -584,6 +583,11 @@ static int cmdq_get_clocks(struct device *dev, struct cmdq *cmdq)
 	struct device_node *node, *parent = dev->of_node->parent;
 	struct clk_bulk_data *clks;
 
+	cmdq->clocks = devm_kcalloc(dev, cmdq->pdata->gce_num,
+				    sizeof(cmdq->clocks), GFP_KERNEL);
+	if (!cmdq->clocks)
+		return -ENOMEM;
+
 	if (cmdq->pdata->gce_num == 1) {
 		clks = &cmdq->clocks[0];
 
-- 
2.45.2

[PATCH 4/4] dt-bindings: mailbox: mediatek,gce-mailbox: Stop requiring clock-names

[AngeloGioacchino Del Regno]

On all MediaTek SoCs, each GCE Mailbox have got only one single
clock, and this is true in both SoCs with a single mailbox and
ones with multiple mailboxes.

Stop requiring clock-names on all GCEs, as there is and there
will always be only one CMDQ clock per HW instance.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../bindings/mailbox/mediatek,gce-mailbox.yaml        | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
index cef9d7601398..ff5d010fbcf0 100644
--- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
@@ -56,17 +56,6 @@ required:
   - interrupts
   - clocks
 
-allOf:
-  - if:
-      not:
-        properties:
-          compatible:
-            contains:
-              const: mediatek,mt8195-gce
-    then:
-      required:
-        - clock-names
-
 additionalProperties: false
 
 examples:
-- 
2.45.2

Re: [PATCH 4/4] dt-bindings: mailbox: mediatek,gce-mailbox: Stop requiring clock-names

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

On Thu, Jun 20, 2024 at 12:07:50PM +0200, AngeloGioacchino Del Regno wrote:
> On all MediaTek SoCs, each GCE Mailbox have got only one single
> clock, and this is true in both SoCs with a single mailbox and
> ones with multiple mailboxes.
> 
> Stop requiring clock-names on all GCEs, as there is and there
> will always be only one CMDQ clock per HW instance.

I'd like the commit message here to mention that there are no users
outside of Linux for this before making the property non-required.
And obviously you can only say that if it is true - although I think you
said to me earlier today that U-Boot etc don't support this device, only
Linux.

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../bindings/mailbox/mediatek,gce-mailbox.yaml        | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> index cef9d7601398..ff5d010fbcf0 100644
> --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> @@ -56,17 +56,6 @@ required:
>    - interrupts
>    - clocks
>  
> -allOf:
> -  - if:
> -      not:
> -        properties:
> -          compatible:
> -            contains:
> -              const: mediatek,mt8195-gce
> -    then:
> -      required:
> -        - clock-names
> -
>  additionalProperties: false
>  
>  examples:
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]