[RFC,1/3] dt-binding: soc: qcom: Add binding for RFSA

This adds the binding for describing shared memory buffers for
implementing the remote filesystem protocol.

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

My initial attempt was to mimic the ramoops of just adding the compatible to
the reserved-memory node, but I have not been able to figure out a sane way of
getting hold of the base address in the case that the memory region is
described my a "size" only (done on some platforms).

The problem is that we create the reserved_mem objects (and remove the
memblocks) while we're still operating on the flattened representation, so
without a phandle it doesn't seem like we have anything to perform the
comparison with later on.

 .../devicetree/bindings/soc/qcom/qcom,rfsa.txt     | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt

On 04/22/17 10:35, Bjorn Andersson wrote:
> In some cases drivers referencing a reserved-memory region might want to
> remap the entire region, but when defining the reserved-memory by "size"
> the client driver has no means to know the associated base address of
> the reserved memory region.
> 
> This patch adds an accessor for such drivers to acquire a handle to
> their associated reserved-memory for this purpose.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> I would have preferred if we could provide a mechanism for drivers to find the
> reserved_mem of their own device_node, but without a phandle I have not been
> able to figure out a sane way to make the match.
> 
> Suggestions are very welcome.
> 
>  drivers/of/of_reserved_mem.c    | 26 ++++++++++++++++++++++++++
>  include/linux/of_reserved_mem.h |  8 ++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index d507c3569a88..aa69c9590a5c 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -397,3 +397,29 @@ void of_reserved_mem_device_release(struct device *dev)
>  	rmem->ops->device_release(rmem, dev);
>  }
>  EXPORT_SYMBOL_GPL(of_reserved_mem_device_release);
> +
> +/**
> + * of_get_reserved_mem_by_idx() - acquire reserved_mem from memory-region
> + * @np:		node pointer containing the "memory-region"
> + * @idx:	index within memory-region
> + *
> + * This function allows drivers to acquire a reference to the reserved_mem
> + * struct which is referenced by their memory-region.
> + *
> + * Returns a reserved_mem reference, or NULL on error.
> + */
> +struct reserved_mem *of_get_reserved_mem_by_idx(struct device_node *np, int idx)
> +{
> +	struct device_node *target;
> +	struct reserved_mem *rmem;
> +
> +	target = of_parse_phandle(np, "memory-region", idx);
> +	if (!target)
> +		return NULL;
> +
> +	rmem = __find_rmem(target);
> +	of_node_put(target);
> +
> +	return rmem;
> +}
> +EXPORT_SYMBOL_GPL(of_get_reserved_mem_by_idx);
> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
> index f8e1992d6423..a9abbe7dd3de 100644
> --- a/include/linux/of_reserved_mem.h
> +++ b/include/linux/of_reserved_mem.h
> @@ -34,6 +34,8 @@ int of_reserved_mem_device_init_by_idx(struct device *dev,
>  				       struct device_node *np, int idx);
>  void of_reserved_mem_device_release(struct device *dev);
>  
> +struct reserved_mem *of_get_reserved_mem_by_idx(struct device_node *np, int idx);
> +
>  int early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
>  					     phys_addr_t align,
>  					     phys_addr_t start,
> @@ -52,6 +54,12 @@ static inline int of_reserved_mem_device_init_by_idx(struct device *dev,
>  }
>  static inline void of_reserved_mem_device_release(struct device *pdev) { }
>  
> +static inline struct reserved_mem *of_get_reserved_mem_by_idx(struct device_node *np,
> +							      int idx);
> +{
> +	return NULL;
> +}
> +
>  static inline void fdt_init_reserved_mem(void) { }
>  static inline void fdt_reserved_mem_save_node(unsigned long node,
>  		const char *uname, phys_addr_t base, phys_addr_t size) { }
> 

Reviewed-by: Frank Rowand <frank.rowand@sony.com>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bjorn,

On 4/22/2017 11:05 PM, Bjorn Andersson wrote:
> The rfsa driver is used for allocating and exposing regions of shared
> memory with remote processors for the purpose of exchanging sector-data
> between the remote filesystem service and its clients.
> 
> It provides accessors for the properties needed by the user space remote
> filesystem implementation through sysfs and a character device that can be used
> to read and write the requested chunks of data.
> 

couple of minor nits.

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/soc/qcom/Kconfig  |   8 ++
>  drivers/soc/qcom/Makefile |   1 +
>  drivers/soc/qcom/rfsa.c   | 261 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/soc/qcom/rfsa.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 9fca977ef18d..788a63cd430e 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -24,6 +24,14 @@ config QCOM_PM
>  	  modes. It interface with various system drivers to put the cores in
>  	  low power modes.
>  
> +config QCOM_RFSA
> +	tristate "Qualcomm Remote Filesystem Access driver"

 depends on ARCH_QCOM ?

> +	help
> +	  The Qualcomm remote filesystem access driver is used for allocating
> +	  and exposing regions of shared memory with remote processors for the
> +	  purpose of exchanging sector-data between the remote filesystem
> +	  service and its clients.
> +
>  config QCOM_SMEM
>  	tristate "Qualcomm Shared Memory Manager (SMEM)"
>  	depends on ARCH_QCOM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 414f0de274fa..d1bbc791ddc0 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>  obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
>  obj-$(CONFIG_QCOM_PM)	+=	spm.o
> +obj-$(CONFIG_QCOM_RFSA)	+=	rfsa.o
>  obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
>  obj-$(CONFIG_QCOM_SMEM) +=	smem.o
>  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> diff --git a/drivers/soc/qcom/rfsa.c b/drivers/soc/qcom/rfsa.c
> new file mode 100644
> index 000000000000..1b79976dad9d
> --- /dev/null
> +++ b/drivers/soc/qcom/rfsa.c
> @@ -0,0 +1,261 @@
> +/*
> + * Copyright (c) 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/cdev.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/of_fdt.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +
> +#define QCOM_RFSA_DEV_MAX	(MINORMASK + 1)
> +
> +static dev_t qcom_rfsa_major;
> +
> +struct qcom_rfsa {
> +	struct device dev;
> +	struct cdev cdev;
> +
> +	void *base;
> +	phys_addr_t addr;
> +	phys_addr_t size;
> +
> +	unsigned int client_id;
> +};
> +
> +static ssize_t qcom_rfsa_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf);
> +
> +static DEVICE_ATTR(phys_addr, 0400, qcom_rfsa_show, NULL);
> +static DEVICE_ATTR(size, 0400, qcom_rfsa_show, NULL);
> +static DEVICE_ATTR(client_id, 0400, qcom_rfsa_show, NULL);
> +
> +static ssize_t qcom_rfsa_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct qcom_rfsa *rfsa = container_of(dev, struct qcom_rfsa, dev);
> +
> +	if (attr == &dev_attr_phys_addr)
> +		return sprintf(buf, "%pa\n", &rfsa->addr);
> +	if (attr == &dev_attr_size)
> +		return sprintf(buf, "%pa\n", &rfsa->size);
> +	if (attr == &dev_attr_client_id)
> +		return sprintf(buf, "%d\n", rfsa->client_id);
> +
> +	return -EINVAL;
> +}
> +
> +static struct attribute *qcom_rfsa_attrs[] = {
> +	&dev_attr_phys_addr.attr,
> +	&dev_attr_size.attr,
> +	&dev_attr_client_id.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(qcom_rfsa);
> +
> +static int qcom_rfsa_open(struct inode *inode, struct file *filp)
> +{
> +	struct qcom_rfsa *rfsa = container_of(inode->i_cdev, struct qcom_rfsa, cdev);
> +
> +	get_device(&rfsa->dev);
> +	filp->private_data = rfsa;
> +
> +	return 0;
> +}
> +static ssize_t qcom_rfsa_read(struct file *filp,
> +			      char __user *buf, size_t count, loff_t *f_pos)
> +{
> +	struct qcom_rfsa *rfsa = filp->private_data;
> +
> +	if (*f_pos >= rfsa->size)
> +		return 0;
> +
> +	if (*f_pos + count >= rfsa->size)
> +		count = rfsa->size - *f_pos;
> +
> +	if (copy_to_user(buf, rfsa->base + *f_pos, count))
> +		return -EFAULT;
> +
> +	*f_pos += count;
> +	return count;
> +}
> +
> +static ssize_t qcom_rfsa_write(struct file *filp,
> +			       const char __user *buf, size_t count,
> +			       loff_t *f_pos)
> +{
> +	struct qcom_rfsa *rfsa = filp->private_data;
> +
> +	if (*f_pos >= rfsa->size)
> +		return 0;
> +
> +	if (*f_pos + count >= rfsa->size)
> +		count = rfsa->size - *f_pos;
> +
> +	if (copy_from_user(rfsa->base + *f_pos, buf, count))
> +		return -EFAULT;
> +
> +	*f_pos += count;
> +	return count;
> +}
> +
> +static int qcom_rfsa_release(struct inode *inode, struct file *filp)
> +{
> +	struct qcom_rfsa *rfsa = filp->private_data;
> +
> +	put_device(&rfsa->dev);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations qcom_rfsa_fops = {
> +	.owner = THIS_MODULE,
> +	.open = qcom_rfsa_open,
> +	.read = qcom_rfsa_read,
> +	.write = qcom_rfsa_write,
> +	.release = qcom_rfsa_release,
> +	.llseek = default_llseek,
> +};
> +
> +static void qcom_rfsa_release_device(struct device *dev)
> +{
> +	struct qcom_rfsa *rfsa = container_of(dev, struct qcom_rfsa, dev);
> +
> +	kfree(rfsa);
> +}
> +
> +static int qcom_rfsa_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct reserved_mem *rmem;
> +	struct qcom_rfsa *rfsa;
> +	u32 client_id;
> +	int ret;
> +
> +	rmem = of_get_reserved_mem_by_idx(node, 0);
> +	if (!rmem) {
> +		dev_err(&pdev->dev, "failed to acquire memory region\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(node, "qcom,client-id", &client_id);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to parse \"qcom,client-id\"\n");
> +		return ret;
> +
> +	}
> +
> +	rfsa = kzalloc(sizeof(*rfsa), GFP_KERNEL);
> +	if (!rfsa)
> +		return -ENOMEM;
> +
> +	rfsa->addr = rmem->base;
> +	rfsa->client_id = client_id;
> +	rfsa->size = rmem->size;
> +
> +	device_initialize(&rfsa->dev);
> +	rfsa->dev.parent = &pdev->dev;
> +	rfsa->dev.groups = qcom_rfsa_groups;
> +
> +	cdev_init(&rfsa->cdev, &qcom_rfsa_fops);
> +	rfsa->cdev.owner = THIS_MODULE;
> +
> +	dev_set_name(&rfsa->dev, "qcom_rfsa%d", client_id);
> +	rfsa->dev.id = client_id;
> +	rfsa->dev.devt = MKDEV(MAJOR(qcom_rfsa_major), client_id);
> +
> +	ret = cdev_device_add(&rfsa->cdev, &rfsa->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add cdev: %d\n", ret);
> +		put_device(&rfsa->dev);
> +		return ret;
> +	}
> +
> +	rfsa->dev.release = qcom_rfsa_release_device;
> +
> +	rfsa->base = devm_memremap(&rfsa->dev, rfsa->addr, rfsa->size, MEMREMAP_WC);
> +	if (IS_ERR(rfsa->base)) {
> +		dev_err(&pdev->dev, "failed to remap rfsa region\n");
> +
> +		device_del(&rfsa->dev);
> +		put_device(&rfsa->dev);
> +
> +		return PTR_ERR(rfsa->base);
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, rfsa);
> +
> +	return 0;
> +}
> +
> +static int qcom_rfsa_remove(struct platform_device *pdev)
> +{
> +	struct qcom_rfsa *rfsa = dev_get_drvdata(&pdev->dev);
> +
> +	cdev_del(&rfsa->cdev);
> +	device_del(&rfsa->dev);

cdev_device_del instead of the above two ?

Regards,
 Sricharan

On Sat, Apr 22, 2017 at 10:35:17AM -0700, Bjorn Andersson wrote:
> This adds the binding for describing shared memory buffers for
> implementing the remote filesystem protocol.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> My initial attempt was to mimic the ramoops of just adding the compatible to
> the reserved-memory node, but I have not been able to figure out a sane way of
> getting hold of the base address in the case that the memory region is
> described my a "size" only (done on some platforms).

I prefer the ramoops way.

> The problem is that we create the reserved_mem objects (and remove the
> memblocks) while we're still operating on the flattened representation, so
> without a phandle it doesn't seem like we have anything to perform the
> comparison with later on.

I'm not sure I follow.

> 
>  .../devicetree/bindings/soc/qcom/qcom,rfsa.txt     | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> new file mode 100644
> index 000000000000..b4de0de74e46
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> @@ -0,0 +1,43 @@
> +Qualcomm Remote File System Access binding
> +
> +This binding describes the Qualcomm RFSA, which serves the purpose of managing
> +the shared memory region used for remote processors to access block device data
> +using the Remote Filesystem protocol.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be:
> +		    "qcom,rfsa"

No versioning?

> +
> +- memory-region:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: handle to memory reservation the associated rfsa region.
> +
> +- qcom,client-id:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: identifier of the client to use this region for buffers.

What determines these numbers?

> +
> += EXAMPLE
> +The following example shows the RFSA setup for APQ8016, with the RFSA region
> +for the Hexagon DSP (id #1) located at 0x86700000.
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		rmtfs: rmtfs@86700000 {

I think you should have a compatible here even with the 2 node approach.

> +			reg = <0x0 0x86700000 0x0 0xe0000>;
> +			no-map;
> +		};
> +	};
> +
> +	hexagon-rfsa {
> +		compatible = "qcom,rfsa";
> +		memory-region = <&rmtfs>;
> +
> +		qcom,client-id = <1>;
> +	};
> -- 
> 2.12.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri 28 Apr 10:42 PDT 2017, Rob Herring wrote:

> On Sat, Apr 22, 2017 at 10:35:17AM -0700, Bjorn Andersson wrote:
> > This adds the binding for describing shared memory buffers for
> > implementing the remote filesystem protocol.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > My initial attempt was to mimic the ramoops of just adding the compatible to
> > the reserved-memory node, but I have not been able to figure out a sane way of
> > getting hold of the base address in the case that the memory region is
> > described my a "size" only (done on some platforms).
> 
> I prefer the ramoops way.
> 

Me too :)

> > The problem is that we create the reserved_mem objects (and remove the
> > memblocks) while we're still operating on the flattened representation, so
> > without a phandle it doesn't seem like we have anything to perform the
> > comparison with later on.
> 
> I'm not sure I follow.
> 

In my first attempt I extended of_platform_default_populate_init() to
also probe my platform_driver and like ramoops acquired the values of
reg and memremap() these. This works fine.


But for some platforms the memory-region doesn't need a fixed location,
it's just required to be a consecutive chunk of physical ram. So I
replace the "reg = <>" with a "size = <>", this causes
__reserved_mem_alloc_size() to carve out a chunk of memory somewhere
and update the associated reserved_mem entry. The reserved_mem will be
populated with the phandle from the [flattened] of_node.

Later my platform_device is instantiated and I need to figure out the
base address that was picked earlier - so that I know which region to
memremap().

But as my "rfsa-node" stands on its own it will not have any "phandle",
so the reserved_mem->phandle will remain 0. Further more I only have the
unflattened of_node - so I can't just compare the address with the
flattened version previously used.

So the problem at hand is how to match my pdev->dev.of_node to an entry
in the reserved_mem array (in of_reserved_mem.c).

> > 
> >  .../devicetree/bindings/soc/qcom/qcom,rfsa.txt     | 43 ++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> > new file mode 100644
> > index 000000000000..b4de0de74e46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> > @@ -0,0 +1,43 @@
> > +Qualcomm Remote File System Access binding
> > +
> > +This binding describes the Qualcomm RFSA, which serves the purpose of managing
> > +the shared memory region used for remote processors to access block device data
> > +using the Remote Filesystem protocol.
> > +
> > +- compatible:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: must be:
> > +		    "qcom,rfsa"
> 
> No versioning?
> 

The binding is used to describe a chunk of memory and an associated
client-id of this memory, so I'm not sure if we need a version.

> > +
> > +- memory-region:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: handle to memory reservation the associated rfsa region.
> > +
> > +- qcom,client-id:
> > +	Usage: required
> > +	Value type: <u32>
> > +	Definition: identifier of the client to use this region for buffers.
> 
> What determines these numbers?
> 

The bigger picture of this is that the remote processors (e.g. modem)
needs access to block storage, so it sends request to a service on the
system with storage access (i.e the application processor) which will
read and write from the file system, storing blocks of data in the
rfsa-memory.

So the client-id is the hard coded identifier of each such remote system
requesting IO - each one with its own memory carveout.


In later platforms we need to configure the SMMU for each remote in
order to give them access to these cerveouts, so I don't see that its
possible to mash them together in one chunk and handle the client-id
thing only in the application.

> > +
> > += EXAMPLE
> > +The following example shows the RFSA setup for APQ8016, with the RFSA region
> > +for the Hexagon DSP (id #1) located at 0x86700000.
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		rmtfs: rmtfs@86700000 {
> 
> I think you should have a compatible here even with the 2 node approach.
> 

I'm hoping we can figure out a way to fix the Linux implementation so
that we can describe it fully in one node.

> > +			reg = <0x0 0x86700000 0x0 0xe0000>;
> > +			no-map;
> > +		};
> > +	};
> > +
> > +	hexagon-rfsa {
> > +		compatible = "qcom,rfsa";
> > +		memory-region = <&rmtfs>;
> > +
> > +		qcom,client-id = <1>;
> > +	};
> > -- 
> > 2.12.0
> > 

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

On Sat, Apr 22, 2017 at 10:35:18AM -0700, Bjorn Andersson wrote:
> In some cases drivers referencing a reserved-memory region might want to
> remap the entire region, but when defining the reserved-memory by "size"
> the client driver has no means to know the associated base address of
> the reserved memory region.
> 
> This patch adds an accessor for such drivers to acquire a handle to
> their associated reserved-memory for this purpose.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Andy Gross <andy.gross@linaro.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html