[v9,0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP

Message

Ben Levinsky Aug. 27, 2020, 1:58 a.m. UTC

The driver was tested on Xilinx ZynqMP

For sake of ease of review, only support ZynqMP. Once accepted, then
add support for Versal platform and R5 loading onto OCM.

v2:
- remove domain struct as per review from Mathieu
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around
  function pointer in xilinx eemi ops struct
- update zynqmp_r5 yaml parsing to not raise warnings for extra
  information in children of R5 node. The warning "node has a unit
  name, but no reg or ranges property" will still be raised though 
  as this particular node is needed to describe the
  '#address-cells' and '#size-cells' information.
v4:
- add default values for enums
- fix formatting as per checkpatch.pl --strict. Note that 1 warning and
  1 check are still raised as each is due to fixing the warning
  results in that particular line going over 80 characters.
- remove warning '/example-0/rpu@ff9a0000/r5@0: 
  node has a unit name, but no reg or ranges property'
  by adding reg to r5 node.
v5:
- update device tree sample and yaml parsing to not raise any warnings
- description for memory-region in yaml parsing
- compatible string in yaml parsing for TCM
- parse_fw change from use of rproc_of_resm_mem_entry_init to
  rproc_mem_entry_init and use of alloc/release
- var's of type zynqmp_r5_pdata all have same local variable name
- use dev_dbg instead of dev_info
v6:
- adding memory carveouts is handled much more similarly.
  All mem carveouts are now described in reserved memory as needed.
  That is, TCM nodes are not coupled to remoteproc anymore.
  This is reflected in the remoteproc R5 driver and the device tree
  binding.
- remove mailbox from device tree binding as it is not necessary for elf
  loading 
v7:
- remove unused headers
- zynqmp_r5_remoteproc_probe:lockstep_mode from u32* to u32
- device-tree binding "lockstep-mode"  to "xlnx,cluster-mode"
- remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at
  probe()
- remove is_r5_mode_set from  zynqmp rpu remote processor private data
- do not error out if no mailbox is provided since mailboxes are optional
- remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as pdata
  is handled in zynqmp_r5_remoteproc_remove
v8:
- remove old acks, reviewed-by's in commit message
v9:
- if zynqmp_r5_remoteproc.c pdata->tx_mc_skbs not initialized, then do not
  call skb_queue_empty
- update arguments and documentation for zynqmp_pm_set_rpu_mode
- in fn zynqmp_pm_force_powerdown, change arg 'target' to 'node'
- zynqmp_pm_request_wakeup update code style
- edit 3/5 patch commit message
- document zynqmp_pm_set_tcm_config and zynqmp_pm_get_rpu_mode
  documentation to include expected return val
- remove unused fn zynqmp_pm_get_node_status
- update 5/5 patch commit message to document supported configurations
  and how they are booted by the driver.
- remove copyrights other than SPDX from zynqmp_r5_remoteproc.c
- compilation warnings no longer raised
- remove unused includes from zynqmp_r5_remoteproc.c
- remove unused  var autoboot from zynqmp_r5_remoteproc.c
- reorder zynqmp_r5_pdata fpr small mem savings due to alignment
- zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode uses second arg
- zynqmp_r5_remoteproc.c use of zynqmp_pm_set_tcm_config now does not
  have output arg
- in tcm handling, unconditionally use &= 0x000fffff mask since all nodes
  in this fn are for tcm
- update comments for translating dma field in tcm handling to device
  address
- update calls to rproc_mem_entry_init in parse_mem_regions so that there
  are only 2 cases for types of carveouts instead of 3
- in parse_mem_regions, check if device tree node is null before using it
- add example device tree nodes used in parse_mem_regions and tcm parsing
- add comment for vring id node length
- add check for string length so that vring id is at least min length
- move tcm nodes from reserved mem to instead own device tree nodes
   and only use them if enabled in device tree
- add comment for explaining handling of rproc_elf_load_rsc_table
- remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick
- remove unused field mems in struct zynqmp_r5_pdata
- remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling
  is done by zyqmp_r5_pm_request_tcm
- remove obsolete setting of dma_ops and parent device dma_mask
- remove obsolete use of of_dma_configure
- add comment for call to r5_set_mode fn
- make mbox usage optional and gracefully inform user via dev_dbg if not
  present
- change lockstep_mode from u32* to u32
- update zynqmp_pm_set_rpu_mode and zynqmp_pm_set_rpu_mode documentation
  and remove unused args
v10:
- add include types.h to xlnx-zynqmp.h for compilation

Ben Levinsky (5):
  firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU
    configuration.
  firmware: xilinx: Add shutdown/wakeup APIs
  firmware: xilinx: Add RPU configuration APIs
  dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc
    bindings
  remoteproc: Add initial zynqmp R5 remoteproc driver

 .../xilinx,zynqmp-r5-remoteproc.yaml          | 113 +++
 drivers/firmware/xilinx/zynqmp.c              |  86 ++
 drivers/remoteproc/Kconfig                    |  10 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/zynqmp_r5_remoteproc.c     | 898 ++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h          |  63 ++
 6 files changed, 1171 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
 create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c

Comments

Michael Auchter Aug. 27, 2020, 1:47 p.m. UTC | #1

Hey Ben,

On Wed, Aug 26, 2020 at 06:58:05PM -0700, Ben Levinsky wrote:
> v10:
> - add include types.h to xlnx-zynqmp.h for compilation

I appreciate the quick turnaround on v10, but it looks like much of my
feedback on v9 went unacknowledged. 

Most concerning is the fact that loading firmware on to R5 1 is _still_
broken in v10 due to the incorrect TCM banks being used.

Thanks,
 Michael

> 
> Ben Levinsky (5):
>   firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU
>     configuration.
>   firmware: xilinx: Add shutdown/wakeup APIs
>   firmware: xilinx: Add RPU configuration APIs
>   dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc
>     bindings
>   remoteproc: Add initial zynqmp R5 remoteproc driver
> 
>  .../xilinx,zynqmp-r5-remoteproc.yaml          | 113 +++
>  drivers/firmware/xilinx/zynqmp.c              |  86 ++
>  drivers/remoteproc/Kconfig                    |  10 +
>  drivers/remoteproc/Makefile                   |   1 +
>  drivers/remoteproc/zynqmp_r5_remoteproc.c     | 898 ++++++++++++++++++
>  include/linux/firmware/xlnx-zynqmp.h          |  63 ++
>  6 files changed, 1171 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
>  create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
> 
> -- 
> 2.17.1
>

Ben Levinsky Aug. 27, 2020, 3:34 p.m. UTC | #2

Hi Michael,

Thanks for comment. Maybe I missed some of the comments then? I had thought that your comments were the following and that I had answered them in the code:
V8 3/5:
- zynqmp_pm_set_rpu_mode: pass arg1 instead of 0 to zynqmp_pm_invoke_fn
This should be reflected in v9 3/5
- update kernel docs for zynqmp_pm_set_rpu_mode
may have misunderstood the comment here. I updated the function and its comments above the function so that there is no obsolete iocl_id or arg2 mentioned
V8 5/5:
-  " In the event that zynqmp_r5_probe() fails before zynqmp_r5_setup_mbox()
has run, this will be called on an uninitialized skb_queue. (Also
obviously an issue once mailboxes are made optional again)."
To remedy this I added logic in v9 in the zynqmp_r5_release() function so that the driver checks if a pointer field in the struct is NULL or no before discarding skb's

Were there other comments?

With that being said, I will make sure the R51 case is more completely covered.

Thanks
Ben


> -----Original Message-----
> From: Michael Auchter <[e-mail]>
> Sent: Thursday, August 27, 2020 6:48 AM
> To: Ben Levinsky <[e-mail]>
> Cc: Stefano Stabellini <[e-mail]>; Michal Simek
> <[e-mail]>; [e-mail];
> [e-mail]; Ed T. Mooring <[e-mail]>; linux-
> [e-mail]; [e-mail]; Jiaying Liang
> <[e-mail]>; [e-mail]; linux-arm-
> [e-mail]
> Subject: Re: [PATCH v9 0/5] Provide basic driver to control Arm R5 co-
> processor found on Xilinx ZynqMP
> 
> Hey Ben,
> 
> On Wed, Aug 26, 2020 at 06:58:05PM -0700, Ben Levinsky wrote:
> > v10:
> > - add include types.h to xlnx-zynqmp.h for compilation
> 
> I appreciate the quick turnaround on v10, but it looks like much of my
> feedback on v9 went unacknowledged.
> 
> Most concerning is the fact that loading firmware on to R5 1 is _still_
> broken in v10 due to the incorrect TCM banks being used.
> 
> Thanks,
>  Michael
> 
> >
> > Ben Levinsky (5):
> >   firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU
> >     configuration.
> >   firmware: xilinx: Add shutdown/wakeup APIs
> >   firmware: xilinx: Add RPU configuration APIs
> >   dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc
> >     bindings
> >   remoteproc: Add initial zynqmp R5 remoteproc driver
> >
> >  .../xilinx,zynqmp-r5-remoteproc.yaml          | 113 +++
> >  drivers/firmware/xilinx/zynqmp.c              |  86 ++
> >  drivers/remoteproc/Kconfig                    |  10 +
> >  drivers/remoteproc/Makefile                   |   1 +
> >  drivers/remoteproc/zynqmp_r5_remoteproc.c     | 898
> ++++++++++++++++++
> >  include/linux/firmware/xlnx-zynqmp.h          |  63 ++
> >  6 files changed, 1171 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> remoteproc.yaml
> >  create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
> >
> > --
> > 2.17.1
> >

Michael Auchter Aug. 27, 2020, 6:43 p.m. UTC | #3

Hey Ben,

On Thu, Aug 27, 2020 at 03:34:33PM +0000, Ben Levinsky wrote:
> Hi Michael,
> 
> Thanks for comment. Maybe I missed some of the comments then? I had thought that your comments were the following and that I had answered them in the code:
> V8 3/5:
> - zynqmp_pm_set_rpu_mode: pass arg1 instead of 0 to zynqmp_pm_invoke_fn
> This should be reflected in v9 3/5
> - update kernel docs for zynqmp_pm_set_rpu_mode
> may have misunderstood the comment here. I updated the function and its comments above the function so that there is no obsolete iocl_id or arg2 mentioned
> V8 5/5:
> -  " In the event that zynqmp_r5_probe() fails before zynqmp_r5_setup_mbox()
> has run, this will be called on an uninitialized skb_queue. (Also
> obviously an issue once mailboxes are made optional again)."
> To remedy this I added logic in v9 in the zynqmp_r5_release() function so that the driver checks if a pointer field in the struct is NULL or no before discarding skb's
> 
> Were there other comments?

Yeah, there were a few others on v9, you can see the email here:

https://lore.kernel.org/linux-remoteproc/[e-mail]/T/#m1b326e5f059712dd33bee1bcd47e3c0ae245055e

It looks like the first comment regarding the compilation failure due to
the lack of linux/types.h was addressed in v10, but none of the
subsequent comments; perhaps you just overlooked them?

Thanks,
 Michael

> 
> With that being said, I will make sure the R51 case is more completely covered.
> 
> Thanks
> Ben
> 
> 
> > -----Original Message-----
> > From: Michael Auchter <[e-mail]>
> > Sent: Thursday, August 27, 2020 6:48 AM
> > To: Ben Levinsky <[e-mail]>
> > Cc: Stefano Stabellini <[e-mail]>; Michal Simek
> > <[e-mail]>; [e-mail];
> > [e-mail]; Ed T. Mooring <[e-mail]>; linux-
> > [e-mail]; [e-mail]; Jiaying Liang
> > <[e-mail]>; [e-mail]; linux-arm-
> > [e-mail]
> > Subject: Re: [PATCH v9 0/5] Provide basic driver to control Arm R5 co-
> > processor found on Xilinx ZynqMP
> > 
> > Hey Ben,
> > 
> > On Wed, Aug 26, 2020 at 06:58:05PM -0700, Ben Levinsky wrote:
> > > v10:
> > > - add include types.h to xlnx-zynqmp.h for compilation
> > 
> > I appreciate the quick turnaround on v10, but it looks like much of my
> > feedback on v9 went unacknowledged.
> > 
> > Most concerning is the fact that loading firmware on to R5 1 is _still_
> > broken in v10 due to the incorrect TCM banks being used.
> > 
> > Thanks,
> >  Michael
> > 
> > >
> > > Ben Levinsky (5):
> > >   firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU
> > >     configuration.
> > >   firmware: xilinx: Add shutdown/wakeup APIs
> > >   firmware: xilinx: Add RPU configuration APIs
> > >   dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc
> > >     bindings
> > >   remoteproc: Add initial zynqmp R5 remoteproc driver
> > >
> > >  .../xilinx,zynqmp-r5-remoteproc.yaml          | 113 +++
> > >  drivers/firmware/xilinx/zynqmp.c              |  86 ++
> > >  drivers/remoteproc/Kconfig                    |  10 +
> > >  drivers/remoteproc/Makefile                   |   1 +
> > >  drivers/remoteproc/zynqmp_r5_remoteproc.c     | 898
> > ++++++++++++++++++
> > >  include/linux/firmware/xlnx-zynqmp.h          |  63 ++
> > >  6 files changed, 1171 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> > remoteproc.yaml
> > >  create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
> > >
> > > --
> > > 2.17.1
> > >