Re: [PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
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 = &rproc_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, th
RE: [PATCH v22 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
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 = &rproc_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
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.
> 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;
> + str
