Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP
v14 cover Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP
The driver was tested on Xilinx ZynqMP QEMU
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
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
v11:
- update usage of zynqmp_pm_get_rpu_mode to return rpu mode in enum
- update zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode arguments to remove unused args
- use enums instead of u32 where possible in zynqmp_r5_remoteproc
- zynqmp_r5_remoteproc: update prints to not use carriage return, just newline
- zynqmp_r5_remoteproc: look up tcm banks via property instead of string name
- print device tree nodes with %pOF instead of %s with node name field
- update tcm release to unmap VA
- handle r5-1 use case
- device tree binding r5 node to have link to TCMs via meta-memory-regions property
- fix device tree binding so no warnings from 'make dt_binding_check' v12:
- update signed off by so that latest developer name is last
- in drivers/firmware/zynqmp.c, update zynqmp_pm_set_rpu_mode so rpu_mode is only set if no error
- update args for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config fn arg's to reflect what is expected in the function and the usage in zynqmp_r5_remoteproc accordingly
v13:
- zynqmp_pm_get_rpu_mode argument zynqmp_pm_get_rpu_mode is
only set if no error
v14:
- rebase off v5.9-rc3 kernel
- concerns were raised about the new property meta-memory-regions. There is no clear direction so for the moment I kept it in the series
- in device tree binding, place IPC nodes in RAM in the reserved memory section
- change zynqmp_r5_remoteproc::rpus and rpu_mode to static
- fix typo
- zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
property specified in device tree
- use u32 instead of u32* to store in remoteproc memory entry private data for pnode_id information
- always call r5_set_mode on probe
- remove alloc of zynqmp_r5_pdata in zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static allocation already
- error at probe time if lockstep-mode property not present in device tree
- update commit message as per review
- remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is present
- remove unused macros
- update comment ordering of zynqmp_r5_pdata to match struct definition
- zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
- remove obsolete TODOs
- only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
- remove uneven dev_dbg/dev_err fn calls
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 | 119 +++ drivers/firmware/xilinx/zynqmp.c | 96 +++ drivers/remoteproc/Kconfig | 8 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/zynqmp_r5_remoteproc.c | 775 ++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 60 ++ 6 files changed, 1059 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
Signature · 7 lines
2.17.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v14 1/5 firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration.
Add ZynqMP firmware ioctl enums for RPU configuration.
---
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around function pointer in xilinx eemi ops struct
v4:
- add default values for enums
--- include/linux/firmware/xlnx-zynqmp.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
include/linux/firmware/xlnx-zynqmp.h +19 -0
@@ -104,6 +104,10 @@ enum pm_ret_status { };enum pm_ioctl_id { +IOCTL_GET_RPU_OPER_MODE =0 , +IOCTL_SET_RPU_OPER_MODE =1 , +IOCTL_RPU_BOOT_ADDR_CONFIG =2 , +IOCTL_TCM_COMB_CONFIG =3 ,IOCTL_SD_DLL_RESET =6 ,IOCTL_SET_SD_TAPDELAY ,IOCTL_SET_PLL_FRAC_MODE ,
@@ -129,6 +133,21 @@ enum pm_query_id { PM_QID_CLOCK_GET_MAX_DIVISOR, }; +enum rpu_oper_mode { +PM_RPU_MODE_LOCKSTEP =0 , +PM_RPU_MODE_SPLIT =1 , +}; + +enum rpu_boot_mem { +PM_RPU_BOOTMEM_LOVEC =0 , +PM_RPU_BOOTMEM_HIVEC =1 , +}; + +enum rpu_tcm_comb { +PM_RPU_TCM_SPLIT =0 , +PM_RPU_TCM_COMB =1 , +}; +enum zynqmp_pm_reset_action {PM_RESET_ACTION_RELEASE ,PM_RESET_ACTION_ASSERT ,
Signature · 7 lines
2.17.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v14 2/5 firmware: xilinx: Add shutdown/wakeup APIs
In reply to Ben LevinskyAdd shutdown/wakeup a resource eemi operations to shutdown or bringup a resource.
Note alignment of args matches convention of other fn's in this file. The reason being that the long fn name results in aligned args that otherwise go over 80 chars so shift right to avoid this
---
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around function pointer in xilinx eemi ops struct
- fix formatting
v4:
- add default values for enumv3:
- add xilinx-related platform mgmt fn's instead of wrapping around function pointer in xilinx eemi ops struct
- fix formatting
v4:
- add default values for enum
v9:
- zynqmp_pm_force_powerdown update arg 'target' to 'node'
- zynqmp_pm_request_wakeup fix code style
v10:
- add types.h to includes for compilation
--- drivers/firmware/xilinx/zynqmp.c | 35 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 23 ++++++++++++++++++ 2 files changed, 58 insertions(+)
drivers/firmware/xilinx/zynqmp.c +35 -0
@@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node) }EXPORT_SYMBOL_GPL (zynqmp_pm_release_node); +/** +* zynqmp_pm_force_pwrdwn - PM call to request for another PU or subsystem to +* be powered down forcefully +* @node: Node ID of the targeted PU or subsystem +* @ack: Flag to specify whether acknowledge is requested +* +* Return: status, either success or error+reason +*/ +int zynqmp_pm_force_pwrdwn (const u32 node, +const enum zynqmp_pm_request_ack ack) +{ +return zynqmp_pm_invoke_fn (PM_FORCE_POWERDOWN , node, ack,0 ,0 ,NULL ); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_force_pwrdwn); + +/** +* zynqmp_pm_request_wake - PM call to wake up selected master or subsystem +* @node: Node ID of the master or subsystem +* @set_addr: Specifies whether the address argument is relevant +* @address: Address from which to resume when woken up +* @ack: Flag to specify whether acknowledge requested +* +* Return: status, either success or error+reason +*/ +int zynqmp_pm_request_wake (const u32 node, +const bool set_addr, +const u64 address, +const enum zynqmp_pm_request_ack ack) +{ +/* set_addr flag is encoded into 1st bit of address */ +return zynqmp_pm_invoke_fn (PM_REQUEST_WAKEUP , node, address | set_addr, + address >>32 , ack,NULL ); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_request_wake); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves * @node: Node ID of the slave
include/linux/firmware/xlnx-zynqmp.h +23 -0
@@ -12,6 +12,7 @@#ifndef __FIRMWARE_ZYNQMP_H__#define __FIRMWARE_ZYNQMP_H__ +#include <linux/types.h> #define ZYNQMP_PM_VERSION_MAJOR 1#define ZYNQMP_PM_VERSION_MINOR 0
@@ -64,6 +65,8 @@enum pm_api_id {PM_GET_API_VERSION =1 , +PM_FORCE_POWERDOWN =8 , +PM_REQUEST_WAKEUP =10 ,PM_SYSTEM_SHUTDOWN =12 ,PM_REQUEST_NODE =13 ,PM_RELEASE_NODE ,
@@ -376,6 +379,12 @@ int zynqmp_pm_write_pggs(u32 index, u32 value);int zynqmp_pm_read_pggs (u32 index,u32 *value);int zynqmp_pm_system_shutdown (const u32 type,const u32 subtype);int zynqmp_pm_set_boot_health_status (u32 value); +int zynqmp_pm_force_pwrdwn (const u32 target, +const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_request_wake (const u32 node, +const bool set_addr, +const u64 address, +const enum zynqmp_pm_request_ack ack);#else static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops (void ) {
@@ -526,6 +535,20 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value) {return -ENODEV ; } + +static inline int zynqmp_pm_force_pwrdwn (const u32 target, +const enum zynqmp_pm_request_ack ack) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_request_wake (const u32 node, +const bool set_addr, +const u64 address, +const enum zynqmp_pm_request_ack ack) +{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */
Signature · 7 lines
2.17.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v14 3/5 firmware: xilinx: Add RPU configuration APIs
In reply to Ben LevinskyThis patch adds APIs to access to configure RPU and its processor-specific memory.
That is query the run-time mode of RPU as either split or lockstep as well as API to set this mode. In addition add APIs to access configuration of the RPUs' tightly coupled memory (TCM).
---
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around
function pointer in xilinx eemi ops struct
v4:
- add default values for enums
v9:
- update commit message
- for zynqmp_pm_set_tcm_config and zynqmp_pm_get_rpu_mode update docs for expected output, arguments as well removing unused args
- remove unused fn zynqmp_pm_get_node_status
v11:
- update usage of zynqmp_pm_get_rpu_mode to return rpu mode in enum
- update zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode arguments to remove unused args v12:
- in drivers/firmware/zynqmp.c, update zynqmp_pm_set_rpu_mode so rpu_mode
is only set if no error
- update args for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config fn arg's to reflect what is expected in the function and the usage in zynqmp_r5_remoteproc accordingly
- zynqmp_pm_get_rpu_mode argument zynqmp_pm_get_rpu_mode is
only set if no error
--- drivers/firmware/xilinx/zynqmp.c | 61 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 18 ++++++++ 2 files changed, 79 insertions(+)
drivers/firmware/xilinx/zynqmp.c +61 -0
@@ -846,6 +846,67 @@ int zynqmp_pm_release_node(const u32 node) }EXPORT_SYMBOL_GPL (zynqmp_pm_release_node); +/** +* zynqmp_pm_get_rpu_mode() - Get RPU mode +* @node_id: Node ID of the device +* @rpu_mode: return by reference value +* either split or lockstep +* +* Return: return 0 on success or error+reason. +* if success, then rpu_mode will be set +* to current rpu mode. +*/ +int zynqmp_pm_get_rpu_mode (u32 node_id,enum rpu_oper_mode *rpu_mode) +{ +u32 ret_payload[PAYLOAD_ARG_CNT ]; +int ret; + + ret =zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_GET_RPU_OPER_MODE ,0 ,0 , ret_payload); + +/* only set rpu_mode if no error */ +if (ret ==XST_PM_SUCCESS ) + *rpu_mode = ret_payload[0 ]; + +return ret; +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_rpu_mode); + +/** +* zynqmp_pm_set_rpu_mode() - Set RPU mode +* @node_id: Node ID of the device +* @rpu_mode: Argument 1 to requested IOCTL call. either split or lockstep +* +* This function is used to set RPU mode to split or +* lockstep +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_rpu_mode (u32 node_id,enum rpu_oper_mode rpu_mode) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_SET_RPU_OPER_MODE , (u32 )rpu_mode, +0 ,NULL ); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_rpu_mode); + +/** +* zynqmp_pm_set_tcm_config - configure TCM +* @tcm_mode: Argument 1 to requested IOCTL call +* either PM_RPU_TCM_COMB or PM_RPU_TCM_SPLIT +* +* This function is used to set RPU mode to split or combined +* +* Return: status: 0 for success, else failure +*/ +int zynqmp_pm_set_tcm_config (u32 node_id,enum rpu_tcm_comb tcm_mode) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_TCM_COMB_CONFIG , (u32 )tcm_mode,0 , +NULL ); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_tcm_config); +/** * zynqmp_pm_force_pwrdwn - PM call to request for another PU or subsystem to * be powered down forcefully
include/linux/firmware/xlnx-zynqmp.h +18 -0
@@ -385,6 +385,9 @@ int zynqmp_pm_request_wake(const u32 node,const bool set_addr, const u64 address, const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_get_rpu_mode (u32 node_id,enum rpu_oper_mode *rpu_mode); +int zynqmp_pm_set_rpu_mode (u32 node_id,u32 arg1); +int zynqmp_pm_set_tcm_config (u32 node_id,u32 arg1);#else static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops (void ) {
@@ -549,6 +552,21 @@ static inline int zynqmp_pm_request_wake(const u32 node, {return -ENODEV ; } + +static inline int zynqmp_pm_get_rpu_mode (u32 node_id,enum rpu_oper_mode *rpu_mode) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_rpu_mode (u32 node_id,u32 arg1) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_tcm_config (u32 node_id,u32 arg1) +{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */
Signature · 7 lines
2.17.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v14 4/5 dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
In reply to Ben LevinskyAdd binding for ZynqMP R5 OpenAMP.
Represent the RPU domain resources in one device node. Each RPU processor is a subnode of the top RPU domain node.
---
v3:
- 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::
- 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
v6:
- remove coupling TCM nodes with remoteproc
- remove mailbox as it is optional not needed
v7:
- change lockstep-mode to xlnx,cluster-mode
v9:
- show example IPC nodes and tcm bank nodes
v11:
- add property meta-memory-regions to illustrate link between r5 and TCM banks
- update so no warnings from 'make dt_binding_check'
v14:
- concerns were raised about the new property meta-memory-regions. There is no clear direction so for the moment I kept it in the series
- place IPC nodes in RAM in the reserved memory section
--- .../xilinx,zynqmp-r5-remoteproc.yaml | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml new file +119 -0
@@ -0,0 +1,119 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Xilinx R5 remote processor controller bindings + +description: + This document defines the binding for the remoteproc component that loads and + boots firmwares on the Xilinx Zynqmp and Versal family chipset. + + Note that the Linux has global addressing view of the R5-related memory (TCM) + so the absolute address ranges are provided in TCM reg's. +maintainers: + - Ed Mooring <[email protected]> + - Ben Levinsky <[email protected]> + +properties: + compatible: + const: "xlnx,zynqmp-r5-remoteproc-1.0" + + lockstep-mode: + description: + R5 core configuration (split is 0 or lock-step and 1) + maxItems: 1 + + interrupts: + description: + Interrupt mapping for remoteproc IPI. It is required if the + user uses the remoteproc driver with the RPMsg kernel driver. + maxItems: 6 + + memory-region: + description: + collection of memory carveouts used for elf-loading and inter-processor + communication. + maxItems: 4 + minItems: 4 + meta-memory-regions: + description: + collection of memories that are not present in the top level memory + nodes' mapping. For example, R5s' TCM banks. These banks are needed + for R5 firmware meta data such as the R5 firmware's heap and stack + pnode-id: + maxItems: 1 + mboxes: + maxItems: 2 + mbox-names: + maxItems: 2 + +examples: + - | + reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + ranges; + elf_load: rproc@3ed000000 { + no-map; + reg = <0x3ed00000 0x40000>; + }; + + rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 { + no-map; + reg = <0x3ed40000 0x4000>; + }; + rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 { + no-map; + reg = <0x3ed44000 0x4000>; + }; + rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 { + no-map; + reg = <0x3ed48000 0x100000>; + }; + + }; + + /* + * Below nodes are required if using TCM to load R5 firmware + * if not, then either do not provide nodes are label as disabled in + * status property + */ + tcm0a: tcm_0a@ffe00000 { + reg = <0xffe00000 0x10000>; + pnode-id = <0xf>; + no-map; + status = "okay"; + phandle = <0x40>; + compatible = "xlnx,tcm"; + }; + tcm0b: tcm_1a@ffe20000 { + reg = <0xffe20000 0x10000>; + pnode-id = <0x10>; + no-map; + status = "okay"; + compatible = "xlnx,tcm"; + phandle = <0x41>; + }; + + rpu { + compatible = "xlnx,zynqmp-r5-remoteproc-1.0"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + lockstep-mode = <1>; + r5_0 { + ranges; + #address-cells = <1>; + #size-cells = <1>; + memory-region = <&elf_load>, + <&rpu0vdev0vring0>, + <&rpu0vdev0vring1>, + <&rpu0vdev0buffer>; + meta-memory-regions = <&tcm_0a>, <&tcm_0b>; + pnode-id = <0x7>; + }; + }; + +...
Signature · 7 lines
2.17.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v14 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
Hi Ben,
One query below -
Quoted 11 lines · Ben Levinsky
Add binding for ZynqMP R5 OpenAMP.
Represent the RPU domain resources in one device node. Each RPU processor is a subnode of the top RPU domain node.
Signed-off-by Jason Wu <[email protected]>Signed-off-by Wendy Liang <[email protected]>Signed-off-by Michal Simek <[email protected]>Signed-off-by Ben Levinsky <[email protected]>---
[...]
Quoted 26 lines
.../xilinx,zynqmp-r5-remoteproc.yaml | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yamlDocumentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml new file +16
-0@@ -0,0 +1,119 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Xilinx R5 remote processor controller bindings + +description: + This document defines the binding for the remoteproc component that loads and + boots firmwares on the Xilinx Zynqmp and Versal family chipset. + + Note that the Linux has global addressing view of the R5-related memory (TCM) + so the absolute address ranges are provided in TCM reg's. +maintainers: + - Ed Mooring <[email protected]>
Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml line 26
Quoted 10 lines
Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml new file +10
-0+ - Ben Levinsky <[email protected]> + +properties: + compatible: + const: "xlnx,zynqmp-r5-remoteproc-1.0" + + lockstep-mode: + description: + R5 core configuration (split is 0 or lock-step and 1) + maxItems: 1
Looking at the driver, it seems that it is possible to set the R5s to operate in split or lock-step mode dynamically.
If so, the device tree shouldn't really contain this property. Wouldn't it be better to give the users flexibility to choose the mode at run time?
Thanks,
Punit
Quoted 93 lines
Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml new file +93
-0+ + interrupts: + description: + Interrupt mapping for remoteproc IPI. It is required if the + user uses the remoteproc driver with the RPMsg kernel driver. + maxItems: 6 + + memory-region: + description: + collection of memory carveouts used for elf-loading and inter-processor + communication. + maxItems: 4 + minItems: 4 + meta-memory-regions: + description: + collection of memories that are not present in the top level memory + nodes' mapping. For example, R5s' TCM banks. These banks are needed + for R5 firmware meta data such as the R5 firmware's heap and stack + pnode-id: + maxItems: 1 + mboxes: + maxItems: 2 + mbox-names: + maxItems: 2 + +examples: + - | + reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + ranges; + elf_load: rproc@3ed000000 { + no-map; + reg = <0x3ed00000 0x40000>; + }; + + rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 { + no-map; + reg = <0x3ed40000 0x4000>; + }; + rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 { + no-map; + reg = <0x3ed44000 0x4000>; + }; + rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 { + no-map; + reg = <0x3ed48000 0x100000>; + }; + + }; + + /* + * Below nodes are required if using TCM to load R5 firmware + * if not, then either do not provide nodes are label as disabled in + * status property + */ + tcm0a: tcm_0a@ffe00000 { + reg = <0xffe00000 0x10000>; + pnode-id = <0xf>; + no-map; + status = "okay"; + phandle = <0x40>; + compatible = "xlnx,tcm"; + }; + tcm0b: tcm_1a@ffe20000 { + reg = <0xffe20000 0x10000>; + pnode-id = <0x10>; + no-map; + status = "okay"; + compatible = "xlnx,tcm"; + phandle = <0x41>; + }; + + rpu { + compatible = "xlnx,zynqmp-r5-remoteproc-1.0"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + lockstep-mode = <1>; + r5_0 { + ranges; + #address-cells = <1>; + #size-cells = <1>; + memory-region = <&elf_load>, + <&rpu0vdev0vring0>, + <&rpu0vdev0vring1>, + <&rpu0vdev0buffer>; + meta-memory-regions = <&tcm_0a>, <&tcm_0b>; + pnode-id = <0x7>; + }; + }; + +...
PATCH v14 5/5 remoteproc: Add initial zynqmp R5 remoteproc driver
In reply to Ben LevinskyR5 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.
---
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
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.
v5:
- 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
- use lockstep-mode property for configuring RPU
v7:
- remove unused headers
- change u32 *lockstep_mode -> u32 lockstep_mode;
- change device-tree binding "lockstep-mode" to xlnx,cluster-mode
- remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at remoteproc-probe time
- remove is_r5_mode_set from zynqmp rpu remote processor private data
- do not error out if no mailbox is provided
- 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:
- as mboxes are now optional, if pdata->tx_mc_skbs not initialized then do not call skb_queue_empty
- update usage for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config and zynqmp_pm_get_rpu_mode
- 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
- 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 var lockstep_mode from u32* to u32
v11:
- use enums instead of u32 where possible in zynqmp_r5_remoteproc
- update usage of zynqmp_pm_set/get_rpu_mode and zynqmp_pm_set_tcm_config
- update prints to not use carriage return, just newline
- look up tcm banks via property in r5 node instead of string name
- print device tree nodes with %pOF instead of %s with node name field
- update tcm release to unmap VA
- handle r5-1 use case
v12:
- update signed off by so that latest developer name is last
- do not cast enums to u32s for zynqmp_pm* functions
v14:
- change zynqmp_r5_remoteproc::rpus and rpu_mode to static
- fix typo
- zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
property specified in device tree
- use u32 instead of u32* to store in remoteproc memory entry private data for pnode_id information
- always call r5_set_mode on probe
- remove alloc of zynqmp_r5_pdata in zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static allocation already
- error at probe time if lockstep-mode property not present in device tree
- update commit message as per review
- remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is present
- remove unused macros
- update comment ordering of zynqmp_r5_pdata to match struct definition
- zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
- remove obsolete TODOs
- only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
- remove uneven dev_dbg/dev_err fn calls
--- drivers/remoteproc/Kconfig | 8 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/zynqmp_r5_remoteproc.c | 775 ++++++++++++++++++++++ 3 files changed, 784 insertions(+) create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
drivers/remoteproc/Kconfig +8 -0
@@ -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
drivers/remoteproc/Makefile +1 -0
@@ -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
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +775 -0
775 lines not on this page · raw message
Signature · 7 lines
2.17.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hey Ben,
Split mode is still not functional in this patch series (as was the case with the last few revisions).
Before sending out the next revision, can you _please_ ensure you're testing all supported configurations?
Quoted 5 lines · Ben Levinsky
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/** +* RPU core configuration +*/ +static enum rpu_oper_mode rpu_mode; +
<.. snip ..>
Quoted 14 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +14
-0+static int zynqmp_r5_remoteproc_probe (struct platform_device *pdev) +{ +int ret, i =0 ; +u32 lockstep_mode; +struct device *dev = &pdev->dev; +struct device_node *nc; + + ret =of_property_read_u32 (dev->of_node, +"lockstep-mode" , + &lockstep_mode); +if (ret <0 ) { +return ret; + }else if (lockstep_mode !=PM_RPU_MODE_LOCKSTEP && + lockstep_mode !=PM_RPU_MODE_SPLIT ) {
drivers/remoteproc/zynqmp_r5_remoteproc.c line 705
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+dev_err (dev, +"Invalid lockstep-mode %x in %pOF\n" , + lockstep_mode, dev->of_node); +return -EINVAL ; + } + + rpu_mode = lockstep_mode; + +dev_dbg (dev,"RPU configuration: %s\n" , + lockstep_mode ?"lockstep" :"split" );
The binding documents lockstep-mode as:
Review
Quoted 4 lines
(quoted diff) +4
-0+ lockstep-mode: + description: + R5 core configuration (split is 0 or lock-step and 1) + maxItems: 1
(Which needs to be reworded, but it looks like the intent was "split is 0 and lock-step is 1")
However, rpu_oper_mode is defined as:
Review
Quoted 4 lines
(quoted diff) +4
-0+enum rpu_oper_mode { + PM_RPU_MODE_LOCKSTEP = 0, + PM_RPU_MODE_SPLIT = 1, +};
so the assignment "rpu_mode = lockstep_mode" is incorrect.
- Michael
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hi Michael,
Thanks for the comments,
Quoted 15 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Thursday, September 17, 2020 3:11 PM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; Jiaying Liang <[email protected]>; [email protected]; linux-arm- [email protected]; Jiaying Liang <[email protected]>; Michal Simek <[email protected]>; Ed T. Mooring <[email protected]>; Jason Wu <[email protected]>
Subject: Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Quoted 8 lines
Hey Ben,
Split mode is still not functional in this patch series (as was the case with the last few revisions).
Before sending out the next revision, can you _please_ ensure you're testing all supported configurations?
[Ben Levinsky] I will make sure to update in next revision.
As per review, I tested on QEMU and hardware firmware loading in split mode on R5 0 split, R5 1 split and R5 lockstep and was able to successfully load, start and establish IPC links
That being said, I will update the to reflect the values between the enum for rpu operation mode and the documentation in the binding.
For testing, I can provide a pointer to a publicly available device tree I am using if that helps. If not, can you expand on the testing of supported configurations?
Quoted 27 lines · Ben Levinsky
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/** +* RPU core configuration +*/ +static enum rpu_oper_mode rpu_mode; +<.. snip ..>
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +18
-0+static int zynqmp_r5_remoteproc_probe (struct platform_device *pdev) +{ +int ret, i =0 ; +u32 lockstep_mode; +struct device *dev = &pdev->dev; +struct device_node *nc; + + ret =of_property_read_u32 (dev->of_node, +"lockstep-mode" , + &lockstep_mode); +if (ret <0 ) { +return ret; + }else if (lockstep_mode !=PM_RPU_MODE_LOCKSTEP && + lockstep_mode !=PM_RPU_MODE_SPLIT ) { +dev_err (dev, +"Invalid lockstep-mode %x in %pOF\n" , + lockstep_mode, dev->of_node); +return -EINVAL ;
Quoted 14 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 705
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + + rpu_mode = lockstep_mode; + +dev_dbg (dev,"RPU configuration: %s\n" , + lockstep_mode ?"lockstep" :"split" );The binding documents lockstep-mode as:
Quoted 4 lines
(quoted diff) +4
-0+ lockstep-mode: + description: + R5 core configuration (split is 0 or lock-step and 1) + maxItems: 1
will update this as you note so that lockstep and split mode are accurately reflected.
Quoted 12 lines
(Which needs to be reworded, but it looks like the intent was "split is 0 and lock-step is 1")
However,
rpu_oper_modeis defined as:Review
Quoted 4 lines
(quoted diff) +4
-0+enum rpu_oper_mode { + PM_RPU_MODE_LOCKSTEP = 0, + PM_RPU_MODE_SPLIT = 1, +};so the assignment "
rpu_mode=lockstep_mode" is incorrect.
once the binding is updated, why would this still be incorrect? Assuming the documentation is updated, the above line would be ok, right?
Thank you for the review
Ben
Quoted 1 line
- Michael
RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Quoted 47 lines
-----Original Message-----
From: Ben Levinsky <[email protected]>
Sent: Thursday, September 17, 2020 3:19 PM
To: Michael Auchter <[email protected]>
Cc: [email protected]; Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; Jiaying Liang <[email protected]>; [email protected]; linux-arm- [email protected]; Jiaying Liang <[email protected]>; Michal Simek <[email protected]>; Ed T. Mooring <[email protected]>; Jason Wu <[email protected]>
Subject: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverHi Michael,
Thanks for the comments,
Quoted 15 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Thursday, September 17, 2020 3:11 PM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; Jiaying Liang <[email protected]>; [email protected]; linux-arm- [email protected]; Jiaying Liang <[email protected]>; Michal Simek <[email protected]>; Ed T. Mooring <[email protected]>; Jason Wu <[email protected]>
Subject: Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 8 lines
Hey Ben,
Split mode is still not functional in this patch series (as was the case with the last few revisions).
Before sending out the next revision, can you _please_ ensure you're testing all supported configurations?
[Ben Levinsky] I will make sure to update in next revision.
As per review, I tested on QEMU and hardware firmware loading in split
mode on R5 0 split, R5 1 split and R5 lockstep and was able to successfully load, start and establish IPC links
Quoted 7 lines
That being said, I will update the to reflect the values between the enum for rpu operation mode and the documentation in the binding.
For testing, I can provide a pointer to a publicly available device tree I am using if that helps. If not, can you expand on the testing of supported configurations?
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into And I planned to add OCM support if/when the driver gets in.
Thanks,
Ben
Quoted 63 lines
Quoted 27 lines · Ben Levinsky
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/** +* RPU core configuration +*/ +static enum rpu_oper_mode rpu_mode; +<.. snip ..>
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +18
-0+static int zynqmp_r5_remoteproc_probe (struct platform_device *pdev) +{ +int ret, i =0 ; +u32 lockstep_mode; +struct device *dev = &pdev->dev; +struct device_node *nc; + + ret =of_property_read_u32 (dev->of_node, +"lockstep-mode" , + &lockstep_mode); +if (ret <0 ) { +return ret; + }else if (lockstep_mode !=PM_RPU_MODE_LOCKSTEP && + lockstep_mode !=PM_RPU_MODE_SPLIT ) { +dev_err (dev, +"Invalid lockstep-mode %x in %pOF\n" , + lockstep_mode, dev->of_node); +return -EINVAL ;Quoted 14 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 705
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + + rpu_mode = lockstep_mode; + +dev_dbg (dev,"RPU configuration: %s\n" , + lockstep_mode ?"lockstep" :"split" );The binding documents lockstep-mode as:
Quoted 4 lines
(quoted diff) +4
-0+ lockstep-mode: + description: + R5 core configuration (split is 0 or lock-step and 1) + maxItems: 1will update this as you note so that lockstep and split mode are accurately reflected.
Quoted 12 lines
(Which needs to be reworded, but it looks like the intent was "split is 0 and lock-step is 1")
However,
rpu_oper_modeis defined as:Review
Quoted 4 lines
(quoted diff) +4
-0+enum rpu_oper_mode { + PM_RPU_MODE_LOCKSTEP = 0, + PM_RPU_MODE_SPLIT = 1, +};so the assignment "
rpu_mode=lockstep_mode" is incorrect.once the binding is updated, why would this still be incorrect? Assuming the documentation is updated, the above line would be ok, right?
Thank you for the review
BenQuoted 1 line
- Michael
Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Quoted 3 lines · Ben Levinsky
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into
The firmware is being loaded to TCM.
I'm able to use this driver to load and run my firmware on both R5 cores, but only after I change the incorrect:
rpu_mode = lockstep_mode
assignment to:
rpu_mode = lockstep_mode ? PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hi Michael, Punit,
Quoted 19 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 3 lines · Ben Levinsky
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into
The firmware is being loaded to TCM.
I'm able to use this driver to load and run my firmware on both R5 cores, but only after I change the incorrect:
Quoted 6 lines
rpu_mode=lockstep_modeassignment to:
rpu_mode = lockstep_mode ? PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;
There was a point raised by Punit that as "it is possible to set R5 to operatore in split or lock-step mode dynamically" which is true and can be done via sysfs and the Xilinx firmware kernel code. A suggestion that might clean up the driver so that the whole rpu_mode, tcm_mode configuration can be simplified and pulled out of the driver:
- as Punit suggested, remove the lockstep-mode property
- the zynqmp_remoteproc_r5 driver ONLY loads firmware and does start/stop.
- the zynqmp_remoteproc_r5 driver does not configure and memory regions or the RPU. Let the Xilinx firmware sysfs interface handle this.
Few advantages to this:
1. no extra configuration code in the zynqmp r5 remoteproc probe() for either R5 in the RPU cluster
2. less state to manage in the remoteproc driver
3. simpler documentation in the device tree binding
Again thank you both for the thoughtful review comments on this Thanks
Ben
Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hey Ben,
Quoted 23 lines · Ben Levinsky
Hi Michael, Punit,
Quoted 21 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 3 lines · Ben Levinsky
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into
The firmware is being loaded to TCM.
I'm able to use this driver to load and run my firmware on both R5 cores, but only after I change the incorrect:
rpu_mode=lockstep_mode
Quoted 7 lines
Quoted 4 lines
assignment to:
rpu_mode = lockstep_mode ? PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;There was a point raised by Punit that as "it is possible to set R5 to operatore in split or lock-step mode dynamically" which is true and can be done via sysfs and the Xilinx firmware kernel code.
I'm not familiar with this, and don't see an obvious way to do this (from looking at drivers/firmware/xilinx/). Can you point me to this code?
Quoted 6 lines
A suggestion that might clean up the driver so that the whole
rpu_mode,tcm_modeconfiguration can be simplified and pulled out of
the driver:
- as Punit suggested, remove the lockstep-mode property
- thezynqmp_remoteproc_r5driver ONLY loads firmware and does start/stop.
- thezynqmp_remoteproc_r5driver does not configure and memory regions or the RPU. Let the Xilinx firmware sysfs interface handle this.
I don't think this is a good approach.
- How will someone know to configure the RPU mode and TCM mode via sysfs?
- What happens when someone changes the RPU mode after remoteproc has already booted some firmware on it?
- What if the kernel is the one booting the R5, not the user?
Split vs. lockstep, IMO, needs to be specified as part of the device tree, and this driver needs to handle configuring the RPU mode and TCM modes appropriately.
Split vs. lockstep already necessitates different entries in the device tree:
- In the binding, each core references its TCMs via the meta-memory-regions phandles, and the referenced nodes necessarily encode this size. In split mode, each core has access to 64K of TCMA/TCMB, while in lockstep R5 0 has access to 128K of TCMA/TCMB. So, the "xlnx,tcm" nodes' reg entries need to differ between lockstep and split.
- In lockstep mode, it does not make sense to have both r5@0 and r5@1 child nodes: only r5@0 makes sense. Though, I just realized that I think this driver will currently permit that, and register two remoteprocs even in lockstep mode... What happens if someone tries to load firmware on to r5_1 when they're in lockstep mode? This should probably be prevented.
Thanks,
Michael
Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
HI Michael, Ben, Punit,
Quoted 39 lines · Michael Auchter
Hey Ben,
Quoted 23 lines · Ben Levinsky
Hi Michael, Punit,
Quoted 21 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 3 lines · Ben Levinsky
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into
The firmware is being loaded to TCM.
I'm able to use this driver to load and run my firmware on both R5 cores, but only after I change the incorrect:
rpu_mode=lockstep_modeQuoted 7 lines
Quoted 4 lines
assignment to:
rpu_mode=lockstep_mode?PM_RPU_MODE_LOCKSTEP
:PM_RPU_MODE_SPLIT;There was a point raised by Punit that as "it is possible to set R5 to operatore in split or lock-step mode dynamically" which is true and can be done via sysfs and the Xilinx firmware kernel code.
I'm not familiar with this, and don't see an obvious way to do this (from looking at
drivers/firmware/xilinx/). Can you point me to this code?
Quoted 8 lines
Quoted 6 lines
A suggestion that might clean up the driver so that the whole
rpu_mode,tcm_modeconfiguration can be simplified and pulled out of
the driver:
- as Punit suggested, remove the lockstep-mode property
- thezynqmp_remoteproc_r5driver ONLY loads firmware and does start/stop.
- thezynqmp_remoteproc_r5driver does not configure and memory regions or the RPU. Let the Xilinx firmware sysfs interface handle this.I don't think this is a good approach.
[Wendy] The TCMs are presented differently in the system depending on if RPU is in
lockstep or split mode.
Not sure if it is allowed to list TCMs registers properties for both split mode and lockstep
mode in the same device node.
Even though, driver can have this information in the code, but I feel the device tree is a
better place for this information.
And also for predefined shared memories, you will need to know the RPU op mode ahead,
so that you can specify which shared memories belong to which RPU.
To dynamic setup the RPU mode, besides sysfs, setup, if remoteproc can support device tree overlay, the RPUs can be described with dtbo and loaded at runtime.
Just want to understand the case which needs to set RPU mode at runtime? I think testing can be one case.
Best Regards,
Wendy
Quoted 26 lines
- How will someone know to configure the RPU mode and TCM mode via sysfs?
- What happens when someone changes the RPU mode after remoteproc has already booted some firmware on it?
- What if the kernel is the one booting the R5, not the user?Split vs. lockstep, IMO, needs to be specified as part of the device tree, and this driver needs to handle configuring the RPU mode and TCM modes appropriately.
Split vs. lockstep already necessitates different entries in the device tree:
- In the binding, each core references its TCMs via the meta-memory-regions phandles, and the referenced nodes necessarily encode this size. In split mode, each core has access to 64K of TCMA/TCMB, while in lockstep R5 0 has access to 128K of TCMA/TCMB. So, the "xlnx,tcm" nodes' reg entries need to differ between lockstep and split.
- In lockstep mode, it does not make sense to have both r5@0 and r5@1 child nodes: only r5@0 makes sense. Though, I just realized that I think this driver will currently permit that, and register two remoteprocs even in lockstep mode... What happens if someone tries to load firmware on tor5_1when they're in lockstep mode? This should probably be prevented.Thanks,
Michael
RE: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hi All,
Quoted 18 lines
-----Original Message-----
From: Wendy Liang <[email protected]>
Sent: Friday, September 18, 2020 6:53 PM
To: Michael Auchter <[email protected]>
Cc: Ben Levinsky <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverHI Michael, Ben, Punit,
Quoted 4 lines · Michael Auchter
Hey Ben,
On Fri, Sep 18, 2020 at 06:01:19PM +0000, Ben Levinsky wrote:
Quoted 36 lines
Quoted 7 lines
Quoted 7 lines
Hi Michael, Punit,
Quoted 5 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; [email protected];linux-
Quoted 28 lines
Quoted 16 lines
Quoted 16 lines
[email protected]; [email protected]
Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 3 lines · Ben Levinsky
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into
The firmware is being loaded to TCM.
I'm able to use this driver to load and run my firmware on both R5 cores, but only after I change the incorrect:
rpu_mode=lockstep_modeQuoted 7 lines
Quoted 4 lines
assignment to:
rpu_mode=lockstep_mode?PM_RPU_MODE_LOCKSTEP
:PM_RPU_MODE_SPLIT;There was a point raised by Punit that as "it is possible to set R5 to operatore in split or lock-step mode dynamically" which is true and can be done via sysfs and the Xilinx firmware kernel code.
I'm not familiar with this, and don't see an obvious way to do this (from looking at
drivers/firmware/xilinx/). Can you point me to this code?
[Ben Levinsky] A way to do this, though it seems later comments show it is not an implementation to pursue, is use the RPU configuration API and present it via sysfs interface a la https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842232/Zynq+UltraScale+MPSoC+Power+Management+-+Linux+Kernel#ZynqUltraScale%EF%BC%8BMPSoCPowerManagement-LinuxKernel-EnableClock
Quoted 10 lines
Quoted 5 lines
Quoted 5 lines
A suggestion that might clean up the driver so that the whole
rpu_mode,tcm_modeconfiguration can be simplified and pulled out of the driver:
- as Punit suggested, remove the lockstep-mode property
- thezynqmp_remoteproc_r5driver ONLY loads firmware and doesstart/stop.
Quoted 1 line
Quoted 1 line
- the
zynqmp_remoteproc_r5driver does not configure and memoryregions or the RPU. Let the Xilinx firmware sysfs interface handle this.
Quoted 2 lines
I don't think this is a good approach.
[Ben Levinsky] ok, noted. Can keep the configuration but still as wendy said just have lockstep property to denote lockstep mode in RPU and otherwise be split, for simplicity?
Quoted 15 lines
[Wendy] The TCMs are presented differently in the system depending on if RPU is in
lockstep or split mode.Not sure if it is allowed to list TCMs registers properties for both split mode and lockstep
mode in the same device node.Even though, driver can have this information in the code, but I feel the device tree is a
better place for this information.
And also for predefined shared memories, you will need to know the RPU op mode ahead,
so that you can specify which shared memories belong to which RPU.
Quoted 8 lines
To dynamic setup the RPU mode, besides sysfs, setup, if remoteproc can support
device tree overlay, the RPUs can be described with dtbo and loaded at runtime.Just want to understand the case which needs to set RPU mode at runtime? I think testing can be one case.
[Ben Levinsky] for testing, so far it has been r50/1 split and r5 lockstep
Quoted 14 lines
Best Regards,
WendyQuoted 1 line
- How will someone know to configure the RPU mode and TCM mode via
sysfs?
Quoted 1 line
- What happens when someone changes the RPU mode after remoteproc
has
Quoted 7 lines
already booted some firmware on it?
- What if the kernel is the one booting the R5, not the user?Split vs. lockstep, IMO, needs to be specified as part of the device tree, and this driver needs to handle configuring the RPU mode and TCM modes appropriately.
[Ben Levinsky] Ok, as Wendy suggested would instead the presence of a "lockstep=mode" property indicate lockstep mode and otherwise imply split mode?
Quoted 7 lines
Quoted 7 lines
Split vs. lockstep already necessitates different entries in the device tree:
- In the binding, each core references its TCMs via the meta-memory-regions phandles, and the referenced nodes necessarily encode this size. In split mode, each core has access to 64K of TCMA/TCMB, while in lockstep R5 0 has access to 128K of TCMA/TCMB. So, the "xlnx,tcm" nodes' reg entries need to differ between lockstep and
Quoted 8 lines
Quoted 8 lines
split.
- In lockstep mode, it does not make sense to have both r5@0 and r5@1 child nodes: only r5@0 makes sense. Though, I just realized that I think this driver will currently permit that, and register two remoteprocs even in lockstep mode... What happens if someone tries to load firmware on tor5_1when they're in lockstep mode? This should probably be prevented.
[Ben Levinsky] Good Point. the loading of R5 1 while in lockstep is an uncovered corner case.. for this, before loading/starting or requesting memory the state of global rpu mode can be checked and this can act as a guard for probing a remoteproc instance for r5-1 if either is in lockstep and similar safeguard for firmware loading for R5-1 if in lockstep mode
That is, add the lockstep property only if in lockstep mode and use the presence of it or lack thereof for subsequent, single R5-specific driver remoteproc R5 probes or firmware loading
In addition to the above property and its behavior, would correcting the inconsistencies of the Documentation vs the split/lockstep code in the remoteproc r5 device tree binding, its corresponding remoteproc r5 driver address the above concerns as well as the memory handling as you noted earlier?
Also in the next series I can point to a sample R5 application and device trees for the split mode and lockstep cases I used for testing in the cover letter.
Quoted 2 lines
Quoted 2 lines
Thanks,
Michael
Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hi Ben,
Quoted 123 lines · Ben Levinsky
Hi All,
Quoted 18 lines
-----Original Message-----
From: Wendy Liang <[email protected]>
Sent: Friday, September 18, 2020 6:53 PM
To: Michael Auchter <[email protected]>
Cc: Ben Levinsky <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverHI Michael, Ben, Punit,
Quoted 4 lines · Michael Auchter
Hey Ben,
On Fri, Sep 18, 2020 at 06:01:19PM +0000, Ben Levinsky wrote:
Quoted 36 lines
Quoted 7 lines
Quoted 7 lines
Hi Michael, Punit,
Quoted 5 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; [email protected];linux-
Quoted 28 lines
Quoted 16 lines
Quoted 16 lines
[email protected]; [email protected]
Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 3 lines · Ben Levinsky
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into
The firmware is being loaded to TCM.
I'm able to use this driver to load and run my firmware on both R5 cores, but only after I change the incorrect:
rpu_mode=lockstep_modeQuoted 7 lines
Quoted 4 lines
assignment to:
rpu_mode=lockstep_mode?PM_RPU_MODE_LOCKSTEP
:PM_RPU_MODE_SPLIT;There was a point raised by Punit that as "it is possible to set R5 to operatore in split or lock-step mode dynamically" which is true and can be done via sysfs and the Xilinx firmware kernel code.
I'm not familiar with this, and don't see an obvious way to do this (from looking at
drivers/firmware/xilinx/). Can you point me to this code?[Ben Levinsky] A way to do this, though it seems later comments show it is not an implementation to pursue, is use the RPU configuration API and present it via sysfs interface a la https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842232/Zynq+UltraScale+MPSoC+Power+Management+-+Linux+Kernel#ZynqUltraScale%EF%BC%8BMPSoCPowerManagement-LinuxKernel-EnableClock
Quoted 10 lines
Quoted 5 lines
Quoted 5 lines
A suggestion that might clean up the driver so that the whole
rpu_mode,tcm_modeconfiguration can be simplified and pulled out of the driver:
- as Punit suggested, remove the lockstep-mode property
- thezynqmp_remoteproc_r5driver ONLY loads firmware and doesstart/stop.
Quoted 1 line
Quoted 1 line
- the
zynqmp_remoteproc_r5driver does not configure and memoryregions or the RPU. Let the Xilinx firmware sysfs interface handle this.
Quoted 2 lines
I don't think this is a good approach.
[Ben Levinsky] ok, noted. Can keep the configuration but still as wendy said just have lockstep property to denote lockstep mode in RPU and otherwise be split, for simplicity?
Quoted 15 lines
[Wendy] The TCMs are presented differently in the system depending on if RPU is in
lockstep or split mode.Not sure if it is allowed to list TCMs registers properties for both split mode and lockstep
mode in the same device node.Even though, driver can have this information in the code, but I feel the device tree is a
better place for this information.
And also for predefined shared memories, you will need to know the RPU op mode ahead,
so that you can specify which shared memories belong to which RPU.Quoted 8 lines
To dynamic setup the RPU mode, besides sysfs, setup, if remoteproc can support
device tree overlay, the RPUs can be described with dtbo and loaded at runtime.Just want to understand the case which needs to set RPU mode at runtime? I think testing can be one case.
[Ben Levinsky] for testing, so far it has been r50/1 split and r5 lockstep
Quoted 14 lines
Best Regards,
WendyQuoted 1 line
- How will someone know to configure the RPU mode and TCM mode via
sysfs?
Quoted 1 line
- What happens when someone changes the RPU mode after remoteproc
has
Quoted 7 lines
already booted some firmware on it?
- What if the kernel is the one booting the R5, not the user?Split vs. lockstep, IMO, needs to be specified as part of the device tree, and this driver needs to handle configuring the RPU mode and TCM modes appropriately.
[Ben Levinsky] Ok, as Wendy suggested would instead the presence of a "lockstep=mode" property indicate lockstep mode and otherwise imply split mode?
Quoted 7 lines
Quoted 7 lines
Split vs. lockstep already necessitates different entries in the device tree:
- In the binding, each core references its TCMs via the meta-memory-regions phandles, and the referenced nodes necessarily encode this size. In split mode, each core has access to 64K of TCMA/TCMB, while in lockstep R5 0 has access to 128K of TCMA/TCMB. So, the "xlnx,tcm" nodes' reg entries need to differ between lockstep andQuoted 8 lines
Quoted 8 lines
split.
- In lockstep mode, it does not make sense to have both r5@0 and r5@1 child nodes: only r5@0 makes sense. Though, I just realized that I think this driver will currently permit that, and register two remoteprocs even in lockstep mode... What happens if someone tries to load firmware on tor5_1when they're in lockstep mode? This should probably be prevented.
Quoted 1 line
[Ben Levinsky] Good Point. the loading of R5 1 while in lockstep is an uncovered corner case.. for this, before loading/starting or requesting memory the state of global rpu mode can be checked and this can act as a guard for probing a remoteproc instance for r5-1 if either is in lockstep and similar safeguard for firmware loading for R5-1 if in lockstep mode
[Wendy] As op mode is described in the device tree, in lockstep mode, r5-1 doesn't need to show in the sysfs.
Thanks,
Wendy
Quoted 9 lines
That is, add the lockstep property only if in lockstep mode and use the presence of it or lack thereof for subsequent, single R5-specific driver remoteproc R5 probes or firmware loading
In addition to the above property and its behavior, would correcting the inconsistencies of the Documentation vs the split/lockstep code in the remoteproc r5 device tree binding, its corresponding remoteproc r5 driver address the above concerns as well as the memory handling as you noted earlier?
Also in the next series I can point to a sample R5 application and device trees for the split mode and lockstep cases I used for testing in the cover letter.
Quoted 2 lines
Quoted 2 lines
Thanks,
Michael
Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
In reply to Ben LevinskyHi Ben
Quoted 89 lines · Ben Levinsky
Hi All,
Quoted 18 lines
-----Original Message-----
From: Wendy Liang <[email protected]>
Sent: Friday, September 18, 2020 6:53 PM
To: Michael Auchter <[email protected]>
Cc: Ben Levinsky <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverHI Michael, Ben, Punit,
Quoted 4 lines · Michael Auchter
Hey Ben,
On Fri, Sep 18, 2020 at 06:01:19PM +0000, Ben Levinsky wrote:
Quoted 36 lines
Quoted 7 lines
Quoted 7 lines
Hi Michael, Punit,
Quoted 5 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; [email protected];linux-
Quoted 28 lines
Quoted 16 lines
Quoted 16 lines
[email protected]; [email protected]
Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 3 lines · Ben Levinsky
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into
The firmware is being loaded to TCM.
I'm able to use this driver to load and run my firmware on both R5 cores, but only after I change the incorrect:
rpu_mode=lockstep_modeQuoted 7 lines
Quoted 4 lines
assignment to:
rpu_mode=lockstep_mode?PM_RPU_MODE_LOCKSTEP
:PM_RPU_MODE_SPLIT;There was a point raised by Punit that as "it is possible to set R5 to operatore in split or lock-step mode dynamically" which is true and can be done via sysfs and the Xilinx firmware kernel code.
I'm not familiar with this, and don't see an obvious way to do this (from looking at
drivers/firmware/xilinx/). Can you point me to this code?[Ben Levinsky] A way to do this, though it seems later comments show it is not an implementation to pursue, is use the RPU configuration API and present it via sysfs interface a la https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842232/Zynq+UltraScale+MPSoC+Power+Management+-+Linux+Kernel#ZynqUltraScale%EF%BC%8BMPSoCPowerManagement-LinuxKernel-EnableClock
Quoted 10 lines
Quoted 5 lines
Quoted 5 lines
A suggestion that might clean up the driver so that the whole
rpu_mode,tcm_modeconfiguration can be simplified and pulled out of the driver:
- as Punit suggested, remove the lockstep-mode property
- thezynqmp_remoteproc_r5driver ONLY loads firmware and doesstart/stop.
Quoted 1 line
Quoted 1 line
- the
zynqmp_remoteproc_r5driver does not configure and memoryregions or the RPU. Let the Xilinx firmware sysfs interface handle this.
Quoted 2 lines
I don't think this is a good approach.
[Ben Levinsky] ok, noted. Can keep the configuration but still as wendy said just have lockstep property to denote lockstep mode in RPU and otherwise be split, for simplicity?
Quoted 20 lines
[Wendy] The TCMs are presented differently in the system depending on if RPU is in
lockstep or split mode.Not sure if it is allowed to list TCMs registers properties for both split mode and lockstep
mode in the same device node.Even though, driver can have this information in the code, but I feel the device tree is a
better place for this information.
And also for predefined shared memories, you will need to know the RPU op mode ahead,
so that you can specify which shared memories belong to which RPU.To dynamic setup the RPU mode, besides sysfs, setup, if remoteproc can support
device tree overlay, the RPUs can be described with dtbo and loaded at runtime.
Quoted 4 lines
Quoted 3 lines
Just want to understand the case which needs to set RPU mode at runtime? I think testing can be one case.
[Ben Levinsky] for testing, so far it has been r50/1 split and r5 lockstep
[Wendy] I tried to understand the need to change the RPU mode at runtime. What I can think of is for testing purposes.
Thanks,
Wendy
Quoted 40 lines
Quoted 14 lines
Best Regards,
WendyQuoted 1 line
- How will someone know to configure the RPU mode and TCM mode via
sysfs?
Quoted 1 line
- What happens when someone changes the RPU mode after remoteproc
has
Quoted 7 lines
already booted some firmware on it?
- What if the kernel is the one booting the R5, not the user?Split vs. lockstep, IMO, needs to be specified as part of the device tree, and this driver needs to handle configuring the RPU mode and TCM modes appropriately.
[Ben Levinsky] Ok, as Wendy suggested would instead the presence of a "lockstep=mode" property indicate lockstep mode and otherwise imply split mode?
Quoted 7 lines
Quoted 7 lines
Split vs. lockstep already necessitates different entries in the device tree:
- In the binding, each core references its TCMs via the meta-memory-regions phandles, and the referenced nodes necessarily encode this size. In split mode, each core has access to 64K of TCMA/TCMB, while in lockstep R5 0 has access to 128K of TCMA/TCMB. So, the "xlnx,tcm" nodes' reg entries need to differ between lockstep andQuoted 8 lines
Quoted 8 lines
split.
- In lockstep mode, it does not make sense to have both r5@0 and r5@1 child nodes: only r5@0 makes sense. Though, I just realized that I think this driver will currently permit that, and register two remoteprocs even in lockstep mode... What happens if someone tries to load firmware on tor5_1when they're in lockstep mode? This should probably be prevented.[Ben Levinsky] Good Point. the loading of R5 1 while in lockstep is an uncovered corner case.. for this, before loading/starting or requesting memory the state of global rpu mode can be checked and this can act as a guard for probing a remoteproc instance for r5-1 if either is in lockstep and similar safeguard for firmware loading for R5-1 if in lockstep mode
That is, add the lockstep property only if in lockstep mode and use the presence of it or lack thereof for subsequent, single R5-specific driver remoteproc R5 probes or firmware loading
In addition to the above property and its behavior, would correcting the inconsistencies of the Documentation vs the split/lockstep code in the remoteproc r5 device tree binding, its corresponding remoteproc r5 driver address the above concerns as well as the memory handling as you noted earlier?
Also in the next series I can point to a sample R5 application and device trees for the split mode and lockstep cases I used for testing in the cover letter.
Quoted 2 lines
Quoted 2 lines
Thanks,
Michael
Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
In reply to Ben LevinskyQuoted 33 lines · Ben Levinsky
Hi All,
Quoted 31 lines
-----Original Message-----
From: Wendy Liang <[email protected]>
Sent: Friday, September 18, 2020 6:53 PM
To: Michael Auchter <[email protected]>
Cc: Ben Levinsky <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverHI Michael, Ben, Punit,
Quoted 11 lines · Michael Auchter
Hey Ben,
Quoted 7 lines · Ben Levinsky
Hi Michael, Punit,
Quoted 5 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; [email protected];linux-
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
[email protected]; [email protected]
Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverOn Thu, Sep 17, 2020 at 10:50:42PM +0000, Ben Levinsky wrote:
[...]
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
A suggestion that might clean up the driver so that the whole
rpu_mode,tcm_modeconfiguration can be simplified and pulled out of the driver:
- as Punit suggested, remove the lockstep-mode property
- thezynqmp_remoteproc_r5driver ONLY loads firmware and doesstart/stop.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
- the
zynqmp_remoteproc_r5driver does not configure and memoryregions or the RPU. Let the Xilinx firmware sysfs interface handle this.
Quoted 2 lines
I don't think this is a good approach.
[Ben Levinsky] ok, noted. Can keep the configuration but still as wendy said just have lockstep property to denote lockstep mode in RPU and otherwise be split, for simplicity?
That would be a better approach than the current proposal.
Quoted 20 lines
Quoted 20 lines
[Wendy] The TCMs are presented differently in the system depending on if RPU is in
lockstep or split mode.Not sure if it is allowed to list TCMs registers properties for both split mode and lockstep
mode in the same device node.Even though, driver can have this information in the code, but I feel the device tree is a
better place for this information.
And also for predefined shared memories, you will need to know the RPU op mode ahead,
so that you can specify which shared memories belong to which RPU.To dynamic setup the RPU mode, besides sysfs, setup, if remoteproc can support
device tree overlay, the RPUs can be described with dtbo and loaded at runtime.
Quoted 19 lines
Quoted 2 lines
Just want to understand the case which needs to set RPU mode at runtime? I think testing can be one case.
[Ben Levinsky] for testing, so far it has been r50/1 split and r5 lockstep
Quoted 14 lines
Best Regards,
WendyQuoted 1 line
- How will someone know to configure the RPU mode and TCM mode via
sysfs?
Quoted 1 line
- What happens when someone changes the RPU mode after remoteproc
has
Quoted 7 lines
already booted some firmware on it?
- What if the kernel is the one booting the R5, not the user?Split vs. lockstep, IMO, needs to be specified as part of the device tree, and this driver needs to handle configuring the RPU mode and TCM modes appropriately.
Typically, the device tree is expected to describe the hardware to the kernel rather than telling it what the hardware should look like. More below.
Quoted 12 lines
[Ben Levinsky] Ok, as Wendy suggested would instead the presence of a "lockstep=mode" property indicate lockstep mode and otherwise imply split mode?
Quoted 8 lines
Quoted 8 lines
Split vs. lockstep already necessitates different entries in the device tree:
- In the binding, each core references its TCMs via the meta-memory-regions phandles, and the referenced nodes necessarily encode this size. In split mode, each core has access to 64K of TCMA/TCMB, while in lockstep R5 0 has access to 128K of TCMA/TCMB. So, the "xlnx,tcm" nodes' reg entries need to differ between lockstep and split.
But considering the dependency between split/lockstep mode and available memory sizes as described here it maybe OK to have the firmware (via DT) specify the configured mode. Though IMO it is overloading the device tree functionality (not the first time) because that's the hammer we've got.
Even in this scenario, ideally it would be the boot firmware's responsibility to configure the RPU in the desired mode and communicate the mode to the kernel. The driver can use the mode information to verify that the system is in the expected state during probe and error out if not.
Though taking this approach will not help the "testing" usecase mentioned by Wendy.
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
- In lockstep mode, it does not make sense to have both r5@0 and r5@1 child nodes: only r5@0 makes sense. Though, I just realized that I think this driver will currently permit that, and register two remoteprocs even in lockstep mode... What happens if someone tries to load firmware on to
r5_1when they're in lockstep mode? This should probably be prevented.
Quoted 6 lines
[Ben Levinsky] Good Point. the loading of R5 1 while in lockstep is an uncovered corner case.. for this, before loading/starting or requesting memory the state of global rpu mode can be checked and this can act as a guard for probing a remoteproc instance for r5-1 if either is in lockstep and similar safeguard for firmware loading for R5-1 if in lockstep mode
IIUC, if the second R5 is not registered with remoteproc, it is not possible to request loading firmware to it from userspace. So no special case guards should be needed.
Thanks,
Punit
[...]
Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
In reply to Ben LevinskyQuoted 32 lines · Ben Levinsky
Hi Michael,
Thanks for the comments,
Quoted 15 lines
-----Original Message-----
From: Michael Auchter <[email protected]>
Sent: Thursday, September 17, 2020 3:11 PM
To: Ben Levinsky <[email protected]>
Cc: [email protected]; Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; Jiaying Liang <[email protected]>; [email protected]; linux-arm- [email protected]; Jiaying Liang <[email protected]>; Michal Simek <[email protected]>; Ed T. Mooring <[email protected]>; Jason Wu <[email protected]>
Subject: Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 8 lines
Hey Ben,
Split mode is still not functional in this patch series (as was the case with the last few revisions).
Before sending out the next revision, can you _please_ ensure you're testing all supported configurations?
[Ben Levinsky] I will make sure to update in next revision. As per review, I tested on QEMU and hardware firmware loading in split mode on R5 0 split, R5 1 split and R5 lockstep and was able to successfully load, start and establish IPC links
Quoted 6 lines
That being said, I will update the to reflect the values between the enum for rpu operation mode and the documentation in the binding.
For testing, I can provide a pointer to a publicly available device tree I am using if that helps. If not, can you expand on the testing of supported configurations?
I'm testing exclusively split mode configuration. I load and run firmware on R5 0, and then do the same on R5 1.
Given the logic error, I admit that I'm confused how this could have worked in your tests, unless the device tree you used to test split mode contained "lockstep-mode = <1>", and the lockstep device tree contained "lockstep-mode = <0>".
But if that was the case, then that means the device trees used for testing changed this property's value between v13 and v14, for seemingly no reason.
Quoted 55 lines
Quoted 27 lines · Ben Levinsky
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/** +* RPU core configuration +*/ +static enum rpu_oper_mode rpu_mode; +<.. snip ..>
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +18
-0+static int zynqmp_r5_remoteproc_probe (struct platform_device *pdev) +{ +int ret, i =0 ; +u32 lockstep_mode; +struct device *dev = &pdev->dev; +struct device_node *nc; + + ret =of_property_read_u32 (dev->of_node, +"lockstep-mode" , + &lockstep_mode); +if (ret <0 ) { +return ret; + }else if (lockstep_mode !=PM_RPU_MODE_LOCKSTEP && + lockstep_mode !=PM_RPU_MODE_SPLIT ) { +dev_err (dev, +"Invalid lockstep-mode %x in %pOF\n" , + lockstep_mode, dev->of_node); +return -EINVAL ;Quoted 14 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 705
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + + rpu_mode = lockstep_mode; + +dev_dbg (dev,"RPU configuration: %s\n" , + lockstep_mode ?"lockstep" :"split" );The binding documents lockstep-mode as:
Quoted 4 lines
(quoted diff) +4
-0+ lockstep-mode: + description: + R5 core configuration (split is 0 or lock-step and 1) + maxItems: 1will update this as you note so that lockstep and split mode are accurately reflected.
Quoted 12 lines
(Which needs to be reworded, but it looks like the intent was "split is 0 and lock-step is 1")
However,
rpu_oper_modeis defined as:Review
Quoted 4 lines
(quoted diff) +4
-0+enum rpu_oper_mode { + PM_RPU_MODE_LOCKSTEP = 0, + PM_RPU_MODE_SPLIT = 1, +};so the assignment "
rpu_mode=lockstep_mode" is incorrect.
Quoted 3 lines
once the binding is updated, why would this still be incorrect? Assuming the documentation is updated, the above line would be ok, right?
It might not be incorrect, depending on how you change the binding.
If you update the binding documentation to say "lockstep-mode: 0 is lockstep, 1 is split", then this line would be fine. However, that would seem strange to me, as this reads like a boolean: setting this to 0 would logically indicate that the device is not configured in lockstep mode.
I don't think this is what you were proposing, but I'm not sure.
v13 did this correctly, and lockstep-mode == 0 implied split mode:
of_property_read_u32(dev->of_node, "lockstep-mode", &lockstep_mode);
if (!lockstep_mode) {
rpu_mode = PM_RPU_MODE_SPLIT;
} else if (lockstep_mode == 1) {
rpu_mode = PM_RPU_MODE_LOCKSTEP;
} Changing this is what broke v14.
Quoted 5 lines
Thank you for the review
BenQuoted 1 line
- Michael
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel