[RFC 0/4] STE-modem remoteproc driver

[RFC 0/4] STE-modem remoteproc driver

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Hi Ohad,

This is the initial stab on the remoteproc driver for ST-Ericsson modems.
Please let me know what you think.

remoteproc_ste_modem features:
For kicking to work I need to declare what IDs I'm currently using, so I had to
iterate over the rproc->notifyids to find the IDs. 

A sysfs file is introduced for switching on/off the modem, as modem
start-up must be controlled from user space. 

As mentioned before I need the firmware to be allocated first in the shared
memory are. So in order to reserve  space for firmware at the start of the
memory region, I had to allocate dma memory before calling rproc_register(),
and then releasing it afterward.


I do not yet have an implementation of the stemod_kick module available. It
might take a while before this is upstreamed. The stemod_kick functions are
looked up dynamically so this is not a build issue.

The platform device is not yet upstreamed either. But it should be pretty simple,
it basically just provides the shared memory area to the driver.

I also included one fix in this patch-set. It doesn't seems like carveout for
non-iommu is working correctly. I added a simple patch for this.
Let me know what you think, and how to proceed for this fix.

Best regards,
Sjur Brændeland

Sjur Brændeland (4):
  remoteproc: Bugfix assign device address to carveout (noiommu)
  remoteproc: Add custom STE-modem firmware loader.
  remoteproc: Add API (header file) for kicking STE modems
  remoteproc: Add driver for STE Modem

 drivers/remoteproc/Kconfig                       |   10 +
 drivers/remoteproc/Makefile                      |    3 +
 drivers/remoteproc/remoteproc_core.c             |    4 +
 drivers/remoteproc/remoteproc_internal.h         |    1 +
 drivers/remoteproc/remoteproc_ste_modem_loader.c |  179 ++++++++++++
 drivers/remoteproc/ste_modem_rproc.c             |  333 ++++++++++++++++++++++
 drivers/remoteproc/stemod_kick.h                 |  113 ++++++++
 7 files changed, 643 insertions(+), 0 deletions(-)
 create mode 100644 drivers/remoteproc/remoteproc_ste_modem_loader.c
 create mode 100644 drivers/remoteproc/ste_modem_rproc.c
 create mode 100644 drivers/remoteproc/stemod_kick.h

-- 
1.7.5.4

[RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Carveout did not set device address when IOMMU is not supported.
Fix this by assigning dma address to device address to carveout
if IOMMU is not supported.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index bee4644..8185c11 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -628,6 +628,10 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		 * dual M3 subsystem).
 		 */
 		rsc->pa = dma;
+	} else {
+		rsc->da = dma;
+		dev_dbg(dev, "carveout: %s va %p dma %x\n",
+					rsc->name, va, rsc->da);
 	}
 
 	carveout->va = va;
-- 
1.7.5.4

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Ohad Ben-Cohen]

Hi Sjur,

On Fri, Jun 22, 2012 at 5:31 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Carveout did not set device address when IOMMU is not supported.
> Fix this by assigning dma address to device address to carveout
> if IOMMU is not supported.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/remoteproc/remoteproc_core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index bee4644..8185c11 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -628,6 +628,10 @@ static int rproc_handle_carveout(struct rproc *rproc,
>                  * dual M3 subsystem).
>                  */
>                 rsc->pa = dma;
> +       } else {
> +               rsc->da = dma;
> +               dev_dbg(dev, "carveout: %s va %p dma %x\n",
> +                                       rsc->name, va, rsc->da);

To simplify things, will the below work for you?

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index c4d4a21..f03d074 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -713,23 +713,27 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		list_add_tail(&mapping->node, &rproc->mappings);

 		dev_dbg(dev, "carveout mapped 0x%x to 0x%x\n", rsc->da, dma);
-
-		/*
-		 * Some remote processors might need to know the pa
-		 * even though they are behind an IOMMU. E.g., OMAP4's
-		 * remote M3 processor needs this so it can control
-		 * on-chip hardware accelerators that are not behind
-		 * the IOMMU, and therefor must know the pa.
-		 *
-		 * Generally we don't want to expose physical addresses
-		 * if we don't have to (remote processors are generally
-		 * _not_ trusted), so we might want to do this only for
-		 * remote processor that _must_ have this (e.g. OMAP4's
-		 * dual M3 subsystem).
-		 */
-		rsc->pa = dma;
 	}

+	/*
+	 * Some remote processors might need to know the pa
+	 * even though they are behind an IOMMU. E.g., OMAP4's
+	 * remote M3 processor needs this so it can control
+	 * on-chip hardware accelerators that are not behind
+	 * the IOMMU, and therefor must know the pa.
+	 *
+	 * Generally we don't want to expose physical addresses
+	 * if we don't have to (remote processors are generally
+	 * _not_ trusted), so we might want to do this only for
+	 * remote processor that _must_ have this (e.g. OMAP4's
+	 * dual M3 subsystem).
+	 *
+	 * Non-IOMMU processors might also want to have this info.
+	 * In this case, the device address and the physical address
+	 * are the same.
+	 */
+	rsc->pa = dma;
+
 	carveout->va = va;
 	carveout->len = rsc->len;
 	carveout->dma = dma;

Thanks,
Ohad.

RE: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Sjur BRENDELAND]

Hi Ohad,
 
> To simplify things, will the below work for you?

Looks good, feel free to add my Acked-by

Regards,
Sjur

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Ohad Ben-Cohen]

Hi Sjur,

On Mon, Jul 2, 2012 at 6:11 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
>> To simplify things, will the below work for you?
>
> Looks good, feel free to add my Acked-by

Thanks! I'll queue it up for 3.6.

Ohad.

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Ohad Ben-Cohen]

On Mon, Jul 2, 2012 at 6:14 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Mon, Jul 2, 2012 at 6:11 PM, Sjur BRENDELAND
> <sjur.brandeland@stericsson.com> wrote:
>>> To simplify things, will the below work for you?
>>
>> Looks good, feel free to add my Acked-by
>
> Thanks! I'll queue it up for 3.6.

Applied.

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Sjur Brændeland]

Hi Ohad,

> Carveout did not set device address when IOMMU is not supported.
> Fix this by assigning dma address to device address to carveout
> if IOMMU is not supported.

I realize that we have the same issue with the virtio rings.
Are there any way we can assign the device address of the virtio rings
to the resource table in shared memory? Or do we have to calculate the
virtio ring addresses based on number rings and the number of elements
in the ring?

Regards,
Sjur

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Ohad Ben-Cohen]

Hi Sjur,

On Wed, Aug 8, 2012 at 7:55 PM, Sjur Brændeland <sjurbren@gmail.com> wrote:
> I realize that we have the same issue with the virtio rings.
> Are there any way we can assign the device address of the virtio rings
> to the resource table in shared memory?

It's a gap we have today, but it should definitely be fixed.

> Or do we have to calculate the
> virtio ring addresses based on number rings and the number of elements
> in the ring?

No, that's not the long term intention. It can be used as an interim
solution, but I expect we do fix this and start supporting dynamic
assignments of the vrings locations at some point.

Thanks,
Ohad.

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Sjur Brændeland]

Hi Ohad,
>> I realize that we have the same issue with the virtio rings.
>> Are there any way we can assign the device address of the virtio rings
>> to the resource table in shared memory?
>
> It's a gap we have today, but it should definitely be fixed.

Ok, good.

Any thoughts on how to go about to fix this?
It does look a more comlex to solve than the carveout,
as the vrings are allocated in the first pass of fw parsing,
but fw is loaded to device memory in the second pass.

Regards,
Sjur

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Ohad Ben-Cohen]

Hi Sjur,

On Thu, Aug 9, 2012 at 11:35 PM, Sjur Brændeland <sjurbren@gmail.com> wrote:
> Any thoughts on how to go about to fix this?

The general direction I have in mind is to put the resource table in
its final location while we do the first pass of fw parsing.

This will solve all sort of open issues we have (or going to have soon):

1. dynamically-allocated address of the vrings can be communicated
2. vdev statuses can be communicated
3. virtio config space will finally become bi-directional as it should
4. dynamically probed rproc-to-rproc IPC could then take place

It's the real deal :)

The only problem with this approach is that the resource table isn't
reloaded throughout cycles of power up/down, and that is insecure.
We'll have to manually reload it somewhere after the rproc is powered
down (or before it is powered up again).

This change will break existing firmwares, but it looks required and inevitable.

Thanks,
Ohad.

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Ohad Ben-Cohen]

On Fri, Aug 10, 2012 at 6:30 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> This will solve all sort of open issues we have (or going to have soon):
>
> 1. dynamically-allocated address of the vrings can be communicated
> 2. vdev statuses can be communicated
> 3. virtio config space will finally become bi-directional as it should
> 4. dynamically probed rproc-to-rproc IPC could then take place

and 5. let the remote processor know about the notifyids that we've
dynamically allocated.

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Sjur Brændeland]

Hi Ohad,

On Sat, Aug 11, 2012 at 2:34 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Fri, Aug 10, 2012 at 6:30 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> This will solve all sort of open issues we have (or going to have soon):
>>
...
> 1. dynamically-allocated address of the vrings can be communicated
> 2. vdev statuses can be communicated
> 3. virtio config space will finally become bi-directional as it should

I just posted a RFC patch-set on this to linux-kernel@vger.kernel.org.
Review comments is welcomed :-)

> and 5. let the remote processor know about the notifyids that we've
> dynamically allocated.

I have run into one issue with the dynamically allocated notifyids. When
allocating multiple virtio devices, only ids for the first virtio
device is allocated
when the remote device is started.  This is fails for me, because I need to know
all of the kick-ids before starting the remote device.

Do you have any preference on how to solve this, Ohad?

One idea could be to call rproc->start from a work-queue and wait for completion
of firmware loading before actually starting. In this way we will not
trigger start-up
of the remote device before all virtio devices and their kick-ids are allocated.

Thanks,
Sjur

Re: [RFC 1/4] remoteproc: Bugfix assign device address to carveout (noiommu)

[Ohad Ben-Cohen]

Hi Sjur,

On Thu, Dec 6, 2012 at 9:45 PM, Sjur Brændeland <sjurbren@gmail.com> wrote:
> I just posted a RFC patch-set on this to linux-kernel@vger.kernel.org.
> Review comments is welcomed :-)

Great, will take a look, thanks a lot!

> I have run into one issue with the dynamically allocated notifyids. When
> allocating multiple virtio devices, only ids for the first virtio
> device is allocated
> when the remote device is started.  This is fails for me, because I need to know
> all of the kick-ids before starting the remote device.
>
> Do you have any preference on how to solve this, Ohad?
>
> One idea could be to call rproc->start from a work-queue and wait for completion
> of firmware loading before actually starting. In this way we will not
> trigger start-up
> of the remote device before all virtio devices and their kick-ids are allocated.

Yes, it would work, but I'm not sure it would be very intuitive for
others to understand why we spawn a new context upon powering on the
processor.

Seems to me we may want instead to fix the problem directly by
splitting rproc_handle_vdev() to two stages, where the second stage
will only include the rproc_add_virtio_dev() part, and will be invoked
after all vdev have been processed.

Will that work for you ?

Thanks,
Ohad.

[RFC 2/4] remoteproc: Add custom STE-modem firmware loader.

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add custom firmware loader for STE firmware. This plugin adds
functions for extracting the resource table and loading the
firmware image into shared memory.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/Kconfig                       |   10 ++
 drivers/remoteproc/Makefile                      |    2 +
 drivers/remoteproc/remoteproc_internal.h         |    1 +
 drivers/remoteproc/remoteproc_ste_modem_loader.c |  179 ++++++++++++++++++++++
 4 files changed, 192 insertions(+), 0 deletions(-)
 create mode 100644 drivers/remoteproc/remoteproc_ste_modem_loader.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 24d880e..9beb5ed 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -25,4 +25,14 @@ config OMAP_REMOTEPROC
 	  It's safe to say n here if you're not interested in multimedia
 	  offloading or just want a bare minimum kernel.
 
+config STE_MODEM_RPROC
+	tristate "STE-Modem remoteproc support"
+	select REMOTEPROC
+	depends on EXPERIMENTAL
+	default n
+	help
+	  Say y or m here to support STE-Modem shared memory driver.
+	  This can be either built-in or a loadable module.
+	  If unsure say N.
+
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 934ce6e..b91ecb0b 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -8,3 +8,5 @@ remoteproc-y				+= remoteproc_debugfs.o
 remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
+obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_remoteproc.o
+ste_modem_remoteproc-y 			+= remoteproc_ste_modem_loader.o
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 7823156..bc36302 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -80,5 +80,6 @@ struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
 }
 
 extern const struct rproc_fw_ops rproc_elf_fw_ops;
+extern const struct rproc_fw_ops rproc_ste_modem_fw_ops;
 
 #endif /* REMOTEPROC_INTERNAL_H */
diff --git a/drivers/remoteproc/remoteproc_ste_modem_loader.c b/drivers/remoteproc/remoteproc_ste_modem_loader.c
new file mode 100644
index 0000000..139222d
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_ste_modem_loader.c
@@ -0,0 +1,179 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Author: Sjur Brendeland / sjur.brandeland@stericsson.com
+ * License terms: GNU General Public License (GPL) version 2.
+ */
+
+#define pr_fmt(fmt)	"%s: " fmt, __func__
+#include <linux/module.h>
+#include <linux/firmware.h>
+#include <linux/remoteproc.h>
+#include "remoteproc_internal.h"
+
+#define TOC_RSC_TAB_NAME "rsc-table"
+
+/* struct toc_entry - Table of content entry
+ *
+ * @start: Offset to the image data.
+ * @size:  Size of the images in bytes.
+ * @flags: Use 0 if no flags are in use.
+ * @entry_point: Modem internal information. Where to jump to start executing.
+ *		 Only applicable when using SDRAM. Set to 0xffffffff if unused.
+ * @load_addr: Modem internal information. Location in SDRAM to move image.
+ *		Set to 0xffffffff if not applicable.
+ * @name: Name of image.
+ */
+struct toc_entry {
+	__le32 start;
+	__le32 size;
+	__le32 flags;
+	__le32 entry_point;
+	__le32 load_addr;
+	char name[12];
+};
+
+/** struct toc - Table of content
+ * @table: Table of toc entries.
+ * The Table Of Content is located at the start of the firmware image and
+ * at offset zero in the shared memory region. The resource table typically
+ * contains the initial boot image (boot strap) and other information elements
+ * such as remoteproc resource table. Each entry is identified by a unique
+ * @name.
+ */
+struct toc {
+	struct toc_entry table[32];
+};
+
+/**
+ * ste_load_segments() - load firmware segments to memory
+ * @rproc: remote processor which will be booted using these fw segments
+ * @fw: the TOC and firmware image to load
+ *
+ * This function loads the firmware segments to memory. STE Modem SHM
+ * does not use an IOMMU, and expects the firmware containing the
+ * "Table Of Content" (TOC) first in the firmware. The TOC specifies the
+ * offset and size of the boot image.
+ */
+static int
+ste_load_segments(struct rproc *rproc, const struct firmware *fw)
+{
+	/*
+	 * STE-Modem does not use a IOMMU and the virtual and physical
+	 * addresses of the device (modem) is not known. Instead
+	 * offsets from the start of the shared memory region is used
+	 * as device address (da).
+	 *
+	 * The STE-Modem must provide a carveout as the first entry with
+	 * sufficient space for the firmware image. The device address in
+	 * this carveout must be set to zero.
+	 *
+	 * The firmware must be copied into offset zero, i.e at the start of
+	 * the shared memory area.
+	 */
+
+	u32 offset = 0;
+	void *ptr = rproc_da_to_va(rproc, offset, fw->size);
+
+	if (!ptr) {
+		dev_err(rproc->dev, "bad offset 0x%x mem 0x%zx\n",
+			offset, fw->size);
+		return -EINVAL;
+	}
+
+	memcpy(ptr, fw->data, fw->size);
+	return 0;
+}
+
+/* Find the entry for resource table in the Table of Content */
+static struct toc_entry *__find_rsc_entry(const struct firmware *fw)
+{
+	int i;
+	struct toc *toc;
+	int entries = ARRAY_SIZE(toc->table);
+
+	if (!fw)
+		return NULL;
+
+	toc = (void *)fw->data;
+
+	/* Search the table for the resource table */
+	for (i = 0; i < entries && toc->table[i].start != 0xffffffff; i++) {
+		if (!strncmp(toc->table[i].name, TOC_RSC_TAB_NAME,
+			     sizeof(toc->table[i].name))) {
+			if (toc->table[i].start > fw->size)
+				return NULL;
+			return &toc->table[i];
+		}
+	}
+	return NULL;
+}
+
+/**
+ * ste_find_rsc_table() - find the resource table
+ * @rproc: the rproc handle
+ * @fw: the firmware image
+ * @tablesz: place holder for providing back the table size
+ *
+ * This function finds the resource table inside the remote processor's
+ * firmware. It is used both upon the registration of @rproc (in order
+ * to look for and register the supported virito devices), and when the
+ * @rproc is booted.
+ *
+ * Returns the pointer to the resource table if it is found, and write its
+ * size into @tablesz. If a valid table isn't found, NULL is returned
+ * (and @tablesz isn't set).
+ */
+static struct resource_table *
+ste_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
+							int *tablesz)
+{
+	struct resource_table *table;
+	struct device *dev = rproc->dev;
+	struct toc_entry *entry = __find_rsc_entry(fw);
+
+	if (!entry) {
+		dev_err(dev, "resource table not found in fw\n");
+		return NULL;
+	}
+
+	table = (void *) (fw->data + entry->start);
+
+	/* make sure we have the entire table */
+	if (entry->start + entry->size > fw->size) {
+		dev_err(dev, "resource table truncated\n");
+		return NULL;
+	}
+
+	/* make sure table has at least the header */
+	if (sizeof(struct resource_table) > entry->size) {
+		dev_err(dev, "header-less resource table\n");
+		return NULL;
+	}
+
+	/* we don't support any version beyond the first */
+	if (table->ver != 1) {
+		dev_err(dev, "unsupported fw ver: %d\n", table->ver);
+		return NULL;
+	}
+
+	/* make sure reserved bytes are zeroes */
+	if (table->reserved[0] || table->reserved[1]) {
+		dev_err(dev, "non zero reserved bytes\n");
+		return NULL;
+	}
+
+	/* make sure the offsets array isn't truncated */
+	if (table->num * sizeof(table->offset[0]) +
+	    sizeof(struct resource_table) > entry->size) {
+		dev_err(dev, "resource table incomplete\n");
+		return NULL;
+	}
+
+	*tablesz = entry->size;
+	return table;
+}
+
+const struct rproc_fw_ops rproc_ste_modem_fw_ops = {
+	.load = ste_load_segments,
+	.find_rsc_table = ste_find_rsc_table
+};
-- 
1.7.5.4

[RFC 3/4] remoteproc: Add API (header file) for kicking STE modems

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add the API definition for STE Modem interrupt mecanism (kicks).

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/stemod_kick.h |  113 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 113 insertions(+), 0 deletions(-)
 create mode 100644 drivers/remoteproc/stemod_kick.h

diff --git a/drivers/remoteproc/stemod_kick.h b/drivers/remoteproc/stemod_kick.h
new file mode 100644
index 0000000..dbfa909
--- /dev/null
+++ b/drivers/remoteproc/stemod_kick.h
@@ -0,0 +1,113 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2011
+ * Author: Sjur Brendeland / sjur.brandeland@stericsson.com
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#ifndef __INC_STEMOD_KICK_H
+#define __INC_STEMOD_KICK_H
+#include <linux/types.h>
+
+/**
+ * stemod_kick_subscribe - Subscribe for notification change from the modem.
+ *
+ * @notifyid:	The notification ID the callback is associated with.
+ *
+ * @notify_cb:	Callback function to be called when the requested notify-id
+ *		is set by the external device (modem).
+ *
+ * @data: Client data to be provided in the callback function.
+ *
+ * Install a callback function for a notification ID.
+ * Returns negative upon error.
+ *
+ * This function may block, and cannot be called from IRQ context.
+ * Precondition: @notifyid must be defined as as getter in
+ * stemod_kick_notifyid_alloc().
+ * Returns zero on success, and negative upon error.
+ *
+ * Callback context:
+ *		The "notify_cb" callback might be called from the
+ *		IRQ context. The callback function is not allowed to block
+ *		or spend much CPU time in the callback, It must defer
+ *		work to soft-IRQ or work queues.
+ */
+int stemod_kick_subscribe(int notifyid,
+			  void (*notify_cb)(int notifyid, void *data),
+			  void *data);
+
+/**
+ * stemod_kick_notifyid_alloc - Allocate the usage of notification IDs.
+ *
+ * @rx_mask:	Bit-mask defining the notifyids that can be
+ *			subscribed by stemod_kick_subscribe().
+ * @tx_mask:	Bit-mask defining the notifyids that can be set by
+ *			stemod_kick_set_notifyid()
+ *
+ * The @rx_mask defines the notification-id  that can be subscribed by
+ * the function stemod_kick_subscribe().
+ * The @tx_mask defines the notification-ids that can be set by the
+ * function stemod_kick_set_notifyid().
+ *
+ * This function may block.
+ *
+ * Returns zero on success, and negative upon error.
+ *
+ */
+int stemod_kick_notifyid_alloc(u32 rx_mask, u32 tx_mask);
+
+/**
+ * stemod_kick_register_errhandler - Register an error handler.
+ *
+ * @errhandler: error handler called from driver upon severe errors
+ *		that requires reset of the remote device.
+ */
+void stemod_kick_register_errhandler(void (*errhandler)(int errno));
+
+/**
+ * stemod_kick_reset() - Reset the C2C driver
+ *
+ * Reset the Kick Driver due to remote device (modem) restart.
+ * This shall reset state back to initial state, and should only
+ * be used when remote device (modem) has reset.
+ *
+ * All settings, subscriptions and state information in the driver is
+ * reset.
+ * This function may block.
+ *
+ * Returns zero on success, and negative upon error.
+ */
+int stemod_kick_reset(void);
+
+/**
+ * stemod_kick_notifyid() -	Genereate a notification to remote device.
+ *
+ * @notifyid:	The notification ID to generate interrupt for.
+ *
+ * This function is used to send notification to the remote
+ * processor (modem).
+ *
+ * This function is non-blocking, and can be called from Soft-IRQ context.
+ *
+ * Returns zero on success, and negative upon error.
+ *
+ * Precondition: @notifyid must be defined as as setter in
+ * stemod_kick_notifyid_alloc().
+ */
+int stemod_kick_notifyid(int notifyid);
+
+
+/**
+ * stemod_power() -	On/Off switch for modem
+ *
+ * @on:	Switch on or off
+ *
+ * This function is the power switch for the STE-Modem.
+ *
+ * Returns zero on success, and negative upon error.
+ *
+ */
+int stemod_power(bool on);
+
+#endif /*INC_STEMOD_KICK_H*/
-- 
1.7.5.4

[RFC 4/4] remoteproc: Add driver for STE Modem

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Introduce the platform driver for ste-modems.
This driver uses the remoteproc framework for managing the
modem and for creating virtio devices used for communicating
with the modem.

A sysfs file is introduced for switching on/off the modem, as modem
start-up must be controlled from user space.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/Makefile          |    1 +
 drivers/remoteproc/ste_modem_rproc.c |  333 ++++++++++++++++++++++++++++++++++
 2 files changed, 334 insertions(+), 0 deletions(-)
 create mode 100644 drivers/remoteproc/ste_modem_rproc.c

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index b91ecb0b..aec7470 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,4 +9,5 @@ remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_remoteproc.o
+ste_modem_remoteproc-y 			+= ste_modem_rproc.o
 ste_modem_remoteproc-y 			+= remoteproc_ste_modem_loader.o
diff --git a/drivers/remoteproc/ste_modem_rproc.c b/drivers/remoteproc/ste_modem_rproc.c
new file mode 100644
index 0000000..a575e2a
--- /dev/null
+++ b/drivers/remoteproc/ste_modem_rproc.c
@@ -0,0 +1,333 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Author: Sjur Brændeland <sjur.brandeland@stericsson.com>
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ":%s(): " fmt, __func__
+
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/remoteproc.h>
+#include <linux/dma-mapping.h>
+#include "remoteproc_internal.h"
+#include "stemod_kick.h"
+
+/* Maxium number of "channels" */
+#define MAX_VQS 14
+
+/* Maxium size of the STE firmwaree image 160Kb */
+#define STEMOD_FW_SIZE (40 * 4096)
+
+struct sproc {
+	struct rproc *rproc;
+	/* Number of kick identifiers needed */
+	u32 nvqs;
+	/* Track the user requested power state */
+	bool powered;
+	int (*kick_modem)(int notifyid);
+};
+
+/* kick a virtqueue */
+static void ste_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct sproc *sproc = rproc->priv;
+	dev_dbg(rproc->dev, "kick vqid:%d\n", vqid);
+	sproc->kick_modem(vqid + MAX_VQS);
+}
+
+/* modem is kicking us */
+static void ste_rproc_callback(int vqid, void *data)
+{
+	struct sproc *sproc = data;
+	if (rproc_vq_interrupt(sproc->rproc, vqid) == IRQ_NONE)
+		dev_dbg(sproc->rproc->dev,
+			"no message was found in vqid %d\n", vqid);
+}
+
+/* Iterator called by idr_for_each(), tracking notification Ids */
+static int ste_rproc_set_vqid(int id, void *p, void *data)
+{
+	u32 *mask = data;
+	*mask |= 1 << id;
+	return 0;
+}
+
+/* Setup the kick API for notification subscriptions */
+static int ste_rproc_subscribe_to_kicks(struct rproc *rproc)
+{
+	int i, err;
+	u32 txmask = 0, rxmask = 0;
+	int (*kick_subscribe)(int notifyid,
+		       void (*notify_cb)(int notifyid, void *data), void *data);
+	int (*notifyid_alloc)(u32 setter_mask, u32 getter_mask);
+
+	/*
+	 * We need to declare for the HW what notification IDs
+	 * we're going to use. So we create a bitmask with the
+	 * notification IDs used for RX and TX by iterating over
+	 * the notification IDs.
+	 */
+	idr_for_each(&rproc->notifyids, ste_rproc_set_vqid, &rxmask);
+
+	/* Verify that notification ID is in the range 0-13 */
+	if (rxmask & 0x3fff) {
+		dev_err(rproc->dev, "Bad Notification IDs used: %x\n", rxmask);
+		return -EINVAL;
+	}
+	/*
+	 * Bits 0-13 are used for RX kicks and bits 14-27 for TX.
+	 * We simply set TX notification ID to be RX notification ID + 14.
+	 */
+	txmask = rxmask << MAX_VQS;
+
+	notifyid_alloc = symbol_get(stemod_kick_notifyid_alloc);
+	if (notifyid_alloc == NULL)
+		return -EINVAL;
+
+	/* Tell kick-hw what bit we use for RX and TX */
+	err = notifyid_alloc(rxmask, txmask);
+	symbol_put(stemod_kick_notifyid_alloc);
+
+	if (err < 0) {
+		dev_err(rproc->dev, "allocation of bits %x/%x failed:%d\n",
+			rxmask, txmask, err);
+		return err;
+	}
+
+	kick_subscribe = symbol_get(stemod_kick_subscribe);
+	if (kick_subscribe == NULL)
+		return -EINVAL;
+
+	/* Subscribe for RX interrupts */
+	for (err = 0, i = 0; i < MAX_VQS && !err; i++)
+		if (rxmask & (1 << i))
+			err = kick_subscribe(i, ste_rproc_callback,
+						    rproc->priv);
+	symbol_put(stemod_kick_subscribe);
+	if (err) {
+		dev_err(rproc->dev, "subscription of kicks failed:%d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+int power_switch(bool on)
+{
+	int err = 0;
+	int (*power)(bool on);
+
+	power = symbol_get(stemod_power);
+	if (power == NULL)
+		err =  -EINVAL;
+	err = power(on);
+	symbol_put(stemod_power);
+	return err;
+}
+
+/* Start the STE modem */
+static int ste_rproc_start(struct rproc *rproc)
+{
+	int err;
+	dev_info(rproc->dev, "start modem\n");
+
+	err = ste_rproc_subscribe_to_kicks(rproc->priv);
+	if (err)
+		return err;
+
+	/* Power on modem */
+	return power_switch(true);
+}
+
+/* Stop the STE modem */
+static int ste_rproc_stop(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev;
+	int (*reset)(void);
+
+	dev_info(dev, "stop modem\n");
+
+	/* Reset kick HW */
+	reset = symbol_get(stemod_kick_reset);
+	reset();
+	symbol_put(stemod_kick_reset);
+
+	/* Power off modem */
+	return power_switch(true);
+}
+
+static struct rproc_ops ste_rproc_ops = {
+	.start		= ste_rproc_start,
+	.stop		= ste_rproc_stop,
+	.kick		= ste_rproc_kick,
+};
+
+/* Get ste_rproc given platform device */
+static struct sproc *ste_rproc_get_by_dev(const struct device *dev)
+{
+	struct platform_device *pdev;
+	struct rproc *rproc;
+	pdev = container_of(dev, struct platform_device, dev);
+	rproc = platform_get_drvdata(pdev);
+	if (rproc == NULL)
+		return NULL;
+	return rproc->priv;
+}
+
+/* Read sysfs entry 'powered' */
+static ssize_t ste_rproc_powered_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct sproc *sproc = ste_rproc_get_by_dev(dev);
+	ssize_t size;
+
+	if (sproc == NULL)
+		return -EINVAL;
+
+	size = sprintf(buf, "%d\n", sproc->powered);
+	return size;
+}
+
+/* Write sysfs entry 'powered' */
+static ssize_t ste_rproc_powered_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	unsigned long val;
+	int ret = -EINVAL;
+	struct sproc *sproc = ste_rproc_get_by_dev(dev);
+
+	if (sproc == NULL)
+		return -EINVAL;
+
+	if (kstrtoul(buf, 10, &val) < 0)
+		goto err;
+
+	if (sproc->powered && val == 0) {
+		rproc_shutdown(sproc->rproc);
+		sproc->powered = false;
+	} else if (!sproc->powered && val == 1) {
+
+		if (rproc_boot(sproc->rproc))
+			goto err;
+		sproc->powered = true;
+	} else {
+		dev_err(dev, "invalid sysfs state entered\n");
+		goto err;
+	}
+	ret = count;
+err:
+	return ret;
+}
+
+/* Declare sysfs entry powered */
+static DEVICE_ATTR(powered, S_IRUGO | S_IWUSR | S_IWGRP ,
+			ste_rproc_powered_show, ste_rproc_powered_store);
+struct device_attribute *remoteproc_attrs[] = { &dev_attr_powered };
+
+/* Platform device for STE modem is registered */
+static int __devinit ste_rproc_probe(struct platform_device *pdev)
+{
+	struct sproc *ste_proc;
+	struct rproc *rproc;
+	int ret;
+	void *reserved;
+	dma_addr_t dma;
+
+	/*
+	 * Prerequisite: The platform device must declare the shared
+	 * memory region to be used by STE-modem and make memory
+	 * available for rproc by using  dma_declare_coherent_memory
+	 * (or CMA).
+	 */
+	rproc = rproc_alloc(&pdev->dev,
+			    pdev->name,
+			    &ste_rproc_ops,
+			    "ste-modem-fw",
+			    sizeof(*ste_proc));
+	if (!rproc)
+		return -ENOMEM;
+
+	ste_proc = rproc->priv;
+	ste_proc->rproc = rproc;
+	platform_set_drvdata(pdev, rproc);
+
+	/* Inject the STE-modem specific firmware handler */
+	rproc->fw_ops = &rproc_ste_modem_fw_ops;
+
+	/*
+	 * We're dynamically looking up the symbols for the API used
+	 * for generating kicks to and from the modem.
+	 */
+	ste_proc->kick_modem = symbol_get(stemod_kick_notifyid);
+	if (ste_proc->kick_modem == NULL) {
+		ret =  -EINVAL;
+		dev_err(rproc->dev, "cannot load stemod_kick API\n");
+		goto free_rproc;
+	}
+
+	/*
+	 * Registration of the ste_modem_rproc will cause firmware to
+	 * to be fetched and the virtio resource entries to be allocated
+	 * in memory. However STE-modem requires the firmware to be located
+	 * at the start of the shared memory region. So we need to
+	 * reserve space for firmware at the start of the shared memory
+	 * region.
+	 */
+	reserved = dma_alloc_coherent(&pdev->dev, STEMOD_FW_SIZE,
+				      &dma, GFP_KERNEL);
+
+	ret = rproc_register(rproc);
+	if (ret)
+		goto free_rproc;
+
+	/*
+	 * When firmware loading is completed and virtio resource
+	 * entries are allocated in memory, we can release the
+	 * memory space reserved for modem firmware.
+	 * When user switch on the modem, the firmware will be
+	 * loaded at the start of the memory region.
+	 */
+	wait_for_completion(&rproc->firmware_loading_complete);
+	dma_free_coherent(&pdev->dev, PAGE_SIZE * 10, reserved, dma);
+
+	/* Create powered sysfs entry, to start/stop STE modem */
+	ret = device_create_file(&pdev->dev, &dev_attr_powered);
+	if (ret)
+		goto free_rproc;
+
+	return 0;
+
+free_rproc:
+	platform_set_drvdata(pdev, NULL);
+	rproc_free(rproc);
+	return ret;
+}
+
+/* Platform device for STE modem is unregistered */
+static int __devexit ste_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	symbol_put(stemod_kick_notifyid);
+	return rproc_unregister(rproc);
+}
+
+static struct platform_driver ste_rproc_driver = {
+	.probe = ste_rproc_probe,
+	.remove = __devexit_p(ste_rproc_remove),
+	.driver = {
+		.name = "ste-modem",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(ste_rproc_driver);
+MODULE_LICENSE("GPL v2");
-- 
1.7.5.4

Re: [RFC 4/4] remoteproc: Add driver for STE Modem

[Ohad Ben-Cohen]

Hi Sjur,

On Fri, Jun 22, 2012 at 5:31 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Introduce the platform driver for ste-modems.
> This driver uses the remoteproc framework for managing the
> modem and for creating virtio devices used for communicating
> with the modem.

Cool, thanks for posting this.

> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index b91ecb0b..aec7470 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -9,4 +9,5 @@ remoteproc-y                            += remoteproc_virtio.o
>  remoteproc-y                           += remoteproc_elf_loader.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)          += omap_remoteproc.o
>  obj-$(CONFIG_STE_MODEM_RPROC)          += ste_modem_remoteproc.o
> +ste_modem_remoteproc-y                         += ste_modem_rproc.o
>  ste_modem_remoteproc-y                         += remoteproc_ste_modem_loader.o

Just wondering - do we need the loader ops to be a separate file ? is
it only going to be used by this module or do you foresee additional
users in the future ?

> +/* Maxium number of "channels" */
> +#define MAX_VQS 14

Can you explain a bit the background behind this number, and how vqs
and vqids are generally handled by the ste modem?

> +/* Setup the kick API for notification subscriptions */
> +static int ste_rproc_subscribe_to_kicks(struct rproc *rproc)
> +{
> +       int i, err;
> +       u32 txmask = 0, rxmask = 0;
> +       int (*kick_subscribe)(int notifyid,
> +                      void (*notify_cb)(int notifyid, void *data), void *data);
> +       int (*notifyid_alloc)(u32 setter_mask, u32 getter_mask);
> +
> +       /*
> +        * We need to declare for the HW what notification IDs
> +        * we're going to use.

Can you explain why is this needed ? what happens when we call
notifyid_alloc(rxmask, txmask) ?

> +        * notification IDs used for RX and TX by iterating over
> +        * the notification IDs.
> +        */
> +       idr_for_each(&rproc->notifyids, ste_rproc_set_vqid, &rxmask);

If this really is needed, we might want some help from the remoteproc
framework to do this, because it already has all the required info.

It'd be better if we don't fiddle with its internal structures,
because doing say may make it harder for it to evolve.

> +       notifyid_alloc = symbol_get(stemod_kick_notifyid_alloc);

This module is using dynamic symbol lookup quite a lot. This really
stands out because it's quite uncommon.

Why is it necessary ? Can we use static symbols instead ?

Can you share the code for those symbols as well ?

> +/* Get ste_rproc given platform device */
> +static struct sproc *ste_rproc_get_by_dev(const struct device *dev)
> +{
> +       struct platform_device *pdev;
> +       struct rproc *rproc;
> +       pdev = container_of(dev, struct platform_device, dev);
> +       rproc = platform_get_drvdata(pdev);
> +       if (rproc == NULL)
> +               return NULL;
> +       return rproc->priv;

Might be nicer to just

    return rproc ? rproc->priv : NULL;

> +/* Write sysfs entry 'powered' */
> +static ssize_t ste_rproc_powered_store(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       const char *buf, size_t count)
> +{
> +       unsigned long val;
> +       int ret = -EINVAL;
> +       struct sproc *sproc = ste_rproc_get_by_dev(dev);
> +
> +       if (sproc == NULL)
> +               return -EINVAL;
> +
> +       if (kstrtoul(buf, 10, &val) < 0)
> +               goto err;
> +
> +       if (sproc->powered && val == 0) {
> +               rproc_shutdown(sproc->rproc);
> +               sproc->powered = false;
> +       } else if (!sproc->powered && val == 1) {
> +
> +               if (rproc_boot(sproc->rproc))
> +                       goto err;
> +               sproc->powered = true;
> +       } else {
> +               dev_err(dev, "invalid sysfs state entered\n");
> +               goto err;
> +       }
> +       ret = count;
> +err:
> +       return ret;
> +}
> +
> +/* Declare sysfs entry powered */
> +static DEVICE_ATTR(powered, S_IRUGO | S_IWUSR | S_IWGRP ,
> +                       ste_rproc_powered_show, ste_rproc_powered_store);

There's three issues that bother me here:

1. It feels like this kind of functionality isn't limited to the STE
modem, so we better have it implemented in the remoteproc framework.

2. I'm not convinced that sysfs is an appropriate interface for
controlling the remote processor:

- if the remote processor crashes, the user isn't informed about it,
so it won't refresh its state even if a successful recovery took place
- if the user space crashes, we don't get informed about it, and the
remote processor can erroneously stay powered on indefinitely

It seems to me that a char device will solve these issues: we can use
ioctl to control the power, if the user crashes we'll know about it
via the release handler, and if the remote processor crashes we can
let the user know by sending it a notification for it to read via the
fd.

3. I'm not sure we want to let the user space directly control the
power of the remote processor: what if it decides to power off the
rproc while other kernel users utilize it?

We might want to change the semantics of the interface to just allow
the user to increase/decrease a power reference count instead of
directly booting/shutting it down.

> +struct device_attribute *remoteproc_attrs[] = { &dev_attr_powered };
> +
> +/* Platform device for STE modem is registered */
> +static int __devinit ste_rproc_probe(struct platform_device *pdev)
> +{
> +       struct sproc *ste_proc;
> +       struct rproc *rproc;
> +       int ret;
> +       void *reserved;
> +       dma_addr_t dma;
> +
> +       /*
> +        * Prerequisite: The platform device must declare the shared
> +        * memory region to be used by STE-modem and make memory
> +        * available for rproc by using  dma_declare_coherent_memory
> +        * (or CMA).
> +        */
> +       rproc = rproc_alloc(&pdev->dev,
> +                           pdev->name,
> +                           &ste_rproc_ops,
> +                           "ste-modem-fw",
> +                           sizeof(*ste_proc));
> +       if (!rproc)
> +               return -ENOMEM;
> +
> +       ste_proc = rproc->priv;
> +       ste_proc->rproc = rproc;
> +       platform_set_drvdata(pdev, rproc);
> +
> +       /* Inject the STE-modem specific firmware handler */
> +       rproc->fw_ops = &rproc_ste_modem_fw_ops;
> +
> +       /*
> +        * We're dynamically looking up the symbols for the API used
> +        * for generating kicks to and from the modem.
> +        */
> +       ste_proc->kick_modem = symbol_get(stemod_kick_notifyid);
> +       if (ste_proc->kick_modem == NULL) {
> +               ret =  -EINVAL;
> +               dev_err(rproc->dev, "cannot load stemod_kick API\n");
> +               goto free_rproc;
> +       }
> +
> +       /*
> +        * Registration of the ste_modem_rproc will cause firmware to
> +        * to be fetched and the virtio resource entries to be allocated
> +        * in memory. However STE-modem requires the firmware to be located
> +        * at the start of the shared memory region. So we need to
> +        * reserve space for firmware at the start of the shared memory
> +        * region.
> +        */
> +       reserved = dma_alloc_coherent(&pdev->dev, STEMOD_FW_SIZE,
> +                                     &dma, GFP_KERNEL);

This assumes a certain order of allocations which takes place inside
the remoteproc framework, and may break if things change (e.g.
https://lkml.org/lkml/2012/5/20/35)

Instead, we might want the remoteproc framework to help here. The best
suggestion I have is to "teach" remoteproc about different
purpose-specific memory regions, and ask it to allocate memory only
from the relevant region (if one exists). This will prevent different
order of allocations from using memory we must reserve for certain
purposes.

Ludovic is working on such patch, you might want to take its latest
version from him.

Thanks!
Ohad.

RE: [RFC 4/4] remoteproc: Add driver for STE Modem

[Sjur BRENDELAND]

Hi Ohad,

> Cool, thanks for posting this.

Well thank you for your patience reviewing this.

> >  obj-$(CONFIG_STE_MODEM_RPROC)          += ste_modem_remoteproc.o
> > +ste_modem_remoteproc-y                         += ste_modem_rproc.o
> >  ste_modem_remoteproc-y                         +=
> remoteproc_ste_modem_loader.o
> 
> Just wondering - do we need the loader ops to be a separate file ? is
> it only going to be used by this module or do you foresee additional
> users in the future ?

No I don't, squashing this together is a good idea.


> > +/* Maxium number of "channels" */
> > +#define MAX_VQS 14
> 
> Can you explain a bit the background behind this number, and how vqs
> and vqids are generally handled by the ste modem?

The HW Kick mechanism is implemented using a 32-bit register.
These 32 bits are then divided in three groups, status from modem,
modem-to-host and host-to-modem notifications. 
The notifications can be distributed among the remaining 28 bits
at our own choice, but we're simply assuming 14 different notification IDs
in each direction. Hence we can support max 14 Virtio Queues.

> > +/* Setup the kick API for notification subscriptions */
> > +static int ste_rproc_subscribe_to_kicks(struct rproc *rproc)
> > +{
> > +       int i, err;
> > +       u32 txmask = 0, rxmask = 0;
> > +       int (*kick_subscribe)(int notifyid,
> > +                      void (*notify_cb)(int notifyid, void *data),
> void *data);
> > +       int (*notifyid_alloc)(u32 setter_mask, u32 getter_mask);
> > +
> > +       /*
> > +        * We need to declare for the HW what notification IDs
> > +        * we're going to use.
> 
> Can you explain why is this needed ? what happens when we call
> notifyid_alloc(rxmask, txmask) ?

As mentioned above we have 28 bits for modem-to-host and
host-to-modem notifications. How we allocate the bits are
configurable, but we need to tell the HW what bit is used 
in what direction. You may argue that we could just take
14+14, but we would need some way of sanity checking the
mapping to notification-IDs anyhow.

> > +        * notification IDs used for RX and TX by iterating over
> > +        * the notification IDs.
> > +        */
> > +       idr_for_each(&rproc->notifyids, ste_rproc_set_vqid, &rxmask);
> 
> If this really is needed, we might want some help from the remoteproc
> framework to do this, because it already has all the required info.
> 
> It'd be better if we don't fiddle with its internal structures,
> because doing say may make it harder for it to evolve.

Agree, it would be better to have some generic access functions to get
what notification IDs are used for each channel. One option could be
to define a iterator, traversing over all the resource entries.
In that way I could just pick up the notification IDs in the resource
entries. Or we could generalize the Notification-ID iteration.

> > +       notifyid_alloc = symbol_get(stemod_kick_notifyid_alloc);
> 
> This module is using dynamic symbol lookup quite a lot. This really
> stands out because it's quite uncommon.
> 
> Why is it necessary ? Can we use static symbols instead ?
>
> Can you share the code for those symbols as well ?

Unfortunately this driver for the HW-kicks is not yet submitted upstream.
It's not up to me, but I'll check with Linus Walleij about what the plans are...

> > +       if (rproc == NULL)
> > +               return NULL;
> > +       return rproc->priv;
> 
> Might be nicer to just
> 
>     return rproc ? rproc->priv : NULL;

Agree, thanks.


> There's three issues that bother me here:
> 
> 1. It feels like this kind of functionality isn't limited to the STE
> modem, so we better have it implemented in the remoteproc framework.

I'm actually not sure this is a good idea. Usually it is not good to
generalize from only one example. It might be that the STE modem 
has quite different requirements than other remote-proc users.
Not everybody has to unify the approach seen from user-space across
different interface technologies (HSI, HSIC, Shared Memory, and UART).
And not everybody has a 3-stage modem boot protocol to care about...

 
> 2. I'm not convinced that sysfs is an appropriate interface for
> controlling the remote processor:
> - if the remote processor crashes, the user isn't informed about it,
> so it won't refresh its state even if a successful recovery took place

Hm, what do you mean by "user"? A Virtio-Driver or user-space?
A recovery involves a 3-stage boot for STE-modems, there won't be any
recovery without user-space involvement in running the 3-stage boot
process ;-)

> - if the user space crashes, we don't get informed about it, and the
> remote processor can erroneously stay powered on indefinitely

This is not a problem for the STE modem. The user space process controlling
the modem is running as a daemon. If this daemon crashes it is automatically
restarted and forces a modem reboot in order to bring user-space daemon and
modem into sync.

> It seems to me that a char device will solve these issues: we can use
> ioctl to control the power, if the user crashes we'll know about it
> via the release handler, and if the remote processor crashes we can
> let the user know by sending it a notification for it to read via the
> fd.

Currently, I don't see the need for this for the STE modem. (anyway my
impression was that IOCTLs was going out of fashion, but I guess
you could argue the same about new sysfs entries ;-) 

> 3. I'm not sure we want to let the user space directly control the
> power of the remote processor: what if it decides to power off the
> rproc while other kernel users utilize it?
>
> We might want to change the semantics of the interface to just allow
> the user to increase/decrease a power reference count instead of
> directly booting/shutting it down.

In this case we need a way to make the virtio-drivers release the
virtio-queues in order to decrement the refcount, right? 

The Virtio-Console seems a bit difficult here. If I read the code
right in the Virtio-Console driver, the only way to make it release
it's virtio queues is to trigger the Virtio-Consonle remove function
(i.e. unregister the Virtio Device).

So it seems that we need to force an unregistration of the virtio devices,
in order to make it release it's queues.

Currently it looks like this only can be done with rproc_unregister.
This is probably not what we want, so from my point of view we need
a some new functionality to trigger unregistration of the virtio
devices from ste-remoteproc.

But I might off here and or missed something in my code-reading...

> This assumes a certain order of allocations which takes place inside
> the remoteproc framework, and may break if things change (e.g.
> https://lkml.org/lkml/2012/5/20/35)
> 
> Instead, we might want the remoteproc framework to help here. The best
> suggestion I have is to "teach" remoteproc about different
> purpose-specific memory regions, and ask it to allocate memory only
> from the relevant region (if one exists). This will prevent different
> order of allocations from using memory we must reserve for certain
> purposes.
> 
> Ludovic is working on such patch, you might want to take its latest
> version from him.

Ok, looking forward seeing some patches on this then Ludovic :-).

Perhaps another option is to use dma_mark_declared_memory_occupied(),
it seems to be able to force allocation at a certain device address.

Finally - thank you for spending time on code reading and helping
me forward with the STE rproc driver.

Thanks,
Sjur

Re: [RFC 4/4] remoteproc: Add driver for STE Modem

[Ohad Ben-Cohen]

Hi Sjur,

On Mon, Jul 2, 2012 at 6:22 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
> Well thank you for your patience reviewing this.

Sure np !

>> > +       /*
>> > +        * We need to declare for the HW what notification IDs
>> > +        * we're going to use.
>>
>> Can you explain why is this needed ? what happens when we call
>> notifyid_alloc(rxmask, txmask) ?
>
> As mentioned above we have 28 bits for modem-to-host and
> host-to-modem notifications. How we allocate the bits are
> configurable, but we need to tell the HW what bit is used
> in what direction.

Can you please explain a bit the hardware (in the context of those
bits) ? E.g. what happens when we flip one of those bits ? Does it
generate an interrupt or is it just a logical bit which maintains the
state of the channel ?

>> > +        * notification IDs used for RX and TX by iterating over
>> > +        * the notification IDs.
>> > +        */
>> > +       idr_for_each(&rproc->notifyids, ste_rproc_set_vqid, &rxmask);
>>
>> If this really is needed, we might want some help from the remoteproc
>> framework to do this, because it already has all the required info.
>>
>> It'd be better if we don't fiddle with its internal structures,
>> because doing say may make it harder for it to evolve.
>
> Agree, it would be better to have some generic access functions to get
> what notification IDs are used for each channel. One option could be
> to define a iterator, traversing over all the resource entries.
> In that way I could just pick up the notification IDs in the resource
> entries. Or we could generalize the Notification-ID iteration.

What if we make remoteproc export the largest notification id (to the
low level driver) ? since those notification-id numbers are continuous
and starting from zero, I suspect that knowing the largest used id
number may just be enough info for you to configure those hw bits.

>> > +       notifyid_alloc = symbol_get(stemod_kick_notifyid_alloc);
>>
>> This module is using dynamic symbol lookup quite a lot. This really
>> stands out because it's quite uncommon.
>>
>> Why is it necessary ? Can we use static symbols instead ?
>>
>> Can you share the code for those symbols as well ?
>
> Unfortunately this driver for the HW-kicks is not yet submitted upstream.
> It's not up to me, but I'll check with Linus Walleij about what the plans are...

Sure, that's fine, but the extensive symbol_get() usage in this driver
is still quite surprising.

Why is it necessary ? Can we just use static symbols instead (i.e.
just invoke the functions directly and let the linker do its job) ?

>> There's three issues that bother me here:
>>
>> 1. It feels like this kind of functionality isn't limited to the STE
>> modem, so we better have it implemented in the remoteproc framework.
>
> I'm actually not sure this is a good idea. Usually it is not good to
> generalize from only one example. It might be that the STE modem
> has quite different requirements than other remote-proc users.
> Not everybody has to unify the approach seen from user-space across
> different interface technologies (HSI, HSIC, Shared Memory, and UART).
> And not everybody has a 3-stage modem boot protocol to care about...

Sure, 3-stage modem boot protocol sounds very specific to your use case.

But the generalization of your requirement isn't very
platform-specific: in essence, it boils down to "we need to let user
space clients be able to power up the remote processor", and the
solution for that might be useful for others too.

Moreover, adding a user space interface in a low level driver might
really do raise a few eyebrows, especially in this new era of
convergence. It's better to add it once, in a generic framework, so
others could use it, rather than have people duplicate it (possibly in
slightly different ways) in their drivers.

>> 2. I'm not convinced that sysfs is an appropriate interface for
>> controlling the remote processor:
>> - if the remote processor crashes, the user isn't informed about it,
>> so it won't refresh its state even if a successful recovery took place
>
> Hm, what do you mean by "user"? A Virtio-Driver or user-space?
> A recovery involves a 3-stage boot for STE-modems, there won't be any
> recovery without user-space involvement in running the 3-stage boot
> process ;-)

I meant user space here. But I'm not only thinking about your specific
use case, but also generally about what people can do with this new
user space interface.

You may have the recovery use case in mind now, but the generalization
of this user space interface is "let user space be able to power up
the remote processor".

This might actually be useful for others who have other user space use
cases, but we must make sure the interface is, or can be made, capable
of dealing with crashes.

>> - if the user space crashes, we don't get informed about it, and the
>> remote processor can erroneously stay powered on indefinitely
>
> This is not a problem for the STE modem. The user space process controlling
> the modem is running as a daemon. If this daemon crashes it is automatically
> restarted and forces a modem reboot in order to bring user-space daemon and
> modem into sync.

Sure. The bigger picture, though, is a random user space context which
needs the remote processor powered up. And when we expose such an
interface to user space, we can't trust it to do the right thing, and
must be able to cope with it crashing horribly.

>> It seems to me that a char device will solve these issues: we can use
>> ioctl to control the power, if the user crashes we'll know about it
>> via the release handler, and if the remote processor crashes we can
>> let the user know by sending it a notification for it to read via the
>> fd.
>
> Currently, I don't see the need for this for the STE modem. (anyway my
> impression was that IOCTLs was going out of fashion, but I guess
> you could argue the same about new sysfs entries ;-)

I suspect that a char device is the only sane way to expose this
interface without relying on the user space doing the right thing...
we just can't tell who's going to use this interface once we expose
it, and how good their programming skills are :)

>> 3. I'm not sure we want to let the user space directly control the
>> power of the remote processor: what if it decides to power off the
>> rproc while other kernel users utilize it?
>>
>> We might want to change the semantics of the interface to just allow
>> the user to increase/decrease a power reference count instead of
>> directly booting/shutting it down.
>
> In this case we need a way to make the virtio-drivers release the
> virtio-queues in order to decrement the refcount, right?
>
> The Virtio-Console seems a bit difficult here. If I read the code
> right in the Virtio-Console driver, the only way to make it release
> it's virtio queues is to trigger the Virtio-Consonle remove function
> (i.e. unregister the Virtio Device).
>
> So it seems that we need to force an unregistration of the virtio devices,
> in order to make it release it's queues.
>
> Currently it looks like this only can be done with rproc_unregister.
> This is probably not what we want, so from my point of view we need
> a some new functionality to trigger unregistration of the virtio
> devices from ste-remoteproc.

Yeah, no code is needed; something like this should do the trick:

root@omap4430-panda:/sys/bus/virtio/drivers/virtio_console# echo
virtio1 > unbind

(replace "virtio1" with the name of the virtio device, in your system,
that's bound to virtio_console)

>> This assumes a certain order of allocations which takes place inside
>> the remoteproc framework, and may break if things change (e.g.
>> https://lkml.org/lkml/2012/5/20/35)
>>
>> Instead, we might want the remoteproc framework to help here. The best
>> suggestion I have is to "teach" remoteproc about different
>> purpose-specific memory regions, and ask it to allocate memory only
>> from the relevant region (if one exists). This will prevent different
>> order of allocations from using memory we must reserve for certain
>> purposes.
>>
>> Ludovic is working on such patch, you might want to take its latest
>> version from him.
>
> Ok, looking forward seeing some patches on this then Ludovic :-).
>
> Perhaps another option is to use dma_mark_declared_memory_occupied(),
> it seems to be able to force allocation at a certain device address.
>
> Finally - thank you for spending time on code reading and helping
> me forward with the STE rproc driver.

Sure, thanks for helping make the kernel better :)

Ohad.

RE: [RFC 4/4] remoteproc: Add driver for STE Modem

[Sjur BRENDELAND]

Hi Ohad,

> Can you please explain a bit the hardware (in the context of those
> bits) ? E.g. what happens when we flip one of those bits ? Does it
> generate an interrupt or is it just a logical bit which maintains the
> state of the channel ?

The simple story is that when Host writes a bit indicated with TX-mask
it generates an interrupt on the modem-side. And likewise when the
modem writes a bit indicated with RX mask the Host will receive an
interrupt.

>> Agree, it would be better to have some generic access functions to get
>> what notification IDs are used for each channel. One option could be
>> to define a iterator, traversing over all the resource entries.
>> In that way I could just pick up the notification IDs in the resource
>> entries. Or we could generalize the Notification-ID iteration.
> 
> What if we make remoteproc export the largest notification id (to the
> low level driver) ? since those notification-id numbers are continuous
> and starting from zero, I suspect that knowing the largest used id
> number may just be enough info for you to configure those hw bits.

Yes, if you can guarantee this, I could make a very simple solution.

> >> This module is using dynamic symbol lookup quite a lot. This really
> >> stands out because it's quite uncommon.
...
> Why is it necessary ? Can we just use static symbols instead (i.e.
> just invoke the functions directly and let the linker do its job) ?

Ok, I guess a more standard way to solve this is use #ifdef's and
dummy inline functions in the header file instead. I'll do that 
next time around then.

...
> Sure. The bigger picture, though, is a random user space context which
> needs the remote processor powered up. And when we expose such an
> interface to user space, we can't trust it to do the right thing, and
> must be able to cope with it crashing horribly.
> 
>>> It seems to me that a char device will solve these issues: we can use
>>> ioctl to control the power, if the user crashes we'll know about it
>>> via the release handler, and if the remote processor crashes we can
>>> let the user know by sending it a notification for it to read via the fd.
>>
>> Currently, I don't see the need for this for the STE modem. (anyway my
>> impression was that IOCTLs was going out of fashion, but I guess
>> you could argue the same about new sysfs entries ;-)
> 
> I suspect that a char device is the only sane way to expose this
> interface without relying on the user space doing the right thing...
> we just can't tell who's going to use this interface once we expose
> it, and how good their programming skills are :)

OK, we can do it this way as well. It feels a bit over-the-top in my case,
but I understand where you're coming from any why.

>> The Virtio-Console seems a bit difficult here. If I read the code
>> right in the Virtio-Console driver, the only way to make it release
>> it's virtio queues is to trigger the Virtio-Consonle remove function
>> (i.e. unregister the Virtio Device).
>>
>> So it seems that we need to force an unregistration of the virtio devices,
>> in order to make it release it's queues.
>>
>> Currently it looks like this only can be done with rproc_unregister.
>> This is probably not what we want, so from my point of view we need
>> a some new functionality to trigger unregistration of the virtio
>> devices from ste-remoteproc.
> 
> Yeah, no code is needed; something like this should do the trick:
> 
> root@omap4430-panda:/sys/bus/virtio/drivers/virtio_console# echo
> virtio1 > unbind
> 
> (replace "virtio1" with the name of the virtio device, in your system,
> that's bound to virtio_console)

Sweet, that's nice and simple.


Regards,
Sjur

Re: [RFC 4/4] remoteproc: Add driver for STE Modem

[Ohad Ben-Cohen]

Hi Sjur,

On Fri, Jul 6, 2012 at 12:18 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
> The simple story is that when Host writes a bit indicated with TX-mask
> it generates an interrupt on the modem-side. And likewise when the
> modem writes a bit indicated with RX mask the Host will receive an
> interrupt.

Ok, thanks.

> Yes, if you can guarantee this, I could make a very simple solution.

Great!

> Ok, I guess a more standard way to solve this is use #ifdef's and
> dummy inline functions in the header file instead. I'll do that
> next time around then.

I'm still puzzled a bit about this: can we just depend on some
CONFIG_STE_MODEM_HANDLERS (totally made up name) instead, without even
adding the #ifdefs ?

Since ste_modem_rproc is anyway platform specific, I'm wondering
whether there's any legitimate configuration where it is needed, but
the symbols it depends on aren't around.

> OK, we can do it this way as well. It feels a bit over-the-top in my case,
> but I understand where you're coming from any why.

Thanks! It should really be super simple and straight-forward to
implement. If you see that for some reason it isn't, let me know and
I'll help.

Ohad.

RE: [RFC 4/4] remoteproc: Add driver for STE Modem

[Sjur BRENDELAND]

Hi Ohad,

>>> It seems to me that a char device will solve these issues: we can use
>>> ioctl to control the power, if the user crashes we'll know about it
>>> via the release handler, and if the remote processor crashes we can
>>> let the user know by sending it a notification for it to read via the fd.

How about skipping definition of ioctls and simply call rproc_boot() upon
open and rproc_shutdown() upon close, this is perhaps quirky, but simpler.
I'd also like to use a char-misc device, then we get the device 
node with proper name automatically from existing udev rules.

>> In this case we need a way to make the virtio-drivers release the
>> virtio-queues in order to decrement the refcount, right?
...
>> So it seems that we need to force an unregistration of the virtio devices,
>> in order to make it release its queues.
...
> Yeah, no code is needed; something like this should do the trick:
> 
> root@omap4430-panda:/sys/bus/virtio/drivers/virtio_console# echo
> virtio1 > unbind

Looking on this once more after vacation I prefer if the ste-rproc driver
could enforce shutdown if we use the char-device.
In our case user-space will detect modem crashes and power off
the device. So if user-space uses char-device to request power off,
it feels like kernel space should do everything possible to really
trigger the shutdown, including unbinding the virtio devices.
Depending on additional "echo .../virtio > unbind" seems like an
unnecessary extra dependency. 


Alternatively, we could let user-space handle the power switch
for the modem directly. Remoteproc could simply notify user-space
(e.g. using gen-netlink) when rproc usage count has dropped to zero.
And this could trigger power-down. This would be more aligned with
our existing solution for power control.

Regards,
Sjur