* [RFCv2] remoteproc: Add STE modem driver for remoteproc
@ 2012-09-18 18:29 sjur.brandeland
2012-09-18 18:48 ` Alan Cox
2012-09-19 9:09 ` Ohad Ben-Cohen
0 siblings, 2 replies; 6+ messages in thread
From: sjur.brandeland @ 2012-09-18 18:29 UTC (permalink / raw)
To: Ohad Ben-Cohen
Cc: Sjur Brændeland, linux-kernel, Linus Walleij, Sjur Brændeland
From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Add support for the STE modem shared memory driver.
This driver hooks into the remoteproc framework
in order to manage configuration and the virtio
devices.
This driver adds custom firmware handlers, because
STE modem uses a custom firmware layout.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
Changes since v1:
o Implement a driver for struct ste_modem_device. This driver is using
the platform-device bus. New include file is added in
include/linux/modem_shm/.
o Changed from a stand-alone API for "kick" and control to a device.
Now we use a device and driver with ops structure. Hopefully this
cleans up the interface.
o Removed the character device for user-space control of remoteproc.
I will have to come back to this later.
drivers/remoteproc/Kconfig | 12 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/ste_modem_rproc.c | 408 ++++++++++++++++++++++++++++++++++
include/linux/modem_shm/ste_modem.h | 71 ++++++
4 files changed, 492 insertions(+)
create mode 100644 drivers/remoteproc/ste_modem_rproc.c
create mode 100644 include/linux/modem_shm/ste_modem.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index e7d440c..14b70fd 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -28,4 +28,16 @@ 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
+ select VIRTIO_CONSOLE
+ depends on EXPERIMENTAL
+ depends on HAS_DMA
+ 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..391b651 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -8,3 +8,4 @@ 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_rproc.o
diff --git a/drivers/remoteproc/ste_modem_rproc.c b/drivers/remoteproc/ste_modem_rproc.c
new file mode 100644
index 0000000..9290bf3
--- /dev/null
+++ b/drivers/remoteproc/ste_modem_rproc.c
@@ -0,0 +1,408 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Author: Sjur Brændeland <sjur.brandeland@stericsson.com>
+ * License terms: GNU General Public License (GPL), version 2
+ */
+#include <linux/module.h>
+#include <linux/remoteproc.h>
+#include <linux/dma-mapping.h>
+#include <linux/remoteproc.h>
+#include <linux/modem_shm/ste_modem.h>
+
+#include "remoteproc_internal.h"
+
+#define SPROC_MAX_NOTIFY_ID 14
+#define SPROC_RESOURCE_NAME "rsc-table"
+#define SPROC_MODEM_NAME "ste-modem"
+#define SPROC_MODEM_FIRMWARE SPROC_MODEM_NAME "-fw.bin"
+
+#define sproc_dbg(sproc, fmt, ...) \
+ dev_dbg(&sproc->mdev->pdev.dev, fmt, ##__VA_ARGS__)
+#define sproc_err(sproc, fmt, ...) \
+ dev_err(&sproc->mdev->pdev.dev, fmt, ##__VA_ARGS__)
+
+/* STE-modem control structure */
+struct sproc {
+ struct rproc *rproc;
+ struct ste_modem_device *mdev;
+ int error;
+ void *fw_addr;
+ size_t fw_size;
+ dma_addr_t fw_dma_addr;
+};
+
+/* STE-Modem firmware entry */
+struct ste_toc_entry {
+ __le32 start;
+ __le32 size;
+ __le32 flags;
+ __le32 entry_point;
+ __le32 load_addr;
+ char name[12];
+};
+
+/*
+ * 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 ste_toc {
+ struct ste_toc_entry table[32];
+};
+
+/*
+ * Loads the firmware to shared memory.
+ * The memory area to load fw into must be pre-allocated before
+ * this function is called (because remoteproc has already allocated
+ * device memory when this function is called)
+ */
+static int sproc_load_segments(struct rproc *rproc, const struct firmware *fw)
+{
+ struct sproc *sproc = rproc->priv;
+
+ /* Convert to pages before checking if we have enough space for fw*/
+ if (sproc->fw_addr == NULL ||
+ PFN_DOWN(sproc->fw_size) < PFN_DOWN(fw->size)) {
+ sproc_err(sproc, "Not sufficient space for firmware\n");
+ return -EINVAL;
+ }
+
+ memcpy(sproc->fw_addr, fw->data, fw->size);
+ return 0;
+}
+
+/* Find the entry for resource table in the Table of Content */
+static struct ste_toc_entry *sproc_find_rsc_entry(const struct firmware *fw)
+{
+ int i;
+ struct ste_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, SPROC_RESOURCE_NAME,
+ sizeof(toc->table[i].name))) {
+ if (toc->table[i].start > fw->size)
+ return NULL;
+ return &toc->table[i];
+ }
+ }
+ return NULL;
+}
+
+/*
+ * Find the resource table inside the remote processor's firmware.
+ * This function will allocate area used for firmware image in the memory
+ * region shared with the modem.
+ */
+static struct resource_table *
+sproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
+ int *tablesz)
+{
+ struct resource_table *table;
+ struct ste_toc_entry *entry = sproc_find_rsc_entry(fw);
+ struct sproc *sproc = rproc->priv;
+
+ if (!entry) {
+ sproc_err(sproc, "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) {
+ sproc_err(sproc, "resource table truncated\n");
+ return NULL;
+ }
+
+ /* make sure table has at least the header */
+ if (sizeof(struct resource_table) > entry->size) {
+ sproc_err(sproc, "header-less resource table\n");
+ return NULL;
+ }
+
+ /* we don't support any version beyond the first */
+ if (table->ver != 1) {
+ sproc_err(sproc, "unsupported fw ver: %d\n", table->ver);
+ return NULL;
+ }
+
+ /* make sure reserved bytes are zeroes */
+ if (table->reserved[0] || table->reserved[1]) {
+ sproc_err(sproc, "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) {
+ sproc_err(sproc, "resource table incomplete\n");
+ return NULL;
+ }
+
+ /* If the fw size has grown, release the previous fw allocation */
+ if (sproc->fw_addr && PFN_DOWN(sproc->fw_size) < PFN_DOWN(fw->size)) {
+ dma_free_coherent(&rproc->dev, sproc->fw_size,
+ sproc->fw_addr,
+ sproc->fw_dma_addr);
+ sproc->fw_addr = NULL;
+ sproc->fw_size = 0;
+ sproc->fw_dma_addr = 0;
+ }
+
+ /*
+ * 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.
+ * This cannot be done in the function sproc_load_segments because
+ * then dma_alloc_coherent is already called by Core and the
+ * start of the share memory area would already have been occupied.
+ */
+ if (!sproc->fw_addr) {
+ sproc->fw_addr = dma_alloc_coherent(rproc->dev.parent, fw->size,
+ &sproc->fw_dma_addr,
+ GFP_KERNEL);
+ if (!sproc->fw_addr) {
+ sproc_err(sproc,
+ "cannot allocate space (%zd) for fw image\n",
+ fw->size);
+ return NULL;
+ }
+
+ if (sproc->mdev->shm_pa != (unsigned long)sproc->fw_addr) {
+ sproc_err(sproc,
+ "bad fw address (%lx), should have been %p\n",
+ sproc->mdev->shm_pa,
+ sproc->fw_addr);
+ dma_free_coherent(rproc->dev.parent, sproc->fw_size,
+ sproc->fw_addr,
+ sproc->fw_dma_addr);
+ sproc->fw_addr = NULL;
+ return NULL;
+ }
+ }
+
+ sproc->fw_size = fw->size;
+ *tablesz = entry->size;
+ return table;
+}
+
+/* STE modem firmware handler operations */
+const struct rproc_fw_ops sproc_fw_ops = {
+ .load = sproc_load_segments,
+ .find_rsc_table = sproc_find_rsc_table
+};
+
+/* Kick the modem with specified notification id */
+static void sproc_kick(struct rproc *rproc, int vqid)
+{
+ struct sproc *sproc = rproc->priv;
+
+ sproc_dbg(sproc, "kick vqid:%d\n", vqid);
+
+ /*
+ * We need different notification IDs for RX and TX so add
+ * an offset on TX notification IDs.
+ */
+ sproc->mdev->ops.kick(sproc->mdev, vqid + SPROC_MAX_NOTIFY_ID);
+}
+
+/* Received a kick from a modem, kick the virtqueue */
+static void sproc_kick_callback(struct ste_modem_device *mdev, int vqid)
+{
+ struct sproc *sproc = mdev->drv_data;
+ if (rproc_vq_interrupt(sproc->rproc, vqid) == IRQ_NONE)
+ sproc_dbg(sproc,
+ "no message was found in vqid %d\n", vqid);
+}
+
+/* Start the STE modem */
+static int sproc_start(struct rproc *rproc)
+{
+ struct sproc *sproc = rproc->priv;
+ int i, err;
+
+ sproc_dbg(sproc, "start ste-modem\n");
+
+ /* Sanity test the max_notifyid */
+ if (rproc->max_notifyid > SPROC_MAX_NOTIFY_ID) {
+ sproc_err(sproc, "Notification IDs too high:%d\n",
+ rproc->max_notifyid);
+ return -EINVAL;
+ }
+
+ /* Subscribe to notifications */
+ for (i = 0; i < rproc->max_notifyid; i++) {
+ err = sproc->mdev->ops.kick_subscribe(sproc->mdev, i);
+ if (err) {
+ sproc_err(sproc,
+ "subscription of kicks failed:%d\n", err);
+ return err;
+ }
+ }
+
+ /* Request modem start-up*/
+ return sproc->mdev->ops.power(sproc->mdev, true);
+}
+
+/* Stop the STE modem */
+static int sproc_stop(struct rproc *rproc)
+{
+ struct sproc *sproc = rproc->priv;
+ sproc_dbg(sproc, "stop ste-modem\n");
+
+ return sproc->mdev->ops.power(sproc->mdev, false);
+}
+
+static struct rproc_ops sproc_ops = {
+ .start = sproc_start,
+ .stop = sproc_stop,
+ .kick = sproc_kick,
+};
+
+/* STE modem device is unregistered */
+static int sproc_drv_remove(struct platform_device *pdev)
+{
+ struct ste_modem_device *mdev =
+ container_of(pdev, struct ste_modem_device, pdev);
+ struct sproc *sproc = mdev->drv_data;
+
+ sproc_dbg(sproc, "remove ste-modem\n");
+
+ /* Unregister as remoteproc device */
+ rproc_del(sproc->rproc);
+
+ /* Release DMA memory */
+ dma_release_declared_memory(&pdev->dev);
+
+ mdev->drv_data = NULL;
+ return 0;
+}
+
+/* Handle probe of a modem device */
+static int sproc_probe(struct platform_device *pdev)
+{
+ struct ste_modem_device *mdev =
+ container_of(pdev, struct ste_modem_device, pdev);
+ dma_addr_t device_addr = 0;
+ struct sproc *sproc;
+ struct rproc *rproc;
+ int err;
+
+ dev_dbg(&mdev->pdev.dev, "probe ste-modem\n");
+ rproc = rproc_alloc(&mdev->pdev.dev,
+ mdev->pdev.name,
+ &sproc_ops,
+ SPROC_MODEM_FIRMWARE,
+ sizeof(*sproc));
+ if (!rproc)
+ return -ENOMEM;
+
+ sproc = rproc->priv;
+ sproc->mdev = mdev;
+ sproc->rproc = rproc;
+ mdev->drv_data = sproc;
+ /*
+ * Get the memory region shared with the modem
+ * and declare it as dma memory.
+ */
+ sproc_dbg(sproc, "Shared memory region pa:0x%lx sz:0x%zx\n",
+ (unsigned long) mdev->shm_pa,
+ (unsigned long) mdev->shm_size);
+
+ err = dma_declare_coherent_memory(&mdev->pdev.dev,
+ (dma_addr_t) mdev->shm_pa,
+ device_addr,
+ mdev->shm_size,
+ DMA_MEMORY_MAP |
+ DMA_MEMORY_EXCLUSIVE |
+ DMA_MEMORY_INCLUDES_CHILDREN);
+ if (!err) {
+ sproc_err(sproc,
+ "Cannot declare modem-shm memory region\n");
+ err = -ENOMEM;
+ goto free_rproc;
+ }
+
+ /* Set the STE-modem specific firmware handler */
+ rproc->fw_ops = &sproc_fw_ops;
+
+ /* Register as a remoteproc device */
+ err = rproc_add(rproc);
+ if (err)
+ goto free_dmamem;
+
+ return 0;
+
+free_dmamem:
+ dma_release_declared_memory(&mdev->pdev.dev);
+free_rproc:
+ mdev->drv_data = NULL;
+ rproc_put(rproc);
+ return err;
+}
+
+/**
+ * register_ste_shm_modem() - Register a modem into the remoteproc framework.
+ * @mdev: Modem device to register with the remoteproc framework.
+ */
+int register_ste_shm_modem(struct ste_modem_device *mdev)
+{
+ dev_dbg(&mdev->pdev.dev, "register ste-modem\n");
+
+ if (!mdev->ops.power || !mdev->ops.kick || !mdev->ops.kick_subscribe)
+ return -EINVAL;
+
+ return platform_device_register(&mdev->pdev);
+}
+EXPORT_SYMBOL(register_ste_shm_modem);
+
+/**
+ * unregister_ste_shm_modem() - Unregister from the remoteproc framework.
+ * @mdev: Device to unregister from the remoteproc framework.
+ */
+int unregister_ste_shm_modem(struct ste_modem_device *mdev)
+{
+ dev_dbg(&mdev->pdev.dev, "unregister ste-modem\n");
+
+ platform_device_unregister(&mdev->pdev);
+ return 0;
+}
+EXPORT_SYMBOL(unregister_ste_shm_modem);
+
+static struct ste_modem_driver sproc_driver = {
+ .drv = {
+ .driver = {
+ .name = SPROC_MODEM_NAME,
+ .owner = THIS_MODULE,
+ },
+ .probe = sproc_probe,
+ .remove = sproc_drv_remove,
+ },
+ .ops = {
+ .kick = sproc_kick_callback
+ }
+};
+
+static int __init sproc_init(void)
+{
+ return platform_driver_register(&sproc_driver.drv);
+}
+module_init(sproc_init);
+
+static void __exit sproc_exit(void)
+{
+ platform_driver_unregister(&sproc_driver.drv);
+}
+module_exit(sproc_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("STE Modem driver using the Remote Processor Framework");
diff --git a/include/linux/modem_shm/ste_modem.h b/include/linux/modem_shm/ste_modem.h
new file mode 100644
index 0000000..d4632d1
--- /dev/null
+++ b/include/linux/modem_shm/ste_modem.h
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Author: Sjur Brendeland / sjur.brandeland@stericsson.com
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#ifndef __INC_MODEM_DEV_H
+#define __INC_MODEM_DEV_H
+#include <linux/types.h>
+#include <linux/platform_device.h>
+
+struct ste_modem_device;
+
+/**
+ * struct ste_modem_dev_ops - Functions to control modem and modem interface.
+ *
+ * @power: Main power switch, used for cold-start or complete power off.
+ * @kick: Kick the modem.
+ * @kick_subscribe: Subscribe for notifications from the modem.
+ *
+ * This structure contains functions used by the ste remoteproc driver
+ * to manage the modem.
+ */
+struct ste_modem_dev_ops {
+ int (*power)(struct ste_modem_device *mdev, bool on);
+ int (*kick)(struct ste_modem_device *mdev, int notify_id);
+ int (*kick_subscribe)(struct ste_modem_device *mdev, int notify_id);
+};
+
+/**
+ * struct ste_modem_drv_ops - Callbacks for modem initiated events.
+ * @kick: Called when the modem kicks the host.
+ *
+ * This structure contains callbacks for actions triggered by the modem.
+ */
+struct ste_modem_drv_ops {
+ void (*kick)(struct ste_modem_device *mdev, int notify_id);
+};
+
+/**
+ * struct ste_modem_device - represent the STE modem device
+ * @pdev: Reference to platform device
+ * @ops: Operations used to manage the modem.
+ * @shm_pa: Physical address of the shared memory region.
+ * @shm_size: Size of the shared memory area.
+ * @drv_data: Driver private data.
+ *
+ */
+struct ste_modem_device {
+ struct platform_device pdev;
+ struct ste_modem_dev_ops ops;
+ unsigned long shm_pa;
+ size_t shm_size;
+ void *drv_data;
+};
+
+/**
+ * struct ste_modem_driver - Modem driver structure.
+ * @drv: Reference to driver
+ * @ops: Notification and status callbacks functions from modem.
+*/
+struct ste_modem_driver {
+ struct platform_driver drv;
+ struct ste_modem_drv_ops ops;
+};
+
+int register_ste_shm_modem(struct ste_modem_device *mdev);
+int unregister_ste_shm_modem(struct ste_modem_device *mdev);
+
+#endif /*INC_MODEM_DEV_H*/
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFCv2] remoteproc: Add STE modem driver for remoteproc
2012-09-18 18:29 [RFCv2] remoteproc: Add STE modem driver for remoteproc sjur.brandeland
@ 2012-09-18 18:48 ` Alan Cox
2012-09-18 19:09 ` Sjur Brændeland
2012-09-19 9:09 ` Ohad Ben-Cohen
1 sibling, 1 reply; 6+ messages in thread
From: Alan Cox @ 2012-09-18 18:48 UTC (permalink / raw)
To: sjur.brandeland
Cc: Ohad Ben-Cohen, Sjur Brændeland, linux-kernel, Linus Walleij
O> + /* make sure the offsets array isn't truncated */
> + if (table->num * sizeof(table->offset[0]) +
> + sizeof(struct resource_table) > entry->size) {
> + sproc_err(sproc, "resource table incomplete\n");
> + return NULL;
None of these checks appear to be robust against maths overflow. So the
question I'd ask is how far do you trust your inputs ?
Alan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFCv2] remoteproc: Add STE modem driver for remoteproc
2012-09-18 18:48 ` Alan Cox
@ 2012-09-18 19:09 ` Sjur Brændeland
0 siblings, 0 replies; 6+ messages in thread
From: Sjur Brændeland @ 2012-09-18 19:09 UTC (permalink / raw)
To: Alan Cox; +Cc: Ohad Ben-Cohen, linux-kernel, Linus Walleij
On Tue, Sep 18, 2012 at 8:48 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > + /* make sure the offsets array isn't truncated */
>> + if (table->num * sizeof(table->offset[0]) +
>> + sizeof(struct resource_table) > entry->size) {
>> + sproc_err(sproc, "resource table incomplete\n");
>> + return NULL;
>
> None of these checks appear to be robust against maths overflow. So the
> question I'd ask is how far do you trust your inputs ?
The input is coming from user space via firmware loading framework,
so I guess it shouldn't be trusted at all. I'll try to improve these
sanity checks
in my next respin.
Thanks,
Sjur
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFCv2] remoteproc: Add STE modem driver for remoteproc
2012-09-18 18:29 [RFCv2] remoteproc: Add STE modem driver for remoteproc sjur.brandeland
2012-09-18 18:48 ` Alan Cox
@ 2012-09-19 9:09 ` Ohad Ben-Cohen
2012-09-19 10:08 ` Sjur BRENDELAND
1 sibling, 1 reply; 6+ messages in thread
From: Ohad Ben-Cohen @ 2012-09-19 9:09 UTC (permalink / raw)
To: sjur.brandeland; +Cc: Sjur Brændeland, linux-kernel, Linus Walleij
Hi Sjur,
On Tue, Sep 18, 2012 at 9:29 PM, <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add support for the STE modem shared memory driver.
> This driver hooks into the remoteproc framework
> in order to manage configuration and the virtio
> devices.
>
> This driver adds custom firmware handlers, because
> STE modem uses a custom firmware layout.
Very nice driver, thanks for all your work. I have some comments below.
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
...
> drivers/remoteproc/Kconfig | 12 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/ste_modem_rproc.c | 408 ++++++++++++++++++++++++++++++++++
> include/linux/modem_shm/ste_modem.h | 71 ++++++
Why did you decide to create a separate folder for this header ? if
it's STE specific, maybe use an 'ste' prefix for it too ?
> 4 files changed, 492 insertions(+)
> create mode 100644 drivers/remoteproc/ste_modem_rproc.c
> create mode 100644 include/linux/modem_shm/ste_modem.h
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index e7d440c..14b70fd 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -28,4 +28,16 @@ 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
> + select VIRTIO_CONSOLE
This is non-standard.
Usually we select libraries, which we don't want the user involved in
configuring, but here you select a complete driver.
If you must have it for the STE modem, then you should at least meet
its dependencies by selecting VIRTIO and VIRTIO_RING too.
> +++ b/drivers/remoteproc/ste_modem_rproc.c
> @@ -0,0 +1,408 @@
> +/*
> + * Copyright (C) ST-Ericsson AB 2012
> + * Author: Sjur Brændeland <sjur.brandeland@stericsson.com>
> + * License terms: GNU General Public License (GPL), version 2
> + */
> +#include <linux/module.h>
> +#include <linux/remoteproc.h>
...
> +#include <linux/remoteproc.h>
redundant #include ?
> + * Loads the firmware to shared memory.
> + * The memory area to load fw into must be pre-allocated before
> + * this function is called (because remoteproc has already allocated
> + * device memory when this function is called)
> + */
> +static int sproc_load_segments(struct rproc *rproc, const struct firmware *fw)
> +{
> + struct sproc *sproc = rproc->priv;
> +
> + /* Convert to pages before checking if we have enough space for fw*/
Why do you have to convert to pages before checking ?
Btw - please put a 'space' before ending the comment.
> + if (sproc->fw_addr == NULL ||
> + PFN_DOWN(sproc->fw_size) < PFN_DOWN(fw->size)) {
> + sproc_err(sproc, "Not sufficient space for firmware\n");
> + return -EINVAL;
> + }
> +
> + memcpy(sproc->fw_addr, fw->data, fw->size);
> + return 0;
> +}
> +
> +/* Find the entry for resource table in the Table of Content */
> +static struct ste_toc_entry *sproc_find_rsc_entry(const struct firmware *fw)
> +{
> + int i;
> + struct ste_toc *toc;
> + int entries = ARRAY_SIZE(toc->table);
Using a local variable for this gives the impression that entries
might change, but really you just use it as a macro.
Maybe just #define something like TABLE_SIZE instead ?
> + * Find the resource table inside the remote processor's firmware.
> + * This function will allocate area used for firmware image in the memory
> + * region shared with the modem.
> + */
> +static struct resource_table *
> +sproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
> + int *tablesz)
> +{
...
> + /*
> + * 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.
> + * This cannot be done in the function sproc_load_segments because
> + * then dma_alloc_coherent is already called by Core and the
> + * start of the share memory area would already have been occupied.
> + */
> + if (!sproc->fw_addr) {
> + sproc->fw_addr = dma_alloc_coherent(rproc->dev.parent, fw->size,
This doesn't look good: this function should just find an offset
within the firmware and return it, and not do any memory allocations.
I understand the reason why you do that, but I think we had a nice
generic solution which shouldn't be too hard to implement (i.e. let
remoteproc maintain dedicated, purpose-specific, memory pools).
Moreover, if we implement it into the core, others could use it too.
Any chance you can look into it ? Ludovic started spinning some code
internally but was probably sucked away for other tasks meanwhile.
> +/* STE modem device is unregistered */
> +static int sproc_drv_remove(struct platform_device *pdev)
> +{
> + struct ste_modem_device *mdev =
> + container_of(pdev, struct ste_modem_device, pdev);
> + struct sproc *sproc = mdev->drv_data;
> +
> + sproc_dbg(sproc, "remove ste-modem\n");
> +
> + /* Unregister as remoteproc device */
> + rproc_del(sproc->rproc);
You also need to rproc_put() to unroll rproc_alloc().
> +/* Handle probe of a modem device */
> +static int sproc_probe(struct platform_device *pdev)
> +{
> + struct ste_modem_device *mdev =
> + container_of(pdev, struct ste_modem_device, pdev);
> + dma_addr_t device_addr = 0;
> + struct sproc *sproc;
> + struct rproc *rproc;
> + int err;
> +
> + dev_dbg(&mdev->pdev.dev, "probe ste-modem\n");
> + rproc = rproc_alloc(&mdev->pdev.dev,
> + mdev->pdev.name,
> + &sproc_ops,
> + SPROC_MODEM_FIRMWARE,
You're hardcoding the firmware name here, which is fine if all modem
devices always use the same file.
If not, but you're anyway expecting only a single modem device on any
given board, then please explicitly prevent this case (e.g. by
checking the pdev id and bailing out if it's different than zero).
> + sizeof(*sproc));
> + if (!rproc)
> + return -ENOMEM;
> +
> + sproc = rproc->priv;
> + sproc->mdev = mdev;
> + sproc->rproc = rproc;
> + mdev->drv_data = sproc;
> + /*
> + * Get the memory region shared with the modem
> + * and declare it as dma memory.
> + */
> + sproc_dbg(sproc, "Shared memory region pa:0x%lx sz:0x%zx\n",
> + (unsigned long) mdev->shm_pa,
> + (unsigned long) mdev->shm_size);
> +
> + err = dma_declare_coherent_memory(&mdev->pdev.dev,
> + (dma_addr_t) mdev->shm_pa,
> + device_addr,
> + mdev->shm_size,
> + DMA_MEMORY_MAP |
> + DMA_MEMORY_EXCLUSIVE |
> + DMA_MEMORY_INCLUDES_CHILDREN);
> + if (!err) {
> + sproc_err(sproc,
> + "Cannot declare modem-shm memory region\n");
> + err = -ENOMEM;
> + goto free_rproc;
> + }
This code should probably be part of the device creation - whoever
creates it, should probably attach the memory to it.
This way you don't couple the memory type with the driver itself, and
just let the driver work with whatever memory is attached to the
device.
Where do you create the device ? some platform code ? should probably
move this code up there.
> +/**
> + * register_ste_shm_modem() - Register a modem into the remoteproc framework.
> + * @mdev: Modem device to register with the remoteproc framework.
> + */
> +int register_ste_shm_modem(struct ste_modem_device *mdev)
> +{
> + dev_dbg(&mdev->pdev.dev, "register ste-modem\n");
> +
> + if (!mdev->ops.power || !mdev->ops.kick || !mdev->ops.kick_subscribe)
> + return -EINVAL;
> +
> + return platform_device_register(&mdev->pdev);
> +}
> +EXPORT_SYMBOL(register_ste_shm_modem);
> +
> +/**
> + * unregister_ste_shm_modem() - Unregister from the remoteproc framework.
> + * @mdev: Device to unregister from the remoteproc framework.
> + */
> +int unregister_ste_shm_modem(struct ste_modem_device *mdev)
> +{
> + dev_dbg(&mdev->pdev.dev, "unregister ste-modem\n");
> +
> + platform_device_unregister(&mdev->pdev);
> + return 0;
> +}
> +EXPORT_SYMBOL(unregister_ste_shm_modem);
Who is going to call these functions ?
Usually drivers don't register their own devices - platform code does
(or device tree).
It's better to put these functions there (possibly while adding the
ops checks back to the driver's probe function, because that is indeed
the responsibility of the driver to check).
Moreover, these functions are really just wrappers around
platform_device_{un}register, so I I'd personally just drop them and
call the code directly. This way your code will be a bit more readable
for others.
> +static int __init sproc_init(void)
> +{
> + return platform_driver_register(&sproc_driver.drv);
> +}
> +module_init(sproc_init);
> +
> +static void __exit sproc_exit(void)
> +{
> + platform_driver_unregister(&sproc_driver.drv);
> +}
> +module_exit(sproc_exit);
Replace boilerplate code with module_platform_driver ?
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("STE Modem driver using the Remote Processor Framework");
> diff --git a/include/linux/modem_shm/ste_modem.h b/include/linux/modem_shm/ste_modem.h
> new file mode 100644
> index 0000000..d4632d1
> --- /dev/null
> +++ b/include/linux/modem_shm/ste_modem.h
> @@ -0,0 +1,71 @@
> +/*
> + * Copyright (C) ST-Ericsson AB 2012
> + * Author: Sjur Brendeland / sjur.brandeland@stericsson.com
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#ifndef __INC_MODEM_DEV_H
> +#define __INC_MODEM_DEV_H
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +
> +struct ste_modem_device;
> +
> +/**
> + * struct ste_modem_dev_ops - Functions to control modem and modem interface.
> + *
> + * @power: Main power switch, used for cold-start or complete power off.
> + * @kick: Kick the modem.
> + * @kick_subscribe: Subscribe for notifications from the modem.
> + *
> + * This structure contains functions used by the ste remoteproc driver
> + * to manage the modem.
> + */
> +struct ste_modem_dev_ops {
> + int (*power)(struct ste_modem_device *mdev, bool on);
> + int (*kick)(struct ste_modem_device *mdev, int notify_id);
> + int (*kick_subscribe)(struct ste_modem_device *mdev, int notify_id);
> +};
> +
> +/**
> + * struct ste_modem_drv_ops - Callbacks for modem initiated events.
> + * @kick: Called when the modem kicks the host.
> + *
> + * This structure contains callbacks for actions triggered by the modem.
> + */
> +struct ste_modem_drv_ops {
> + void (*kick)(struct ste_modem_device *mdev, int notify_id);
> +};
> +
> +/**
> + * struct ste_modem_device - represent the STE modem device
> + * @pdev: Reference to platform device
> + * @ops: Operations used to manage the modem.
> + * @shm_pa: Physical address of the shared memory region.
> + * @shm_size: Size of the shared memory area.
> + * @drv_data: Driver private data.
> + *
> + */
> +struct ste_modem_device {
> + struct platform_device pdev;
> + struct ste_modem_dev_ops ops;
> + unsigned long shm_pa;
phys_addr_t ?
> + size_t shm_size;
> + void *drv_data;
> +};
> +
> +/**
> + * struct ste_modem_driver - Modem driver structure.
> + * @drv: Reference to driver
> + * @ops: Notification and status callbacks functions from modem.
> +*/
> +struct ste_modem_driver {
> + struct platform_driver drv;
> + struct ste_modem_drv_ops ops;
> +};
> +
> +int register_ste_shm_modem(struct ste_modem_device *mdev);
> +int unregister_ste_shm_modem(struct ste_modem_device *mdev);
> +
> +#endif /*INC_MODEM_DEV_H*/
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [RFCv2] remoteproc: Add STE modem driver for remoteproc
2012-09-19 9:09 ` Ohad Ben-Cohen
@ 2012-09-19 10:08 ` Sjur BRENDELAND
2012-09-19 14:29 ` Ohad Ben-Cohen
0 siblings, 1 reply; 6+ messages in thread
From: Sjur BRENDELAND @ 2012-09-19 10:08 UTC (permalink / raw)
To: Ohad Ben-Cohen; +Cc: Sjur Brændeland, linux-kernel, Linus Walleij
Hi Ohad,
> Very nice driver, thanks for all your work. I have some comments below.
Thank you :-). I'll try to make a new spin asap.
> > include/linux/modem_shm/ste_modem.h | 71 ++++++
>
> Why did you decide to create a separate folder for this header ? if
> it's STE specific, maybe use an 'ste' prefix for it too ?
There has been some attempt to upstream a shm driver for another modem
vendor as well, see https://lkml.org/lkml/2012/8/27/15.
This driver used .../driver/modem_shm, and
.../include/linux/modem_shm/. I feel that this driver belongs in the same
family. This other driver did not include any vendor prefix though.
What about .../include/linux/modem_shm/ste/modem.h or maybe just
.../include/linux/modem_shm/modem.h?
> > +config STE_MODEM_RPROC
> > + tristate "STE-Modem remoteproc support"
> > + select REMOTEPROC
> > + select VIRTIO_CONSOLE
>
> This is non-standard.
>
> Usually we select libraries, which we don't want the user involved in
> configuring, but here you select a complete driver.
>
> If you must have it for the STE modem, then you should at least meet
> its dependencies by selecting VIRTIO and VIRTIO_RING too.
OK, I just skip these select statements.
> > +#include <linux/remoteproc.h>
>
> redundant #include ?
I'll fix.
> > +static int sproc_load_segments(struct rproc *rproc, const struct firmware
> *fw)
> > +{
> > + struct sproc *sproc = rproc->priv;
> > +
> > + /* Convert to pages before checking if we have enough space for fw*/
>
> Why do you have to convert to pages before checking ?
>
> Btw - please put a 'space' before ending the comment.
It's because dma_alloc_coherent gives me whole pages (?) and
I don't want to do unnecessary reallocation.
But the code is simpler if I remove this, so I might just do that.
...
> > +/* Find the entry for resource table in the Table of Content */
> > +static struct ste_toc_entry *sproc_find_rsc_entry(const struct firmware
> *fw)
> > +{
> > + int i;
> > + struct ste_toc *toc;
> > + int entries = ARRAY_SIZE(toc->table);
>
> Using a local variable for this gives the impression that entries
> might change, but really you just use it as a macro.
>
> Maybe just #define something like TABLE_SIZE instead ?
OK, I'll fix.
>
> > + * Find the resource table inside the remote processor's firmware.
> > + * This function will allocate area used for firmware image in the memory
> > + * region shared with the modem.
> > + */
> > +static struct resource_table *
> > +sproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
> > + int *tablesz)
> > +{
> ...
> > + /*
> > + * 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.
> > + * This cannot be done in the function sproc_load_segments because
> > + * then dma_alloc_coherent is already called by Core and the
> > + * start of the share memory area would already have been occupied.
> > + */
> > + if (!sproc->fw_addr) {
> > + sproc->fw_addr = dma_alloc_coherent(rproc->dev.parent, fw-
> >size,
>
> This doesn't look good: this function should just find an offset
> within the firmware and return it, and not do any memory allocations.
>
> I understand the reason why you do that, ...
I am afraid I *must* put the TOC at the start of memory. There is no way
around this. But I can pre-allocate space for firmware and just bail out if
it is not enough room. This is a much simpler approach.
> but I think we had a nice
> generic solution which shouldn't be too hard to implement (i.e. let
> remoteproc maintain dedicated, purpose-specific, memory pools).
> Moreover, if we implement it into the core, others could use it too.
> Any chance you can look into it ? Ludovic started spinning some code
> internally but was probably sucked away for other tasks meanwhile.
I propose we pre-allocate some memory for now, but look into at a more
generic solution when 3.7 merge window has closed.
> > +/* STE modem device is unregistered */
> > +static int sproc_drv_remove(struct platform_device *pdev)
> > +{
> > + struct ste_modem_device *mdev =
> > + container_of(pdev, struct ste_modem_device, pdev);
> > + struct sproc *sproc = mdev->drv_data;
> > +
> > + sproc_dbg(sproc, "remove ste-modem\n");
> > +
> > + /* Unregister as remoteproc device */
> > + rproc_del(sproc->rproc);
>
> You also need to rproc_put() to unroll rproc_alloc().
OK, Thanks.
>
> > +/* Handle probe of a modem device */
> > +static int sproc_probe(struct platform_device *pdev)
> > +{
> > + struct ste_modem_device *mdev =
> > + container_of(pdev, struct ste_modem_device, pdev);
> > + dma_addr_t device_addr = 0;
> > + struct sproc *sproc;
> > + struct rproc *rproc;
> > + int err;
> > +
> > + dev_dbg(&mdev->pdev.dev, "probe ste-modem\n");
> > + rproc = rproc_alloc(&mdev->pdev.dev,
> > + mdev->pdev.name,
> > + &sproc_ops,
> > + SPROC_MODEM_FIRMWARE,
>
> You're hardcoding the firmware name here, which is fine if all modem
> devices always use the same file.
Yes, we will always use just one file.
>
> If not, but you're anyway expecting only a single modem device on any
> given board, then please explicitly prevent this case (e.g. by
> checking the pdev id and bailing out if it's different than zero).
>
...
> > + err = dma_declare_coherent_memory(&mdev->pdev.dev,
> > + (dma_addr_t) mdev->shm_pa,
> > + device_addr,
> > + mdev->shm_size,
> > + DMA_MEMORY_MAP |
> > + DMA_MEMORY_EXCLUSIVE |
> > + DMA_MEMORY_INCLUDES_CHILDREN);
>
> This code should probably be part of the device creation - whoever
> creates it, should probably attach the memory to it.
>
> This way you don't couple the memory type with the driver itself, and
> just let the driver work with whatever memory is attached to the
> device.
>
> Where do you create the device ? some platform code ? should probably
> move this code up there.
Yes, I wasn't sure about this, I did have it in the driver below at one point.
I'll just move this to the underlying device.
> > +/**
> > + * register_ste_shm_modem() - Register a modem into the remoteproc
> framework.
> > + * @mdev: Modem device to register with the remoteproc framework.
> > + */
> > +int register_ste_shm_modem(struct ste_modem_device *mdev)
> > +{
> > + dev_dbg(&mdev->pdev.dev, "register ste-modem\n");
> > +
> > + if (!mdev->ops.power || !mdev->ops.kick || !mdev-
> >ops.kick_subscribe)
> > + return -EINVAL;
> > +
> > + return platform_device_register(&mdev->pdev);
> > +}
> > +EXPORT_SYMBOL(register_ste_shm_modem);
> > +
> > +/**
> > + * unregister_ste_shm_modem() - Unregister from the remoteproc
> framework.
> > + * @mdev: Device to unregister from the remoteproc framework.
> > + */
> > +int unregister_ste_shm_modem(struct ste_modem_device *mdev)
> > +{
> > + dev_dbg(&mdev->pdev.dev, "unregister ste-modem\n");
> > +
> > + platform_device_unregister(&mdev->pdev);
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(unregister_ste_shm_modem);
>
> Who is going to call these functions ?
>
> Usually drivers don't register their own devices - platform code does
> (or device tree).
>
> It's better to put these functions there (possibly while adding the
> ops checks back to the driver's probe function, because that is indeed
> the responsibility of the driver to check).
>
> Moreover, these functions are really just wrappers around
> platform_device_{un}register, so I I'd personally just drop them and
> call the code directly. This way your code will be a bit more readable
> for others.
OK, will remove these register/unregister functions.
The reason I did include them was to be able to move away from the
platform bus, i.e. by just simply hooking the device up under remoteproc
without a driver. But currently this does not work because rproc_boot()
requires the device to have a driver due to the call
try_module_get(dev->parent->driver->owner).
> > +static int __init sproc_init(void)
> > +{
> > + return platform_driver_register(&sproc_driver.drv);
> > +}
> > +module_init(sproc_init);
> > +
> > +static void __exit sproc_exit(void)
> > +{
> > + platform_driver_unregister(&sproc_driver.drv);
> > +}
> > +module_exit(sproc_exit);
>
> Replace boilerplate code with module_platform_driver ?
I tried, but the macros cannot handle the sproc_driver.drv as argument.
> > +++ b/include/linux/modem_shm/ste_modem.h
> > @@ -0,0 +1,71 @@
> > +/*
> > + * Copyright (C) ST-Ericsson AB 2012
> > + * Author: Sjur Brendeland / sjur.brandeland@stericsson.com
> > + *
> > + * License terms: GNU General Public License (GPL) version 2
> > + */
> > +
...
> > +/**
> > + * struct ste_modem_device - represent the STE modem device
> > + * @pdev: Reference to platform device
> > + * @ops: Operations used to manage the modem.
> > + * @shm_pa: Physical address of the shared memory region.
> > + * @shm_size: Size of the shared memory area.
> > + * @drv_data: Driver private data.
> > + *
> > + */
> > +struct ste_modem_device {
> > + struct platform_device pdev;
> > + struct ste_modem_dev_ops ops;
> > + unsigned long shm_pa;
>
> phys_addr_t ?
Yes.
Thanks,
Sjur
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFCv2] remoteproc: Add STE modem driver for remoteproc
2012-09-19 10:08 ` Sjur BRENDELAND
@ 2012-09-19 14:29 ` Ohad Ben-Cohen
0 siblings, 0 replies; 6+ messages in thread
From: Ohad Ben-Cohen @ 2012-09-19 14:29 UTC (permalink / raw)
To: Sjur BRENDELAND; +Cc: Sjur Brændeland, linux-kernel, Linus Walleij
Hi Sjur,
On Wed, Sep 19, 2012 at 1:08 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
>> > include/linux/modem_shm/ste_modem.h | 71 ++++++
>>
>> Why did you decide to create a separate folder for this header ? if
>> it's STE specific, maybe use an 'ste' prefix for it too ?
>
> There has been some attempt to upstream a shm driver for another modem
> vendor as well, see https://lkml.org/lkml/2012/8/27/15.
>
> This driver used .../driver/modem_shm, and
> .../include/linux/modem_shm/. I feel that this driver belongs in the same
> family. This other driver did not include any vendor prefix though.
>
> What about .../include/linux/modem_shm/ste/modem.h or maybe just
> .../include/linux/modem_shm/modem.h?
Do your driver and Arun's share any code ? More importantly - is there
any common functionality to consolidate ?
If it's just the name "modem_shm" that is common, I'm not sure there's
merit in having a common folder ?
But if you do, why not use ste_modem_shm ? It looks like Arun comes
from STE too :)
>> > + /*
>> > + * 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.
>> > + * This cannot be done in the function sproc_load_segments because
>> > + * then dma_alloc_coherent is already called by Core and the
>> > + * start of the share memory area would already have been occupied.
>> > + */
>> > + if (!sproc->fw_addr) {
>> > + sproc->fw_addr = dma_alloc_coherent(rproc->dev.parent, fw-
>> >size,
>>
>> This doesn't look good: this function should just find an offset
>> within the firmware and return it, and not do any memory allocations.
>>
>> I understand the reason why you do that, ...
>
> I am afraid I *must* put the TOC at the start of memory. There is no way
> around this.
Sure that's fine. Let's just do it the right way :)
> But I can pre-allocate space for firmware and just bail out if
> it is not enough room. This is a much simpler approach.
>
>> but I think we had a nice
>> generic solution which shouldn't be too hard to implement (i.e. let
>> remoteproc maintain dedicated, purpose-specific, memory pools).
>> Moreover, if we implement it into the core, others could use it too.
>> Any chance you can look into it ? Ludovic started spinning some code
>> internally but was probably sucked away for other tasks meanwhile.
>
> I propose we pre-allocate some memory for now
Care to elaborate what do you mean exactly ? or just send a patch :)
>> > +static int __init sproc_init(void)
>> > +{
>> > + return platform_driver_register(&sproc_driver.drv);
>> > +}
>> > +module_init(sproc_init);
>> > +
>> > +static void __exit sproc_exit(void)
>> > +{
>> > + platform_driver_unregister(&sproc_driver.drv);
>> > +}
>> > +module_exit(sproc_exit);
>>
>> Replace boilerplate code with module_platform_driver ?
>
> I tried, but the macros cannot handle the sproc_driver.drv as argument.
I see, but I wonder why did you have to create the ste_modem_driver
struct in the first place ? How does the modem code access
sproc_kick_callback ?
Can you instead just ditch ste_modem_driver and provide
sproc_kick_callback in kick_subscribe or something ?
I'm asking not because using module_platform_driver is so important,
but because this may suggest something more importantly may need to be
fixed here.
Thanks,
Ohad.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-19 14:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-18 18:29 [RFCv2] remoteproc: Add STE modem driver for remoteproc sjur.brandeland
2012-09-18 18:48 ` Alan Cox
2012-09-18 19:09 ` Sjur Brændeland
2012-09-19 9:09 ` Ohad Ben-Cohen
2012-09-19 10:08 ` Sjur BRENDELAND
2012-09-19 14:29 ` Ohad Ben-Cohen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).