Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-10-05 Thread Matt Redfearn

Hi Bjorn,

Thanks for the review and comments, much appreciated :-)


On 03/10/16 23:16, Bjorn Andersson wrote:

On Tue 20 Sep 01:47 PDT 2016, Matt Redfearn wrote:


Add a remoteproc driver to steal, load the firmware, and boot an offline
MIPS core, turning it into a coprocessor.

This driver provides a sysfs to allow arbitrary firmware to be loaded
onto a core, which may expose virtio devices. Coprocessor firmware must
abide by the UHI coprocessor boot protocol.

Hi Matt,

Sorry for my very slow response, I kept getting side tracked on the
sysfs part every time I attempted to review this. After discussing with
others it's obvious that being able to boot a remoteproc with a specific
firmware image for some amount of time is a very common request.

Rather than adding a MIPS specific interface for controlling this rproc
I would like for us to bring this to the core.


Yes, makes perfect sense. I had a bit of fun allowing for the firmware 
name / information to be changed in an allocated struct rproc, but I 
have a proof of concept working now and will post it soon. The main 
issue remaining is that when the virtio devices are removed in the 
context of the sysfs write, a warning from dma-mapping.h is triggered 
because interrupts are disabled:


[   28.413958] WARNING: CPU: 0 PID: 121 at 
./include/linux/dma-mapping.h:433 free_buf+0x1a8/0x288

[   28.423542] Modules linked in:
[   28.426951] CPU: 0 PID: 121 Comm: sh Tainted: GW 4.8.0+ #729
[   28.434600] Stack :     80d26cea 
003e 80c5 
   80c5 80c5 00040800 80c5 8f658bc8 80b992dc 
0079
   80d23824  0067544c 0067542c 8048af58 80c5 
0001
  80c5  80b9fcd0 8f7c7b74 80d23824 8051d8bc  
0067544c
  0007 80c5 8f7c7b74 00040800    


  ...
[   28.474274] Call Trace:
[   28.477005] [<8040c538>] show_stack+0x74/0xc0
[   28.481860] [<80757240>] dump_stack+0xd0/0x110
[   28.486813] [<80430d98>] __warn+0xfc/0x130
[   28.491379] [<80430ee0>] warn_slowpath_null+0x2c/0x3c
[   28.497007] [<807e7c6c>] free_buf+0x1a8/0x288
[   28.501862] [<807ea590>] remove_port_data+0x50/0xac
[   28.507298] [<807ea6a0>] unplug_port+0xb4/0x1bc
[   28.512346] [<807ea858>] virtcons_remove+0xb0/0xfc
[   28.517689] [<807b6734>] virtio_dev_remove+0x58/0xc0
[   28.523223] [<807f918c>] __device_release_driver+0xac/0x134
[   28.529433] [<807f924c>] device_release_driver+0x38/0x50
[   28.535352] [<807f7edc>] bus_remove_device+0xfc/0x130
[   28.540980] [<807f4b74>] device_del+0x17c/0x21c
[   28.546027] [<807f4c38>] device_unregister+0x24/0x38
[   28.551562] [<807b6b50>] unregister_virtio_device+0x28/0x44
[   28.55] [<80948ab0>] rproc_change_firmware+0xb4/0x114
[   28.563795] [<80949464>] firmware_store+0x2c/0x40
[   28.569039] [<8060186c>] kernfs_fop_write+0x154/0x1dc
[   28.574669] [<805823f0>] __vfs_write+0x5c/0x17c
[   28.579719] [<805834ac>] vfs_write+0xe0/0x190
[   28.584574] [<80584520>] SyS_write+0x80/0xf4
[   28.589336] [<80415908>] syscall_common+0x34/0x58
[   28.594570]
[   28.596228] ---[ end trace 3f6cae675f2fcee9 ]---


I'm still investigating how to decouple removal of the virtio devices 
from interrupts being disabled.






I would also appreciate if Ralf had some input on the MIPS specifics.


Also regarding the 32-bit requirement, have you investigated what is
needed to support 64-bit ELFs?


Not yet - I wanted to get the 32bit version accepted first, then we can 
look at the required changes to support 64bit since no doubt that will 
necessitate quite a few changes to the core code.




[..]

diff --git a/drivers/remoteproc/mips_remoteproc.c 
b/drivers/remoteproc/mips_remoteproc.c

[..]

+int mips_rproc_op_start(struct rproc *rproc)
+{
+   struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
+   int err;
+   int cpu = mproc->cpu;
+
+   if (mips_rprocs[cpu]) {
+   dev_err(>dev, "CPU%d in use\n", cpu);
+   return -EBUSY;
+   }
+   mips_rprocs[cpu] = rproc;
+
+   /* Create task for the CPU to use before handing off to firmware */
+   mproc->tsk = fork_idle(cpu);
+   if (IS_ERR(mproc->tsk)) {
+   dev_err(>dev, "fork_idle() failed for CPU%d\n", cpu);
+   return -ENOMEM;
+   }
+
+   /* We won't be needing the Linux IPIs anymore */
+   if (mips_smp_ipi_free(get_cpu_mask(cpu)))
+   return -EINVAL;
+
+   /*
+* Direct IPIs from the remote processor to CPU0 since that can't be
+* offlined while the remote CPU is running.
+*/
+   mproc->ipi_linux = irq_reserve_ipi(ipi_domain(), get_cpu_mask(0));
+   if (!mproc->ipi_linux) {
+   dev_err(>dev, "Failed to reserve incoming kick\n");
+   goto exit_rproc_nofrom;
+   }
+
+   mproc->ipi_remote = irq_reserve_ipi(ipi_domain(), get_cpu_mask(cpu));
+   if (!mproc->ipi_remote) {
+   

Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-10-05 Thread Matt Redfearn

Hi Bjorn,

Thanks for the review and comments, much appreciated :-)


On 03/10/16 23:16, Bjorn Andersson wrote:

On Tue 20 Sep 01:47 PDT 2016, Matt Redfearn wrote:


Add a remoteproc driver to steal, load the firmware, and boot an offline
MIPS core, turning it into a coprocessor.

This driver provides a sysfs to allow arbitrary firmware to be loaded
onto a core, which may expose virtio devices. Coprocessor firmware must
abide by the UHI coprocessor boot protocol.

Hi Matt,

Sorry for my very slow response, I kept getting side tracked on the
sysfs part every time I attempted to review this. After discussing with
others it's obvious that being able to boot a remoteproc with a specific
firmware image for some amount of time is a very common request.

Rather than adding a MIPS specific interface for controlling this rproc
I would like for us to bring this to the core.


Yes, makes perfect sense. I had a bit of fun allowing for the firmware 
name / information to be changed in an allocated struct rproc, but I 
have a proof of concept working now and will post it soon. The main 
issue remaining is that when the virtio devices are removed in the 
context of the sysfs write, a warning from dma-mapping.h is triggered 
because interrupts are disabled:


[   28.413958] WARNING: CPU: 0 PID: 121 at 
./include/linux/dma-mapping.h:433 free_buf+0x1a8/0x288

[   28.423542] Modules linked in:
[   28.426951] CPU: 0 PID: 121 Comm: sh Tainted: GW 4.8.0+ #729
[   28.434600] Stack :     80d26cea 
003e 80c5 
   80c5 80c5 00040800 80c5 8f658bc8 80b992dc 
0079
   80d23824  0067544c 0067542c 8048af58 80c5 
0001
  80c5  80b9fcd0 8f7c7b74 80d23824 8051d8bc  
0067544c
  0007 80c5 8f7c7b74 00040800    


  ...
[   28.474274] Call Trace:
[   28.477005] [<8040c538>] show_stack+0x74/0xc0
[   28.481860] [<80757240>] dump_stack+0xd0/0x110
[   28.486813] [<80430d98>] __warn+0xfc/0x130
[   28.491379] [<80430ee0>] warn_slowpath_null+0x2c/0x3c
[   28.497007] [<807e7c6c>] free_buf+0x1a8/0x288
[   28.501862] [<807ea590>] remove_port_data+0x50/0xac
[   28.507298] [<807ea6a0>] unplug_port+0xb4/0x1bc
[   28.512346] [<807ea858>] virtcons_remove+0xb0/0xfc
[   28.517689] [<807b6734>] virtio_dev_remove+0x58/0xc0
[   28.523223] [<807f918c>] __device_release_driver+0xac/0x134
[   28.529433] [<807f924c>] device_release_driver+0x38/0x50
[   28.535352] [<807f7edc>] bus_remove_device+0xfc/0x130
[   28.540980] [<807f4b74>] device_del+0x17c/0x21c
[   28.546027] [<807f4c38>] device_unregister+0x24/0x38
[   28.551562] [<807b6b50>] unregister_virtio_device+0x28/0x44
[   28.55] [<80948ab0>] rproc_change_firmware+0xb4/0x114
[   28.563795] [<80949464>] firmware_store+0x2c/0x40
[   28.569039] [<8060186c>] kernfs_fop_write+0x154/0x1dc
[   28.574669] [<805823f0>] __vfs_write+0x5c/0x17c
[   28.579719] [<805834ac>] vfs_write+0xe0/0x190
[   28.584574] [<80584520>] SyS_write+0x80/0xf4
[   28.589336] [<80415908>] syscall_common+0x34/0x58
[   28.594570]
[   28.596228] ---[ end trace 3f6cae675f2fcee9 ]---


I'm still investigating how to decouple removal of the virtio devices 
from interrupts being disabled.






I would also appreciate if Ralf had some input on the MIPS specifics.


Also regarding the 32-bit requirement, have you investigated what is
needed to support 64-bit ELFs?


Not yet - I wanted to get the 32bit version accepted first, then we can 
look at the required changes to support 64bit since no doubt that will 
necessitate quite a few changes to the core code.




[..]

diff --git a/drivers/remoteproc/mips_remoteproc.c 
b/drivers/remoteproc/mips_remoteproc.c

[..]

+int mips_rproc_op_start(struct rproc *rproc)
+{
+   struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
+   int err;
+   int cpu = mproc->cpu;
+
+   if (mips_rprocs[cpu]) {
+   dev_err(>dev, "CPU%d in use\n", cpu);
+   return -EBUSY;
+   }
+   mips_rprocs[cpu] = rproc;
+
+   /* Create task for the CPU to use before handing off to firmware */
+   mproc->tsk = fork_idle(cpu);
+   if (IS_ERR(mproc->tsk)) {
+   dev_err(>dev, "fork_idle() failed for CPU%d\n", cpu);
+   return -ENOMEM;
+   }
+
+   /* We won't be needing the Linux IPIs anymore */
+   if (mips_smp_ipi_free(get_cpu_mask(cpu)))
+   return -EINVAL;
+
+   /*
+* Direct IPIs from the remote processor to CPU0 since that can't be
+* offlined while the remote CPU is running.
+*/
+   mproc->ipi_linux = irq_reserve_ipi(ipi_domain(), get_cpu_mask(0));
+   if (!mproc->ipi_linux) {
+   dev_err(>dev, "Failed to reserve incoming kick\n");
+   goto exit_rproc_nofrom;
+   }
+
+   mproc->ipi_remote = irq_reserve_ipi(ipi_domain(), get_cpu_mask(cpu));
+   if (!mproc->ipi_remote) {
+   

Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-10-03 Thread Bjorn Andersson
On Tue 20 Sep 01:47 PDT 2016, Matt Redfearn wrote:

> Add a remoteproc driver to steal, load the firmware, and boot an offline
> MIPS core, turning it into a coprocessor.
> 
> This driver provides a sysfs to allow arbitrary firmware to be loaded
> onto a core, which may expose virtio devices. Coprocessor firmware must
> abide by the UHI coprocessor boot protocol.

Hi Matt,

Sorry for my very slow response, I kept getting side tracked on the
sysfs part every time I attempted to review this. After discussing with
others it's obvious that being able to boot a remoteproc with a specific
firmware image for some amount of time is a very common request.

Rather than adding a MIPS specific interface for controlling this rproc
I would like for us to bring this to the core.


I would also appreciate if Ralf had some input on the MIPS specifics.


Also regarding the 32-bit requirement, have you investigated what is
needed to support 64-bit ELFs?

[..]
> diff --git a/drivers/remoteproc/mips_remoteproc.c 
> b/drivers/remoteproc/mips_remoteproc.c
[..]
> +int mips_rproc_op_start(struct rproc *rproc)
> +{
> + struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
> + int err;
> + int cpu = mproc->cpu;
> +
> + if (mips_rprocs[cpu]) {
> + dev_err(>dev, "CPU%d in use\n", cpu);
> + return -EBUSY;
> + }
> + mips_rprocs[cpu] = rproc;
> +
> + /* Create task for the CPU to use before handing off to firmware */
> + mproc->tsk = fork_idle(cpu);
> + if (IS_ERR(mproc->tsk)) {
> + dev_err(>dev, "fork_idle() failed for CPU%d\n", cpu);
> + return -ENOMEM;
> + }
> +
> + /* We won't be needing the Linux IPIs anymore */
> + if (mips_smp_ipi_free(get_cpu_mask(cpu)))
> + return -EINVAL;
> +
> + /*
> +  * Direct IPIs from the remote processor to CPU0 since that can't be
> +  * offlined while the remote CPU is running.
> +  */
> + mproc->ipi_linux = irq_reserve_ipi(ipi_domain(), get_cpu_mask(0));
> + if (!mproc->ipi_linux) {
> + dev_err(>dev, "Failed to reserve incoming kick\n");
> + goto exit_rproc_nofrom;
> + }
> +
> + mproc->ipi_remote = irq_reserve_ipi(ipi_domain(), get_cpu_mask(cpu));
> + if (!mproc->ipi_remote) {
> + dev_err(>dev, "Failed to reserve outgoing kick\n");
> + goto exit_rproc_noto;
> + }
> +
> + /* register incoming ipi */
> + err = devm_request_threaded_irq(>dev, mproc->ipi_linux,
> + mips_rproc_ipi_handler,
> + mips_rproc_vq_int, 0,
> + "mips-rproc IPI in", mproc->rproc);

Based on how you've designed this I think it makes sense to just depend
on the fact that stop() will always be called and hence you do not
benefit from the devm_ version of this api.

> + if (err) {
> + dev_err(>dev, "Failed to register incoming kick: %d\n",
> + err);
> + goto exit_rproc_noint;
> + }
> +
> + if (!mips_cps_steal_cpu_and_execute(cpu, _rproc_cpu_entry,
> + mproc->tsk))
> + return 0;

Please flip this around, to follow the pattern of the others, like:

if (mips_cps_steal_cpu_and_execute()) {
dev_err()
goto exit_free_irq;
}

return 0;

exit_free_irq:

> +
> + dev_err(>dev, "Failed to steal CPU%d for remote\n", cpu);
> + devm_free_irq(>dev, mproc->ipi_linux, mproc->rproc);
> +exit_rproc_noint:
> + irq_destroy_ipi(mproc->ipi_remote, get_cpu_mask(cpu));
> +exit_rproc_noto:
> + irq_destroy_ipi(mproc->ipi_linux, get_cpu_mask(0));
> +exit_rproc_nofrom:
> + free_task(mproc->tsk);
> + mips_rprocs[cpu] = NULL;
> +
> + /* Set up the Linux IPIs again */
> + mips_smp_ipi_allocate(get_cpu_mask(cpu));
> + return -EINVAL;
> +}
> +
> +int mips_rproc_op_stop(struct rproc *rproc)
> +{
> + struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
> +
> + if (mproc->ipi_linux)

stop() should not be called unless start() succeeded, so ipi_linux
should not be able to be 0.

> + devm_free_irq(>dev, mproc->ipi_linux, mproc->rproc);
> +
> + irq_destroy_ipi(mproc->ipi_linux, get_cpu_mask(0));
> + irq_destroy_ipi(mproc->ipi_remote, get_cpu_mask(mproc->cpu));
> +
> + /* Set up the Linux IPIs again */
> + mips_smp_ipi_allocate(get_cpu_mask(mproc->cpu));
> +
> + free_task(mproc->tsk);
> +
> + mips_rprocs[mproc->cpu] = NULL;
> +
> + return mips_cps_halt_and_return_cpu(mproc->cpu);
> +}
> +
> +
[..]
> +
> +/* Steal a core and run some firmware on it */
> +int mips_rproc_start(struct mips_rproc *mproc, const char *firmware, size_t 
> len)
> +{
> + int err = -EINVAL;
> + struct mips_rproc **priv;
> +
> + /* Duplicate the filename, dropping whitespace from the end via len */
> + mproc->firmware 

Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-10-03 Thread Bjorn Andersson
On Tue 20 Sep 01:47 PDT 2016, Matt Redfearn wrote:

> Add a remoteproc driver to steal, load the firmware, and boot an offline
> MIPS core, turning it into a coprocessor.
> 
> This driver provides a sysfs to allow arbitrary firmware to be loaded
> onto a core, which may expose virtio devices. Coprocessor firmware must
> abide by the UHI coprocessor boot protocol.

Hi Matt,

Sorry for my very slow response, I kept getting side tracked on the
sysfs part every time I attempted to review this. After discussing with
others it's obvious that being able to boot a remoteproc with a specific
firmware image for some amount of time is a very common request.

Rather than adding a MIPS specific interface for controlling this rproc
I would like for us to bring this to the core.


I would also appreciate if Ralf had some input on the MIPS specifics.


Also regarding the 32-bit requirement, have you investigated what is
needed to support 64-bit ELFs?

[..]
> diff --git a/drivers/remoteproc/mips_remoteproc.c 
> b/drivers/remoteproc/mips_remoteproc.c
[..]
> +int mips_rproc_op_start(struct rproc *rproc)
> +{
> + struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
> + int err;
> + int cpu = mproc->cpu;
> +
> + if (mips_rprocs[cpu]) {
> + dev_err(>dev, "CPU%d in use\n", cpu);
> + return -EBUSY;
> + }
> + mips_rprocs[cpu] = rproc;
> +
> + /* Create task for the CPU to use before handing off to firmware */
> + mproc->tsk = fork_idle(cpu);
> + if (IS_ERR(mproc->tsk)) {
> + dev_err(>dev, "fork_idle() failed for CPU%d\n", cpu);
> + return -ENOMEM;
> + }
> +
> + /* We won't be needing the Linux IPIs anymore */
> + if (mips_smp_ipi_free(get_cpu_mask(cpu)))
> + return -EINVAL;
> +
> + /*
> +  * Direct IPIs from the remote processor to CPU0 since that can't be
> +  * offlined while the remote CPU is running.
> +  */
> + mproc->ipi_linux = irq_reserve_ipi(ipi_domain(), get_cpu_mask(0));
> + if (!mproc->ipi_linux) {
> + dev_err(>dev, "Failed to reserve incoming kick\n");
> + goto exit_rproc_nofrom;
> + }
> +
> + mproc->ipi_remote = irq_reserve_ipi(ipi_domain(), get_cpu_mask(cpu));
> + if (!mproc->ipi_remote) {
> + dev_err(>dev, "Failed to reserve outgoing kick\n");
> + goto exit_rproc_noto;
> + }
> +
> + /* register incoming ipi */
> + err = devm_request_threaded_irq(>dev, mproc->ipi_linux,
> + mips_rproc_ipi_handler,
> + mips_rproc_vq_int, 0,
> + "mips-rproc IPI in", mproc->rproc);

Based on how you've designed this I think it makes sense to just depend
on the fact that stop() will always be called and hence you do not
benefit from the devm_ version of this api.

> + if (err) {
> + dev_err(>dev, "Failed to register incoming kick: %d\n",
> + err);
> + goto exit_rproc_noint;
> + }
> +
> + if (!mips_cps_steal_cpu_and_execute(cpu, _rproc_cpu_entry,
> + mproc->tsk))
> + return 0;

Please flip this around, to follow the pattern of the others, like:

if (mips_cps_steal_cpu_and_execute()) {
dev_err()
goto exit_free_irq;
}

return 0;

exit_free_irq:

> +
> + dev_err(>dev, "Failed to steal CPU%d for remote\n", cpu);
> + devm_free_irq(>dev, mproc->ipi_linux, mproc->rproc);
> +exit_rproc_noint:
> + irq_destroy_ipi(mproc->ipi_remote, get_cpu_mask(cpu));
> +exit_rproc_noto:
> + irq_destroy_ipi(mproc->ipi_linux, get_cpu_mask(0));
> +exit_rproc_nofrom:
> + free_task(mproc->tsk);
> + mips_rprocs[cpu] = NULL;
> +
> + /* Set up the Linux IPIs again */
> + mips_smp_ipi_allocate(get_cpu_mask(cpu));
> + return -EINVAL;
> +}
> +
> +int mips_rproc_op_stop(struct rproc *rproc)
> +{
> + struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
> +
> + if (mproc->ipi_linux)

stop() should not be called unless start() succeeded, so ipi_linux
should not be able to be 0.

> + devm_free_irq(>dev, mproc->ipi_linux, mproc->rproc);
> +
> + irq_destroy_ipi(mproc->ipi_linux, get_cpu_mask(0));
> + irq_destroy_ipi(mproc->ipi_remote, get_cpu_mask(mproc->cpu));
> +
> + /* Set up the Linux IPIs again */
> + mips_smp_ipi_allocate(get_cpu_mask(mproc->cpu));
> +
> + free_task(mproc->tsk);
> +
> + mips_rprocs[mproc->cpu] = NULL;
> +
> + return mips_cps_halt_and_return_cpu(mproc->cpu);
> +}
> +
> +
[..]
> +
> +/* Steal a core and run some firmware on it */
> +int mips_rproc_start(struct mips_rproc *mproc, const char *firmware, size_t 
> len)
> +{
> + int err = -EINVAL;
> + struct mips_rproc **priv;
> +
> + /* Duplicate the filename, dropping whitespace from the end via len */
> + mproc->firmware 

Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-10-03 Thread Matt Redfearn

Hi Bjorn / Ohad,

Please could you give some feedback on this patch?

Thanks,

Matt


On 20/09/16 09:47, Matt Redfearn wrote:

Add a remoteproc driver to steal, load the firmware, and boot an offline
MIPS core, turning it into a coprocessor.

This driver provides a sysfs to allow arbitrary firmware to be loaded
onto a core, which may expose virtio devices. Coprocessor firmware must
abide by the UHI coprocessor boot protocol.

Signed-off-by: Lisa Parratt 
Signed-off-by: Matt Redfearn 

---

Changes in v2: None

  Documentation/ABI/testing/sysfs-class-mips-rproc |  24 +
  drivers/remoteproc/Kconfig   |  11 +
  drivers/remoteproc/Makefile  |   1 +
  drivers/remoteproc/mips_remoteproc.c | 651 +++
  4 files changed, 687 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-class-mips-rproc
  create mode 100644 drivers/remoteproc/mips_remoteproc.c

diff --git a/Documentation/ABI/testing/sysfs-class-mips-rproc 
b/Documentation/ABI/testing/sysfs-class-mips-rproc
new file mode 100644
index ..c09d02a755e4
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-mips-rproc
@@ -0,0 +1,24 @@
+What:  /sys/class/mips-rproc/rproc#/firmware
+Date:  August 2015
+Contact:   Lisa Parratt 
+Description:   MIPS VPE remoteproc start
+
+   This node only exists when a VPE is considered offline by Linux. Writes
+   to this file will start firmware running on a VPE.
+
+   If the VPE is idle, specifying a name will cause a remoteproc instance
+   to be allocated, which will cause the core to be stolen, the firmware
+   image to be loaded, and the remoteproc instance to be started.
+   Otherwise, the operation will fail.
+
+What:  /sys/class/mips-rproc/rproc#/stop
+Date:  August 2015
+Contact:   Lisa Parratt 
+Description:   MIPS VPE remoteproc stop
+
+   This node only exists when a VPE is considered offline by Linux. Writes
+   to this file will stop firmware running on a VPE.
+
+   If the VPE is running a remote proc instance, the instance will be
+   stopped, the core returned, and the instance freed.
+   Otherwise, the operation will fail.
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 1a8bf76a925f..05db52e0e668 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -100,4 +100,15 @@ config ST_REMOTEPROC
  processor framework.
  This can be either built-in or a loadable module.
  
+config MIPS_REMOTEPROC

+   tristate "MIPS remoteproc support"
+   depends on MIPS_CPS && HAS_DMA
+   select CMA
+   select REMOTEPROC
+   select MIPS_STEAL
+   help
+ Say y here to support using offline cores/VPEs as remote processors
+ via the remote processor framework.
+ If unsure say N.
+
  endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 92d3758bd15c..de19cd320f3a 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_DA8XX_REMOTEPROC)+= 
da8xx_remoteproc.o
  obj-$(CONFIG_QCOM_MDT_LOADER) += qcom_mdt_loader.o
  obj-$(CONFIG_QCOM_Q6V5_PIL)   += qcom_q6v5_pil.o
  obj-$(CONFIG_ST_REMOTEPROC)   += st_remoteproc.o
+obj-$(CONFIG_MIPS_REMOTEPROC)  += mips_remoteproc.o
diff --git a/drivers/remoteproc/mips_remoteproc.c 
b/drivers/remoteproc/mips_remoteproc.c
new file mode 100644
index ..944ad66280b4
--- /dev/null
+++ b/drivers/remoteproc/mips_remoteproc.c
@@ -0,0 +1,651 @@
+/*
+ * MIPS Remote Processor driver
+ *
+ * Copyright (C) 2016 Imagination Technologies
+ * Lisa Parratt 
+ * Matt Redfearn 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+struct mips_rproc {
+   struct rproc*rproc;
+   char*firmware;
+   struct task_struct  *tsk;
+   struct device   dev;
+   unsigned intcpu;
+   int ipi_linux;
+   int ipi_remote;
+};
+
+static struct rproc *mips_rprocs[NR_CPUS];
+
+#define to_mips_rproc(d) container_of(d, struct mips_rproc, dev)
+
+
+/* Compute the largest page mask a physical address can be mapped with */
+static unsigned long mips_rproc_largest_pm(unsigned long pa,
+  unsigned long maxmask)
+{
+   unsigned long 

Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-10-03 Thread Matt Redfearn

Hi Bjorn / Ohad,

Please could you give some feedback on this patch?

Thanks,

Matt


On 20/09/16 09:47, Matt Redfearn wrote:

Add a remoteproc driver to steal, load the firmware, and boot an offline
MIPS core, turning it into a coprocessor.

This driver provides a sysfs to allow arbitrary firmware to be loaded
onto a core, which may expose virtio devices. Coprocessor firmware must
abide by the UHI coprocessor boot protocol.

Signed-off-by: Lisa Parratt 
Signed-off-by: Matt Redfearn 

---

Changes in v2: None

  Documentation/ABI/testing/sysfs-class-mips-rproc |  24 +
  drivers/remoteproc/Kconfig   |  11 +
  drivers/remoteproc/Makefile  |   1 +
  drivers/remoteproc/mips_remoteproc.c | 651 +++
  4 files changed, 687 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-class-mips-rproc
  create mode 100644 drivers/remoteproc/mips_remoteproc.c

diff --git a/Documentation/ABI/testing/sysfs-class-mips-rproc 
b/Documentation/ABI/testing/sysfs-class-mips-rproc
new file mode 100644
index ..c09d02a755e4
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-mips-rproc
@@ -0,0 +1,24 @@
+What:  /sys/class/mips-rproc/rproc#/firmware
+Date:  August 2015
+Contact:   Lisa Parratt 
+Description:   MIPS VPE remoteproc start
+
+   This node only exists when a VPE is considered offline by Linux. Writes
+   to this file will start firmware running on a VPE.
+
+   If the VPE is idle, specifying a name will cause a remoteproc instance
+   to be allocated, which will cause the core to be stolen, the firmware
+   image to be loaded, and the remoteproc instance to be started.
+   Otherwise, the operation will fail.
+
+What:  /sys/class/mips-rproc/rproc#/stop
+Date:  August 2015
+Contact:   Lisa Parratt 
+Description:   MIPS VPE remoteproc stop
+
+   This node only exists when a VPE is considered offline by Linux. Writes
+   to this file will stop firmware running on a VPE.
+
+   If the VPE is running a remote proc instance, the instance will be
+   stopped, the core returned, and the instance freed.
+   Otherwise, the operation will fail.
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 1a8bf76a925f..05db52e0e668 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -100,4 +100,15 @@ config ST_REMOTEPROC
  processor framework.
  This can be either built-in or a loadable module.
  
+config MIPS_REMOTEPROC

+   tristate "MIPS remoteproc support"
+   depends on MIPS_CPS && HAS_DMA
+   select CMA
+   select REMOTEPROC
+   select MIPS_STEAL
+   help
+ Say y here to support using offline cores/VPEs as remote processors
+ via the remote processor framework.
+ If unsure say N.
+
  endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 92d3758bd15c..de19cd320f3a 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_DA8XX_REMOTEPROC)+= 
da8xx_remoteproc.o
  obj-$(CONFIG_QCOM_MDT_LOADER) += qcom_mdt_loader.o
  obj-$(CONFIG_QCOM_Q6V5_PIL)   += qcom_q6v5_pil.o
  obj-$(CONFIG_ST_REMOTEPROC)   += st_remoteproc.o
+obj-$(CONFIG_MIPS_REMOTEPROC)  += mips_remoteproc.o
diff --git a/drivers/remoteproc/mips_remoteproc.c 
b/drivers/remoteproc/mips_remoteproc.c
new file mode 100644
index ..944ad66280b4
--- /dev/null
+++ b/drivers/remoteproc/mips_remoteproc.c
@@ -0,0 +1,651 @@
+/*
+ * MIPS Remote Processor driver
+ *
+ * Copyright (C) 2016 Imagination Technologies
+ * Lisa Parratt 
+ * Matt Redfearn 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+struct mips_rproc {
+   struct rproc*rproc;
+   char*firmware;
+   struct task_struct  *tsk;
+   struct device   dev;
+   unsigned intcpu;
+   int ipi_linux;
+   int ipi_remote;
+};
+
+static struct rproc *mips_rprocs[NR_CPUS];
+
+#define to_mips_rproc(d) container_of(d, struct mips_rproc, dev)
+
+
+/* Compute the largest page mask a physical address can be mapped with */
+static unsigned long mips_rproc_largest_pm(unsigned long pa,
+  unsigned long maxmask)
+{
+   unsigned long mask;
+   /* Find address bits limiting alignment */
+   unsigned long shift = ffs(pa);
+
+   /* Obey MIPS restrictions on page sizes */
+   

Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-09-20 Thread Matt Redfearn

Hi Thomas,


On 20/09/16 10:47, Thomas Gleixner wrote:

On Tue, 20 Sep 2016, Matt Redfearn wrote:

+/* Intercept CPU hotplug events for syfs purposes */
+static int mips_rproc_callback(struct notifier_block *nfb, unsigned long 
action,
+  void *hcpu)
+{

Please convert to cpu hotplug state machine.


OK, I've done that for this and the MIPS GIC patch, using the dynamic 
CPUHP_AP_ONLINE_DYN state - I hope that is ok.





+   unsigned int cpu = (unsigned long)hcpu;
+
+   switch (action) {
+   case CPU_UP_PREPARE:
+   case CPU_DOWN_FAILED:
+   mips_rproc_device_unregister(cpu);
+   break;
+   case CPU_DOWN_PREPARE:
+   mips_rproc_device_register(cpu);
+   break;
+   }

There is no reason why you need to setup the rproc device on
DOWN_PREPARE. It's sufficient to do that when the CPU is dead, so you can
use a symetric callback prep/dead.


Sure, the new state machine makes this much nicer registering the 
on/offline callbacks on one state.





+   /* Dynamically create mips-rproc class devices based on hotplug data */
+   get_online_cpus();
+   for_each_possible_cpu(cpu)
+   if (!cpu_online(cpu))
+   mips_rproc_device_register(cpu);
+   register_hotcpu_notifier(_rproc_notifier);
+   put_online_cpus();

Perhaps we should add support for "reverse" functionality to the state
machine core. I'll have a look later how hard that'd be.


Yeah - I've had to work around the framework a little here since we 
require the opposite sense to the callbacks - activate the driver when 
the cpu is offlined and vice versa. In practice the only issue this gave 
me was that by default the framework invokes the teardown callback when 
the state is removed, so I used __cpuhp_remove_state() to avoid creating 
a remote processor device as the driver is being removed.


Thanks,
Matt



Thanks,

tglx




Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-09-20 Thread Matt Redfearn

Hi Thomas,


On 20/09/16 10:47, Thomas Gleixner wrote:

On Tue, 20 Sep 2016, Matt Redfearn wrote:

+/* Intercept CPU hotplug events for syfs purposes */
+static int mips_rproc_callback(struct notifier_block *nfb, unsigned long 
action,
+  void *hcpu)
+{

Please convert to cpu hotplug state machine.


OK, I've done that for this and the MIPS GIC patch, using the dynamic 
CPUHP_AP_ONLINE_DYN state - I hope that is ok.





+   unsigned int cpu = (unsigned long)hcpu;
+
+   switch (action) {
+   case CPU_UP_PREPARE:
+   case CPU_DOWN_FAILED:
+   mips_rproc_device_unregister(cpu);
+   break;
+   case CPU_DOWN_PREPARE:
+   mips_rproc_device_register(cpu);
+   break;
+   }

There is no reason why you need to setup the rproc device on
DOWN_PREPARE. It's sufficient to do that when the CPU is dead, so you can
use a symetric callback prep/dead.


Sure, the new state machine makes this much nicer registering the 
on/offline callbacks on one state.





+   /* Dynamically create mips-rproc class devices based on hotplug data */
+   get_online_cpus();
+   for_each_possible_cpu(cpu)
+   if (!cpu_online(cpu))
+   mips_rproc_device_register(cpu);
+   register_hotcpu_notifier(_rproc_notifier);
+   put_online_cpus();

Perhaps we should add support for "reverse" functionality to the state
machine core. I'll have a look later how hard that'd be.


Yeah - I've had to work around the framework a little here since we 
require the opposite sense to the callbacks - activate the driver when 
the cpu is offlined and vice versa. In practice the only issue this gave 
me was that by default the framework invokes the teardown callback when 
the state is removed, so I used __cpuhp_remove_state() to avoid creating 
a remote processor device as the driver is being removed.


Thanks,
Matt



Thanks,

tglx




Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-09-20 Thread Thomas Gleixner
On Tue, 20 Sep 2016, Matt Redfearn wrote:
> +/* Intercept CPU hotplug events for syfs purposes */
> +static int mips_rproc_callback(struct notifier_block *nfb, unsigned long 
> action,
> +void *hcpu)
> +{

Please convert to cpu hotplug state machine.

> + unsigned int cpu = (unsigned long)hcpu;
> +
> + switch (action) {
> + case CPU_UP_PREPARE:
> + case CPU_DOWN_FAILED:
> + mips_rproc_device_unregister(cpu);
> + break;
> + case CPU_DOWN_PREPARE:
> + mips_rproc_device_register(cpu);
> + break;
> + }

There is no reason why you need to setup the rproc device on
DOWN_PREPARE. It's sufficient to do that when the CPU is dead, so you can
use a symetric callback prep/dead.

> + /* Dynamically create mips-rproc class devices based on hotplug data */
> + get_online_cpus();
> + for_each_possible_cpu(cpu)
> + if (!cpu_online(cpu))
> + mips_rproc_device_register(cpu);
> + register_hotcpu_notifier(_rproc_notifier);
> + put_online_cpus();

Perhaps we should add support for "reverse" functionality to the state
machine core. I'll have a look later how hard that'd be.

Thanks,

tglx


Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

2016-09-20 Thread Thomas Gleixner
On Tue, 20 Sep 2016, Matt Redfearn wrote:
> +/* Intercept CPU hotplug events for syfs purposes */
> +static int mips_rproc_callback(struct notifier_block *nfb, unsigned long 
> action,
> +void *hcpu)
> +{

Please convert to cpu hotplug state machine.

> + unsigned int cpu = (unsigned long)hcpu;
> +
> + switch (action) {
> + case CPU_UP_PREPARE:
> + case CPU_DOWN_FAILED:
> + mips_rproc_device_unregister(cpu);
> + break;
> + case CPU_DOWN_PREPARE:
> + mips_rproc_device_register(cpu);
> + break;
> + }

There is no reason why you need to setup the rproc device on
DOWN_PREPARE. It's sufficient to do that when the CPU is dead, so you can
use a symetric callback prep/dead.

> + /* Dynamically create mips-rproc class devices based on hotplug data */
> + get_online_cpus();
> + for_each_possible_cpu(cpu)
> + if (!cpu_online(cpu))
> + mips_rproc_device_register(cpu);
> + register_hotcpu_notifier(_rproc_notifier);
> + put_online_cpus();

Perhaps we should add support for "reverse" functionality to the state
machine core. I'll have a look later how hard that'd be.

Thanks,

tglx