Re: [PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver

2020-11-05 Thread Mathieu Poirier 
Hi,

On Wed, Nov 04, 2020 at 09:57:40PM +, Ben Levinsky wrote:
> Hi Mathieu,
> 
> > -Original Message-
> > From: Mathieu Poirier 
> > Sent: Wednesday, November 4, 2020 1:30 PM
> > To: Ben Levinsky 
> > Cc: michael.auch...@ni.com; Stefano Stabellini ;
> > devicet...@vger.kernel.org; linux-remotep...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> > Subject: Re: [PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> > driver
> > 
> > On Tue, Nov 03, 2020 at 10:23:39AM -0800, Ben Levinsky wrote:
> > > R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> > > remotproc driver, we can boot the R5 sub-system in different 2
> > > configurations -
> > >   * split
> > >   * lock-step
> > >
> > > The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
> > > Platform Management Unit that handles the R5 configuration, memory
> > access
> > > and R5 lifecycle management. The interface to this manager is done in this
> > > driver via zynqmp_pm_* function calls.
> > >
> > > Signed-off-by: Wendy Liang 
> > > Signed-off-by: Michal Simek 
> > > Signed-off-by: Ed Mooring 
> > > Signed-off-by: Jason Wu 
> > > Signed-off-by: Ben Levinsky 
> > > ---
> > > - update documented param list for zynqmp_r5_probe
> > > - remove use-after-free lines in zynqmp_r5_probe,
> > >   zynqmp_r5_remoteproc_probe, and zynqmp_r5_remoteproc_remove
> > > - update path for error handling in zynqmp_r5_probe and
> > >   zynqmp_r5_remoteproc_probe
> > > - as the mbox properties are optional, update zynqmp_r5_rproc_kick
> > >   so that the mailbox functionality only occurs if these
> > >   properties are provided
> > > ---
> > >  drivers/remoteproc/Kconfig|   8 +
> > >  drivers/remoteproc/Makefile   |   1 +
> > >  drivers/remoteproc/zynqmp_r5_remoteproc.c | 778
> > ++
> > 
> > Many of my comments from V19 have not been addressed and as such I will
> > not move
> > foward with this set.  Moreover I will not consider a new revision before
> > November 18th.
> > 
> 
> Thank you for the advance notice. 
> 
> As for V19 comments that are unresolved, can you expand on which ones?
> From the v19 review here is a list of the comments with your comments 
> surrounded by quotes
> and how I had thought these were addressed. 
> 
> "
> As far as I can see zynqmp_r5_rproc::dt_node is not needed
> " 
> 
>   I removed the field zynqmp_r5_rproc::dt_node and its no longer used in 
> the file
> 
> "
> > +   z_rproc->dev = _ptr->dev;
> > +   z_rproc->dev->of_node = node;
> 
> This is quite hackish to me.  The problem is that @pdev is the rpu'a DT node
> rather than the r5_X's DT node.  I suggest to have a look at k3_r5_probe()[1],
> especially at how devm_of_platform_populate() is called on @dev and from there
> how the platform device for each r5 is retreived using the same
> for_each_available_child_of_node() loop you also have.
> "
>   As I understand it this is resolved as now @pdev is the r5_X's DT node 
> similar to the k3_r5_probe()
>   Also the 2 lines in question are now removed in the v22 patch
>   
> 
> "> +  struct zynqmp_r5_rproc *core;
> 
> Please call variables of type zynqmp_r5_rproc the same name throughout the 
> file.
> "
>   So in zynqmp_r5_probe there is an instance of a zynqmp_r5_rproc** with 
> the name core, this is because
>   there is already arg with name z_rproc and z_rproc_ptr or something 
> like this may be confusing.
> 
>   With that being said, instead the zynqmp_r5_rproc** can be renamed to 
> z_rproc if this is what you 
>   mean?
> 
> "
> > +   rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode") ?
> > +   PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;


> 
> Indentation problem.  Note that is it now permitted to go beyond 80 characters
> per line.
> "
> 
>   In v22 patch this is now:
>   rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode") 
> ?
>   PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;
> 
>   Is this indentation alright?

rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode") ?
PM_RPU_MODE_LOCKSTEP : 
PM_RPU_MODE_SPLIT;

As always, the following lines need t

RE: [PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver

2020-11-04 Thread Ben Levinsky 
Hi Mathieu,

> -Original Message-
> From: Mathieu Poirier 
> Sent: Wednesday, November 4, 2020 1:30 PM
> To: Ben Levinsky 
> Cc: michael.auch...@ni.com; Stefano Stabellini ;
> devicet...@vger.kernel.org; linux-remotep...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> driver
> 
> On Tue, Nov 03, 2020 at 10:23:39AM -0800, Ben Levinsky wrote:
> > R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> > remotproc driver, we can boot the R5 sub-system in different 2
> > configurations -
> > * split
> > * lock-step
> >
> > The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
> > Platform Management Unit that handles the R5 configuration, memory
> access
> > and R5 lifecycle management. The interface to this manager is done in this
> > driver via zynqmp_pm_* function calls.
> >
> > Signed-off-by: Wendy Liang 
> > Signed-off-by: Michal Simek 
> > Signed-off-by: Ed Mooring 
> > Signed-off-by: Jason Wu 
> > Signed-off-by: Ben Levinsky 
> > ---
> > - update documented param list for zynqmp_r5_probe
> > - remove use-after-free lines in zynqmp_r5_probe,
> >   zynqmp_r5_remoteproc_probe, and zynqmp_r5_remoteproc_remove
> > - update path for error handling in zynqmp_r5_probe and
> >   zynqmp_r5_remoteproc_probe
> > - as the mbox properties are optional, update zynqmp_r5_rproc_kick
> >   so that the mailbox functionality only occurs if these
> >   properties are provided
> > ---
> >  drivers/remoteproc/Kconfig|   8 +
> >  drivers/remoteproc/Makefile   |   1 +
> >  drivers/remoteproc/zynqmp_r5_remoteproc.c | 778
> ++
> 
> Many of my comments from V19 have not been addressed and as such I will
> not move
> foward with this set.  Moreover I will not consider a new revision before
> November 18th.
> 

Thank you for the advance notice. 

As for V19 comments that are unresolved, can you expand on which ones?
>From the v19 review here is a list of the comments with your comments 
>surrounded by quotes
and how I had thought these were addressed. 

"
As far as I can see zynqmp_r5_rproc::dt_node is not needed
" 

I removed the field zynqmp_r5_rproc::dt_node and its no longer used in 
the file

"
> + z_rproc->dev = _ptr->dev;
> + z_rproc->dev->of_node = node;

This is quite hackish to me.  The problem is that @pdev is the rpu'a DT node
rather than the r5_X's DT node.  I suggest to have a look at k3_r5_probe()[1],
especially at how devm_of_platform_populate() is called on @dev and from there
how the platform device for each r5 is retreived using the same
for_each_available_child_of_node() loop you also have.
"
As I understand it this is resolved as now @pdev is the r5_X's DT node 
similar to the k3_r5_probe()
Also the 2 lines in question are now removed in the v22 patch


"> +struct zynqmp_r5_rproc *core;

Please call variables of type zynqmp_r5_rproc the same name throughout the file.
"
So in zynqmp_r5_probe there is an instance of a zynqmp_r5_rproc** with 
the name core, this is because
there is already arg with name z_rproc and z_rproc_ptr or something 
like this may be confusing.

With that being said, instead the zynqmp_r5_rproc** can be renamed to 
z_rproc if this is what you 
mean?

"
> + rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode") ?
> + PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;

Indentation problem.  Note that is it now permitted to go beyond 80 characters
per line.
"

In v22 patch this is now:
rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode") 
?
PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;

Is this indentation alright?

"
> + i = 0;

What is the above for?  As fas as I can tell 'i' is not used in the loop below.
"
In v19 this var was deprecated, but as per feedback from Michael A. in 
later revisions there was unhandled 
case of R5_0 successfully initializating but R5_1 failing. To 
accommodate this case the var i was
re-introduced. If the name is unclear then it can be refactored if this 
is the intention

"
> + struct list_head *pos, *cluster = (struct list_head *)
> +  platform_get_drvdata(pdev);

Either align the 'p' with the '(' or simply do the assignement on another line
below.
"

This may be formatting on my terminal vs. the patch format itself but 
in v22 I think this is resolved.



If still 1 or mor

Re: [PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver

2020-11-04 Thread Mathieu Poirier 
On Tue, Nov 03, 2020 at 10:23:39AM -0800, Ben Levinsky wrote:
> R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> remotproc driver, we can boot the R5 sub-system in different 2
> configurations -
>   * split
>   * lock-step
> 
> The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
> Platform Management Unit that handles the R5 configuration, memory access
> and R5 lifecycle management. The interface to this manager is done in this
> driver via zynqmp_pm_* function calls.
> 
> Signed-off-by: Wendy Liang 
> Signed-off-by: Michal Simek 
> Signed-off-by: Ed Mooring 
> Signed-off-by: Jason Wu 
> Signed-off-by: Ben Levinsky 
> ---
> - update documented param list for zynqmp_r5_probe
> - remove use-after-free lines in zynqmp_r5_probe,
>   zynqmp_r5_remoteproc_probe, and zynqmp_r5_remoteproc_remove
> - update path for error handling in zynqmp_r5_probe and
>   zynqmp_r5_remoteproc_probe
> - as the mbox properties are optional, update zynqmp_r5_rproc_kick
>   so that the mailbox functionality only occurs if these
>   properties are provided
> ---
>  drivers/remoteproc/Kconfig|   8 +
>  drivers/remoteproc/Makefile   |   1 +
>  drivers/remoteproc/zynqmp_r5_remoteproc.c | 778 ++

Many of my comments from V19 have not been addressed and as such I will not move
foward with this set.  Moreover I will not consider a new revision before
November 18th. 

>  3 files changed, 787 insertions(+)
>  create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c6659dfea7c7..c2fe54b1d94f 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -275,6 +275,14 @@ config TI_K3_DSP_REMOTEPROC
> It's safe to say N here if you're not interested in utilizing
> the DSP slave processors.
>  
> +config ZYNQMP_R5_REMOTEPROC
> + tristate "ZynqMP R5 remoteproc support"
> + depends on PM && ARCH_ZYNQMP
> + select RPMSG_VIRTIO
> + select ZYNQMP_IPI_MBOX
> + help
> +   Say y or m here to support ZynqMP R5 remote processors via the remote
> +   processor framework.
>  endif # REMOTEPROC
>  
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 3dfa28e6c701..ef1abff654c2 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
>  obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
>  obj-$(CONFIG_STM32_RPROC)+= stm32_rproc.o
>  obj-$(CONFIG_TI_K3_DSP_REMOTEPROC)   += ti_k3_dsp_remoteproc.o
> +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC)   += zynqmp_r5_remoteproc.o
> diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c 
> b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> new file mode 100644
> index ..48bffa91bc00
> --- /dev/null
> +++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> @@ -0,0 +1,779 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Zynq R5 Remote Processor driver
> + *
> + * Based on origin OMAP and Zynq Remote Processor driver
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "remoteproc_internal.h"
> +
> +#define MAX_RPROCS   2 /* Support up to 2 RPU */
> +#define MAX_MEM_PNODES   4 /* Max power nodes for one RPU memory 
> instance */
> +
> +#define BANK_LIST_PROP   "meta-memory-regions"
> +#define DDR_LIST_PROP"memory-regions"
> +
> +/* IPI buffer MAX length */
> +#define IPI_BUF_LEN_MAX  32U
> +/* RX mailbox client buffer max length */
> +#define RX_MBOX_CLIENT_BUF_MAX   (IPI_BUF_LEN_MAX + \
> +  sizeof(struct zynqmp_ipi_message))
> +
> +/**
> + * struct zynqmp_r5_mem - zynqmp rpu memory data
> + * @pnode_id: TCM power domain ids
> + * @res: memory resource
> + * @node: list node
> + */
> +struct zynqmp_r5_mem {
> + u32 pnode_id[MAX_MEM_PNODES];
> + struct resource res;
> + struct list_head node;
> +};
> +
> +/**
> + * struct zynqmp_r5_rproc - zynqmp rpu remote processor state
> + *   this is for each individual R5 core's state
> + *
> + * @rx_mc_buf: rx mailbox client buffer to save the rx message
> + * @tx_mc: tx mailbox client
> + * @rx_mc: rx mailbox client * @dev: device of RPU instance
> + * @mbox_work: mbox_work for the RPU remoteproc
> + * @tx_mc_skbs: socket buffers for tx mailbox client
> + * @dev: device of RPU instance
> + * @rproc: rproc handle
> + * @tx_chan: tx mailbox channel
> + * @rx_chan: rx mailbox channel
> + * @pnode_id: RPU CPU power domain id
> + * @elem: linked list item
> + * @dt_node: device tree node that holds information for 1 R5 core.
> + */
> +struct zynqmp_r5_rproc {
> + unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
> + struct mbox_client tx_mc;
> +

[PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver

2020-11-03 Thread Ben Levinsky 
R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
remotproc driver, we can boot the R5 sub-system in different 2
configurations -
* split
* lock-step

The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
Platform Management Unit that handles the R5 configuration, memory access
and R5 lifecycle management. The interface to this manager is done in this
driver via zynqmp_pm_* function calls.

Signed-off-by: Wendy Liang 
Signed-off-by: Michal Simek 
Signed-off-by: Ed Mooring 
Signed-off-by: Jason Wu 
Signed-off-by: Ben Levinsky 
---
- update documented param list for zynqmp_r5_probe
- remove use-after-free lines in zynqmp_r5_probe,
  zynqmp_r5_remoteproc_probe, and zynqmp_r5_remoteproc_remove
- update path for error handling in zynqmp_r5_probe and
  zynqmp_r5_remoteproc_probe
- as the mbox properties are optional, update zynqmp_r5_rproc_kick
  so that the mailbox functionality only occurs if these
  properties are provided
---
 drivers/remoteproc/Kconfig|   8 +
 drivers/remoteproc/Makefile   |   1 +
 drivers/remoteproc/zynqmp_r5_remoteproc.c | 778 ++
 3 files changed, 787 insertions(+)
 create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c6659dfea7c7..c2fe54b1d94f 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -275,6 +275,14 @@ config TI_K3_DSP_REMOTEPROC
  It's safe to say N here if you're not interested in utilizing
  the DSP slave processors.
 
+config ZYNQMP_R5_REMOTEPROC
+   tristate "ZynqMP R5 remoteproc support"
+   depends on PM && ARCH_ZYNQMP
+   select RPMSG_VIRTIO
+   select ZYNQMP_IPI_MBOX
+   help
+ Say y or m here to support ZynqMP R5 remote processors via the remote
+ processor framework.
 endif # REMOTEPROC
 
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 3dfa28e6c701..ef1abff654c2 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_ST_REMOTEPROC)   += st_remoteproc.o
 obj-$(CONFIG_ST_SLIM_REMOTEPROC)   += st_slim_rproc.o
 obj-$(CONFIG_STM32_RPROC)  += stm32_rproc.o
 obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
+obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o
diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c 
b/drivers/remoteproc/zynqmp_r5_remoteproc.c
new file mode 100644
index ..48bffa91bc00
--- /dev/null
+++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c
@@ -0,0 +1,779 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Zynq R5 Remote Processor driver
+ *
+ * Based on origin OMAP and Zynq Remote Processor driver
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+#define MAX_RPROCS 2 /* Support up to 2 RPU */
+#define MAX_MEM_PNODES 4 /* Max power nodes for one RPU memory instance */
+
+#define BANK_LIST_PROP "meta-memory-regions"
+#define DDR_LIST_PROP  "memory-regions"
+
+/* IPI buffer MAX length */
+#define IPI_BUF_LEN_MAX32U
+/* RX mailbox client buffer max length */
+#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
+sizeof(struct zynqmp_ipi_message))
+
+/**
+ * struct zynqmp_r5_mem - zynqmp rpu memory data
+ * @pnode_id: TCM power domain ids
+ * @res: memory resource
+ * @node: list node
+ */
+struct zynqmp_r5_mem {
+   u32 pnode_id[MAX_MEM_PNODES];
+   struct resource res;
+   struct list_head node;
+};
+
+/**
+ * struct zynqmp_r5_rproc - zynqmp rpu remote processor state
+ * this is for each individual R5 core's state
+ *
+ * @rx_mc_buf: rx mailbox client buffer to save the rx message
+ * @tx_mc: tx mailbox client
+ * @rx_mc: rx mailbox client * @dev: device of RPU instance
+ * @mbox_work: mbox_work for the RPU remoteproc
+ * @tx_mc_skbs: socket buffers for tx mailbox client
+ * @dev: device of RPU instance
+ * @rproc: rproc handle
+ * @tx_chan: tx mailbox channel
+ * @rx_chan: rx mailbox channel
+ * @pnode_id: RPU CPU power domain id
+ * @elem: linked list item
+ * @dt_node: device tree node that holds information for 1 R5 core.
+ */
+struct zynqmp_r5_rproc {
+   unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
+   struct mbox_client tx_mc;
+   struct mbox_client rx_mc;
+   struct work_struct mbox_work;
+   struct sk_buff_head tx_mc_skbs;
+   struct device *dev;
+   struct rproc *rproc;
+   struct mbox_chan *tx_chan;
+   struct mbox_chan *rx_chan;
+   u32 pnode_id;
+   struct list_head elem;
+};
+
+/*
+ * r5_set_mode - set RPU operation mode
+ * @z_rproc: Remote processor private data
+ * @rpu_mode: mode specified by device tree to configure the RPU to
+ *
+ * set RPU operation mode
+ *
+ *