[PATCH 0/6] Add chrdev and name query support for GLINK

[PATCH 0/6] Add chrdev and name query support for GLINK

[Chris Lew]

Add support for GLINK rpmsg transport to register a rpmsg chrdev to
create the rpmsg_ctrl nodes for userspace clients to open rpmsg epts.
Create a label property to identify the glink node with. This label
will be used as a name attr for GLINK devices. Expose this name through
a new API - rpmsg_get_rproc_name() for kernel clients.

Chris Lew (6):
  dt-bindings: soc: qcom: Add label for GLINK bindings
  rpmsg: glink: Store edge name for glink device
  rpmsg: glink: Add support for rpmsg glink chrdev
  rpmsg: glink: Expose rpmsg name attr for glink
  rpmsg: Introduce rpmsg_get_rproc_name
  rpmsg: glink: Add get_rproc_name device op

 .../devicetree/bindings/soc/qcom/qcom,glink.txt    |  5 ++
 drivers/rpmsg/qcom_glink_native.c                  | 72 ++++++++++++++++++++++
 drivers/rpmsg/rpmsg_core.c                         | 21 +++++++
 drivers/rpmsg/rpmsg_internal.h                     |  3 +
 include/linux/rpmsg.h                              | 10 +++
 5 files changed, 111 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Chris Lew]

Add a label property to identify the edge this node represents.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---
 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index 9663cab52246..0b8cc533ca83 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -10,6 +10,11 @@ edge.
 	Value type: <stringlist>
 	Definition: must be "qcom,glink-rpm"
 
+- label:
+	Usage: optional
+	Value type: <string>
+	Definition: should specify the subsystem name this edge corresponds to.
+
 - interrupts:
 	Usage: required
 	Value type: <prop-encoded-array>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/6] rpmsg: glink: Store edge name for glink device

[Chris Lew]

Channels may need to identify the edge their channel was probed for.
Store the edge name by reading the label property from device tree or
default to the node name.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---
 drivers/rpmsg/qcom_glink_native.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index cd9d643433d3..179132226dc2 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -101,6 +101,8 @@ struct glink_core_rx_intent {
 struct qcom_glink {
 	struct device *dev;
 
+	const char *name;
+
 	struct mbox_client mbox_client;
 	struct mbox_chan *mbox_chan;
 
@@ -1575,6 +1577,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 	idr_init(&glink->lcids);
 	idr_init(&glink->rcids);
 
+	ret = of_property_read_string(dev->of_node, "label", &glink->name);
+	if (ret < 0)
+		glink->name = dev->of_node->name;
+
 	glink->mbox_client.dev = dev;
 	glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
 	if (IS_ERR(glink->mbox_chan)) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/6] rpmsg: glink: Add support for rpmsg glink chrdev

[Chris Lew]

RPMSG provides a char device interface to userspace. Probe the rpmsg
chrdev channel to enable the rpmsg_ctrl device creation on glink
transports.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---
 drivers/rpmsg/qcom_glink_native.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 179132226dc2..786f2eca01f1 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -132,6 +132,13 @@ enum {
 	GLINK_STATE_CLOSING,
 };
 
+struct qcom_glink_device {
+	struct rpmsg_device rpdev;
+	struct qcom_glink *glink;
+};
+
+#define to_glink_device(_x) container_of(_x, struct qcom_glink_device, rpdev)
+
 /**
  * struct glink_channel - internal representation of a channel
  * @rpdev:	rpdev reference, only used for primary endpoints
@@ -1339,6 +1346,10 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
 	return NULL;
 }
 
+static const struct rpmsg_device_ops glink_chrdev_ops = {
+	.create_ept = qcom_glink_create_ept,
+};
+
 static const struct rpmsg_device_ops glink_device_ops = {
 	.create_ept = qcom_glink_create_ept,
 	.announce_create = qcom_glink_announce_create,
@@ -1547,6 +1558,30 @@ static void qcom_glink_work(struct work_struct *work)
 	}
 }
 
+static void qcom_glink_device_release(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct qcom_glink_device *gdev = to_glink_device(rpdev);
+
+	kfree(gdev);
+}
+
+static int qcom_glink_create_chrdev(struct qcom_glink *glink)
+{
+	struct qcom_glink_device *gdev;
+
+	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
+	if (!gdev)
+		return -ENOMEM;
+
+	gdev->glink = glink;
+	gdev->rpdev.ops = &glink_chrdev_ops;
+	gdev->rpdev.dev.parent = glink->dev;
+	gdev->rpdev.dev.release = qcom_glink_device_release;
+
+	return rpmsg_chrdev_register_device(&gdev->rpdev);
+}
+
 struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 					   unsigned long features,
 					   struct qcom_glink_pipe *rx,
@@ -1605,6 +1640,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 	if (ret)
 		return ERR_PTR(ret);
 
+	ret = qcom_glink_create_chrdev(glink);
+	if (ret)
+		dev_err(glink->dev, "failed to register chrdev\n");
+
 	return glink;
 }
 EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 4/6] rpmsg: glink: Expose rpmsg name attr for glink

[Chris Lew]

Expose the name field as an attr so clients listening to uevents for
rpmsg can identify the edge the events correspond to.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---
 drivers/rpmsg/qcom_glink_native.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 786f2eca01f1..a897ccea3098 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1558,6 +1558,22 @@ static void qcom_glink_work(struct work_struct *work)
 	}
 }
 
+static ssize_t rpmsg_name_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct qcom_glink_device *gdev = to_glink_device(rpdev);
+
+	return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", gdev->glink->name);
+}
+static DEVICE_ATTR_RO(rpmsg_name);
+
+static struct attribute *qcom_glink_attrs[] = {
+	&dev_attr_rpmsg_name.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(qcom_glink);
+
 static void qcom_glink_device_release(struct device *dev)
 {
 	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
@@ -1597,6 +1613,8 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 		return ERR_PTR(-ENOMEM);
 
 	glink->dev = dev;
+	glink->dev->groups = qcom_glink_groups;
+
 	glink->tx_pipe = tx;
 	glink->rx_pipe = rx;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 5/6] rpmsg: Introduce rpmsg_get_rproc_name

[Chris Lew]

Add support for client's to query the edge name their channel is
registered for. This is useful for clients who share the same channel
identifier across different remote procs.

Signed-off-by: Chris Lew <clew-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h |  3 +++
 include/linux/rpmsg.h          | 10 ++++++++++
 3 files changed, 34 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index dffa3aab7178..d6ebd678b089 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -78,6 +78,27 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
 }
 EXPORT_SYMBOL(rpmsg_create_ept);
 
+
+/**
+ * rpmsg_get_rproc_name() - Return the name of the rproc for this device
+ * @rpdev: rpmsg channel device
+ *
+ * Expose the rproc name for clients who open the same channel across different
+ * rprocs and need to differentiate during their probe.
+ *
+ * Returns char string on success and NULL on failure.
+ */
+const char *rpmsg_get_rproc_name(struct rpmsg_device *rpdev)
+{
+	if (WARN_ON(!rpdev))
+		return NULL;
+
+	if (!rpdev->ops->get_rproc_name)
+		return NULL;
+
+	return rpdev->ops->get_rproc_name(rpdev);
+}
+
 /**
  * rpmsg_destroy_ept() - destroy an existing rpmsg endpoint
  * @ept: endpoing to destroy
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 0cf9c7e2ee83..83a028b6883f 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -31,6 +31,7 @@
  * @create_ept:		create backend-specific endpoint, requried
  * @announce_create:	announce presence of new channel, optional
  * @announce_destroy:	announce destruction of channel, optional
+ * @get_rproc_name:	return name of the rproc for this device, optional
  *
  * Indirection table for the operations that a rpmsg backend should implement.
  * @announce_create and @announce_destroy are optional as the backend might
@@ -43,6 +44,8 @@ struct rpmsg_device_ops {
 
 	int (*announce_create)(struct rpmsg_device *ept);
 	int (*announce_destroy)(struct rpmsg_device *ept);
+
+	const char *(*get_rproc_name)(struct rpmsg_device *rpdev);
 };
 
 /**
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 10d6ae8bbb7d..167982dc5b4f 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -160,6 +160,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
 unsigned int rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
 			poll_table *wait);
 
+const char *rpmsg_get_rproc_name(struct rpmsg_device *dev);
+
 #else
 
 static inline int register_rpmsg_device(struct rpmsg_device *dev)
@@ -267,6 +269,14 @@ static inline unsigned int rpmsg_poll(struct rpmsg_endpoint *ept,
 	return 0;
 }
 
+static inline const char *rpmsg_get_rproc_name(struct rpmsg_device *rpdev)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return NULL;
+}
+
 #endif /* IS_ENABLED(CONFIG_RPMSG) */
 
 /* use a macro to avoid include chaining to get THIS_MODULE */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] rpmsg: glink: Add get_rproc_name device op

[Chris Lew]

Add support for clients to query the edge name for the glink device
their channel is registered for.

Signed-off-by: Chris Lew <clew-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/rpmsg/qcom_glink_native.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index a897ccea3098..4748dea0748e 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1346,6 +1346,14 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
 	return NULL;
 }
 
+static const char *qcom_glink_get_rproc_name(struct rpmsg_device *rpdev)
+{
+	struct glink_channel *channel = to_glink_channel(rpdev->ept);
+	struct qcom_glink *glink = channel->glink;
+
+	return glink->name;
+}
+
 static const struct rpmsg_device_ops glink_chrdev_ops = {
 	.create_ept = qcom_glink_create_ept,
 };
@@ -1353,6 +1361,7 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
 static const struct rpmsg_device_ops glink_device_ops = {
 	.create_ept = qcom_glink_create_ept,
 	.announce_create = qcom_glink_announce_create,
+	.get_rproc_name = qcom_glink_get_rproc_name,
 };
 
 static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] rpmsg: glink: Expose rpmsg name attr for glink

[Stephen Boyd]

On 12/18/2017 02:02 PM, Chris Lew wrote:
> Expose the name field as an attr so clients listening to uevents for
> rpmsg can identify the edge the events correspond to.
>
> Signed-off-by: Chris Lew <clew-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 786f2eca01f1..a897ccea3098 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1558,6 +1558,22 @@ static void qcom_glink_work(struct work_struct *work)
>  	}
>  }
>  
> +static ssize_t rpmsg_name_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> +	struct qcom_glink_device *gdev = to_glink_device(rpdev);
> +
> +	return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", gdev->glink->name);
> +}
> +static DEVICE_ATTR_RO(rpmsg_name);
> +
> +static struct attribute *qcom_glink_attrs[] = {

const?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] rpmsg: Introduce rpmsg_get_rproc_name

[Bjorn Andersson]

On Mon 18 Dec 14:02 PST 2017, Chris Lew wrote:

> Add support for client's to query the edge name their channel is
> registered for. This is useful for clients who share the same channel
> identifier across different remote procs.
> 

I presume this will result in a strcmp in some client driver?

When we're registering the rpmsg device, as part of handling of an
arriving "open request", we do look for an of_node with matching
qcom,glink-channels and if one is found we point the dev->of_node of the
new device to this node.

So I would suggest that you, in your client driver, use this to decide
which instance you're on; regardless if you're using compatible based
driver matching.

Does this work for you?

Regards,
Bjorn

Re: [PATCH 2/6] rpmsg: glink: Store edge name for glink device

[Bjorn Andersson]

On Mon 18 Dec 14:02 PST 2017, Chris Lew wrote:

> Channels may need to identify the edge their channel was probed for.
> Store the edge name by reading the label property from device tree or
> default to the node name.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

Re: [PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Rob Herring]

On Mon, Dec 18, 2017 at 02:02:09PM -0800, Chris Lew wrote:
> Add a label property to identify the edge this node represents.

Why does a user need to know this?

> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> index 9663cab52246..0b8cc533ca83 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> @@ -10,6 +10,11 @@ edge.
>  	Value type: <stringlist>
>  	Definition: must be "qcom,glink-rpm"
>  
> +- label:
> +	Usage: optional
> +	Value type: <string>
> +	Definition: should specify the subsystem name this edge corresponds to.
> +
>  - interrupts:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Bjorn Andersson]

On Wed 20 Dec 10:30 PST 2017, Rob Herring wrote:

> On Mon, Dec 18, 2017 at 02:02:09PM -0800, Chris Lew wrote:
> > Add a label property to identify the edge this node represents.
> 
> Why does a user need to know this?
> 

We have multiple remoteproc instances, each one having one or more
associated SMD or GLINK links (this node), exposing logical
communication channels. Some of these logical channels are exposed to
user space and we need a way to distinguish them there.

In the current implementation of SMD this value goes straight into an
sysfs attribute that we can use when writing udev rules and for the DIAG
implementation to pair up channels related to the same remoteproc. This
adds the equivalent information for glink-backed channels.


I'm therefor in favor of picking this patch.

Regards,
Bjorn

> > 
> > Signed-off-by: Chris Lew <clew@codeaurora.org>
> > ---
> >  Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> > index 9663cab52246..0b8cc533ca83 100644
> > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> > @@ -10,6 +10,11 @@ edge.
> >  	Value type: <stringlist>
> >  	Definition: must be "qcom,glink-rpm"
> >  
> > +- label:
> > +	Usage: optional
> > +	Value type: <string>
> > +	Definition: should specify the subsystem name this edge corresponds to.
> > +
> >  - interrupts:
> >  	Usage: required
> >  	Value type: <prop-encoded-array>
> > -- 
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> > 

Re: [PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Stephen Boyd]

On 12/20/2017 05:35 PM, Bjorn Andersson wrote:
> On Wed 20 Dec 10:30 PST 2017, Rob Herring wrote:
>
>> On Mon, Dec 18, 2017 at 02:02:09PM -0800, Chris Lew wrote:
>>> Add a label property to identify the edge this node represents.
>> Why does a user need to know this?
>>
> We have multiple remoteproc instances, each one having one or more
> associated SMD or GLINK links (this node), exposing logical
> communication channels. Some of these logical channels are exposed to
> user space and we need a way to distinguish them there.
>
> In the current implementation of SMD this value goes straight into an
> sysfs attribute that we can use when writing udev rules and for the DIAG
> implementation to pair up channels related to the same remoteproc. This
> adds the equivalent information for glink-backed channels.
>
>
> I'm therefor in favor of picking this patch.

Please add these details to the commit log. Just writing what the patch
is doing isn't very helpful.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Rob Herring]

On Wed, Dec 20, 2017 at 7:35 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 20 Dec 10:30 PST 2017, Rob Herring wrote:
>
>> On Mon, Dec 18, 2017 at 02:02:09PM -0800, Chris Lew wrote:
>> > Add a label property to identify the edge this node represents.
>>
>> Why does a user need to know this?
>>
>
> We have multiple remoteproc instances, each one having one or more
> associated SMD or GLINK links (this node), exposing logical
> communication channels. Some of these logical channels are exposed to
> user space and we need a way to distinguish them there.

They should have some sort of discoverable property. There must be
some reason why you need a specific channel. I think of label as being
a sticker or silkscreen on a device which doesn't seem to be the case
here. This seems more like wanting fixed device numbering like disks
and I assume you know the position on fixed device numbers.

Rob

Re: [PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Bjorn Andersson]

On Wed 20 Dec 10:30 PST 2017, Rob Herring wrote:

> On Mon, Dec 18, 2017 at 02:02:09PM -0800, Chris Lew wrote:
> > Add a label property to identify the edge this node represents.
> 
> Why does a user need to know this?
> 

Each SMD or GLINK edge will expose a dev node as /dev/rpmsg_ctrlX that
can be used create dev nodes for channels that user space wants to
access directly (without a specific kernel driver).

The problem is that there's currently nothing in the device hierarchy
that identifies which edge, and hence remoteproc, these channels are
associated with.

So for SMD I added the ability to specify a "label" on the SMD edge,
which is propagated in sysfs and allows me to write udev rules creating
symlinks based on the edge name (e.g. /dev/modem/DIAG vs /dev/rpmsg4)
and it also allows user space tools to figure out which channels comes
from the same edge/remoteproc (e.g. which DIAG control channel is
related to which DIAG data channel).

This patch attempts to add the equivalent property on GLINK edges.


As SMD and GLINK edges can exist without a parent remoteproc (e.g. for
the always-on subsystems) and it's possible for a single remoteproc to
have multiple edges, it's inadequate to put this label on the
remoteproc.


PS. Downstream solutions of remoteproc + virtio-rpmsg has "solved" this
problem by using aliases for remoteprocs and then only allow one
virtio-rpmsg node per remoteproc, to get deterministic identification of
channels.

Regards,
Bjorn

Re: [PATCH 4/6] rpmsg: glink: Expose rpmsg name attr for glink

[Chris Lew]

On 12/18/2017 2:48 PM, Stephen Boyd wrote:
> On 12/18/2017 02:02 PM, Chris Lew wrote:
>> Expose the name field as an attr so clients listening to uevents for
>> rpmsg can identify the edge the events correspond to.
>>
>> Signed-off-by: Chris Lew <clew-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>   drivers/rpmsg/qcom_glink_native.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index 786f2eca01f1..a897ccea3098 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -1558,6 +1558,22 @@ static void qcom_glink_work(struct work_struct *work)
>>   	}
>>   }
>>   
>> +static ssize_t rpmsg_name_show(struct device *dev,
>> +			       struct device_attribute *attr, char *buf)
>> +{
>> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
>> +	struct qcom_glink_device *gdev = to_glink_device(rpdev);
>> +
>> +	return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", gdev->glink->name);
>> +}
>> +static DEVICE_ATTR_RO(rpmsg_name);
>> +
>> +static struct attribute *qcom_glink_attrs[] = {
> 
> const?
> 

Thanks Stephen, will change to "static const struct attribute *"

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] rpmsg: Introduce rpmsg_get_rproc_name

[Chris Lew]

On 12/19/2017 9:52 AM, Bjorn Andersson wrote:
> On Mon 18 Dec 14:02 PST 2017, Chris Lew wrote:
> 
>> Add support for client's to query the edge name their channel is
>> registered for. This is useful for clients who share the same channel
>> identifier across different remote procs.
>>
> 
> I presume this will result in a strcmp in some client driver?
> 
> When we're registering the rpmsg device, as part of handling of an
> arriving "open request", we do look for an of_node with matching
> qcom,glink-channels and if one is found we point the dev->of_node of the
> new device to this node.
> 
> So I would suggest that you, in your client driver, use this to decide
> which instance you're on; regardless if you're using compatible based
> driver matching.
> 
> Does this work for you?
> 
> Regards,
> Bjorn
> 

Yea I think this works for us, we can drop this patch.

Just to confirm my understanding, clients can use rpdev->dev.of_node to 
get the parent of_node and from there get the label field? Also to 
ensure of_node is set, client's need to add their channel to the dt with 
qcom,glink-channels.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Chris Lew]

On 12/21/2017 11:36 AM, Stephen Boyd wrote:
> On 12/20/2017 05:35 PM, Bjorn Andersson wrote:
>> On Wed 20 Dec 10:30 PST 2017, Rob Herring wrote:
>>
>>> On Mon, Dec 18, 2017 at 02:02:09PM -0800, Chris Lew wrote:
>>>> Add a label property to identify the edge this node represents.
>>> Why does a user need to know this?
>>>
>> We have multiple remoteproc instances, each one having one or more
>> associated SMD or GLINK links (this node), exposing logical
>> communication channels. Some of these logical channels are exposed to
>> user space and we need a way to distinguish them there.
>>
>> In the current implementation of SMD this value goes straight into an
>> sysfs attribute that we can use when writing udev rules and for the DIAG
>> implementation to pair up channels related to the same remoteproc. This
>> adds the equivalent information for glink-backed channels.
>>
>>
>> I'm therefor in favor of picking this patch.
> 
> Please add these details to the commit log. Just writing what the patch
> is doing isn't very helpful.
> 

Ok, will do.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/6] rpmsg: glink: Add support for rpmsg glink chrdev

[Bjorn Andersson]

On Mon 18 Dec 14:02 PST 2017, Chris Lew wrote:

> RPMSG provides a char device interface to userspace. Probe the rpmsg
> chrdev channel to enable the rpmsg_ctrl device creation on glink
> transports.
> 
> Signed-off-by: Chris Lew <clew-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 179132226dc2..786f2eca01f1 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -132,6 +132,13 @@ enum {
>  	GLINK_STATE_CLOSING,
>  };
>  
> +struct qcom_glink_device {
> +	struct rpmsg_device rpdev;
> +	struct qcom_glink *glink;
> +};
> +
> +#define to_glink_device(_x) container_of(_x, struct qcom_glink_device, rpdev)
> +
>  /**
>   * struct glink_channel - internal representation of a channel
>   * @rpdev:	rpdev reference, only used for primary endpoints
> @@ -1339,6 +1346,10 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
>  	return NULL;
>  }
>  
> +static const struct rpmsg_device_ops glink_chrdev_ops = {
> +	.create_ept = qcom_glink_create_ept,
> +};

I can't remember and I see no apparent reason why we don't just
advertise the intents as the last step of create_ept, meaning that the
difference in ops would go away.

This would also make sure that we do advertise some intents for channels
associated with the rpmsg_char - and even if we don't have an attached
driver we can override the intent sizes on a channel.

> +
>  static const struct rpmsg_device_ops glink_device_ops = {
>  	.create_ept = qcom_glink_create_ept,
>  	.announce_create = qcom_glink_announce_create,
> @@ -1547,6 +1558,30 @@ static void qcom_glink_work(struct work_struct *work)
>  	}
>  }
>  
> +static void qcom_glink_device_release(struct device *dev)
> +{
> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> +	struct qcom_glink_device *gdev = to_glink_device(rpdev);
> +
> +	kfree(gdev);
> +}
> +
> +static int qcom_glink_create_chrdev(struct qcom_glink *glink)
> +{
> +	struct qcom_glink_device *gdev;
> +
> +	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
> +	if (!gdev)
> +		return -ENOMEM;
> +
> +	gdev->glink = glink;
> +	gdev->rpdev.ops = &glink_chrdev_ops;
> +	gdev->rpdev.dev.parent = glink->dev;
> +	gdev->rpdev.dev.release = qcom_glink_device_release;
> +
> +	return rpmsg_chrdev_register_device(&gdev->rpdev);

This rpdev will end up being the first parameter to
qcom_glink_create_ept() where we do:

	container_of(rpdev->ept, struct glink_channel, ept)

to get the struct glink_channel that backs the endpoint of the passed
rpmsg_device. So the registered rpdev must reference a rpmsg_endpoint
that is contained within a glink_channel.


So I think you need to create two objects here; a glink_channel with
the glink pointer referencing the glink instance and a rpmsg_device
referencing the rpmsg_endpoint of the channel.

You can get a initialized (and dangling) glink_channel by calling
qcom_glink_alloc_channel(). Note though that this element won't be freed
automagically (the rpmsg_device will).

> +}
> +

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html