On Tue, 2019-08-27 at 12:13 +0200, Matthias Brugger wrote:

On 27/08/2019 05:59, Bibby Hsieh wrote:

quoted

On Fri, 2019-08-23 at 16:21 +0200, Matthias Brugger wrote:

quoted

On 20/08/2019 10:49, Bibby Hsieh wrote:

quoted

GCE cannot know the register base address, this function
can help cmdq client to get the cmdq_client_reg structure.

Signed-off-by: Bibby Hsieh <redacted>
Reviewed-by: CK Hu <redacted>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 29 ++++++++++++++++++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  | 21 +++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index c53f8476c68d..80f75a1075b4 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c

@@ -27,6 +27,35 @@ struct cmdq_instruction {
 	u8 op;
 };
 
+int cmdq_dev_get_client_reg(struct device *dev,
+			    struct cmdq_client_reg *client_reg, int idx)
+{

Can't we do/call this in cmdq_mbox_create parsing the number of gce-client-reg
properties we have and allocating these using a pointer to cmdq_client_reg in
cmdq_client?
We will have to free the pointer then in cmdq_mbox_destroy.

Regards,
Matthias

I don't think we need to keep the cmdq_client_reg in cmdq_client
structure.
Because our client will have own data structure, they will copy the
client_reg information into their own structure.

In the design now, we do not allocate the cmdq_client_reg, client pass
the cmdq_client_reg pointer into this API.
Client will destroy the pointer after they get the information they
want.

My point wasn't so much about the lifecycle of the object, but the fact that we
add another call, which can be already full-filled by a necessary previous call
to cmdq_mbox_create. So I would prefer to add the information gathering for
cmdq_client_reg in this call, and let it live there for the time cmdq_client
lives. In the end we are talking about 40 bits of memory.

Thanks for the comments. :D

Actually, I'm working for developing the chandes for MTK DRM apply cmdq
interface.
For MTK DRM, all the components included in MTK_CRTC use one mailbox
channel. According to [1], we create mailbox channel by mmsys device
node after get all the informations (include cmdq_client_reg) about
every device node of display components respectively. Please refer to
[2], [3] and [4], I'm going to upstream them recently.

If create mailbox channel and get the cmdq_client_reg in the same device
node, your suggestion is good for me.
But from display and mdp's viewpoint now, I don't think it is convenient
for them.

So I still prefer separate this function out of cmdq_mbox_create.:D


[1]
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8173.dtsi#907
[2] get cmdq_client_reg in mtk_ddp_comp_init()
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1746354/12/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c#431
[3] create mailbox channel in mtk_drm_crtc_create()
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1746354/12/drivers/gpu/drm/mediatek/mtk_drm_crtc.c#814
[4] After component_bind_all(), the mtk_drm_crtc_create will be called 
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e15c2dc6ceb4810a2090cd11a512932095866559/drivers/gpu/drm/mediatek/mtk_drm_drv.c#452

Thanks.
Bibby

Regards,
Matthias

quoted

Thanks for the comments so much.

Bibby

quoted

quoted

+	struct of_phandle_args spec;
+	int err;
+
+	if (!client_reg)
+		return -ENOENT;
+
+	err = of_parse_phandle_with_fixed_args(dev->of_node,
+					       "mediatek,gce-client-reg",
+					       3, idx, &spec);
+	if (err < 0) {
+		dev_err(dev,
+			"error %d can't parse gce-client-reg property (%d)",
+			err, idx);
+
+		return err;
+	}
+
+	client_reg->subsys = (u8)spec.args[0];
+	client_reg->offset = (u16)spec.args[1];
+	client_reg->size = (u16)spec.args[2];
+	of_node_put(spec.np);
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_dev_get_client_reg);
+
 static void cmdq_client_timeout(struct timer_list *t)
 {
 	struct cmdq_client *client = from_timer(client, t, timer);

diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index a345870a6d10..02ddd60b212f 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h

@@ -15,6 +15,12 @@
 
 struct cmdq_pkt;
 
+struct cmdq_client_reg {
+	u8 subsys;
+	u16 offset;
+	u16 size;
+};
+
 struct cmdq_client {
 	spinlock_t lock;
 	u32 pkt_cnt;

@@ -24,6 +30,21 @@ struct cmdq_client {
 	u32 timeout_ms; /* in unit of microsecond */
 };
 
+/**
+ * cmdq_dev_get_client_reg() - parse cmdq client reg from the device
+ *			       node of CMDQ client
+ * @dev:	device of CMDQ mailbox client
+ * @client_reg: CMDQ client reg pointer
+ * @idx:	the index of desired reg
+ *
+ * Return: 0 for success; else the error code is returned
+ *
+ * Help CMDQ client parsing the cmdq client reg
+ * from the device node of CMDQ client.
+ */
+int cmdq_dev_get_client_reg(struct device *dev,
+			    struct cmdq_client_reg *client_reg, int idx);
+
 /**
  * cmdq_mbox_create() - create CMDQ mailbox client and channel
  * @dev:	device of CMDQ mailbox client