Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP
v8 cover Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP
The driver was tested on Xilinx ZynqMP and Versal.
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
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 | 73 ++ drivers/firmware/xilinx/zynqmp.c | 134 +++ drivers/remoteproc/Kconfig | 10 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/zynqmp_r5_remoteproc.c | 911 ++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 75 ++ 6 files changed, 1204 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
Signature · 1 line
2.17.1
PATCH v8 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 · 1 line
2.17.1
PATCH v8 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.
---
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
--- drivers/firmware/xilinx/zynqmp.c | 35 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 22 +++++++++++++++++ 2 files changed, 57 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_powerdown - PM call to request for another PU or subsystem to +* be powered down forcefully +* @target: 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_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack) +{ +return zynqmp_pm_invoke_fn (PM_FORCE_POWERDOWN , target, ack,0 ,0 ,NULL ); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_force_powerdown); + +/** +* zynqmp_pm_request_wakeup - 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_wakeup (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_wakeup); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves * @node: Node ID of the slave
include/linux/firmware/xlnx-zynqmp.h +22 -0
@@ -64,6 +64,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 +378,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_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_request_wakeup (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 +534,20 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value) {return -ENODEV ; } + +static inline int zynqmp_pm_force_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_request_wakeup (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 · 1 line
2.17.1
Re: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
Quoted 28 lines · Ben Levinsky
Add shutdown/wakeup a resource eemi operations to shutdown or bringup a resource.
Signed-off-by Ben Levinsky <[email protected]>---
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--- drivers/firmware/xilinx/zynqmp.c | 35 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 22 +++++++++++++++++ 2 files changed, 57 insertions(+)drivers/firmware/xilinx/zynqmp.c +0
-0@@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node) }EXPORT_SYMBOL_GPL (zynqmp_pm_release_node);
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 857
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +9
-0+/** +* zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to +* be powered down forcefully +* @target: 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_powerdown(const u32 target,
If the target is really the node id, why not calling it "node", the same way as below?
Quoted 60 lines
drivers/firmware/xilinx/zynqmp.c +26
-0+ const enum zynqmp_pm_request_ack ack) +{ +return zynqmp_pm_invoke_fn (PM_FORCE_POWERDOWN , target, ack,0 ,0 ,NULL ); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_force_powerdown); + +/** +* zynqmp_pm_request_wakeup - 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_wakeup (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_wakeup); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves * @node: Node ID of the slave include/linux/firmware/xlnx-zynqmp.h +9
-0@@ -64,6 +64,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 +378,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_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_request_wakeup (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 +534,20 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value) {return -ENODEV ; } +
include/linux/firmware/xlnx-zynqmp.h static inline int zynqmp_pm_set_boot_health_status(u32 value) · line 547
Quoted 10 lines
include/linux/firmware/xlnx-zynqmp.h +10
-0+static inline int zynqmp_pm_force_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_request_wakeup (const u32 node, +const bool set_addr, +const u64 address, +const enum zynqmp_pm_request_ack ack)
code style
Quoted 15 lines
include/linux/firmware/xlnx-zynqmp.h +3
-0+{ +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
RE: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
Quoted 56 lines
-----Original Message-----
From: Stefano Stabellini <[email protected]>
Sent: Thursday, August 13, 2020 1:36 PM
To: Ben Levinsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: Re: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIsQuoted 16 lines · Ben Levinsky
Add shutdown/wakeup a resource eemi operations to shutdown or bringup a resource.
Signed-off-by Ben Levinsky <[email protected]>---
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 enumReview
Quoted 6 lines
--- drivers/firmware/xilinx/zynqmp.c | 35 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 22 +++++++++++++++++ 2 files changed, 57 insertions(+)(quoted diff) +0
-0
b/drivers/firmware/xilinx/zynqmp.cdrivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 850
Quoted 9 lines
index
8d1ff2454e2e..f1a0bd35deeb100644drivers/firmware/xilinx/zynqmp.c +2
-0@@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node) }EXPORT_SYMBOL_GPL (zynqmp_pm_release_node); +/** +* zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 857
Quoted 7 lines
drivers/firmware/xilinx/zynqmp.c +7
-0+* be powered down forcefully +* @target: 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_powerdown(const u32 target,
Quoted 2 lines
If the target is really the node id, why not calling it "node", the same way as below?
[Ben Levinsky] good point. Will change to "target" in v9
Quoted 64 lines
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 5 lines
drivers/firmware/xilinx/zynqmp.c +3
-0+ const enum zynqmp_pm_request_ack ack) +{ + return zynqmp_pm_invoke_fn(PM_FORCE_POWERDOWN, target, ack,0, 0, NULL);
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 5 lines
drivers/firmware/xilinx/zynqmp.c +5
-0+} +EXPORT_SYMBOL_GPL (zynqmp_pm_force_powerdown); + +/** +* zynqmp_pm_request_wakeup - PM call to wake up selected master or subsystem
Quoted 4 lines
drivers/firmware/xilinx/zynqmp.c +4
-0+* @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 drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +10
-0+* +* Return: status, either success or error+reason +*/ +int zynqmp_pm_request_wakeup (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,drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 6 lines
drivers/firmware/xilinx/zynqmp.c +4
-0+ address >> 32, ack, NULL); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_request_wakeup); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves
Review
Quoted 2 lines
drivers/firmware/xilinx/zynqmp.c +0
-0* @node: Node ID of the slave (quoted diff) +0
-0
b/include/linux/firmware/xlnx-zynqmp.hinclude/linux/firmware/xlnx-zynqmp.h static inline int
Quoted 26 lines
index
bb347dfe4ba4..4d83a7d69c4c100644include/linux/firmware/xlnx-zynqmp.h +8
-0@@ -64,6 +64,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 +378,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_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_request_wakeup (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 +534,20 @@ static inline int
zynqmp_pm_set_boot_health_status(u32 value)
Quoted 17 lines
Quoted 4 lines
include/linux/firmware/xlnx-zynqmp.h +1
-0{return -ENODEV ; } +include/linux/firmware/xlnx-zynqmp.h static inline int zynqmp_pm_set_boot_health_status(u32 value) · line 547
Quoted 10 lines
include/linux/firmware/xlnx-zynqmp.h +10
-0+static inline int zynqmp_pm_force_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_request_wakeup (const u32 node, +const bool set_addr, +const u64 address, +const enum zynqmp_pm_request_ack ack)code style
[Ben Levinsky] will fix this in v9
Quoted 16 lines
Quoted 16 lines
include/linux/firmware/xlnx-zynqmp.h +3
-1+{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */ --2.17.1
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
RE: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
Hi Stefano
Quoted 114 lines
-----Original Message-----
From: [email protected] <linux-remoteproc- [email protected]> On Behalf Of Ben Levinsky
Sent: Tuesday, August 18, 2020 7:24 AM
To: Stefano Stabellini <[email protected]>
Cc: Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: RE: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIsQuoted 12 lines
-----Original Message-----
From: Stefano Stabellini <[email protected]>
Sent: Thursday, August 13, 2020 1:36 PM
To: Ben Levinsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: Re: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIsQuoted 19 lines
Quoted 18 lines · Ben Levinsky
Add shutdown/wakeup a resource eemi operations to shutdown or bringup a resource.
Signed-off-by Ben Levinsky <[email protected]>---
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--- drivers/firmware/xilinx/zynqmp.c | 35++++++++++++++++++++++++++++
Quoted 23 lines
Review
Quoted 4 lines
include/linux/firmware/xlnx-zynqmp.h | 22 +++++++++++++++++ 2 files changed, 57 insertions(+)(quoted diff) +0
-0
b/drivers/firmware/xilinx/zynqmp.cdrivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 850
Quoted 9 lines
index
8d1ff2454e2e..f1a0bd35deeb100644drivers/firmware/xilinx/zynqmp.c +2
-0@@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node) }EXPORT_SYMBOL_GPL (zynqmp_pm_release_node); +/** +* zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 857
Quoted 7 lines
drivers/firmware/xilinx/zynqmp.c +7
-0+* be powered down forcefully +* @target: 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_powerdown(const u32 target,Quoted 2 lines
If the target is really the node id, why not calling it "node", the same way as below?
[Ben Levinsky] good point. Will change to "target" in v9
Quoted 42 lines
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 5 lines
drivers/firmware/xilinx/zynqmp.c +3
-0+ const enum zynqmp_pm_request_ack ack) +{ + return zynqmp_pm_invoke_fn(PM_FORCE_POWERDOWN, target, ack,0, 0, NULL);
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 5 lines
drivers/firmware/xilinx/zynqmp.c +5
-0+} +EXPORT_SYMBOL_GPL (zynqmp_pm_force_powerdown); + +/** +* zynqmp_pm_request_wakeup - PM call to wake up selected master or subsystem
Quoted 4 lines
drivers/firmware/xilinx/zynqmp.c +4
-0+* @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 drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +10
-0+* +* Return: status, either success or error+reason +*/ +int zynqmp_pm_request_wakeup (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,drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 6 lines
drivers/firmware/xilinx/zynqmp.c +4
-0+ address >> 32, ack, NULL); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_request_wakeup); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves
Review
Quoted 2 lines
drivers/firmware/xilinx/zynqmp.c +0
-0* @node: Node ID of the slave (quoted diff) +0
-0
b/include/linux/firmware/xlnx-zynqmp.hQuoted 5 lines
index
bb347dfe4ba4..4d83a7d69c4c100644include/linux/firmware/xlnx-zynqmp.h +0
-0@@ -64,6 +64,8 @@
Quoted 39 lines
Quoted 8 lines
include/linux/firmware/xlnx-zynqmp.h int zynqmp_pm_write_pggs(u32 index, u32
Quoted 8 lines
include/linux/firmware/xlnx-zynqmp.h +2
-0enum 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 +378,12 @@ int zynqmp_pm_write_pggs(u32 index, u32value);
Quoted 30 lines
include/linux/firmware/xlnx-zynqmp.h static inline int
Quoted 13 lines
include/linux/firmware/xlnx-zynqmp.h +6
-0int 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_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_request_wakeup (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 +534,20 @@ static inline int
zynqmp_pm_set_boot_health_status(u32 value)Quoted 4 lines
include/linux/firmware/xlnx-zynqmp.h +1
-0{return -ENODEV ; } +include/linux/firmware/xlnx-zynqmp.h static inline int zynqmp_pm_set_boot_health_status(u32 value) · line 547
Quoted 10 lines
include/linux/firmware/xlnx-zynqmp.h +10
-0+static inline int zynqmp_pm_force_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_request_wakeup (const u32 node, +const bool set_addr, +const u64 address, +const enum zynqmp_pm_request_ack ack)code style
[Ben Levinsky] for this, the argument goes over 80 chars if aligned like the others. With that in mind for style, is it better to have the set_addr and address args aligned with ack or to push ack to the right but have the >80 chars style issue?
Thanks
Ben
Quoted 18 lines
Quoted 1 line
[Ben Levinsky] will fix this in v9
Quoted 16 lines
Quoted 16 lines
include/linux/firmware/xlnx-zynqmp.h +3
-1+{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */ --2.17.1
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
RE: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
Quoted 153 lines · Ben Levinsky
Quoted 114 lines
-----Original Message-----
From: [email protected] <linux-remoteproc- [email protected]> On Behalf Of Ben Levinsky
Sent: Tuesday, August 18, 2020 7:24 AM
To: Stefano Stabellini <[email protected]>
Cc: Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: RE: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIsQuoted 12 lines
-----Original Message-----
From: Stefano Stabellini <[email protected]>
Sent: Thursday, August 13, 2020 1:36 PM
To: Ben Levinsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: Re: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIsQuoted 19 lines
Quoted 18 lines · Ben Levinsky
Add shutdown/wakeup a resource eemi operations to shutdown or bringup a resource.
Signed-off-by Ben Levinsky <[email protected]>---
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--- drivers/firmware/xilinx/zynqmp.c | 35++++++++++++++++++++++++++++
Quoted 23 lines
Review
Quoted 4 lines
include/linux/firmware/xlnx-zynqmp.h | 22 +++++++++++++++++ 2 files changed, 57 insertions(+)(quoted diff) +0
-0
b/drivers/firmware/xilinx/zynqmp.cdrivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 850
Quoted 9 lines
index
8d1ff2454e2e..f1a0bd35deeb100644drivers/firmware/xilinx/zynqmp.c +2
-0@@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node) }EXPORT_SYMBOL_GPL (zynqmp_pm_release_node); +/** +* zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 857
Quoted 7 lines
drivers/firmware/xilinx/zynqmp.c +7
-0+* be powered down forcefully +* @target: 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_powerdown(const u32 target,Quoted 2 lines
If the target is really the node id, why not calling it "node", the same way as below?
[Ben Levinsky] good point. Will change to "target" in v9
Quoted 42 lines
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 5 lines
drivers/firmware/xilinx/zynqmp.c +3
-0+ const enum zynqmp_pm_request_ack ack) +{ + return zynqmp_pm_invoke_fn(PM_FORCE_POWERDOWN, target, ack,0, 0, NULL);
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 5 lines
drivers/firmware/xilinx/zynqmp.c +5
-0+} +EXPORT_SYMBOL_GPL (zynqmp_pm_force_powerdown); + +/** +* zynqmp_pm_request_wakeup - PM call to wake up selected master or subsystem
Quoted 4 lines
drivers/firmware/xilinx/zynqmp.c +4
-0+* @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 drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +10
-0+* +* Return: status, either success or error+reason +*/ +int zynqmp_pm_request_wakeup (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,drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 6 lines
drivers/firmware/xilinx/zynqmp.c +4
-0+ address >> 32, ack, NULL); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_request_wakeup); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves
Review
Quoted 2 lines
drivers/firmware/xilinx/zynqmp.c +0
-0* @node: Node ID of the slave (quoted diff) +0
-0
b/include/linux/firmware/xlnx-zynqmp.hQuoted 5 lines
index
bb347dfe4ba4..4d83a7d69c4c100644include/linux/firmware/xlnx-zynqmp.h +0
-0@@ -64,6 +64,8 @@Quoted 39 lines
Quoted 8 lines
include/linux/firmware/xlnx-zynqmp.h int zynqmp_pm_write_pggs(u32 index, u32
Quoted 8 lines
include/linux/firmware/xlnx-zynqmp.h +2
-0enum 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 +378,12 @@ int zynqmp_pm_write_pggs(u32 index, u32value);
Quoted 30 lines
include/linux/firmware/xlnx-zynqmp.h static inline int
Quoted 13 lines
include/linux/firmware/xlnx-zynqmp.h +6
-0int 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_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_request_wakeup (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 +534,20 @@ static inline int
zynqmp_pm_set_boot_health_status(u32 value)Quoted 4 lines
include/linux/firmware/xlnx-zynqmp.h +1
-0{return -ENODEV ; } +include/linux/firmware/xlnx-zynqmp.h static inline int zynqmp_pm_set_boot_health_status(u32 value) · line 547
Quoted 10 lines
include/linux/firmware/xlnx-zynqmp.h +10
-0+static inline int zynqmp_pm_force_powerdown (const u32 target, +const enum zynqmp_pm_request_ack ack) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_request_wakeup (const u32 node, +const bool set_addr, +const u64 address, +const enum zynqmp_pm_request_ack ack)code style
Quoted 1 line
[Ben Levinsky] for this, the argument goes over 80 chars if aligned like the others. With that in mind for style, is it better to have the
set_addrand address args aligned with ack or to push ack to the right but have the >80 chars style issue?
I don't have an opinion on code style -- anything that passes scripts/checkpatch.pl is fine for me.
PATCH v8 3/5 firmware: xilinx: Add RPU configuration APIs
In reply to Ben LevinskyThis patch adds APIs to provide access and a configuration interface to the current power state of a sub-system on Zynqmp sub-system.
---
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
--- drivers/firmware/xilinx/zynqmp.c | 99 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 34 ++++++++++ 2 files changed, 133 insertions(+)
drivers/firmware/xilinx/zynqmp.c +99 -0
@@ -846,6 +846,61 @@ 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 +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* Return: RPU mode +*/ +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_GET_RPU_OPER_MODE ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_rpu_mode); + +/** +* zynqmp_pm_set_rpu_mode() - Set RPU mode +* @node_id: Node ID of the device +* @ioctl_id: ID of the requested IOCTL +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_SET_RPU_OPER_MODE ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_rpu_mode); + +/** +* zynqmp_pm_tcm_comb_config - configure TCM +* @node_id: Node ID of the device +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_TCM_COMB_CONFIG ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_tcm_config); +/** * zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to * be powered down forcefully
@@ -881,6 +936,50 @@ int zynqmp_pm_request_wakeup(const u32 node, }EXPORT_SYMBOL_GPL (zynqmp_pm_request_wakeup); +/** +* zynqmp_pm_get_node_status - PM call to request a node's current power state +* @node: ID of the component or sub-system in question +* @status: Current operating state of the requested node +* @requirements: Current requirements asserted on the node, +* used for slave nodes only. +* @usage: Usage information, used for slave nodes only: +* PM_USAGE_NO_MASTER - No master is currently using +* the node +* PM_USAGE_CURRENT_MASTER - Only requesting master is +* currently using the node +* PM_USAGE_OTHER_MASTER - Only other masters are +* currently using the node +* PM_USAGE_BOTH_MASTERS - Both the current and at least +* one other master is currently +* using the node +* +* Return: status, either success or error+reason +*/ +int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) + +{ +u32 ret_payload[PAYLOAD_ARG_CNT ]; +int ret; + +if (!status) +return -EINVAL ; + + ret =zynqmp_pm_invoke_fn (PM_GET_NODE_STATUS , node,0 ,0 ,0 , + ret_payload); +if (ret_payload[0 ] ==XST_PM_SUCCESS ) { + *status = ret_payload[1 ]; +if (requirements) + *requirements = ret_payload[2 ]; +if (usage) + *usage = ret_payload[3 ]; + } + +return ret; +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_node_status); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves * @node: Node ID of the slave
include/linux/firmware/xlnx-zynqmp.h +34 -0
@@ -64,6 +64,7 @@enum pm_api_id {PM_GET_API_VERSION =1 , +PM_GET_NODE_STATUS =3 ,PM_FORCE_POWERDOWN =8 ,PM_REQUEST_WAKEUP =10 ,PM_SYSTEM_SHUTDOWN =12 ,
@@ -384,6 +385,14 @@ int zynqmp_pm_request_wakeup(const u32 node,const bool set_addr, const u64 address, const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_get_node_status (const u32 node,u32 *status, +u32 *requirements,u32 *usage); +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out);#else static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops (void ) {
@@ -548,6 +557,31 @@ static inline int zynqmp_pm_request_wakeup(const u32 node, {return -ENODEV ; } + +static inline int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */
Signature · 1 line
2.17.1
Re: [PATCH v8 3/5] firmware: xilinx: Add RPU configuration APIs
Quoted 2 lines · Ben Levinsky
This patch adds APIs to provide access and a configuration interface to the current power state of a sub-system on Zynqmp sub-system.
This doesn't read correctly
Quoted 18 lines
Signed-off-by Ben Levinsky <[email protected]>---
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--- drivers/firmware/xilinx/zynqmp.c | 99 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 34 ++++++++++ 2 files changed, 133 insertions(+)drivers/firmware/xilinx/zynqmp.c +0
-0@@ -846,6 +846,61 @@ int zynqmp_pm_release_node(const u32 node) }
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 856
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +8
-0EXPORT_SYMBOL_GPL (zynqmp_pm_release_node); +/** +* zynqmp_pm_get_rpu_mode() - Get RPU mode +* @node_id: Node ID of the device +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* Return: RPU mode
What kind of output are we expecting? An enum? Which one?
Quoted 24 lines
drivers/firmware/xilinx/zynqmp.c +24
-0+*/ +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_GET_RPU_OPER_MODE ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_rpu_mode); + +/** +* zynqmp_pm_set_rpu_mode() - Set RPU mode +* @node_id: Node ID of the device +* @ioctl_id: ID of the requested IOCTL +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 890
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +10
-0+ IOCTL_SET_RPU_OPER_MODE, 0, 0, out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_rpu_mode); + +/** +* zynqmp_pm_tcm_comb_config - configure TCM +* @node_id: Node ID of the device +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value
Same question on the type of the output value
Quoted 27 lines
drivers/firmware/xilinx/zynqmp.c +20
-0+* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_TCM_COMB_CONFIG ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_tcm_config); +/** * zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to * be powered down forcefully @@ -881,6 +936,50 @@ int zynqmp_pm_request_wakeup(const u32 node, }EXPORT_SYMBOL_GPL (zynqmp_pm_request_wakeup); +/** +* zynqmp_pm_get_node_status - PM call to request a node's current power state +* @node: ID of the component or sub-system in question +* @status: Current operating state of the requested node +* @requirements: Current requirements asserted on the node, +* used for slave nodes only. +* @usage: Usage information, used for slave nodes only: +* PM_USAGE_NO_MASTER - No master is currently using
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_request_wakeup(const u32 node, · line 956
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +10
-0+* the node +* PM_USAGE_CURRENT_MASTER - Only requesting master is +* currently using the node +* PM_USAGE_OTHER_MASTER - Only other masters are +* currently using the node +* PM_USAGE_BOTH_MASTERS - Both the current and at least +* one other master is currently +* using the node +* +* Return: status, either success or error+reason
Same question on status
Quoted 88 lines
drivers/firmware/xilinx/zynqmp.c +26
-0+*/ +int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) + +{ +u32 ret_payload[PAYLOAD_ARG_CNT ]; +int ret; + +if (!status) +return -EINVAL ; + + ret =zynqmp_pm_invoke_fn (PM_GET_NODE_STATUS , node,0 ,0 ,0 , + ret_payload); +if (ret_payload[0 ] ==XST_PM_SUCCESS ) { + *status = ret_payload[1 ]; +if (requirements) + *requirements = ret_payload[2 ]; +if (usage) + *usage = ret_payload[3 ]; + } + +return ret; +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_node_status); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves * @node: Node ID of the slave include/linux/firmware/xlnx-zynqmp.h +34
-0@@ -64,6 +64,7 @@enum pm_api_id {PM_GET_API_VERSION =1 , +PM_GET_NODE_STATUS =3 ,PM_FORCE_POWERDOWN =8 ,PM_REQUEST_WAKEUP =10 ,PM_SYSTEM_SHUTDOWN =12 ,@@ -384,6 +385,14 @@ int zynqmp_pm_request_wakeup(const u32 node,const bool set_addr, const u64 address, const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_get_node_status (const u32 node,u32 *status, +u32 *requirements,u32 *usage); +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out);#else static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops (void ) {@@ -548,6 +557,31 @@ static inline int zynqmp_pm_request_wakeup(const u32 node, {return -ENODEV ; } + +static inline int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */
RE: [PATCH v8 3/5] firmware: xilinx: Add RPU configuration APIs
Quoted 14 lines
-----Original Message-----
From: Stefano Stabellini <[email protected]>
Sent: Thursday, August 13, 2020 1:36 PM
To: Ben Levinsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: Re: [PATCH v8 3/5] firmware: xilinx: Add RPU configuration APIs
Quoted 6 lines
Quoted 2 lines · Ben Levinsky
This patch adds APIs to provide access and a configuration interface to the current power state of a sub-system on Zynqmp sub-system.
This doesn't read correctly
[Ben Levinsky] ok will update commit message as follows in v9:
firmware: xilinx: Add RPU configuration APIs
This 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).
Quoted 22 lines
Quoted 8 lines
Signed-off-by Ben Levinsky <[email protected]>---
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around function pointer in xilinx eemi ops struct
v4:
- add default values for enumsReview
Quoted 6 lines
--- drivers/firmware/xilinx/zynqmp.c | 99 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 34 ++++++++++ 2 files changed, 133 insertions(+)(quoted diff) +0
-0
b/drivers/firmware/xilinx/zynqmp.cQuoted 7 lines
index
f1a0bd35deeb..fdd61d258e55100644drivers/firmware/xilinx/zynqmp.c +0
-0@@ -846,6 +846,61 @@ int zynqmp_pm_release_node(const u32 node) }EXPORT_SYMBOL_GPL (zynqmp_pm_release_node);
Quoted 11 lines
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 856
Quoted 8 lines
drivers/firmware/xilinx/zynqmp.c +8
-0+/** +* zynqmp_pm_get_rpu_mode() - Get RPU mode +* @node_id: Node ID of the device +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* Return: RPU mode What kind of output are we expecting? An enum? Which one?
[Ben Levinsky] will update the return comment. Return: RPU mode. This is enum rpu_oper_mode
Quoted 29 lines
Quoted 29 lines
drivers/firmware/xilinx/zynqmp.c +28
-0+*/ +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_GET_RPU_OPER_MODE ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_rpu_mode); + +/** +* zynqmp_pm_set_rpu_mode() - Set RPU mode +* @node_id: Node ID of the device +* @ioctl_id: ID of the requested IOCTL +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_SET_RPU_OPER_MODE ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_rpu_mode); +
Quoted 9 lines
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 890
Quoted 6 lines
drivers/firmware/xilinx/zynqmp.c +6
-0+/** +* zynqmp_pm_tcm_comb_config - configure TCM +* @node_id: Node ID of the device +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value Same question on the type of the output value
[Ben Levinsky] will update in v9: Returns status: 0 for success, else failure
Quoted 41 lines
Quoted 5 lines
drivers/firmware/xilinx/zynqmp.c +4
-0+* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node)
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +8
-0+int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_TCM_COMB_CONFIG ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_tcm_config); +/** * zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_request_wakeup(const u32 node, · line 940
Quoted 7 lines
drivers/firmware/xilinx/zynqmp.c +2
-0* be powered down forcefully @@ -881,6 +936,50 @@ int zynqmp_pm_request_wakeup(const u32 node, }EXPORT_SYMBOL_GPL (zynqmp_pm_request_wakeup); +/** +* zynqmp_pm_get_node_status - PM call to request a node's current power state
Quoted 6 lines
drivers/firmware/xilinx/zynqmp.c +6
-0+* @node: ID of the component or sub-system in question +* @status: Current operating state of the requested node +* @requirements: Current requirements asserted on the node, +* used for slave nodes only. +* @usage: Usage information, used for slave nodes only: +* PM_USAGE_NO_MASTER - No master is currently using drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_request_wakeup(const u32 node, · line 956
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +10
-0+* the node +* PM_USAGE_CURRENT_MASTER - Only requesting master is +* currently using the node +* PM_USAGE_OTHER_MASTER - Only other masters are +* currently using the node +* PM_USAGE_BOTH_MASTERS - Both the current and at least +* one other master is currently +* using the node +* +* Return: status, either success or error+reason
Quoted 2 lines
Same question on status
[Ben Levinsky] this is now not called so I can remove this function in v9.
Quoted 92 lines
Quoted 19 lines
drivers/firmware/xilinx/zynqmp.c +18
-0+*/ +int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) + +{ +u32 ret_payload[PAYLOAD_ARG_CNT ]; +int ret; + +if (!status) +return -EINVAL ; + + ret =zynqmp_pm_invoke_fn (PM_GET_NODE_STATUS , node,0 ,0 ,0 , + ret_payload); +if (ret_payload[0 ] ==XST_PM_SUCCESS ) { + *status = ret_payload[1 ]; +if (requirements) + *requirements = ret_payload[2 ];drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_request_wakeup(const u32 node,
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +8
-0+if (usage) + *usage = ret_payload[3 ]; + } + +return ret; +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_node_status); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves
Review
Quoted 2 lines
drivers/firmware/xilinx/zynqmp.c +0
-0* @node: Node ID of the slave (quoted diff) +0
-0
b/include/linux/firmware/xlnx-zynqmp.hinclude/linux/firmware/xlnx-zynqmp.h static inline int zynqmp_pm_request_wakeup(const
Quoted 27 lines
index
4d83a7d69c4c..0caca9e2223a100644include/linux/firmware/xlnx-zynqmp.h +9
-0@@ -64,6 +64,7 @@enum pm_api_id {PM_GET_API_VERSION =1 , +PM_GET_NODE_STATUS =3 ,PM_FORCE_POWERDOWN =8 ,PM_REQUEST_WAKEUP =10 ,PM_SYSTEM_SHUTDOWN =12 ,@@ -384,6 +385,14 @@ int zynqmp_pm_request_wakeup(const u32 node,const bool set_addr, const u64 address, const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_get_node_status (const u32 node,u32 *status, +u32 *requirements,u32 *usage); +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out);#else static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops (void ) {@@ -548,6 +557,31 @@ static inline int zynqmp_pm_request_wakeup(constu32 node,
Quoted 31 lines
include/linux/firmware/xlnx-zynqmp.h +25
-0{return -ENODEV ; } + +static inline int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */
Re: [PATCH v8 3/5] firmware: xilinx: Add RPU configuration APIs
In reply to Ben LevinskyHey Ben,
Quoted 46 lines · Ben Levinsky
This patch adds APIs to provide access and a configuration interface to the current power state of a sub-system on Zynqmp sub-system.
Signed-off-by Ben Levinsky <[email protected]>---
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--- drivers/firmware/xilinx/zynqmp.c | 99 ++++++++++++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 34 ++++++++++ 2 files changed, 133 insertions(+)drivers/firmware/xilinx/zynqmp.c +23
-0@@ -846,6 +846,61 @@ 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 +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* Return: RPU mode +*/ +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_GET_RPU_OPER_MODE ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_rpu_mode); + +/** +* zynqmp_pm_set_rpu_mode() - Set RPU mode +* @node_id: Node ID of the device +* @ioctl_id: ID of the requested IOCTL +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value
drivers/firmware/xilinx/zynqmp.c int zynqmp_pm_release_node(const u32 node) · line 881
Quoted 10 lines
drivers/firmware/xilinx/zynqmp.c +10
-0+* +* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_SET_RPU_OPER_MODE ,0 ,0 , out);
This should pass through arg1 instead of 0 for the argument after IOCTL_SET_RPU_OPER_MODE. As this is, the RPU will always be configured in lockstep mode regardless of a request to switch to split mode.
Also, the kerneldoc for these functions could use some love (ioctl_id no longer exists, arg1 isn't documented, arg2 is documented by very non-specifically).
Quoted 138 lines
drivers/firmware/xilinx/zynqmp.c +66
-0+} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_rpu_mode); + +/** +* zynqmp_pm_tcm_comb_config - configure TCM +* @node_id: Node ID of the device +* @arg1: Argument 1 to requested IOCTL call +* @arg2: Argument 2 to requested IOCTL call +* @out: Returned output value +* +* This function is used to set RPU mode. +* +* Return: Returns status, either success or error+reason +*/ +int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return zynqmp_pm_invoke_fn (PM_IOCTL , node_id, +IOCTL_TCM_COMB_CONFIG ,0 ,0 , out); +} +EXPORT_SYMBOL_GPL (zynqmp_pm_set_tcm_config); +/** * zynqmp_pm_force_powerdown - PM call to request for another PU or subsystem to * be powered down forcefully @@ -881,6 +936,50 @@ int zynqmp_pm_request_wakeup(const u32 node, }EXPORT_SYMBOL_GPL (zynqmp_pm_request_wakeup); +/** +* zynqmp_pm_get_node_status - PM call to request a node's current power state +* @node: ID of the component or sub-system in question +* @status: Current operating state of the requested node +* @requirements: Current requirements asserted on the node, +* used for slave nodes only. +* @usage: Usage information, used for slave nodes only: +* PM_USAGE_NO_MASTER - No master is currently using +* the node +* PM_USAGE_CURRENT_MASTER - Only requesting master is +* currently using the node +* PM_USAGE_OTHER_MASTER - Only other masters are +* currently using the node +* PM_USAGE_BOTH_MASTERS - Both the current and at least +* one other master is currently +* using the node +* +* Return: status, either success or error+reason +*/ +int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) + +{ +u32 ret_payload[PAYLOAD_ARG_CNT ]; +int ret; + +if (!status) +return -EINVAL ; + + ret =zynqmp_pm_invoke_fn (PM_GET_NODE_STATUS , node,0 ,0 ,0 , + ret_payload); +if (ret_payload[0 ] ==XST_PM_SUCCESS ) { + *status = ret_payload[1 ]; +if (requirements) + *requirements = ret_payload[2 ]; +if (usage) + *usage = ret_payload[3 ]; + } + +return ret; +} +EXPORT_SYMBOL_GPL (zynqmp_pm_get_node_status); +/** * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves * @node: Node ID of the slave include/linux/firmware/xlnx-zynqmp.h +34
-0@@ -64,6 +64,7 @@enum pm_api_id {PM_GET_API_VERSION =1 , +PM_GET_NODE_STATUS =3 ,PM_FORCE_POWERDOWN =8 ,PM_REQUEST_WAKEUP =10 ,PM_SYSTEM_SHUTDOWN =12 ,@@ -384,6 +385,14 @@ int zynqmp_pm_request_wakeup(const u32 node,const bool set_addr, const u64 address, const enum zynqmp_pm_request_ack ack); +int zynqmp_pm_get_node_status (const u32 node,u32 *status, +u32 *requirements,u32 *usage); +int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out); +int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out);#else static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops (void ) {@@ -548,6 +557,31 @@ static inline int zynqmp_pm_request_wakeup(const u32 node, {return -ENODEV ; } + +static inline int zynqmp_pm_get_node_status (const u32 node, +u32 *status,u32 *requirements, +u32 *usage) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_get_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_rpu_mode (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +} + +static inline int zynqmp_pm_set_tcm_config (u32 node_id, +u32 arg1,u32 arg2,u32 *out) +{ +return -ENODEV ; +}#endif #endif /* __FIRMWARE_ZYNQMP_H__ */ Signature · 1 line
2.17.1
Thanks,
Michael
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v8 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
--- .../xilinx,zynqmp-r5-remoteproc.yaml | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml new file +73 -0
@@ -0,0 +1,73 @@ +# 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 + 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>; + }; + }; + rpu { + compatible = "xlnx,zynqmp-r5-remoteproc-1.0"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + lockstep-mode = <0>; + r5_0 { + ranges; + #address-cells = <1>; + #size-cells = <1>; + memory-region = <&elf_load>; + pnode-id = <0x7>; + }; + }; + +...
Signature · 1 line
2.17.1
PATCH v8 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 configurations.
---
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
--- drivers/remoteproc/Kconfig | 10 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/zynqmp_r5_remoteproc.c | 911 ++++++++++++++++++++++ 3 files changed, 922 insertions(+) create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
drivers/remoteproc/Kconfig +10 -0
@@ -249,6 +249,16 @@ config STM32_RPROC This can be either built-in or a loadable module. +config ZYNQMP_R5_REMOTEPROC + tristate "ZynqMP_R5 remoteproc support" + depends on ARM64 && PM && ARCH_ZYNQMP + select RPMSG_VIRTIO + select MAILBOX + select ZYNQMP_IPI_MBOX + help + Say y here to support ZynqMP R5 remote processors via the remote + processor framework. + endif # REMOTEPROC endmenu
drivers/remoteproc/Makefile +1 -0
@@ -28,5 +28,6 @@ obj-$(CONFIG_QCOM_WCNSS_PIL) += qcom_wcnss_pil.o qcom_wcnss_pil-y += qcom_wcnss.o qcom_wcnss_pil-y += qcom_wcnss_iris.o obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +911 -0
911 lines not on this page · raw message
Signature · 1 line
2.17.1
Re: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Quoted 3 lines · Ben Levinsky
R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this remotproc driver, we can boot the R5 sub-system in different configurations.
Which different configurations? How can you boot them?
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 88 lines
Signed-off-by Ben Levinsky <[email protected]>Signed-off-by Wendy Liang <[email protected]>Signed-off-by Michal Simek <[email protected]>Signed-off-by Ed Mooring <[email protected]>Signed-off-by Jason Wu <[email protected]>---
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_fwchange from use ofrproc_of_resm_mem_entry_inittorproc_mem_entry_initand use of alloc/release
- var's of typezynqmp_r5_pdataall have same local variable name
- usedev_dbginstead ofdev_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-> u32lockstep_mode;
- change device-tree binding "lockstep-mode" to xlnx,cluster-mode
- removezynqmp_r5_mem_probeand loop to Probe R5 memory devices at remoteproc-probe time
- removeis_r5_mode_setfrom zynqmp rpu remote processor private data
- do not error out if no mailbox is provided
- removezynqmp_r5_remoteproc_probecall ofplatform_set_drvdataas pdata is
handled inzynqmp_r5_remoteproc_remove
v8:
- remove old acks, reviewed-by's in commit message--- drivers/remoteproc/Kconfig | 10 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/zynqmp_r5_remoteproc.c | 911 ++++++++++++++++++++++ 3 files changed, 922 insertions(+) create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.cdrivers/remoteproc/Kconfig +10
-0@@ -249,6 +249,16 @@ config STM32_RPROC This can be either built-in or a loadable module. +config ZYNQMP_R5_REMOTEPROC + tristate "ZynqMP_R5 remoteproc support" + depends on ARM64 && PM && ARCH_ZYNQMP + select RPMSG_VIRTIO + select MAILBOX + select ZYNQMP_IPI_MBOX + help + Say y here to support ZynqMP R5 remote processors via the remote + processor framework. + endif # REMOTEPROC endmenudrivers/remoteproc/Makefile +1
-0@@ -28,5 +28,6 @@ obj-$(CONFIG_QCOM_WCNSS_PIL) += qcom_wcnss_pil.o qcom_wcnss_pil-y += qcom_wcnss.o qcom_wcnss_pil-y += qcom_wcnss_iris.o obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o obj-$(CONFIG_STM32_RPROC) += stm32_rproc.odrivers/remoteproc/zynqmp_r5_remoteproc.c new file +0
-0
I tried to build this but I get 4 warnings:
drivers/remoteproc/zynqmp_r5_remoteproc.c: In function 'handle_tcm_parsing':drivers/remoteproc/zynqmp_r5_remoteproc.c:286:10: warning: return makes pointer from integer without a cast [-Wint-conversion]
return -EINVAL;
^
drivers/remoteproc/zynqmp_r5_remoteproc.c:293:10: warning: return makes pointer from integer without a cast [-Wint-conversion]
return -EINVAL;
^
drivers/remoteproc/zynqmp_r5_remoteproc.c:298:10: warning: return makes pointer from integer without a cast [-Wint-conversion]
return -ENOMEM;
^drivers/remoteproc/zynqmp_r5_remoteproc.c:317:10: warning: return makes pointer from integer without a cast [-Wint-conversion]
return -ENOMEM;
Please fix all warnings before submitting to the list.
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0@@ -0,0 +1,911 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Zynq R5 Remote Processor driver +*
drivers/remoteproc/zynqmp_r5_remoteproc.c line 14
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+* Copyright (C) 2019, 2020 Xilinx Inc. Ben Levinsky <[email protected]> +* Copyright (C) 2015 - 2018 Xilinx Inc. +* Copyright (C) 2015 Jason Wu <[email protected]> +* +* Based on origin OMAP and Zynq Remote Processor driver +* +* Copyright (C) 2012 Michal Simek <[email protected]> +* Copyright (C) 2012 PetaLogix +* Copyright (C) 2011 Texas Instruments, Inc. +* Copyright (C) 2011 Google, Inc.
In my opinion 8 copyright lines are too many. As you probably know, they aren't actually necessary or useful (the SPDX line and git log are the only things that matter). I would remove them all.
drivers/remoteproc/zynqmp_r5_remoteproc.c line 17
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+*/ + +#include <linux/atomic.h>
I just looked at the first #include and noticed that it is not actually used. Please make sure that all headers included are necessary. (You can try to remove them one by one to test if necessary.)
Quoted 41 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +41
-0+#include <linux/cpu.h> +#include <linux/dma-mapping.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/firmware/xlnx-zynqmp.h> +#include <linux/genalloc.h> +#include <linux/idr.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/mailbox_client.h> +#include <linux/mailbox/zynqmp-ipi-message.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/of_reserved_mem.h> +#include <linux/pfn.h> +#include <linux/platform_device.h> +#include <linux/remoteproc.h> +#include <linux/skbuff.h> +#include <linux/slab.h> +#include <linux/sysfs.h> + +#include "remoteproc_internal.h" + +#define MAX_RPROCS 2/* Support up to 2 RPU */ +#define MAX_MEM_PNODES 4/* Max power nodes for one RPU memory instance */ + +#define DEFAULT_FIRMWARE_NAME "rproc-rpu-fw" + +/* PM proc states */ +#define PM_PROC_STATE_ACTIVE 1U + +/* IPI buffer MAX length */ +#define IPI_BUF_LEN_MAX 32U +/* RX mailbox client buffer max length */ +#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \ + sizeof(struct zynqmp_ipi_message)) + +#define ZYNQMP_R5_NUM_TCM_BANKS 4
drivers/remoteproc/zynqmp_r5_remoteproc.c line 68
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ +/* lookup table mapping power-node-ID of TCM bank to absolute base address */ +static unsigned long tcm_addr_to_pnode[ZYNQMP_R5_NUM_TCM_BANKS ][2 ] = { + {0xFFE00000 ,0xF }, + {0xFFE20000 ,0x10 }, + {0xFFE90000 ,0x10 }, + {0xFFEB0000 ,0x11 }, +}; + +static bool autoboot__read_mostly ;
Please add a description of this parameter. What does it do? Why would anybody enable it?
Quoted 30 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +30
-0+/** +* struct zynqmp_r5_mem - zynqmp rpu memory data +* @pnode_id: TCM power domain ids +* @res: memory resource +* @node: list node +*/ +struct zynqmp_r5_mem { +u32 pnode_id[MAX_MEM_PNODES ]; +struct resource res; +struct list_head node; +}; + +/** +* struct zynqmp_r5_pdata - zynqmp rpu remote processor private data +* @dev: device of RPU instance +* @rproc: rproc handle +* @pnode_id: RPU CPU power domain id +* @mems: memory resources +* @is_r5_mode_set: indicate if r5 operation mode is set +* @tx_mc: tx mailbox client +* @rx_mc: rx mailbox client +* @tx_chan: tx mailbox channel +* @rx_chan: rx mailbox channel +* @mbox_work: mbox_work for the RPU remoteproc +* @tx_mc_skbs: socket buffers for tx mailbox client +* @rx_mc_buf: rx mailbox client buffer to save the rx message +*/ +struct zynqmp_r5_pdata { +struct device dev; +struct rproc *rproc;
drivers/remoteproc/zynqmp_r5_remoteproc.c line 109
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+u32 pnode_id; +struct list_head mems; +bool is_r5_mode_set; +struct mbox_client tx_mc; +struct mbox_client rx_mc; +struct mbox_chan *tx_chan; +struct mbox_chan *rx_chan; +struct work_struct mbox_work; +struct sk_buff_head tx_mc_skbs; +unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX ];
A simple small reordering of the struct fields would lead to small memory savings due to alignment.
drivers/remoteproc/zynqmp_r5_remoteproc.c line 117
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+}; + +/** +* table of RPUs +*/ +struct zynqmp_r5_pdata rpus[MAX_RPROCS ]; +/** +* RPU core configuration
^ spurious white space
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +18
-0+*/ +enum rpu_oper_mode rpu_mode; + +/* +* r5_set_mode - set RPU operation mode +* @pdata: Remote processor private data +* +* set RPU oepration mode +* +* Return: 0 for success, negative value for failure +*/ +static int r5_set_mode (struct zynqmp_r5_pdata *pdata) +{ +u32 val[PAYLOAD_ARG_CNT ] = {0 }, expect, tcm_mode; +struct device *dev = &pdata->dev; +int ret; + + expect = (u32 )rpu_mode;
drivers/remoteproc/zynqmp_r5_remoteproc.c line 145
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ ret =zynqmp_pm_get_rpu_mode (pdata->pnode_id,0 ,0 , val); +if (ret <0 ) { +dev_err (dev,"failed to get RPU oper mode.\n" ); +return ret; + } +if (val[0 ] == expect) { +dev_dbg (dev,"RPU mode matches: %x\n" , val[0 ]); + }else { + ret =zynqmp_pm_set_rpu_mode (pdata->pnode_id, + expect,0 , val);
It looks like zynqmp_pm_set_rpu_mode is actually ignoring the second argument, so expect is not correctly handled?
drivers/remoteproc/zynqmp_r5_remoteproc.c line 155
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+if (ret <0 ) { +dev_err (dev, +"failed to set RPU oper mode.\n" ); +return ret; + } + } + + tcm_mode = (expect == (u32 )PM_RPU_MODE_LOCKSTEP ) ? +PM_RPU_TCM_COMB :PM_RPU_TCM_SPLIT ; + ret =zynqmp_pm_set_tcm_config (pdata->pnode_id, tcm_mode,0 , val);
Similarly, it looks like the second argument to zynqmp_pm_set_tcm_config is actually ignored by the implementation of the function. So tcm_mode here is not correctly handled by zynqmp_pm_set_tcm_config.
Also val has not been zeroed from the previous call.
Quoted 137 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +137
-0+if (ret <0 ) { +dev_err (dev,"failed to config TCM to %x.\n" , + expect); +return ret; + } + pdata->is_r5_mode_set = true; +return 0 ; +} + +/* +* ZynqMP R5 remoteproc memory release function +*/ +static int zynqmp_r5_mem_release (struct rproc *rproc, +struct rproc_mem_entry *mem) +{ +struct zynqmp_r5_mem *priv; +int i, ret; +struct device *dev = &rproc->dev; + + priv = mem->priv; +if (!priv) +return 0 ; +for (i =0 ; i <MAX_MEM_PNODES ; i++) { +if (priv->pnode_id[i]) { +dev_dbg (dev,"%s, pnode %d\n" , + __func__, priv->pnode_id[i]); + ret =zynqmp_pm_release_node (priv->pnode_id[i]); +if (ret <0 ) { +dev_err (dev, +"failed to release power node: %u\n" , + priv->pnode_id[i]); +return ret; + } + }else { +break ; + } + } +return 0 ; +} + +/* +* ZynqMP R5 remoteproc operations +*/ +static int zynqmp_r5_rproc_start (struct rproc *rproc) +{ +struct device *dev = rproc->dev.parent; +struct zynqmp_r5_pdata *pdata = rproc->priv; +enum rpu_boot_mem bootmem; +int ret; + +if ((rproc->bootaddr &0xF0000000 ) ==0xF0000000 ) + bootmem =PM_RPU_BOOTMEM_HIVEC ; +else + bootmem =PM_RPU_BOOTMEM_LOVEC ; +dev_dbg (dev,"RPU boot from %s." , + bootmem ==PM_RPU_BOOTMEM_HIVEC ?"OCM" :"TCM" ); + ret =zynqmp_pm_request_wakeup (pdata->pnode_id,1 , + bootmem,ZYNQMP_PM_REQUEST_ACK_NO ); +if (ret <0 ) { +dev_err (dev,"failed to boot R5.\n" ); +return ret; + } +return 0 ; +} + +static int zynqmp_r5_rproc_stop (struct rproc *rproc) +{ +struct zynqmp_r5_pdata *pdata = rproc->priv; +int ret; + + ret =zynqmp_pm_force_powerdown (pdata->pnode_id, +ZYNQMP_PM_REQUEST_ACK_BLOCKING ); +if (ret <0 ) { +dev_err (&pdata->dev,"failed to shutdown R5.\n" ); +return ret; + } +return 0 ; +} + +static int zynqmp_r5_rproc_mem_alloc (struct rproc *rproc, +struct rproc_mem_entry *mem) +{ +struct device *dev = rproc->dev.parent; +void *va; + +dev_dbg (rproc->dev.parent,"map memory: %pa\n" , &mem->dma); + va =ioremap_wc (mem->dma, mem->len); +if (IS_ERR_OR_NULL (va)) { +dev_err (dev,"Unable to map memory region: %pa+%lx\n" , + &mem->dma, mem->len); +return -ENOMEM ; + } + +/* Update memory entry va */ + mem->va = va; +return 0 ; +} + +static int zynqmp_r5_rproc_mem_release (struct rproc *rproc, +struct rproc_mem_entry *mem) +{ +dev_dbg (rproc->dev.parent,"unmap memory: %pa\n" , &mem->dma); +iounmap (mem->va); + +return 0 ; +} + +/* +* TCM needs mapping to R5 relative address and xilinx platform mgmt call +*/ +struct rproc_mem_entry *handle_tcm_parsing (struct device *dev, +struct reserved_mem *rmem, +struct device_node *node, +int lookup_idx) +{ +void *va; +dma_addr_t dma; +resource_size_t size; +int ret; +u32 pnode_id; +struct resource rsc; +struct rproc_mem_entry *mem; + + pnode_id = tcm_addr_to_pnode[lookup_idx][1 ]; + ret =zynqmp_pm_request_node (pnode_id, +ZYNQMP_PM_CAPABILITY_ACCESS ,0 , +ZYNQMP_PM_REQUEST_ACK_BLOCKING ); +if (ret <0 ) { +dev_err (dev,"failed to request power node: %u\n" , pnode_id); +return -EINVAL ; + } + + ret =of_address_to_resource (node,0 , &rsc); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ;
drivers/remoteproc/zynqmp_r5_remoteproc.c line 303
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ } + size =resource_size (&rsc); + va =devm_ioremap_wc (dev, rsc.start, size); +if (!va) +return -ENOMEM ; + +/* zero out tcm base address */ +if (rsc.start &0xffe00000 ) { +/* R5 can't see anything past 0xfffff so wipe it */ + rsc.start &=0x000fffff ;
If that is the case why not do:
rsc.start &= 0x000fffff;
unconditionally? if (rsc.start & 0xffe00000) is superfluous.
But I thought that actually the R5s could see TCM at both the low address (< 0x000fffff) and also at the high address (i.e. 0xffe00000).
drivers/remoteproc/zynqmp_r5_remoteproc.c line 309
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+/* +* handle tcm banks 1 a and b (0xffe9000 and +* 0xffeb0000) +*/ +if (rsc.start &0x80000 ) + rsc.start -=0x90000 ;
It is very unclear to me why we have to do this
drivers/remoteproc/zynqmp_r5_remoteproc.c line 312
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+ } + + dma = (dma_addr_t )rsc.start;
Given the way the dma parameter is used by rproc_alloc_registered_carveouts, I think it might be best to pass the original start address (i.e. 0xffe00000) as dma.
drivers/remoteproc/zynqmp_r5_remoteproc.c line 314
Quoted 2 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +2
-0+ mem = rproc_mem_entry_init(dev, va, dma, (int)size, rsc.start, + NULL, zynqmp_r5_mem_release,
I don't know too much about the remoteproc APIs, but shouldn't you be passing zynqmp_r5_rproc_mem_alloc to it instead of NULL?
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +13
-0+ rsc.name); +if (!mem) +return -ENOMEM ; + +return mem; +} + +static int parse_mem_regions (struct rproc *rproc) +{ +int num_mems, i; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; +struct device_node *np = dev->of_node;
drivers/remoteproc/zynqmp_r5_remoteproc.c line 337
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+struct rproc_mem_entry *mem; + + num_mems =of_count_phandle_with_args (np,"memory-region" ,NULL ); +if (num_mems <=0 ) +return 0 ; +for (i =0 ; i < num_mems; i++) { +struct device_node *node; +struct reserved_mem *rmem; + + node =of_parse_phandle (np,"memory-region" , i);
Check node != NULL ?
drivers/remoteproc/zynqmp_r5_remoteproc.c line 344
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+ rmem =of_reserved_mem_lookup (node); +if (!rmem) { +dev_err (dev,"unable to acquire memory-region\n" ); +return -EINVAL ; + } + +if (strstr (node->name,"vdev0buffer" )) {
vdev0buffer is not described in the device tree bindings, is that normal/expected?
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+/* Register DMA region */ + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem->base, + rmem->size, rmem->base,
drivers/remoteproc/zynqmp_r5_remoteproc.c line 358
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+NULL ,NULL , + "vdev0buffer"); +if (!mem) { +dev_err (dev,"unable to initialize memory-region %s\n" , + node->name); +return -ENOMEM ; + } +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma); + }else if (strstr (node->name,"vdev0vring" )) {
Same here
drivers/remoteproc/zynqmp_r5_remoteproc.c line 366
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+int vring_id; +char name[16 ]; + +/* +* can be 1 of multiple vring IDs per IPC channel +* e.g. 'vdev0vring0' and 'vdev0vring1' +*/ + vring_id = node->name[14 ] -'0' ;
Where does the "14" comes from? Are we sure it is not possible to have a node->name smaller than 14 chars?
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+snprintf (name,sizeof (name),"vdev0vring%d" , vring_id); +/* Register vring */ + mem =rproc_mem_entry_init (dev,NULL , + (dma_addr_t )rmem->base, + rmem->size, rmem->base, + zynqmp_r5_rproc_mem_alloc, + zynqmp_r5_rproc_mem_release, + name); +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma);
drivers/remoteproc/zynqmp_r5_remoteproc.c line 386
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ }else { +int idx; + +/* +* if TCM update address space for R5 and +* make xilinx platform mgmt call +*/ +for (idx =0 ; idx <ZYNQMP_R5_NUM_TCM_BANKS ; idx++) { +if (tcm_addr_to_pnode[idx][0 ] == rmem->base) +break ;
There is something I don't quite understand. If the memory region to use is TCM, why would it be also described under reserved-memory? Reserved-memory is for normal memory being reserved, while TCM is not normal memory. Am I missing something?
drivers/remoteproc/zynqmp_r5_remoteproc.c line 397
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +11
-0+ } + +if (idx !=ZYNQMP_R5_NUM_TCM_BANKS ) { + mem =handle_tcm_parsing (dev, rmem, node, idx); + }else { + mem =rproc_mem_entry_init (dev,NULL , + (dma_addr_t )rmem->base, + rmem->size, rmem->base, + zynqmp_r5_rproc_mem_alloc, + zynqmp_r5_rproc_mem_release, + node->name);
This case looks identical to the vdev0vring above. Is the difference really just in the "name"? If so, can we merge the two cases into one?
Quoted 21 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +21
-0+ } + +if (!mem) { +dev_err (dev, +"unable to init memory-region %s\n" , + node->name); +return -ENOMEM ; + } + } +rproc_add_carveout (rproc, mem); + } + +return 0 ; +} + +static int zynqmp_r5_parse_fw (struct rproc *rproc,const struct firmware *fw) +{ +int ret; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; +
drivers/remoteproc/zynqmp_r5_remoteproc.c line 428
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ ret =parse_mem_regions (rproc); +if (ret) { +dev_err (dev,"parse_mem_regions failed %x\n" , ret); +return ret; + } + + ret =rproc_elf_load_rsc_table (rproc, fw); +if (ret == -EINVAL ) { +dev_info (dev,"no resource table found.\n" ); + ret =0 ;
Why do we want to continue ignoring the error in this case?
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +13
-0+ } +return ret; +} + +/* kick a firmware */ +static void zynqmp_r5_rproc_kick (struct rproc *rproc,int vqid) +{ +struct device *dev = rproc->dev.parent; +struct zynqmp_r5_pdata *pdata = rproc->priv; + +dev_dbg (dev,"KICK Firmware to start send messages vqid %d\n" , vqid); + +if (vqid <0 ) {
drivers/remoteproc/zynqmp_r5_remoteproc.c line 451
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+/* If vqid is negative, does not pass the vqid to +* mailbox. As vqid is supposed to be 0 or possive. +* It also gives a way to just kick instead but +* not use the IPI buffer. It is better to provide +* a proper way to pass the short message, which will +* need to sync to upstream first, for now, +* use negative vqid to assume no message will be +* passed with IPI buffer, but just raise interrupt. +* This will be faster as it doesn't need to copy the +* message to the IPI buffer.
I take that by "upstream" you mean the upstream Linux kernel community? Did the conversation happen?
Quoted 40 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +40
-0+* It will ignore the return, as failure is due to +* there already kicks in the mailbox queue. +*/ + (void )mbox_send_message (pdata->tx_chan,NULL ); + }else { +struct sk_buff *skb; +unsigned int skb_len; +struct zynqmp_ipi_message *mb_msg; +int ret; + + skb_len = (unsigned int )(sizeof (vqid) +sizeof (mb_msg)); + skb =alloc_skb (skb_len,GFP_ATOMIC ); +if (!skb) { +dev_err (dev, +"Failed to allocate skb to kick remote.\n" ); +return ; + } + mb_msg = (struct zynqmp_ipi_message *)skb_put (skb, skb_len); + mb_msg->len =sizeof (vqid); +memcpy (mb_msg->data, &vqid,sizeof (vqid)); +skb_queue_tail (&pdata->tx_mc_skbs, skb); + ret =mbox_send_message (pdata->tx_chan, mb_msg); +if (ret <0 ) { +dev_warn (dev,"Failed to kick remote.\n" ); +skb_dequeue_tail (&pdata->tx_mc_skbs); +kfree_skb (skb); + } + } +} + +static struct rproc_ops zynqmp_r5_rproc_ops = { + .start = zynqmp_r5_rproc_start, + .stop = zynqmp_r5_rproc_stop, + .load = rproc_elf_load_segments, + .parse_fw = zynqmp_r5_parse_fw, + .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table, + .sanity_check = rproc_elf_sanity_check, + .get_boot_addr = rproc_elf_get_boot_addr, + .kick = zynqmp_r5_rproc_kick, +};
drivers/remoteproc/zynqmp_r5_remoteproc.c line 502
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ +/* zynqmp_r5_mem_probe() - probes RPU TCM memory device +* @pdata: pointer to the RPU remoteproc private data +* @node: pointer to the memory node +* +* Function to retrieve resources for RPU TCM memory device. +*/ +static int zynqmp_r5_mem_probe (struct zynqmp_r5_pdata *pdata, +struct device_node *node) +{
TCM nodes as children of the zynqmp_r5_remoteproc node are not described in device tree, hence, they should not be parsed here.
Quoted 90 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +90
-0+struct device *dev; +struct zynqmp_r5_mem *mem; +int ret; +struct property *prop; +const __be32 *cur; +u32 val; +int i; + + dev = &pdata->dev; + mem =devm_kzalloc (dev,sizeof (*mem),GFP_KERNEL ); +if (!mem) +return -ENOMEM ; + ret =of_address_to_resource (node,0 , &mem->res); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ; + } + +/* Get the power domain id */ + i =0 ; +if (of_find_property (node,"pnode-id" ,NULL )) { +of_property_for_each_u32 (node,"pnode-id" , prop, cur, val) + mem->pnode_id[i++] = val; + } +list_add_tail (&mem->node, &pdata->mems); +return 0 ; +} + +/** +* zynqmp_r5_release() - ZynqMP R5 device release function +* @dev: pointer to the device struct of ZynqMP R5 +* +* Function to release ZynqMP R5 device. +*/ +static void zynqmp_r5_release (struct device *dev) +{ +struct zynqmp_r5_pdata *pdata; +struct rproc *rproc; +struct sk_buff *skb; + + pdata =dev_get_drvdata (dev); + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + } +if (pdata->tx_chan) +mbox_free_channel (pdata->tx_chan); +if (pdata->rx_chan) +mbox_free_channel (pdata->rx_chan); +/* Discard all SKBs */ +while (!skb_queue_empty (&pdata->tx_mc_skbs)) { + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); + } + +put_device (dev->parent); +} + +/** +* event_notified_idr_cb() - event notified idr callback +* @id: idr id +* @ptr: pointer to idr private data +* @data: data passed to idr_for_each callback +* +* Pass notification to remoteproc virtio +* +* Return: 0. having return is to satisfy the idr_for_each() function +* pointer input argument requirement. +**/ +static int event_notified_idr_cb (int id,void *ptr,void *data) +{ +struct rproc *rproc = data; + + (void )rproc_vq_interrupt (rproc, id); +return 0 ; +} + +/** +* handle_event_notified() - remoteproc notification work funciton +* @work: pointer to the work structure +* +* It checks each registered remoteproc notify IDs. +*/ +static void handle_event_notified (struct work_struct *work) +{ +struct rproc *rproc; +struct zynqmp_r5_pdata *pdata; +
drivers/remoteproc/zynqmp_r5_remoteproc.c line 602
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ pdata = container_of(work, struct zynqmp_r5_pdata, mbox_work); + + (void )mbox_send_message (pdata->rx_chan,NULL ); + rproc = pdata->rproc; +/* +* We only use IPI for interrupt. The firmware side may or may +* not write the notifyid when it trigger IPI. +* And thus, we scan through all the registered notifyids. +*/ +idr_for_each (&rproc->notifyids, event_notified_idr_cb, rproc);
This looks expensive. Should we at least check whether the notifyid was written as first thing before doing the scan?
Quoted 137 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +137
-0+} + +/** +* zynqmp_r5_mb_rx_cb() - Receive channel mailbox callback +* @cl: mailbox client +* @mssg: message pointer +* +* It will schedule the R5 notification work. +*/ +static void zynqmp_r5_mb_rx_cb (struct mbox_client *cl,void *mssg) +{ +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(cl, struct zynqmp_r5_pdata, rx_mc); +if (mssg) { +struct zynqmp_ipi_message *ipi_msg, *buf_msg; +size_t len; + + ipi_msg = (struct zynqmp_ipi_message *)mssg; + buf_msg = (struct zynqmp_ipi_message *)pdata->rx_mc_buf; + len = (ipi_msg->len >=IPI_BUF_LEN_MAX ) ? +IPI_BUF_LEN_MAX : ipi_msg->len; + buf_msg->len = len; +memcpy (buf_msg->data, ipi_msg->data, len); + } +schedule_work (&pdata->mbox_work); +} + +/** +* zynqmp_r5_mb_tx_done() - Request has been sent to the remote +* @cl: mailbox client +* @mssg: pointer to the message which has been sent +* @r: status of last TX - OK or error +* +* It will be called by the mailbox framework when the last TX has done. +*/ +static void zynqmp_r5_mb_tx_done (struct mbox_client *cl,void *mssg,int r) +{ +struct zynqmp_r5_pdata *pdata; +struct sk_buff *skb; + +if (!mssg) +return ; + pdata = container_of(cl, struct zynqmp_r5_pdata, tx_mc); + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); +} + +/** +* zynqmp_r5_setup_mbox() - Setup mailboxes +* +* @pdata: pointer to the ZynqMP R5 processor platform data +* @node: pointer of the device node +* +* Function to setup mailboxes to talk to RPU. +* +* Return: 0 for success, negative value for failure. +*/ +static int zynqmp_r5_setup_mbox (struct zynqmp_r5_pdata *pdata, +struct device_node *node) +{ +struct device *dev = &pdata->dev; +struct mbox_client *mclient; + +/* Setup TX mailbox channel client */ + mclient = &pdata->tx_mc; + mclient->dev = dev; + mclient->rx_callback =NULL ; + mclient->tx_block = false; + mclient->knows_txdone = false; + mclient->tx_done = zynqmp_r5_mb_tx_done; + +/* Setup TX mailbox channel client */ + mclient = &pdata->rx_mc; + mclient->dev = dev; + mclient->rx_callback = zynqmp_r5_mb_rx_cb; + mclient->tx_block = false; + mclient->knows_txdone = false; + +INIT_WORK (&pdata->mbox_work, handle_event_notified); + +/* Request TX and RX channels */ + pdata->tx_chan =mbox_request_channel_byname (&pdata->tx_mc,"tx" ); +if (IS_ERR (pdata->tx_chan)) { +dev_err (dev,"failed to request mbox tx channel.\n" ); + pdata->tx_chan =NULL ; +return -EINVAL ; + } + pdata->rx_chan =mbox_request_channel_byname (&pdata->rx_mc,"rx" ); +if (IS_ERR (pdata->rx_chan)) { +dev_err (dev,"failed to request mbox rx channel.\n" ); + pdata->rx_chan =NULL ; +return -EINVAL ; + } +skb_queue_head_init (&pdata->tx_mc_skbs); +return 0 ; +} + +/** +* zynqmp_r5_probe() - Probes ZynqMP R5 processor device node +* @pdata: pointer to the ZynqMP R5 processor platform data +* @pdev: parent RPU domain platform device +* @node: pointer of the device node +* +* Function to retrieve the information of the ZynqMP R5 device node. +* +* Return: 0 for success, negative value for failure. +*/ +static int zynqmp_r5_probe (struct zynqmp_r5_pdata *pdata, +struct platform_device *pdev, +struct device_node *node) +{ +struct device *dev = &pdata->dev; +struct rproc *rproc; +struct device_node *nc; +int ret; + +/* Create device for ZynqMP R5 device */ + dev->parent = &pdev->dev; + dev->release = zynqmp_r5_release; + dev->of_node = node; +dev_set_name (dev,"%s" ,of_node_full_name (node)); +dev_set_drvdata (dev, pdata); + ret =device_register (dev); +if (ret) { +dev_err (dev,"failed to register device.\n" ); +return ret; + } +get_device (&pdev->dev); + +/* Allocate remoteproc instance */ + rproc =rproc_alloc (dev,dev_name (dev), &zynqmp_r5_rproc_ops,NULL ,0 ); +if (!rproc) { +dev_err (dev,"rproc allocation failed.\n" ); + ret = -ENOMEM ; + goto error; + }
drivers/remoteproc/zynqmp_r5_remoteproc.c line 749
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ rproc->auto_boot = autoboot; + pdata->rproc = rproc; + rproc->priv = pdata; + +/* +* The device has not been spawned from a device tree, so +* arch_setup_dma_ops has not been called, thus leaving +* the device with dummy DMA ops. +* Fix this by inheriting the parent's DMA ops and mask. +*/
This comment looks ominous. Is it still true even in Linux master, after Christoph rework of the dma_ops? Probably not?
Quoted 15 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +15
-0+ rproc->dev.dma_mask = pdev->dev.dma_mask; +set_dma_ops (&rproc->dev,get_dma_ops (&pdev->dev)); + +/* Probe R5 memory devices */ +INIT_LIST_HEAD (&pdata->mems); +for_each_available_child_of_node (node, nc) { + ret =zynqmp_r5_mem_probe (pdata, nc); +if (ret) { +dev_err (dev,"failed to probe memory %s.\n" , +of_node_full_name (nc)); + goto error; + } + } + +/* Set up DMA mask */
drivers/remoteproc/zynqmp_r5_remoteproc.c line 774
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ ret =dma_set_coherent_mask (dev,DMA_BIT_MASK (32 )); +if (ret) { +dev_warn (dev,"dma_set_coherent_mask failed: %d\n" , ret); +/* If DMA is not configured yet, try to configure it. */ + ret =of_dma_configure (dev, node, true); +if (ret) { +dev_err (dev,"failed to configure DMA.\n" ); + goto error; + } + }
It looks like we are following a trial-and-error approach to setup dma coherency. Can we do the right thing without having to try first?
Specifically, in which circumstances of_dma_configure() would need to be called again here?
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/* Get R5 power domain node */ + ret =of_property_read_u32 (node,"pnode-id" , &pdata->pnode_id); +if (ret) { +dev_err (dev,"failed to get power node id.\n" ); + goto error;
drivers/remoteproc/zynqmp_r5_remoteproc.c line 790
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ } + +/* TODO Check if R5 is running */ + +/* Set up R5 if not already setup */ + ret = pdata->is_r5_mode_set ?0 :r5_set_mode (pdata); +if (ret) { +dev_err (dev,"failed to set R5 operation mode.\n" ); +return ret; + }
is_r5_mode_set is set by r5_set_mode(), which is only called here. So it looks like this check is important in cases where zynqmp_r5_probe() is called twice for the same R5 node. But I don't think that is supposed to happen?
drivers/remoteproc/zynqmp_r5_remoteproc.c line 798
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+if (!of_get_property (dev->of_node,"mboxes" ,NULL )) { +dev_dbg (dev,"no mailboxes.\n" ); + goto error; + }else { + ret =zynqmp_r5_setup_mbox (pdata, node); +if (ret <0 ) + goto error;
Given that mboxes is actually required, I think you should update the device tree example with it. Otherwise, if mboxes is not required, then we shouldn't fail with an error here if it is absent.
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +18
-0+ } + +/* Add R5 remoteproc */ + ret =rproc_add (rproc); +if (ret) { +dev_err (dev,"rproc registration failed\n" ); + goto error; + } +return 0 ; +error: +if (pdata->rproc) +rproc_free (pdata->rproc); + pdata->rproc =NULL ; +device_unregister (dev); +put_device (&pdev->dev); +return ret; +} +
drivers/remoteproc/zynqmp_r5_remoteproc.c line 826
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-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; +struct zynqmp_r5_pdata *pdata; + + pdata =devm_kzalloc (dev,sizeof (*pdata),GFP_KERNEL ); + lockstep_mode =devm_kzalloc (dev,sizeof (u32 *),GFP_KERNEL );
Any reasons why lockstep_mode couldn't just be a local variable then use &lockstep_mode when you need a reference?
Quoted 85 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +85
-0+if (!pdata || !lockstep_mode) +return -ENOMEM ; + +platform_set_drvdata (pdev, pdata); + +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 ; + }else { +dev_err (dev, +"Invalid lockstep-mode mode provided - %x %d\n" , + *lockstep_mode, rpu_mode); +return -EINVAL ; + } +dev_dbg (dev,"RPU configuration: %s\r\n" , + (*lockstep_mode) ?"lockstep" :"split" ); + +for_each_available_child_of_node (dev->of_node, nc) { + ret =zynqmp_r5_probe (&rpus[i], pdev, nc); +if (ret) { +dev_err (dev,"failed to probe rpu %s.\n" , +of_node_full_name (nc)); +return ret; + } + i++; + } + +return 0 ; +} + +static int zynqmp_r5_remoteproc_remove (struct platform_device *pdev) +{ +int i; + +for (i =0 ; i <MAX_RPROCS ; i++) { +struct zynqmp_r5_pdata *pdata = &rpus[i]; +struct rproc *rproc; + + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + pdata->rproc =NULL ; + } +if (pdata->tx_chan) { +mbox_free_channel (pdata->tx_chan); + pdata->tx_chan =NULL ; + } +if (pdata->rx_chan) { +mbox_free_channel (pdata->rx_chan); + pdata->rx_chan =NULL ; + } + +device_unregister (&pdata->dev); + } + +return 0 ; +} + +/* Match table for OF platform binding */ +static const struct of_device_id zynqmp_r5_remoteproc_match[] = { + { .compatible ="xlnx,zynqmp-r5-remoteproc-1.0" , }, + {/* end of list */ }, +}; +MODULE_DEVICE_TABLE (of, zynqmp_r5_remoteproc_match); + +static struct platform_driver zynqmp_r5_remoteproc_driver = { + .probe = zynqmp_r5_remoteproc_probe, + .remove = zynqmp_r5_remoteproc_remove, + .driver = { + .name ="zynqmp_r5_remoteproc" , + .of_match_table = zynqmp_r5_remoteproc_match, + }, +}; +module_platform_driver (zynqmp_r5_remoteproc_driver); + +module_param_named (autoboot, autoboot, bool,0444 ); +MODULE_PARM_DESC (autoboot, +"enable | disable autoboot. (default: false)" ); + +MODULE_AUTHOR ("Ben Levinsky <[email protected]>" ); +MODULE_LICENSE ("GPL v2" );
RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hi Stefano
Please see my comments inline
Quoted 14 lines
-----Original Message-----
From: Stefano Stabellini <[email protected]>
Sent: Thursday, August 13, 2020 1:36 PM
To: Ben Levinsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Michal Simek <[email protected]>; Jiaying Liang <[email protected]>; Jason Wu <[email protected]>; linux-arm- [email protected]
Subject: Re: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Quoted 7 lines
Quoted 3 lines · Ben Levinsky
R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this remotproc driver, we can boot the R5 sub-system in different configurations.
Which different configurations? How can you boot them?
[Ben Levinsky] The different configurations are R5 split and lockstep mode. Remoteproc boots the R5 up using Xilinx platform management firmware on a Microblaze core that has isolated access to power up/down. I will document as such in commit message for this patch in v9
Quoted 54 lines
Quoted 6 lines
Signed-off-by Ben Levinsky <[email protected]>Signed-off-by Wendy Liang <[email protected]>Signed-off-by Michal Simek <[email protected]>Signed-off-by Ed Mooring <[email protected]>Signed-off-by Jason Wu <[email protected]>Quoted 9 lines
---
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 1check
Quoted 30 lines
are still raised as each is due to fixing the warning results in that
particular line going over 80 characters.
v5:
-parse_fwchange from use ofrproc_of_resm_mem_entry_inittorproc_mem_entry_initand use of alloc/release
- var's of typezynqmp_r5_pdataall have same local variable name
- usedev_dbginstead ofdev_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-> u32lockstep_mode;
- change device-tree binding "lockstep-mode" to xlnx,cluster-mode
- removezynqmp_r5_mem_probeand loop to Probe R5 memory devices at remoteproc-probe time
- removeis_r5_mode_setfrom zynqmp rpu remote processor private data
- do not error out if no mailbox is provided
- removezynqmp_r5_remoteproc_probecall ofplatform_set_drvdataas pdata is
handled inzynqmp_r5_remoteproc_remove
v8:
- remove old acks, reviewed-by's in commit messageQuoted 4 lines
--- drivers/remoteproc/Kconfig | 10 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/zynqmp_r5_remoteproc.c | 911++++++++++++++++++++++
Quoted 3 lines
3 files changed, 922 insertions(+) create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 7 lines
drivers/remoteproc/Kconfig config STM32_RPROC · line 250
Quoted 7 lines
drivers/remoteproc/Kconfig +0
-0@@ -249,6 +249,16 @@ config STM32_RPROC This can be either built-in or a loadable module.
[Ben Levinsky] Right will update in v9 in the help message
Quoted 49 lines
drivers/remoteproc/Makefile obj-$(CONFIG_QCOM_WCNSS_PIL) +=
Quoted 19 lines
drivers/remoteproc/Kconfig +10
-0+config ZYNQMP_R5_REMOTEPROC + tristate "ZynqMP_R5 remoteproc support" + depends on ARM64 && PM && ARCH_ZYNQMP + select RPMSG_VIRTIO + select MAILBOX + select ZYNQMP_IPI_MBOX + help + Say y here to support ZynqMP R5 remote processors via the remote + processor framework. + endif # REMOTEPROC endmenudrivers/remoteproc/Makefile +0
-0@@ -28,5 +28,6 @@ obj-$(CONFIG_QCOM_WCNSS_PIL) +=qcom_wcnss_pil.o
Review
Quoted 7 lines
drivers/remoteproc/Makefile +1
-0qcom_wcnss_pil-y += qcom_wcnss.o qcom_wcnss_pil-y += qcom_wcnss_iris.o obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o(quoted diff) +0
-0
b/drivers/remoteproc/zynqmp_r5_remoteproc.cdrivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 4 lines
new file mode 100644
index 000000000000..b600759e257edrivers/remoteproc/zynqmp_r5_remoteproc.c new file +0
-0I tried to build this but I get 4 warnings:
drivers/remoteproc/zynqmp_r5_remoteproc.c: In function 'handle_tcm_parsing':drivers/remoteproc/zynqmp_r5_remoteproc.c:286:10: warning: return makes pointer from integer without a cast [-Wint-conversion]return -EINVAL; ^ drivers/remoteproc/zynqmp_r5_remoteproc.c:293:10: warning: return makes pointer from integer without a cast [-Wint-conversion] return -EINVAL; ^ drivers/remoteproc/zynqmp_r5_remoteproc.c:298:10: warning: return makes pointer from integer without a cast [-Wint-conversion] return -ENOMEM; ^
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c:317:10: warning: return makes pointer from integer without a cast [-Wint-conversion]
return-ENOMEM;Please fix all warnings before submitting to the list.
[Ben Levinsky] will fix these in v9, thanks
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 5
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0@@ -0,0 +1,911 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Zynq R5 Remote Processor driver +* +* Copyright (C) 2019, 2020 Xilinx Inc. Ben Levinsky drivers/remoteproc/zynqmp_r5_remoteproc.c line 14
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+* Copyright (C) 2015 - 2018 Xilinx Inc. +* Copyright (C) 2015 Jason Wu <[email protected]> +* +* Based on origin OMAP and Zynq Remote Processor driver +* +* Copyright (C) 2012 Michal Simek <[email protected]> +* Copyright (C) 2012 PetaLogix +* Copyright (C) 2011 Texas Instruments, Inc. +* Copyright (C) 2011 Google, Inc.
Quoted 4 lines
In my opinion 8 copyright lines are too many. As you probably know, they aren't actually necessary or useful (the SPDX line and git log are the only things that matter). I would remove them all.
[Ben Levinsky] will do
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 17
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+*/ + +#include <linux/atomic.h> I just looked at the first #include and noticed that it is not actually used. Please make sure that all headers included are necessary. (You can try to remove them one by one to test if necessary.)
[Ben Levinsky] will do
Quoted 48 lines
Quoted 19 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +18
-0+#include <linux/cpu.h> +#include <linux/dma-mapping.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/firmware/xlnx-zynqmp.h> +#include <linux/genalloc.h> +#include <linux/idr.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/mailbox_client.h> +#include <linux/mailbox/zynqmp-ipi-message.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/of_reserved_mem.h> +#include <linux/pfn.h> drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+#include <linux/platform_device.h> +#include <linux/remoteproc.h> +#include <linux/skbuff.h> +#include <linux/slab.h> +#include <linux/sysfs.h> + +#include "remoteproc_internal.h" + +#define MAX_RPROCS 2/* Support up to 2 RPU */ +#define MAX_MEM_PNODES 4/* Max power nodes for one RPU memory instance */
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+ +#define DEFAULT_FIRMWARE_NAME "rproc-rpu-fw" + +/* PM proc states */ +#define PM_PROC_STATE_ACTIVE 1Udrivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ +/* IPI buffer MAX length */ +#define IPI_BUF_LEN_MAX 32U +/* RX mailbox client buffer max length */ +#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \ + sizeof(struct zynqmp_ipi_message)) + +#define ZYNQMP_R5_NUM_TCM_BANKS 4 + +/* lookup table mapping power-node-ID of TCM bank to absolute base address */
drivers/remoteproc/zynqmp_r5_remoteproc.c line 61
Quoted 1 line
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +1
-0+static unsigned longtcm_addr_to_pnode[ZYNQMP_R5_NUM_TCM_BANKS][2] = {
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 68
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+ {0xFFE00000, 0xF }, + {0xFFE20000, 0x10}, + {0xFFE90000, 0x10}, + {0xFFEB0000, 0x11}, +}; + +static bool autoboot__read_mostly ;Please add a description of this parameter. What does it do? Why would anybody enable it?
[Ben Levinsky] this is not needed on ZynqMP so this can be removed. Previously was used in situations where R5 would be loaded at kernel boot time.
Quoted 36 lines
Quoted 36 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +35
-0+/** +* struct zynqmp_r5_mem - zynqmp rpu memory data +* @pnode_id: TCM power domain ids +* @res: memory resource +* @node: list node +*/ +struct zynqmp_r5_mem { +u32 pnode_id[MAX_MEM_PNODES ]; +struct resource res; +struct list_head node; +}; + +/** +* struct zynqmp_r5_pdata - zynqmp rpu remote processor private data +* @dev: device of RPU instance +* @rproc: rproc handle +* @pnode_id: RPU CPU power domain id +* @mems: memory resources +* @is_r5_mode_set: indicate if r5 operation mode is set +* @tx_mc: tx mailbox client +* @rx_mc: rx mailbox client +* @tx_chan: tx mailbox channel +* @rx_chan: rx mailbox channel +* @mbox_work: mbox_work for the RPU remoteproc +* @tx_mc_skbs: socket buffers for tx mailbox client +* @rx_mc_buf: rx mailbox client buffer to save the rx message +*/ +struct zynqmp_r5_pdata { +struct device dev; +struct rproc *rproc; +u32 pnode_id; +struct list_head mems; +bool is_r5_mode_set; +struct mbox_client tx_mc; +struct mbox_client rx_mc;
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 109
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+struct mbox_chan *tx_chan; +struct mbox_chan *rx_chan; +struct work_struct mbox_work; +struct sk_buff_head tx_mc_skbs; +unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX ];A simple small reordering of the struct fields would lead to small memory savings due to alignment.
[Ben Levinsky] will do. Do you mean ordering in either ascending or descending order?
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 117
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+}; + +/** +* table of RPUs +*/ +struct zynqmp_r5_pdata rpus[MAX_RPROCS ]; +/** +* RPU core configuration ^ spurious white space
[Ben Levinsky] will remove this in v9
Quoted 24 lines
Quoted 24 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +23
-0+*/ +enum rpu_oper_mode rpu_mode; + +/* +* r5_set_mode - set RPU operation mode +* @pdata: Remote processor private data +* +* set RPU oepration mode +* +* Return: 0 for success, negative value for failure +*/ +static int r5_set_mode (struct zynqmp_r5_pdata *pdata) +{ +u32 val[PAYLOAD_ARG_CNT ] = {0 }, expect, tcm_mode; +struct device *dev = &pdata->dev; +int ret; + + expect = (u32 )rpu_mode; + ret =zynqmp_pm_get_rpu_mode (pdata->pnode_id,0 ,0 , val); +if (ret <0 ) { +dev_err (dev,"failed to get RPU oper mode.\n" ); +return ret; + }
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 145
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+if (val[0 ] == expect) { +dev_dbg (dev,"RPU mode matches: %x\n" , val[0 ]); + }else { + ret =zynqmp_pm_set_rpu_mode (pdata->pnode_id, + expect,0 , val);It looks like
zynqmp_pm_set_rpu_modeis actually ignoring the second argument, so expect is not correctly handled?
[Ben Levinsky] will update this so that implementation of this zynqmp_pm_set_rpu_mode using the input argument. Also, will remove use of val as arg as we are not evaluating the output.
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+if (ret <0 ) { +dev_err (dev, +"failed to set RPU oper mode.\n" ); +return ret; + } + } + + tcm_mode = (expect == (u32 )PM_RPU_MODE_LOCKSTEP ) ? +PM_RPU_TCM_COMB :PM_RPU_TCM_SPLIT ; + ret = zynqmp_pm_set_tcm_config(pdata->pnode_id, tcm_mode, 0,val);
Quoted 6 lines
Similarly, it looks like the second argument to
zynqmp_pm_set_tcm_configis actually ignored by the implementation of the function. Sotcm_modehere is not correctly handled byzynqmp_pm_set_tcm_config.Also val has not been zeroed from the previous call.
[Ben Levinsky] Same as above, will update zynqmp_pm_set_tcm_config and remove use of val as input arg.
Quoted 154 lines
Quoted 49 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +48
-0+if (ret <0 ) { +dev_err (dev,"failed to config TCM to %x.\n" , + expect); +return ret; + } + pdata->is_r5_mode_set = true; +return 0 ; +} + +/* +* ZynqMP R5 remoteproc memory release function +*/ +static int zynqmp_r5_mem_release (struct rproc *rproc, +struct rproc_mem_entry *mem) +{ +struct zynqmp_r5_mem *priv; +int i, ret; +struct device *dev = &rproc->dev; + + priv = mem->priv; +if (!priv) +return 0 ; +for (i =0 ; i <MAX_MEM_PNODES ; i++) { +if (priv->pnode_id[i]) { +dev_dbg (dev,"%s, pnode %d\n" , + __func__, priv->pnode_id[i]); + ret =zynqmp_pm_release_node (priv->pnode_id[i]); +if (ret <0 ) { +dev_err (dev, +"failed to release power node: %u\n" , + priv->pnode_id[i]); +return ret; + } + }else { +break ; + } + } +return 0 ; +} + +/* +* ZynqMP R5 remoteproc operations +*/ +static int zynqmp_r5_rproc_start (struct rproc *rproc) +{ +struct device *dev = rproc->dev.parent; +struct zynqmp_r5_pdata *pdata = rproc->priv; +enum rpu_boot_mem bootmem;drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+int ret; + +if ((rproc->bootaddr &0xF0000000 ) ==0xF0000000 ) + bootmem =PM_RPU_BOOTMEM_HIVEC ; +else + bootmem =PM_RPU_BOOTMEM_LOVEC ; +dev_dbg (dev,"RPU boot from %s." , + bootmem ==PM_RPU_BOOTMEM_HIVEC ?"OCM" :"TCM" ); + ret = zynqmp_pm_request_wakeup(pdata->pnode_id, 1, + bootmem,
ZYNQMP_PM_REQUEST_ACK_NO);Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+if (ret <0 ) { +dev_err (dev,"failed to boot R5.\n" ); +return ret; + }drivers/remoteproc/zynqmp_r5_remoteproc.c line 234
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+return 0 ; +} + +static int zynqmp_r5_rproc_stop (struct rproc *rproc) +{ +struct zynqmp_r5_pdata *pdata = rproc->priv; +int ret; + + ret = zynqmp_pm_force_powerdown(pdata->pnode_id, +ZYNQMP_PM_REQUEST_ACK_BLOCKING);Quoted 47 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +47
-0+if (ret <0 ) { +dev_err (&pdata->dev,"failed to shutdown R5.\n" ); +return ret; + } +return 0 ; +} + +static int zynqmp_r5_rproc_mem_alloc (struct rproc *rproc, +struct rproc_mem_entry *mem) +{ +struct device *dev = rproc->dev.parent; +void *va; + +dev_dbg (rproc->dev.parent,"map memory: %pa\n" , &mem->dma); + va =ioremap_wc (mem->dma, mem->len); +if (IS_ERR_OR_NULL (va)) { +dev_err (dev,"Unable to map memory region: %pa+%lx\n" , + &mem->dma, mem->len); +return -ENOMEM ; + } + +/* Update memory entry va */ + mem->va = va; +return 0 ; +} + +static int zynqmp_r5_rproc_mem_release (struct rproc *rproc, +struct rproc_mem_entry *mem) +{ +dev_dbg (rproc->dev.parent,"unmap memory: %pa\n" , &mem->dma); +iounmap (mem->va); + +return 0 ; +} + +/* +* TCM needs mapping to R5 relative address and xilinx platform mgmt call +*/ +struct rproc_mem_entry *handle_tcm_parsing (struct device *dev, +struct reserved_mem *rmem, +struct device_node *node, +int lookup_idx) +{ +void *va; +dma_addr_t dma; +resource_size_t size; +int ret;drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+u32 pnode_id; +struct resource rsc; +struct rproc_mem_entry *mem; + + pnode_id = tcm_addr_to_pnode[lookup_idx][1 ]; + ret =zynqmp_pm_request_node (pnode_id, +ZYNQMP_PM_CAPABILITY_ACCESS ,0 , +ZYNQMP_PM_REQUEST_ACK_BLOCKING ); +if (ret <0 ) { + dev_err(dev, "failed to request power node: %u\n",
pnode_id);Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+return -EINVAL ; + } + + ret =of_address_to_resource (node,0 , &rsc); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ;drivers/remoteproc/zynqmp_r5_remoteproc.c line 303
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ } + size =resource_size (&rsc); + va =devm_ioremap_wc (dev, rsc.start, size); +if (!va) +return -ENOMEM ; + +/* zero out tcm base address */ +if (rsc.start &0xffe00000 ) { +/* R5 can't see anything past 0xfffff so wipe it */ + rsc.start &=0x000fffff ;If that is the case why not do:
Quoted 8 lines
rsc.start &= 0x000fffff;unconditionally? if (rsc.start &
0xffe00000) is superfluous.But I thought that actually the R5s could see TCM at both the low address (<
0x000fffff) and also at the high address (i.e.0xffe00000).
[Ben Levinsky] Here yes can make rsc.start &= 0x000fffff undconditional. Will update in v9 as such
Also, this logic is because this is only for the Xilinx R5 mappings of TCM banks that are at (from the TCM point of view) 0x0 and 0x2000
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 309
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+/* +* handle tcm banks 1 a and b (0xffe9000 and +* 0xffeb0000) +*/ +if (rsc.start &0x80000 ) + rsc.start -=0x90000 ;It is very unclear to me why we have to do this
[Ben Levinsky] This is for TCM bank 0B and bank 1B to map them to 0x00020000 so that the TCM relative addressing lines up. For example (0xffe90000 & 0x000fffff) - 0x90000 = 0x20000
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 312
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+ } + + dma = (dma_addr_t )rsc.start;Given the way the dma parameter is used by
rproc_alloc_registered_carveouts, I think it might be best to pass the original start address (i.e.0xffe00000) as dma.
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 314
Quoted 2 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +2
-0+ mem = rproc_mem_entry_init(dev, va, dma, (int)size, rsc.start, + NULL, zynqmp_r5_mem_release,I don't know too much about the remoteproc APIs, but shouldn't you be passing
zynqmp_r5_rproc_mem_allocto it instead of NULL?
[Ben Levinsky] the difference is that for TCM we have to do make the relative address work for TCM, so the dma input to rproc_mem_entry_init is different in TCM case.
Quoted 17 lines
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ rsc.name); +if (!mem) +return -ENOMEM ; + +return mem; +}drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ +static int parse_mem_regions (struct rproc *rproc) +{ +int num_mems, i; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; +struct device_node *np = dev->of_node; +struct rproc_mem_entry *mem; + + num_mems = of_count_phandle_with_args(np, "memory-region",NULL);
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 337
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+if (num_mems <=0 ) +return 0 ; +for (i =0 ; i < num_mems; i++) { +struct device_node *node; +struct reserved_mem *rmem; + + node =of_parse_phandle (np,"memory-region" , i);Check node != NULL ?
[Ben Levinsky] will add this in v9
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 344
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+ rmem =of_reserved_mem_lookup (node); +if (!rmem) { +dev_err (dev,"unable to acquire memory-region\n" ); +return -EINVAL ; + } + +if (strstr (node->name,"vdev0buffer" )) {vdev0buffer is not described in the device tree bindings, is that normal/expected?
[Ben Levinsky] vdev0buffer is not required, as there might be simple firmware loading with no IPC. Vdev0buffer only needed for IPC.
Quoted 19 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+/* Register DMA region */ + mem =rproc_mem_entry_init (dev,NULL , + (dma_addr_t )rmem->base, + rmem->size, rmem->base, +NULL ,NULL , +"vdev0buffer" ); +if (!mem) { + dev_err(dev, "unable to initialize memory-region %s\n",
drivers/remoteproc/zynqmp_r5_remoteproc.c line 358
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ node->name); +return -ENOMEM ; + } +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma); + }else if (strstr (node->name,"vdev0vring" )) {Same here
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 366
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+int vring_id; +char name[16 ]; + +/* +* can be 1 of multiple vring IDs per IPC channel +* e.g. 'vdev0vring0' and 'vdev0vring1' +*/ + vring_id = node->name[14 ] -'0' ;Where does the "14" comes from? Are we sure it is not possible to have a
node->namesmaller than 14 chars?
[Ben Levinsky] Presently there are only 2 vrings used per Xilinx OpenAMP channel to RPU. In Xilinx kernel we have hard-coded names as these are the only nodes that use it. For example RPU0vdev0vring0 and RPU1vdev0vring0. Hence we only check for vdev0vring and not a sscanf("%*s%i") to parse out the vring ID or other, cleaner solution.
Quoted 23 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 365
Quoted 2 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +1
-0+ snprintf(name, sizeof(name), "vdev0vring%d",
vring_id);drivers/remoteproc/zynqmp_r5_remoteproc.c line 379
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/* Register vring */ + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem->base, + rmem->size, rmem->base, +
zynqmp_r5_rproc_mem_alloc,drivers/remoteproc/zynqmp_r5_remoteproc.c line 371
Quoted 1 line
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +1
-0+
zynqmp_r5_rproc_mem_release,drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +11
-0+ name); +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma); + }else { +int idx; + +/* +* if TCM update address space for R5 and +* make xilinx platform mgmt call +*/ + for (idx = 0; idx < ZYNQMP_R5_NUM_TCM_BANKS;idx++) {
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +1
-0+ if (tcm_addr_to_pnode[idx][0] == rmem-base)
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +1
-0+break ;There is something I don't quite understand. If the memory region to use is TCM, why would it be also described under reserved-memory? Reserved-memory is for normal memory being reserved, while TCM is not normal memory. Am I missing something?
[Ben Levinsky] I can change this in v9 as discussed. That is, have no TCM under reserved mem. Instead have the banks as nodes in device tree with status="[enabled|disabled]" and if enabled, then try to add memories in parse_fw call.
Quoted 46 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+ } + +if (idx !=ZYNQMP_R5_NUM_TCM_BANKS ) { + mem = handle_tcm_parsing(dev, rmem, node,idx);
drivers/remoteproc/zynqmp_r5_remoteproc.c line 399
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+ }else { + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem->base, + rmem->size, rmem->base, +
zynqmp_r5_rproc_mem_alloc,Quoted 1 line
+
zynqmp_r5_rproc_mem_release,drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 1 line
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +1
-0+ node->name);This case looks identical to the vdev0vring above. Is the difference
really just in the "name"? If so, can we merge the two cases into one?
no, because thedevm_ioremap_wccall has to be done before changing the dma address to relative for TCM banks.Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + +if (!mem) { +dev_err (dev, +"unable to init memory-region %s\n" , + node->name);drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+return -ENOMEM ; + } + } +rproc_add_carveout (rproc, mem); + } + +return 0 ; +} + +static int zynqmp_r5_parse_fw(struct rproc *rproc, const struct firmware*fw)
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+{ +int ret; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; + + ret =parse_mem_regions (rproc); +if (ret) { +dev_err (dev,"parse_mem_regions failed %x\n" , ret); +return ret;
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 428
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + + ret =rproc_elf_load_rsc_table (rproc, fw); +if (ret == -EINVAL ) { +dev_info (dev,"no resource table found.\n" ); + ret =0 ;Why do we want to continue ignoring the error in this case?
[Ben Levinsky] as there can be simple firmware loaded onto R5 that do not have resource table. Resource table only needed for specific IPC case.
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +11
-0+ } +return ret; +} + +/* kick a firmware */ +static void zynqmp_r5_rproc_kick (struct rproc *rproc,int vqid) +{ +struct device *dev = rproc->dev.parent; +struct zynqmp_r5_pdata *pdata = rproc->priv; + + dev_dbg(dev, "KICK Firmware to start send messages vqid %d\n",vqid);
Quoted 16 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 451
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +12
-0+ +if (vqid <0 ) { +/* If vqid is negative, does not pass the vqid to +* mailbox. As vqid is supposed to be 0 or possive. +* It also gives a way to just kick instead but +* not use the IPI buffer. It is better to provide +* a proper way to pass the short message, which will +* need to sync to upstream first, for now, +* use negative vqid to assume no message will be +* passed with IPI buffer, but just raise interrupt. +* This will be faster as it doesn't need to copy the +* message to the IPI buffer. I take that by "upstream" you mean the upstream Linux kernel community? Did the conversation happen?
[Ben Levinsky] can remove this in v9 and just use the logic present in the else clause.
Quoted 47 lines
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+* It will ignore the return, as failure is due to +* there already kicks in the mailbox queue. +*/ + (void )mbox_send_message (pdata->tx_chan,NULL ); + }else { +struct sk_buff *skb; +unsigned int skb_len; +struct zynqmp_ipi_message *mb_msg;drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+int ret; + + skb_len = (unsigned int )(sizeof (vqid) +sizeof (mb_msg)); + skb =alloc_skb (skb_len,GFP_ATOMIC ); +if (!skb) { +dev_err (dev, +"Failed to allocate skb to kick remote.\n" ); +return ; + } + mb_msg = (struct zynqmp_ipi_message *)skb_put(skb,
skb_len);Quoted 27 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +27
-0+ mb_msg->len =sizeof (vqid); +memcpy (mb_msg->data, &vqid,sizeof (vqid)); +skb_queue_tail (&pdata->tx_mc_skbs, skb); + ret =mbox_send_message (pdata->tx_chan, mb_msg); +if (ret <0 ) { +dev_warn (dev,"Failed to kick remote.\n" ); +skb_dequeue_tail (&pdata->tx_mc_skbs); +kfree_skb (skb); + } + } +} + +static struct rproc_ops zynqmp_r5_rproc_ops = { + .start = zynqmp_r5_rproc_start, + .stop = zynqmp_r5_rproc_stop, + .load = rproc_elf_load_segments, + .parse_fw = zynqmp_r5_parse_fw, + .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table, + .sanity_check = rproc_elf_sanity_check, + .get_boot_addr = rproc_elf_get_boot_addr, + .kick = zynqmp_r5_rproc_kick, +}; + +/* zynqmp_r5_mem_probe() - probes RPU TCM memory device +* @pdata: pointer to the RPU remoteproc private data +* @node: pointer to the memory node +*
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 502
Quoted 5 lines
+ * Function to retrieve resources for RPU TCM memory device.
+ */drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+static int zynqmp_r5_mem_probe (struct zynqmp_r5_pdata *pdata, +struct device_node *node) +{TCM nodes as children of the
zynqmp_r5_remoteprocnode are not described in device tree, hence, they should not be parsed here.
[Ben Levinsky] ok will remove here.
Quoted 96 lines
Quoted 96 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +95
-0+struct device *dev; +struct zynqmp_r5_mem *mem; +int ret; +struct property *prop; +const __be32 *cur; +u32 val; +int i; + + dev = &pdata->dev; + mem =devm_kzalloc (dev,sizeof (*mem),GFP_KERNEL ); +if (!mem) +return -ENOMEM ; + ret =of_address_to_resource (node,0 , &mem->res); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ; + } + +/* Get the power domain id */ + i =0 ; +if (of_find_property (node,"pnode-id" ,NULL )) { +of_property_for_each_u32 (node,"pnode-id" , prop, cur, val) + mem->pnode_id[i++] = val; + } +list_add_tail (&mem->node, &pdata->mems); +return 0 ; +} + +/** +* zynqmp_r5_release() - ZynqMP R5 device release function +* @dev: pointer to the device struct of ZynqMP R5 +* +* Function to release ZynqMP R5 device. +*/ +static void zynqmp_r5_release (struct device *dev) +{ +struct zynqmp_r5_pdata *pdata; +struct rproc *rproc; +struct sk_buff *skb; + + pdata =dev_get_drvdata (dev); + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + } +if (pdata->tx_chan) +mbox_free_channel (pdata->tx_chan); +if (pdata->rx_chan) +mbox_free_channel (pdata->rx_chan); +/* Discard all SKBs */ +while (!skb_queue_empty (&pdata->tx_mc_skbs)) { + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); + } + +put_device (dev->parent); +} + +/** +* event_notified_idr_cb() - event notified idr callback +* @id: idr id +* @ptr: pointer to idr private data +* @data: data passed to idr_for_each callback +* +* Pass notification to remoteproc virtio +* +* Return: 0. having return is to satisfy the idr_for_each() function +* pointer input argument requirement. +**/ +static int event_notified_idr_cb (int id,void *ptr,void *data) +{ +struct rproc *rproc = data; + + (void )rproc_vq_interrupt (rproc, id); +return 0 ; +} + +/** +* handle_event_notified() - remoteproc notification work funciton +* @work: pointer to the work structure +* +* It checks each registered remoteproc notify IDs. +*/ +static void handle_event_notified (struct work_struct *work) +{ +struct rproc *rproc; +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(work, struct zynqmp_r5_pdata, mbox_work); + + (void )mbox_send_message (pdata->rx_chan,NULL ); + rproc = pdata->rproc; +/*
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 602
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+* We only use IPI for interrupt. The firmware side may or may +* not write the notifyid when it trigger IPI. +* And thus, we scan through all the registered notifyids. +*/ +idr_for_each (&rproc->notifyids, event_notified_idr_cb, rproc);This looks expensive. Should we at least check whether the notifyid was written as first thing before doing the scan?
[Ben Levinsky] this will be at most 2 vrings presently per firmware-load and only done when the firmware is loaded so the latency so should not impact performace or user
Quoted 147 lines
Quoted 28 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +27
-0+} + +/** +* zynqmp_r5_mb_rx_cb() - Receive channel mailbox callback +* @cl: mailbox client +* @mssg: message pointer +* +* It will schedule the R5 notification work. +*/ +static void zynqmp_r5_mb_rx_cb (struct mbox_client *cl,void *mssg) +{ +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(cl, struct zynqmp_r5_pdata, rx_mc); +if (mssg) { +struct zynqmp_ipi_message *ipi_msg, *buf_msg; +size_t len; + + ipi_msg = (struct zynqmp_ipi_message *)mssg; + buf_msg = (struct zynqmp_ipi_message *)pdata->rx_mc_buf; + len = (ipi_msg->len >=IPI_BUF_LEN_MAX ) ? +IPI_BUF_LEN_MAX : ipi_msg->len; + buf_msg->len = len; +memcpy (buf_msg->data, ipi_msg->data, len); + } +schedule_work (&pdata->mbox_work); +}drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ +/** +* zynqmp_r5_mb_tx_done() - Request has been sent to the remote +* @cl: mailbox client +* @mssg: pointer to the message which has been sent +* @r: status of last TX - OK or error +* +* It will be called by the mailbox framework when the last TX has done. +*/ +static void zynqmp_r5_mb_tx_done(struct mbox_client *cl, void *mssg, intr)
Quoted 36 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +36
-0+{ +struct zynqmp_r5_pdata *pdata; +struct sk_buff *skb; + +if (!mssg) +return ; + pdata = container_of(cl, struct zynqmp_r5_pdata, tx_mc); + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); +} + +/** +* zynqmp_r5_setup_mbox() - Setup mailboxes +* +* @pdata: pointer to the ZynqMP R5 processor platform data +* @node: pointer of the device node +* +* Function to setup mailboxes to talk to RPU. +* +* Return: 0 for success, negative value for failure. +*/ +static int zynqmp_r5_setup_mbox (struct zynqmp_r5_pdata *pdata, +struct device_node *node) +{ +struct device *dev = &pdata->dev; +struct mbox_client *mclient; + +/* Setup TX mailbox channel client */ + mclient = &pdata->tx_mc; + mclient->dev = dev; + mclient->rx_callback =NULL ; + mclient->tx_block = false; + mclient->knows_txdone = false; + mclient->tx_done = zynqmp_r5_mb_tx_done; + +/* Setup TX mailbox channel client */ drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ mclient = &pdata->rx_mc; + mclient->dev = dev; + mclient->rx_callback = zynqmp_r5_mb_rx_cb; + mclient->tx_block = false; + mclient->knows_txdone = false; + +INIT_WORK (&pdata->mbox_work, handle_event_notified); + +/* Request TX and RX channels */ + pdata->tx_chan = mbox_request_channel_byname(&pdata->tx_mc,"tx");
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+if (IS_ERR (pdata->tx_chan)) { +dev_err (dev,"failed to request mbox tx channel.\n" ); + pdata->tx_chan =NULL ; +return -EINVAL ; + } + pdata->rx_chan = mbox_request_channel_byname(&pdata->rx_mc,"rx");
Quoted 33 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +33
-0+if (IS_ERR (pdata->rx_chan)) { +dev_err (dev,"failed to request mbox rx channel.\n" ); + pdata->rx_chan =NULL ; +return -EINVAL ; + } +skb_queue_head_init (&pdata->tx_mc_skbs); +return 0 ; +} + +/** +* zynqmp_r5_probe() - Probes ZynqMP R5 processor device node +* @pdata: pointer to the ZynqMP R5 processor platform data +* @pdev: parent RPU domain platform device +* @node: pointer of the device node +* +* Function to retrieve the information of the ZynqMP R5 device node. +* +* Return: 0 for success, negative value for failure. +*/ +static int zynqmp_r5_probe (struct zynqmp_r5_pdata *pdata, +struct platform_device *pdev, +struct device_node *node) +{ +struct device *dev = &pdata->dev; +struct rproc *rproc; +struct device_node *nc; +int ret; + +/* Create device for ZynqMP R5 device */ + dev->parent = &pdev->dev; + dev->release = zynqmp_r5_release; + dev->of_node = node; +dev_set_name (dev,"%s" ,of_node_full_name (node));drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+dev_set_drvdata (dev, pdata); + ret =device_register (dev); +if (ret) { +dev_err (dev,"failed to register device.\n" ); +return ret; + } +get_device (&pdev->dev); + +/* Allocate remoteproc instance */ + rproc = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops,NULL, 0);
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+if (!rproc) { +dev_err (dev,"rproc allocation failed.\n" ); + ret = -ENOMEM ; + goto error; + } + rproc->auto_boot = autoboot; + pdata->rproc = rproc; + rproc->priv = pdata; + +/*
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 749
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+* The device has not been spawned from a device tree, so +* arch_setup_dma_ops has not been called, thus leaving +* the device with dummy DMA ops. +* Fix this by inheriting the parent's DMA ops and mask. +*/ This comment looks ominous. Is it still true even in Linux master, after Christoph rework of the
dma_ops? Probably not?
[Ben Levinsky] tested without it and now is fine. Will remove in v9
Quoted 30 lines
Quoted 16 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +15
-0+ rproc->dev.dma_mask = pdev->dev.dma_mask; +set_dma_ops (&rproc->dev,get_dma_ops (&pdev->dev)); + +/* Probe R5 memory devices */ +INIT_LIST_HEAD (&pdata->mems); +for_each_available_child_of_node (node, nc) { + ret =zynqmp_r5_mem_probe (pdata, nc); +if (ret) { +dev_err (dev,"failed to probe memory %s.\n" , +of_node_full_name (nc)); + goto error; + } + } + +/* Set up DMA mask */ drivers/remoteproc/zynqmp_r5_remoteproc.c line 774
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ ret =dma_set_coherent_mask (dev,DMA_BIT_MASK (32 )); +if (ret) { +dev_warn (dev,"dma_set_coherent_mask failed: %d\n" , ret); +/* If DMA is not configured yet, try to configure it. */ + ret =of_dma_configure (dev, node, true); +if (ret) { +dev_err (dev,"failed to configure DMA.\n" ); + goto error; + } + }It looks like we are following a trial-and-error approach to setup dma coherency. Can we do the right thing without having to try first?
Quoted 3 lines
Specifically, in which circumstances
of_dma_configure()would need to be called again here?
[Ben Levinsky] will remove in v9 - that is only do the dma_set_coherent_mask, that is also done in other remoteproc drivers.
Quoted 13 lines
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +12
-0+/* Get R5 power domain node */ + ret =of_property_read_u32 (node,"pnode-id" , &pdata->pnode_id); +if (ret) { +dev_err (dev,"failed to get power node id.\n" ); + goto error; + } + +/* TODO Check if R5 is running */ + +/* Set up R5 if not already setup */ + ret = pdata->is_r5_mode_set ?0 :r5_set_mode (pdata); +if (ret) {
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 790
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+dev_err (dev,"failed to set R5 operation mode.\n" ); +return ret; + }
is_r5_mode_setis set byr5_set_mode(), which is only called here. So it looks like this check is important in cases wherezynqmp_r5_probe()is called twice for the same R5 node. But I don't think that is supposed to happen?
[Ben Levinsky] this is needed as there are cases where user can repeatedly load different firmware so the check is needed in cases like this where rpu is already configured. It is also possible that a user might repeatedly load/unload the module
Quoted 14 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 798
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+if (!of_get_property (dev->of_node,"mboxes" ,NULL )) { +dev_dbg (dev,"no mailboxes.\n" ); + goto error; + }else { + ret =zynqmp_r5_setup_mbox (pdata, node); +if (ret <0 ) + goto error;Given that mboxes is actually required, I think you should update the device tree example with it. Otherwise, if mboxes is not required, then we shouldn't fail with an error here if it is absent.
[Ben Levinsky] will update so that if no mboxes it will not error out.
Quoted 23 lines
Quoted 23 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +23
-0+ } + +/* Add R5 remoteproc */ + ret =rproc_add (rproc); +if (ret) { +dev_err (dev,"rproc registration failed\n" ); + goto error; + } +return 0 ; +error: +if (pdata->rproc) +rproc_free (pdata->rproc); + pdata->rproc =NULL ; +device_unregister (dev); +put_device (&pdev->dev); +return ret; +} + +static int zynqmp_r5_remoteproc_probe (struct platform_device *pdev) +{ +int ret, i =0 ; +u32 *lockstep_mode; +struct device *dev = &pdev->dev;
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 826
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+struct device_node *nc; +struct zynqmp_r5_pdata *pdata; + + pdata =devm_kzalloc (dev,sizeof (*pdata),GFP_KERNEL ); + lockstep_mode =devm_kzalloc (dev,sizeof (u32 *),GFP_KERNEL );Any reasons why
lockstep_modecouldn't just be a local variable then use&lockstep_modewhen you need a reference?
[Ben Levinsky] good point. Will fix in v9
Quoted 87 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+if (!pdata || !lockstep_mode) +return -ENOMEM ; + +platform_set_drvdata (pdev, pdata); + + of_property_read_u32(dev->of_node, "lockstep-mode",
lockstep_mode);Quoted 79 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +79
-0+ +if (!(*lockstep_mode)) { + rpu_mode =PM_RPU_MODE_SPLIT ; + }else if (*lockstep_mode ==1 ) { + rpu_mode =PM_RPU_MODE_LOCKSTEP ; + }else { +dev_err (dev, +"Invalid lockstep-mode mode provided - %x %d\n" , + *lockstep_mode, rpu_mode); +return -EINVAL ; + } +dev_dbg (dev,"RPU configuration: %s\r\n" , + (*lockstep_mode) ?"lockstep" :"split" ); + +for_each_available_child_of_node (dev->of_node, nc) { + ret =zynqmp_r5_probe (&rpus[i], pdev, nc); +if (ret) { +dev_err (dev,"failed to probe rpu %s.\n" , +of_node_full_name (nc)); +return ret; + } + i++; + } + +return 0 ; +} + +static int zynqmp_r5_remoteproc_remove (struct platform_device *pdev) +{ +int i; + +for (i =0 ; i <MAX_RPROCS ; i++) { +struct zynqmp_r5_pdata *pdata = &rpus[i]; +struct rproc *rproc; + + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + pdata->rproc =NULL ; + } +if (pdata->tx_chan) { +mbox_free_channel (pdata->tx_chan); + pdata->tx_chan =NULL ; + } +if (pdata->rx_chan) { +mbox_free_channel (pdata->rx_chan); + pdata->rx_chan =NULL ; + } + +device_unregister (&pdata->dev); + } + +return 0 ; +} + +/* Match table for OF platform binding */ +static const struct of_device_id zynqmp_r5_remoteproc_match[] = { + { .compatible ="xlnx,zynqmp-r5-remoteproc-1.0" , }, + {/* end of list */ }, +}; +MODULE_DEVICE_TABLE (of, zynqmp_r5_remoteproc_match); + +static struct platform_driver zynqmp_r5_remoteproc_driver = { + .probe = zynqmp_r5_remoteproc_probe, + .remove = zynqmp_r5_remoteproc_remove, + .driver = { + .name ="zynqmp_r5_remoteproc" , + .of_match_table = zynqmp_r5_remoteproc_match, + }, +}; +module_platform_driver (zynqmp_r5_remoteproc_driver); + +module_param_named (autoboot, autoboot, bool,0444 ); +MODULE_PARM_DESC (autoboot, +"enable | disable autoboot. (default: false)" ); + +MODULE_AUTHOR ("Ben Levinsky <[email protected]>" ); +MODULE_LICENSE ("GPL v2" );
RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Quoted 45 lines · Ben Levinsky
Quoted 35 lines
Quoted 35 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +35
-0+/** +* struct zynqmp_r5_mem - zynqmp rpu memory data +* @pnode_id: TCM power domain ids +* @res: memory resource +* @node: list node +*/ +struct zynqmp_r5_mem { +u32 pnode_id[MAX_MEM_PNODES ]; +struct resource res; +struct list_head node; +}; + +/** +* struct zynqmp_r5_pdata - zynqmp rpu remote processor private data +* @dev: device of RPU instance +* @rproc: rproc handle +* @pnode_id: RPU CPU power domain id +* @mems: memory resources +* @is_r5_mode_set: indicate if r5 operation mode is set +* @tx_mc: tx mailbox client +* @rx_mc: rx mailbox client +* @tx_chan: tx mailbox channel +* @rx_chan: rx mailbox channel +* @mbox_work: mbox_work for the RPU remoteproc +* @tx_mc_skbs: socket buffers for tx mailbox client +* @rx_mc_buf: rx mailbox client buffer to save the rx message +*/ +struct zynqmp_r5_pdata { +struct device dev; +struct rproc *rproc; +u32 pnode_id; +struct list_head mems; +bool is_r5_mode_set; +struct mbox_client tx_mc; +struct mbox_client rx_mc;Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 109
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+struct mbox_chan *tx_chan; +struct mbox_chan *rx_chan; +struct work_struct mbox_work; +struct sk_buff_head tx_mc_skbs; +unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX ];A simple small reordering of the struct fields would lead to small memory savings due to alignment.
Quoted 1 line
[Ben Levinsky] will do. Do you mean ordering in either ascending or descending order?
Each field has a different alignment in the struct, so for example after pnode_id there are probably 4 unused bytes because mems is 64bit aligned.
Quoted 46 lines
Quoted 46 lines
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +12
-0+/* +* TCM needs mapping to R5 relative address and xilinx platform mgmt call +*/ +struct rproc_mem_entry *handle_tcm_parsing (struct device *dev, +struct reserved_mem *rmem, +struct device_node *node, +int lookup_idx) +{ +void *va; +dma_addr_t dma; +resource_size_t size; +int ret;drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+u32 pnode_id; +struct resource rsc; +struct rproc_mem_entry *mem; + + pnode_id = tcm_addr_to_pnode[lookup_idx][1 ]; + ret =zynqmp_pm_request_node (pnode_id, +ZYNQMP_PM_CAPABILITY_ACCESS ,0 , +ZYNQMP_PM_REQUEST_ACK_BLOCKING ); +if (ret <0 ) { + dev_err(dev, "failed to request power node: %u\n",
pnode_id);Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+return -EINVAL ; + } + + ret =of_address_to_resource (node,0 , &rsc); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ;drivers/remoteproc/zynqmp_r5_remoteproc.c line 303
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ } + size =resource_size (&rsc); + va =devm_ioremap_wc (dev, rsc.start, size); +if (!va) +return -ENOMEM ; + +/* zero out tcm base address */ +if (rsc.start &0xffe00000 ) { +/* R5 can't see anything past 0xfffff so wipe it */ + rsc.start &=0x000fffff ;If that is the case why not do:
rsc.start &= 0x000fffff;
Quoted 8 lines
Quoted 6 lines
unconditionally? if (rsc.start &
0xffe00000) is superfluous.But I thought that actually the R5s could see TCM at both the low address (<
0x000fffff) and also at the high address (i.e.0xffe00000).[Ben Levinsky] Here yes can make rsc.start &=
0x000fffffundconditional. Will update in v9 as suchAlso, this logic is because this is only for the Xilinx R5 mappings of TCM banks that are at (from the TCM point of view) 0x0 and 0x2000
What I meant is that as far as I understand from the TRM the RPU should also be able to access the same banks at the same address of the APU, which I imagine is in the 0xffe00000 range. But I could be wrong on this.
If we could use the same addresses for RPU and APU, it would simplify this driver.
Quoted 11 lines
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 309
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+/* +* handle tcm banks 1 a and b (0xffe9000 and +* 0xffeb0000) +*/ +if (rsc.start &0x80000 ) + rsc.start -=0x90000 ;It is very unclear to me why we have to do this
[Ben Levinsky] This is for TCM bank 0B and bank 1B to map them to
0x00020000so that the TCM relative addressing lines up. For example (0xffe90000&0x000fffff) -0x90000=0x20000
Could you please explain the mapping in an in-code comment? The comment currently mentions 0xffe9000 and 0xffeb0000 but:
- 0xffe9000 & 0x000fffff = 0xe9000
0xe9000 - 0x90000 = 0x59000
- 0xffeb0000 & 0x000fffff = 0xeb000
0xeb000 - 0x90000 = 0xeb000
Either way we don't get 0x20000. What am I missing?
Quoted 9 lines
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 312
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+ } + + dma = (dma_addr_t )rsc.start;Given the way the dma parameter is used by
rproc_alloc_registered_carveouts, I think it might be best to pass the original start address (i.e.0xffe00000) as dma.
Quoted 8 lines
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 314
Quoted 2 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +2
-0+ mem = rproc_mem_entry_init(dev, va, dma, (int)size, rsc.start, + NULL, zynqmp_r5_mem_release,I don't know too much about the remoteproc APIs, but shouldn't you be passing
zynqmp_r5_rproc_mem_allocto it instead of NULL?[Ben Levinsky] the difference is that for TCM we have to do make the relative address work for TCM, so the dma input to
rproc_mem_entry_initis different in TCM case.
The dma address is the address as seen by the RPU, is that right? So you are trying to set the dma address to something in the range 0 - 0x20000?
Quoted 41 lines
Quoted 17 lines
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ rsc.name); +if (!mem) +return -ENOMEM ; + +return mem; +}drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ +static int parse_mem_regions (struct rproc *rproc) +{ +int num_mems, i; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; +struct device_node *np = dev->of_node; +struct rproc_mem_entry *mem; + + num_mems = of_count_phandle_with_args(np, "memory-region",NULL);
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 337
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+if (num_mems <=0 ) +return 0 ; +for (i =0 ; i < num_mems; i++) { +struct device_node *node; +struct reserved_mem *rmem; + + node =of_parse_phandle (np,"memory-region" , i);Check node != NULL ?
[Ben Levinsky] will add this in v9
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 344
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+ rmem =of_reserved_mem_lookup (node); +if (!rmem) { +dev_err (dev,"unable to acquire memory-region\n" ); +return -EINVAL ; + } + +if (strstr (node->name,"vdev0buffer" )) {vdev0buffer is not described in the device tree bindings, is that normal/expected?
Quoted 1 line
[Ben Levinsky] vdev0buffer is not required, as there might be simple firmware loading with no IPC. Vdev0buffer only needed for IPC.
OK, good. It should probably still be described in the device tree binding as optional property.
Quoted 31 lines
Quoted 19 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+/* Register DMA region */ + mem =rproc_mem_entry_init (dev,NULL , + (dma_addr_t )rmem->base, + rmem->size, rmem->base, +NULL ,NULL , +"vdev0buffer" ); +if (!mem) { + dev_err(dev, "unable to initialize memory-region %s\n",
drivers/remoteproc/zynqmp_r5_remoteproc.c line 358
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ node->name); +return -ENOMEM ; + } +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma); + }else if (strstr (node->name,"vdev0vring" )) {Same here
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 366
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+int vring_id; +char name[16 ]; + +/* +* can be 1 of multiple vring IDs per IPC channel +* e.g. 'vdev0vring0' and 'vdev0vring1' +*/ + vring_id = node->name[14 ] -'0' ;Where does the "14" comes from? Are we sure it is not possible to have a
node->namesmaller than 14 chars?
Quoted 1 line
[Ben Levinsky] Presently there are only 2 vrings used per Xilinx OpenAMP channel to RPU. In Xilinx kernel we have hard-coded names as these are the only nodes that use it. For example RPU0vdev0vring0 and RPU1vdev0vring0. Hence we only check for vdev0vring and not a
sscanf("%*s%i")to parse out the vring ID or other, cleaner solution.
OK, but I think it is best if we use node->name[14] only if we explicitly check for a string of at least 14 characters. Using strstr, it could return the string "vdev0vring" which is less than 14 chars, leading to a bug.
Quoted 88 lines
Quoted 23 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ snprintf(name, sizeof(name), "vdev0vring%d",
vring_id);drivers/remoteproc/zynqmp_r5_remoteproc.c line 379
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/* Register vring */ + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem->base, + rmem->size, rmem->base, +
zynqmp_r5_rproc_mem_alloc,Quoted 1 line
+
zynqmp_r5_rproc_mem_release,drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +11
-0+ name); +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma); + }else { +int idx; + +/* +* if TCM update address space for R5 and +* make xilinx platform mgmt call +*/ + for (idx = 0; idx < ZYNQMP_R5_NUM_TCM_BANKS;idx++) {
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +1
-0+ if (tcm_addr_to_pnode[idx][0] == rmem-base)
(quoted diff) +1
-0+ break;There is something I don't quite understand. If the memory region to use is TCM, why would it be also described under reserved-memory? Reserved-memory is for normal memory being reserved, while TCM is not normal memory. Am I missing something?
[Ben Levinsky] I can change this in v9 as discussed. That is, have no TCM under reserved mem. Instead have the banks as nodes in device tree with status="[enabled|disabled]" and if enabled, then try to add memories in
parse_fwcall.Quoted 46 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+ } + +if (idx !=ZYNQMP_R5_NUM_TCM_BANKS ) { + mem = handle_tcm_parsing(dev, rmem, node,idx);
drivers/remoteproc/zynqmp_r5_remoteproc.c line 399
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+ }else { + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem->base, + rmem->size, rmem->base, +
zynqmp_r5_rproc_mem_alloc,Quoted 1 line
+
zynqmp_r5_rproc_mem_release,Review
Quoted 1 line
(quoted diff) +1
-0+ node->name);This case looks identical to the vdev0vring above. Is the difference
really just in the "name"? If so, can we merge the two cases into one?
no, because thedevm_ioremap_wccall has to be done before changing the dma address to relative for TCM banks.Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + +if (!mem) { +dev_err (dev, +"unable to init memory-region %s\n" , + node->name);drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+return -ENOMEM ; + } + } +rproc_add_carveout (rproc, mem); + } + +return 0 ; +} + +static int zynqmp_r5_parse_fw(struct rproc *rproc, const struct firmware*fw)
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+{ +int ret; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; + + ret =parse_mem_regions (rproc); +if (ret) { +dev_err (dev,"parse_mem_regions failed %x\n" , ret); +return ret;Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 428
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + + ret =rproc_elf_load_rsc_table (rproc, fw); +if (ret == -EINVAL ) { +dev_info (dev,"no resource table found.\n" ); + ret =0 ;Why do we want to continue ignoring the error in this case?
Quoted 1 line
[Ben Levinsky] as there can be simple firmware loaded onto R5 that do not have resource table. Resource table only needed for specific IPC case.
OK, an in-code comment would be good
Quoted 104 lines
Quoted 95 lines
Quoted 95 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +95
-0+struct device *dev; +struct zynqmp_r5_mem *mem; +int ret; +struct property *prop; +const __be32 *cur; +u32 val; +int i; + + dev = &pdata->dev; + mem =devm_kzalloc (dev,sizeof (*mem),GFP_KERNEL ); +if (!mem) +return -ENOMEM ; + ret =of_address_to_resource (node,0 , &mem->res); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ; + } + +/* Get the power domain id */ + i =0 ; +if (of_find_property (node,"pnode-id" ,NULL )) { +of_property_for_each_u32 (node,"pnode-id" , prop, cur, val) + mem->pnode_id[i++] = val; + } +list_add_tail (&mem->node, &pdata->mems); +return 0 ; +} + +/** +* zynqmp_r5_release() - ZynqMP R5 device release function +* @dev: pointer to the device struct of ZynqMP R5 +* +* Function to release ZynqMP R5 device. +*/ +static void zynqmp_r5_release (struct device *dev) +{ +struct zynqmp_r5_pdata *pdata; +struct rproc *rproc; +struct sk_buff *skb; + + pdata =dev_get_drvdata (dev); + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + } +if (pdata->tx_chan) +mbox_free_channel (pdata->tx_chan); +if (pdata->rx_chan) +mbox_free_channel (pdata->rx_chan); +/* Discard all SKBs */ +while (!skb_queue_empty (&pdata->tx_mc_skbs)) { + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); + } + +put_device (dev->parent); +} + +/** +* event_notified_idr_cb() - event notified idr callback +* @id: idr id +* @ptr: pointer to idr private data +* @data: data passed to idr_for_each callback +* +* Pass notification to remoteproc virtio +* +* Return: 0. having return is to satisfy the idr_for_each() function +* pointer input argument requirement. +**/ +static int event_notified_idr_cb (int id,void *ptr,void *data) +{ +struct rproc *rproc = data; + + (void )rproc_vq_interrupt (rproc, id); +return 0 ; +} + +/** +* handle_event_notified() - remoteproc notification work funciton +* @work: pointer to the work structure +* +* It checks each registered remoteproc notify IDs. +*/ +static void handle_event_notified (struct work_struct *work) +{ +struct rproc *rproc; +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(work, struct zynqmp_r5_pdata, mbox_work); + + (void )mbox_send_message (pdata->rx_chan,NULL ); + rproc = pdata->rproc; +/* Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 602
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+* We only use IPI for interrupt. The firmware side may or may +* not write the notifyid when it trigger IPI. +* And thus, we scan through all the registered notifyids. +*/ +idr_for_each (&rproc->notifyids, event_notified_idr_cb, rproc);This looks expensive. Should we at least check whether the notifyid was written as first thing before doing the scan?
Quoted 1 line
[Ben Levinsky] this will be at most 2 vrings presently per firmware-load and only done when the firmware is loaded so the latency so should not impact performace or user
OK
Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +12
-0+/* Get R5 power domain node */ + ret =of_property_read_u32 (node,"pnode-id" , &pdata->pnode_id); +if (ret) { +dev_err (dev,"failed to get power node id.\n" ); + goto error; + } + +/* TODO Check if R5 is running */ + +/* Set up R5 if not already setup */ + ret = pdata->is_r5_mode_set ?0 :r5_set_mode (pdata); +if (ret) {Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 790
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+dev_err (dev,"failed to set R5 operation mode.\n" ); +return ret; + }
is_r5_mode_setis set byr5_set_mode(), which is only called here. So it looks like this check is important in cases wherezynqmp_r5_probe()is called twice for the same R5 node. But I don't think that is supposed to happen?
Quoted 1 line
[Ben Levinsky] this is needed as there are cases where user can repeatedly load different firmware so the check is needed in cases like this where rpu is already configured. It is also possible that a user might repeatedly load/unload the module
OK
RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Quoted 60 lines
-----Original Message-----
From: Stefano Stabellini <[email protected]>
Sent: Wednesday, August 19, 2020 2:21 PM
To: Ben Levinsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 45 lines · Ben Levinsky
Quoted 45 lines
Quoted 30 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +30
-0+/** +* struct zynqmp_r5_mem - zynqmp rpu memory data +* @pnode_id: TCM power domain ids +* @res: memory resource +* @node: list node +*/ +struct zynqmp_r5_mem { +u32 pnode_id[MAX_MEM_PNODES ]; +struct resource res; +struct list_head node; +}; + +/** +* struct zynqmp_r5_pdata - zynqmp rpu remote processor private data +* @dev: device of RPU instance +* @rproc: rproc handle +* @pnode_id: RPU CPU power domain id +* @mems: memory resources +* @is_r5_mode_set: indicate if r5 operation mode is set +* @tx_mc: tx mailbox client +* @rx_mc: rx mailbox client +* @tx_chan: tx mailbox channel +* @rx_chan: rx mailbox channel +* @mbox_work: mbox_work for the RPU remoteproc +* @tx_mc_skbs: socket buffers for tx mailbox client +* @rx_mc_buf: rx mailbox client buffer to save the rx message +*/ +struct zynqmp_r5_pdata { +struct device dev; +struct rproc *rproc;drivers/remoteproc/zynqmp_r5_remoteproc.c line 109
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+u32 pnode_id; +struct list_head mems; +bool is_r5_mode_set; +struct mbox_client tx_mc; +struct mbox_client rx_mc; +struct mbox_chan *tx_chan; +struct mbox_chan *rx_chan; +struct work_struct mbox_work; +struct sk_buff_head tx_mc_skbs; +unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX ];A simple small reordering of the struct fields would lead to small memory savings due to alignment.
Quoted 8 lines
Quoted 1 line
[Ben Levinsky] will do. Do you mean ordering in either ascending or
descending order?
Each field has a different alignment in the struct, so for example after
pnode_idthere are probably 4 unused bytes because mems is 64bit aligned.
[Ben Levinsky] ok will update this so the alignments are done with less unused memory per struct allocation.
Quoted 85 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+/*
+ * TCM needs mapping to R5 relative address and xilinx platform mgmtcall
Quoted 44 lines
Quoted 44 lines
Quoted 10 lines
+ */
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+struct rproc_mem_entry *handle_tcm_parsing (struct device *dev, +struct reserved_mem *rmem, +struct device_node *node, +int lookup_idx) +{ +void *va; +dma_addr_t dma; +resource_size_t size; +int ret;drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+u32 pnode_id; +struct resource rsc; +struct rproc_mem_entry *mem; + + pnode_id = tcm_addr_to_pnode[lookup_idx][1 ]; + ret =zynqmp_pm_request_node (pnode_id, +ZYNQMP_PM_CAPABILITY_ACCESS ,0 , +ZYNQMP_PM_REQUEST_ACK_BLOCKING ); +if (ret <0 ) { + dev_err(dev, "failed to request power node: %u\n",
pnode_id);Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+return -EINVAL ; + } + + ret =of_address_to_resource (node,0 , &rsc); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ;drivers/remoteproc/zynqmp_r5_remoteproc.c line 303
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ } + size =resource_size (&rsc); + va =devm_ioremap_wc (dev, rsc.start, size); +if (!va) +return -ENOMEM ; + +/* zero out tcm base address */ +if (rsc.start &0xffe00000 ) { +/* R5 can't see anything past 0xfffff so wipe it */ + rsc.start &=0x000fffff ;If that is the case why not do:
rsc.start &= 0x000fffff;Quoted 7 lines
Quoted 6 lines
unconditionally? if (rsc.start &
0xffe00000) is superfluous.But I thought that actually the R5s could see TCM at both the low address (<
0x000fffff) and also at the high address (i.e.0xffe00000).[Ben Levinsky] Here yes can make rsc.start &=
0x000fffffundconditional. Willupdate in v9 as such
Quoted 1 line
Also, this logic is because this is only for the Xilinx R5
mappings of TCM banks that are at (from the TCM point of view)
0x0and0x2000What I meant is that as far as I understand from the TRM the RPU should also be able to access the same banks at the same address of the APU, which I imagine is in the
0xffe00000range. But I could be wrong on this.If we could use the same addresses for RPU and APU, it would simplify this driver.
Quoted 11 lines
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 309
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+/* +* handle tcm banks 1 a and b (0xffe9000 and +* 0xffeb0000) +*/ +if (rsc.start &0x80000 ) + rsc.start -=0x90000 ;It is very unclear to me why we have to do this
[Ben Levinsky] This is for TCM bank 0B and bank 1B to map them to
0x00020000so that the TCM relative addressing lines up. For example (0xffe90000&0x000fffff) -0x90000=0x20000Could you please explain the mapping in an in-code comment? The comment currently mentions
0xffe9000and0xffeb0000but:
Quoted 8 lines
-
0xffe9000&0x000fffff=0xe9000
0xe9000-0x90000=0x59000-
0xffeb0000&0x000fffff=0xeb000
0xeb000-0x90000=0xeb000Either way we don't get
0x20000. What am I missing?
[Ben Levinsky] I apologize there is typo in the comment... it should be 0xffe90000 and 0xffeb0000 The output is:0xffe90000 & 0x000fffff = 0x900000x90000 - 0x90000 = 0x0
And0xffeb0000 & 0x000fffff = 0xB00000xB0000 - 0x90000 = 0x20000
So these line up for the relative addressing for RPU's view of TCMs
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 312
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+ } + + dma = (dma_addr_t )rsc.start;Given the way the dma parameter is used by
rproc_alloc_registered_carveouts, I think it might be best to pass the original start address (i.e.0xffe00000) as dma.drivers/remoteproc/zynqmp_r5_remoteproc.c line 314
Quoted 2 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +2
-0+ mem = rproc_mem_entry_init(dev, va, dma, (int)size, rsc.start, + NULL, zynqmp_r5_mem_release,I don't know too much about the remoteproc APIs, but shouldn't you be passing
zynqmp_r5_rproc_mem_allocto it instead of NULL?
Quoted 9 lines
Quoted 1 line
[Ben Levinsky] the difference is that for TCM we have to do make the
relative address work for TCM, so the dma input to
rproc_mem_entry_initis different in TCM case.The dma address is the address as seen by the RPU, is that right? So you are trying to set the dma address to something in the range 0 -
0x20000?
[Ben Levinsky] yes
Quoted 328 lines
Quoted 41 lines
Quoted 17 lines
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ rsc.name); +if (!mem) +return -ENOMEM ; + +return mem; +}drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ +static int parse_mem_regions (struct rproc *rproc) +{ +int num_mems, i; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; +struct device_node *np = dev->of_node; +struct rproc_mem_entry *mem; + + num_mems = of_count_phandle_with_args(np, "memory-region",NULL);
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 337
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+if (num_mems <=0 ) +return 0 ; +for (i =0 ; i < num_mems; i++) { +struct device_node *node; +struct reserved_mem *rmem; + + node =of_parse_phandle (np,"memory-region" , i);Check node != NULL ?
[Ben Levinsky] will add this in v9
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 344
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+ rmem =of_reserved_mem_lookup (node); +if (!rmem) { +dev_err (dev,"unable to acquire memory-region\n" ); +return -EINVAL ; + } + +if (strstr (node->name,"vdev0buffer" )) {vdev0buffer is not described in the device tree bindings, is that normal/expected?
Quoted 1 line
[Ben Levinsky] vdev0buffer is not required, as there might be simple
firmware loading with no IPC. Vdev0buffer only needed for IPC.
OK, good. It should probably still be described in the device tree binding as optional property.
Quoted 31 lines
Quoted 19 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+/* Register DMA region */ + mem =rproc_mem_entry_init (dev,NULL , + (dma_addr_t )rmem->base, + rmem->size, rmem->base, +NULL ,NULL , +"vdev0buffer" ); +if (!mem) { + dev_err(dev, "unable to initialize memory-region %s\n",
drivers/remoteproc/zynqmp_r5_remoteproc.c line 358
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ node->name); +return -ENOMEM ; + } +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma); + }else if (strstr (node->name,"vdev0vring" )) {Same here
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 366
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+int vring_id; +char name[16 ]; + +/* +* can be 1 of multiple vring IDs per IPC channel +* e.g. 'vdev0vring0' and 'vdev0vring1' +*/ + vring_id = node->name[14 ] -'0' ;Where does the "14" comes from? Are we sure it is not possible to have a
node->namesmaller than 14 chars?Quoted 1 line
[Ben Levinsky] Presently there are only 2 vrings used per Xilinx OpenAMP
channel to RPU. In Xilinx kernel we have hard-coded names as these are the only nodes that use it. For example RPU0vdev0vring0 and RPU1vdev0vring0. Hence we only check for vdev0vring and not a
sscanf("%*s%i")to parse out the vring ID or other, cleaner solution.OK, but I think it is best if we use node->name[14] only if we explicitly check for a string of at least 14 characters. Using strstr, it could return the string "vdev0vring" which is less than 14 chars, leading to a bug.
Quoted 32 lines
Quoted 23 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ snprintf(name, sizeof(name), "vdev0vring%d",
vring_id);drivers/remoteproc/zynqmp_r5_remoteproc.c line 379
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+/* Register vring */ + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem->base, + rmem->size, rmem->base, +
zynqmp_r5_rproc_mem_alloc,Quoted 1 line
+
zynqmp_r5_rproc_mem_release,drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +11
-0+ name); +dev_dbg (dev,"parsed %s at %llx\r\n" , mem->name, + mem->dma); + }else { +int idx; + +/* +* if TCM update address space for R5 and +* make xilinx platform mgmt call +*/ + for (idx = 0; idx < ZYNQMP_R5_NUM_TCM_BANKS;idx++) {
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +1
-0+ if (tcm_addr_to_pnode[idx][0] == rmem-base)
(quoted diff) +1
-0+ break;There is something I don't quite understand. If the memory region to use is TCM, why would it be also described under reserved-memory? Reserved-memory is for normal memory being reserved, while TCM is not normal memory. Am I missing something?
Quoted 1 line
[Ben Levinsky] I can change this in v9 as discussed. That is, have no TCM
under reserved mem. Instead have the banks as nodes in device tree with status="[enabled|disabled]" and if enabled, then try to add memories in
parse_fwcall.Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+ } + +if (idx !=ZYNQMP_R5_NUM_TCM_BANKS ) { + mem = handle_tcm_parsing(dev, rmem, node,idx);
drivers/remoteproc/zynqmp_r5_remoteproc.c line 399
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+ }else { + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem->base, + rmem->size, rmem->base, +
zynqmp_r5_rproc_mem_alloc,Quoted 7 lines
Quoted 7 lines
Quoted 1 line
+
zynqmp_r5_rproc_mem_release,Review
Quoted 1 line
(quoted diff) +1
-0+ node->name);This case looks identical to the vdev0vring above. Is the difference really just in the "name"? If so, can we merge the two cases into one? no, because the
devm_ioremap_wccall has to be done before changingthe dma address to relative for TCM banks.
Quoted 17 lines
Quoted 17 lines
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + +if (!mem) { +dev_err (dev, +"unable to init memory-region %s\n" , + node->name);drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+return -ENOMEM ; + } + } +rproc_add_carveout (rproc, mem); + } + +return 0 ; +} + +static int zynqmp_r5_parse_fw(struct rproc *rproc, const structfirmware
Quoted 19 lines
Quoted 10 lines
*fw)
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+{ +int ret; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; + + ret =parse_mem_regions (rproc); +if (ret) { +dev_err (dev,"parse_mem_regions failed %x\n" , ret); +return ret;Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 428
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + + ret =rproc_elf_load_rsc_table (rproc, fw); +if (ret == -EINVAL ) { +dev_info (dev,"no resource table found.\n" ); + ret =0 ;Why do we want to continue ignoring the error in this case?
Quoted 1 line
[Ben Levinsky] as there can be simple firmware loaded onto R5 that do not
have resource table. Resource table only needed for specific IPC case.
OK, an in-code comment would be good
Quoted 104 lines
Quoted 95 lines
Quoted 95 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +95
-0+struct device *dev; +struct zynqmp_r5_mem *mem; +int ret; +struct property *prop; +const __be32 *cur; +u32 val; +int i; + + dev = &pdata->dev; + mem =devm_kzalloc (dev,sizeof (*mem),GFP_KERNEL ); +if (!mem) +return -ENOMEM ; + ret =of_address_to_resource (node,0 , &mem->res); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ; + } + +/* Get the power domain id */ + i =0 ; +if (of_find_property (node,"pnode-id" ,NULL )) { +of_property_for_each_u32 (node,"pnode-id" , prop, cur, val) + mem->pnode_id[i++] = val; + } +list_add_tail (&mem->node, &pdata->mems); +return 0 ; +} + +/** +* zynqmp_r5_release() - ZynqMP R5 device release function +* @dev: pointer to the device struct of ZynqMP R5 +* +* Function to release ZynqMP R5 device. +*/ +static void zynqmp_r5_release (struct device *dev) +{ +struct zynqmp_r5_pdata *pdata; +struct rproc *rproc; +struct sk_buff *skb; + + pdata =dev_get_drvdata (dev); + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + } +if (pdata->tx_chan) +mbox_free_channel (pdata->tx_chan); +if (pdata->rx_chan) +mbox_free_channel (pdata->rx_chan); +/* Discard all SKBs */ +while (!skb_queue_empty (&pdata->tx_mc_skbs)) { + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); + } + +put_device (dev->parent); +} + +/** +* event_notified_idr_cb() - event notified idr callback +* @id: idr id +* @ptr: pointer to idr private data +* @data: data passed to idr_for_each callback +* +* Pass notification to remoteproc virtio +* +* Return: 0. having return is to satisfy the idr_for_each() function +* pointer input argument requirement. +**/ +static int event_notified_idr_cb (int id,void *ptr,void *data) +{ +struct rproc *rproc = data; + + (void )rproc_vq_interrupt (rproc, id); +return 0 ; +} + +/** +* handle_event_notified() - remoteproc notification work funciton +* @work: pointer to the work structure +* +* It checks each registered remoteproc notify IDs. +*/ +static void handle_event_notified (struct work_struct *work) +{ +struct rproc *rproc; +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(work, struct zynqmp_r5_pdata, mbox_work); + + (void )mbox_send_message (pdata->rx_chan,NULL ); + rproc = pdata->rproc; +/* Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 602
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+* We only use IPI for interrupt. The firmware side may or may +* not write the notifyid when it trigger IPI. +* And thus, we scan through all the registered notifyids. +*/ +idr_for_each (&rproc->notifyids, event_notified_idr_cb, rproc);This looks expensive. Should we at least check whether the notifyid was written as first thing before doing the scan?
Quoted 1 line
[Ben Levinsky] this will be at most 2 vrings presently per firmware-load and
only done when the firmware is loaded so the latency so should not impact performace or user
OK
Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +12
-0+/* Get R5 power domain node */ + ret =of_property_read_u32 (node,"pnode-id" , &pdata->pnode_id); +if (ret) { +dev_err (dev,"failed to get power node id.\n" ); + goto error; + } + +/* TODO Check if R5 is running */ + +/* Set up R5 if not already setup */ + ret = pdata->is_r5_mode_set ?0 :r5_set_mode (pdata); +if (ret) {Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 790
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+dev_err (dev,"failed to set R5 operation mode.\n" ); +return ret; + }
is_r5_mode_setis set byr5_set_mode(), which is only called here. So it looks like this check is important in cases wherezynqmp_r5_probe()is called twice for the same R5 node. But I don't think that is supposed to happen?Quoted 1 line
[Ben Levinsky] this is needed as there are cases where user can repeatedly
load different firmware so the check is needed in cases like this where rpu is already configured. It is also possible that a user might repeatedly load/unload the module
OK
RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Hi Stefano
This is just response to few unanswered comments
Thanks
Ben
Quoted 135 lines
-----Original Message-----
From: [email protected] <linux-remoteproc- [email protected]> On Behalf Of Ben Levinsky
Sent: Thursday, August 20, 2020 8:13 AM
To: Stefano Stabellini <[email protected]>
Cc: Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverQuoted 14 lines
-----Original Message-----
From: Stefano Stabellini <[email protected]>
Sent: Wednesday, August 19, 2020 2:21 PM
To: Ben Levinsky <[email protected]>
Cc: Stefano Stabellini <[email protected]>; Michal Simek <[email protected]>; [email protected]; [email protected]; Ed T. Mooring <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Jiaying Liang <[email protected]>; linux-arm- [email protected]
Subject: RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driverOn Tue, 18 Aug 2020, Ben Levinsky wrote:
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
+/**
+ * structzynqmp_r5_mem- zynqmp rpu memory data
+ * @pnode_id: TCM power domain ids
+ * @res: memory resource
+ * @node: list node
+ */Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+struct zynqmp_r5_mem { +u32 pnode_id[MAX_MEM_PNODES ]; +struct resource res; +struct list_head node; +}; + +/** +* struct zynqmp_r5_pdata - zynqmp rpu remote processor private data
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
Quoted 16 lines
+ * @dev: device of RPU instance
+ * @rproc: rproc handle
+ * @pnode_id: RPU CPU power domain id
+ * @mems: memory resources
+ * @is_r5_mode_set: indicate if r5 operation mode is set
+ * @tx_mc: tx mailbox client
+ * @rx_mc: rx mailbox client
+ * @tx_chan: tx mailbox channel
+ * @rx_chan: rx mailbox channel
+ * @mbox_work:mbox_workfor the RPU remoteproc
+ * @tx_mc_skbs: socket buffers for tx mailbox client
+ * @rx_mc_buf: rx mailbox client buffer to save the rx message
+ */drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+struct zynqmp_r5_pdata { +struct device dev; +struct rproc *rproc;drivers/remoteproc/zynqmp_r5_remoteproc.c line 109
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+u32 pnode_id; +struct list_head mems; +bool is_r5_mode_set; +struct mbox_client tx_mc; +struct mbox_client rx_mc; +struct mbox_chan *tx_chan; +struct mbox_chan *rx_chan; +struct work_struct mbox_work; +struct sk_buff_head tx_mc_skbs; +unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX ];A simple small reordering of the struct fields would lead to small memory savings due to alignment.
Quoted 8 lines
Quoted 1 line
[Ben Levinsky] will do. Do you mean ordering in either ascending or
descending order?
Each field has a different alignment in the struct, so for example after
pnode_idthere are probably 4 unused bytes because mems is 64bit aligned.[Ben Levinsky] ok will update this so the alignments are done with less unused memory per struct allocation.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+/*
+ * TCM needs mapping to R5 relative address and xilinx platformmgmt
Quoted 4 lines
call
Quoted 3 lines
Quoted 3 lines
Review
Quoted 3 lines
+ */
(quoted diff) +2
-0+struct rproc_mem_entry *handle_tcm_parsing(struct device *dev, + struct reserved_mem*rmem,
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+struct device_node *node, + int lookup_idx) +{ + void *va; +dma_addr_t dma; +resource_size_t size;Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+int ret; +u32 pnode_id; +struct resource rsc; +struct rproc_mem_entry *mem; + + pnode_id = tcm_addr_to_pnode[lookup_idx][1 ]; + ret = zynqmp_pm_request_node(pnode_id, + ZYNQMP_PM_CAPABILITY_ACCESS,0,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
+
ZYNQMP_PM_REQUEST_ACK_BLOCKING);Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (ret < 0) { + dev_err(dev, "failed to request power node: %u\n",
pnode_id);Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+return -EINVAL ; + } + + ret =of_address_to_resource (node,0 , &rsc); +if (ret <0 ) { +dev_err (dev,"failed to get resource of memory %s" , +of_node_full_name (node)); +return -EINVAL ;drivers/remoteproc/zynqmp_r5_remoteproc.c line 303
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+ } + size =resource_size (&rsc); + va =devm_ioremap_wc (dev, rsc.start, size); +if (!va) +return -ENOMEM ; + +/* zero out tcm base address */ +if (rsc.start &0xffe00000 ) { +/* R5 can't see anything past 0xfffff so wipe it */ + rsc.start &=0x000fffff ;If that is the case why not do:
rsc.start &= 0x000fffff;
Quoted 22 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
unconditionally? if (rsc.start &
0xffe00000) is superfluous.But I thought that actually the R5s could see TCM at both the low address (<
0x000fffff) and also at the high address (i.e.0xffe00000).[Ben Levinsky] Here yes can make rsc.start &=
0x000fffffundconditional.Will
Quoted 14 lines
update in v9 as such
Quoted 1 line
Also, this logic is because this is only for the Xilinx R5
mappings of TCM banks that are at (from the TCM point of view)
0x0and0x2000What I meant is that as far as I understand from the TRM the RPU should also be able to access the same banks at the same address of the APU, which I imagine is in the
0xffe00000range. But I could be wrong on this.If we could use the same addresses for RPU and APU, it would simplify this driver.
[Ben Levinsky] right for R5 it is slightly different as it is 32bit so the device address values need to be adjusted
Quoted 212 lines
Quoted 17 lines
Quoted 11 lines
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 309
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+/* +* handle tcm banks 1 a and b (0xffe9000 and +* 0xffeb0000) +*/ +if (rsc.start &0x80000 ) + rsc.start -=0x90000 ;It is very unclear to me why we have to do this
[Ben Levinsky] This is for TCM bank 0B and bank 1B to map them to
0x00020000so that the TCM relative addressing lines up. For example (0xffe90000&0x000fffff) -0x90000=0x20000Could you please explain the mapping in an in-code comment? The comment currently mentions
0xffe9000and0xffeb0000but:Quoted 8 lines
-
0xffe9000&0x000fffff=0xe9000
0xe9000-0x90000=0x59000-
0xffeb0000&0x000fffff=0xeb000
0xeb000-0x90000=0xeb000Either way we don't get
0x20000. What am I missing?[Ben Levinsky] I apologize there is typo in the comment... it should be
0xffe90000and0xffeb0000
The output is:0xffe90000&0x000fffff=0x900000x90000-0x90000=0x0And
0xffeb0000&0x000fffff=0xB00000xB0000-0x90000=0x20000So these line up for the relative addressing for RPU's view of TCMs
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 312
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+ } + + dma = (dma_addr_t )rsc.start;Given the way the dma parameter is used by
rproc_alloc_registered_carveouts, I think it might be best to pass the original start address (i.e.0xffe00000) as dma.drivers/remoteproc/zynqmp_r5_remoteproc.c line 314
Quoted 2 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +2
-0+ mem = rproc_mem_entry_init(dev, va, dma, (int)size, rsc.start, + NULL, zynqmp_r5_mem_release,I don't know too much about the remoteproc APIs, but shouldn't you be passing
zynqmp_r5_rproc_mem_allocto it instead of NULL?Quoted 9 lines
Quoted 1 line
[Ben Levinsky] the difference is that for TCM we have to do make the
relative address work for TCM, so the dma input to
rproc_mem_entry_initis different in TCM case.The dma address is the address as seen by the RPU, is that right? So you are trying to set the dma address to something in the range 0 -
0x20000?[Ben Levinsky] yes
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+ rsc.name); +if (!mem) +return -ENOMEM ; + +return mem; +} + +static int parse_mem_regions (struct rproc *rproc)Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+{ +int num_mems, i; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; +struct device_node *np = dev->of_node; +struct rproc_mem_entry *mem; + + num_mems = of_count_phandle_with_args(np, "memory-region",
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
NULL);
drivers/remoteproc/zynqmp_r5_remoteproc.c line 337
Quoted 7 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +7
-0+if (num_mems <=0 ) +return 0 ; +for (i =0 ; i < num_mems; i++) { +struct device_node *node; +struct reserved_mem *rmem; + + node =of_parse_phandle (np,"memory-region" , i);Check node != NULL ?
Quoted 5 lines
Quoted 5 lines
[Ben Levinsky] will add this in v9
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+ rmem =of_reserved_mem_lookup (node); +if (!rmem) { + dev_err(dev, "unable to acquire memory-region\n");
Quoted 20 lines
Quoted 10 lines
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 344
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+return -EINVAL ; + } + +if (strstr (node->name,"vdev0buffer" )) {vdev0buffer is not described in the device tree bindings, is that normal/expected?
[Ben Levinsky] vdev0buffer is not required, as there might be simple
firmware loading with no IPC. Vdev0buffer only needed for IPC.
OK, good. It should probably still be described in the device tree binding as optional property.
Quoted 3 lines
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+/* Register DMA region */ + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem-base,
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1
-0+ rmem->size, rmem-base,
Quoted 4 lines
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+NULL ,NULL , + "vdev0buffer"); +if (!mem) { + dev_err(dev, "unable to initializememory-
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
region %s\n",
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 4 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+ node->name); +return -ENOMEM ; + } + dev_dbg(dev, "parsed %s at %llx\r\n", mem-name,
Quoted 6 lines
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 358
Quoted 2 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +2
-0+ mem->dma); + }else if (strstr (node->name,"vdev0vring" )) {Same here
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+int vring_id; +char name[16 ]; + +/* +* can be 1 of multiple vring IDs per IPC channel
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 366
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+* e.g. 'vdev0vring0' and 'vdev0vring1' +*/ + vring_id = node->name[14 ] -'0' ;Where does the "14" comes from? Are we sure it is not possible to have
a
Quoted 30 lines
Quoted 3 lines
Quoted 2 lines
node->namesmaller than 14 chars?[Ben Levinsky] Presently there are only 2 vrings used per Xilinx OpenAMP
channel to RPU. In Xilinx kernel we have hard-coded names as these are the only nodes that use it. For example RPU0vdev0vring0 and RPU1vdev0vring0. Hence we only check for vdev0vring and not a
sscanf("%*s%i")to parse out the vring ID or other, cleaner solution.OK, but I think it is best if we use node->name[14] only if we explicitly check for a string of at least 14 characters. Using strstr, it could return the string "vdev0vring" which is less than 14 chars, leading to a bug.
Quoted 6 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ snprintf(name, sizeof(name), "vdev0vring%d",
vring_id);drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+/* Register vring */ + mem = rproc_mem_entry_init(dev, NULL, + (dma_addr_t)rmem-base,
Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1
-0+ rmem->size, rmem-base,
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
+
zynqmp_r5_rproc_mem_alloc,Quoted 1 line
+
zynqmp_r5_rproc_mem_release,Review
Quoted 2 lines
(quoted diff) +2
-0+ name); + dev_dbg(dev, "parsed %s at %llx\r\n", mem-name,
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+ mem->dma); + }else { +int idx; + +/* +* if TCM update address space for R5 and +* make xilinx platform mgmt call +*/ + for (idx = 0; idx <
ZYNQMP_R5_NUM_TCM_BANKS;Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
idx++) {
Review
Quoted 1 line
(quoted diff) +1
-0+ if (tcm_addr_to_pnode[idx][0] ==rmem-
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 2 lines
base)
(quoted diff) +1
-0+ break;There is something I don't quite understand. If the memory region to
use
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
is TCM, why would it be also described under reserved-memory? Reserved-memory is for normal memory being reserved, while TCM is
not
Quoted 11 lines
Quoted 3 lines
Quoted 2 lines
normal memory. Am I missing something?
[Ben Levinsky] I can change this in v9 as discussed. That is, have no TCM
under reserved mem. Instead have the banks as nodes in device tree with status="[enabled|disabled]" and if enabled, then try to add memories in
parse_fwcall.Quoted 5 lines
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +4
-0+ } + +if (idx !=ZYNQMP_R5_NUM_TCM_BANKS ) { + mem = handle_tcm_parsing(dev,rmem, node,
Quoted 19 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
idx);
Review
Quoted 2 lines
(quoted diff) +2
-0+ } else { + mem = rproc_mem_entry_init(dev,NULL,
Quoted 15 lines
Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1
-0+ (dma_addr_t)rmem-base,
Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1
-0+ rmem->size, rmem-base,
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
+
zynqmp_r5_rproc_mem_alloc,Quoted 1 line
+
zynqmp_r5_rproc_mem_release,Review
Quoted 1 line
(quoted diff) +1
-0+ node->name);This case looks identical to the vdev0vring above. Is the difference really just in the "name"? If so, can we merge the two cases into one? no, because the
devm_ioremap_wccall has to be done before changingthe dma address to relative for TCM banks.
Quoted 1 line
Quoted 1 line
[Ben Levinsky] ok will clean up to merge where able
Quoted 185 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 5 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +5
-0+ } + +if (!mem) { + dev_err(dev, + "unable to init memory-region %s\n",
Quoted 52 lines
Quoted 11 lines
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 11 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +11
-0+ node->name); +return -ENOMEM ; + } + } +rproc_add_carveout (rproc, mem); + } + +return 0 ; +} + +static int zynqmp_r5_parse_fw(struct rproc *rproc, const structfirmware
Quoted 19 lines
Quoted 10 lines
*fw)
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+{ +int ret; +struct zynqmp_r5_pdata *pdata = rproc->priv; +struct device *dev = &pdata->dev; + + ret =parse_mem_regions (rproc); +if (ret) { +dev_err (dev,"parse_mem_regions failed %x\n" , ret); +return ret;Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 428
Quoted 6 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+ } + + ret =rproc_elf_load_rsc_table (rproc, fw); +if (ret == -EINVAL ) { +dev_info (dev,"no resource table found.\n" ); + ret =0 ;Why do we want to continue ignoring the error in this case?
Quoted 1 line
[Ben Levinsky] as there can be simple firmware loaded onto R5 that do not
have resource table. Resource table only needed for specific IPC case.
OK, an in-code comment would be good
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +15
-0+struct device *dev; +struct zynqmp_r5_mem *mem; +int ret; +struct property *prop; +const __be32 *cur; +u32 val; +int i; + + dev = &pdata->dev; + mem =devm_kzalloc (dev,sizeof (*mem),GFP_KERNEL ); +if (!mem) +return -ENOMEM ; + ret =of_address_to_resource (node,0 , &mem->res); +if (ret <0 ) { + dev_err(dev, "failed to get resource of memory %s",Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +8
-0+ of_node_full_name(node)); +return -EINVAL ; + } + +/* Get the power domain id */ + i =0 ; +if (of_find_property (node,"pnode-id" ,NULL )) { + of_property_for_each_u32(node, "pnode-id", prop,cur, val)
Quoted 60 lines
Quoted 60 lines
Quoted 60 lines
Quoted 60 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +60
-0+ mem->pnode_id[i++] = val; + } +list_add_tail (&mem->node, &pdata->mems); +return 0 ; +} + +/** +* zynqmp_r5_release() - ZynqMP R5 device release function +* @dev: pointer to the device struct of ZynqMP R5 +* +* Function to release ZynqMP R5 device. +*/ +static void zynqmp_r5_release (struct device *dev) +{ +struct zynqmp_r5_pdata *pdata; +struct rproc *rproc; +struct sk_buff *skb; + + pdata =dev_get_drvdata (dev); + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + } +if (pdata->tx_chan) +mbox_free_channel (pdata->tx_chan); +if (pdata->rx_chan) +mbox_free_channel (pdata->rx_chan); +/* Discard all SKBs */ +while (!skb_queue_empty (&pdata->tx_mc_skbs)) { + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); + } + +put_device (dev->parent); +} + +/** +* event_notified_idr_cb() - event notified idr callback +* @id: idr id +* @ptr: pointer to idr private data +* @data: data passed to idr_for_each callback +* +* Pass notification to remoteproc virtio +* +* Return: 0. having return is to satisfy the idr_for_each() function +* pointer input argument requirement. +**/ +static int event_notified_idr_cb (int id,void *ptr,void *data) +{ +struct rproc *rproc = data; + + (void )rproc_vq_interrupt (rproc, id); +return 0 ; +} + +/** +* handle_event_notified() - remoteproc notification work funciton +* @work: pointer to the work structure +* Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c
Quoted 8 lines
+ * It checks each registered remoteproc notify IDs.
+ */drivers/remoteproc/zynqmp_r5_remoteproc.c new file +6
-0+static void handle_event_notified (struct work_struct *work) +{ +struct rproc *rproc; +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(work, struct zynqmp_r5_pdata,
mbox_work);Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 602
Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +9
-0+ + (void )mbox_send_message (pdata->rx_chan,NULL ); + rproc = pdata->rproc; +/* +* We only use IPI for interrupt. The firmware side may or may +* not write the notifyid when it trigger IPI. +* And thus, we scan through all the registered notifyids. +*/ +idr_for_each (&rproc->notifyids, event_notified_idr_cb, rproc);This looks expensive. Should we at least check whether the notifyid was written as first thing before doing the scan?
Quoted 1 line
Quoted 1 line
[Ben Levinsky] this will be at most 2 vrings presently per firmware-load
and
Quoted 34 lines
only done when the firmware is loaded so the latency so should not impact performace or user
OK
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* Get R5 power domain node */ + ret = of_property_read_u32(node, "pnode-id", &pdata-
pnode_id);Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+if (ret) { +dev_err (dev,"failed to get power node id.\n" ); + goto error; + } + +/* TODO Check if R5 is running */ + +/* Set up R5 if not already setup */ + ret = pdata->is_r5_mode_set ?0 :r5_set_mode (pdata); +if (ret) {Quoted 9 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c line 790
Quoted 3 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +3
-0+dev_err (dev,"failed to set R5 operation mode.\n" ); +return ret; + }
is_r5_mode_setis set byr5_set_mode(), which is only called here. So it looks like this check is important in cases wherezynqmp_r5_probe()is called twice for the same R5 node. But I don't think that is supposed to happen?Quoted 1 line
[Ben Levinsky] this is needed as there are cases where user can repeatedly
load different firmware so the check is needed in cases like this where rpu is already configured. It is also possible that a user might repeatedly load/unload the module
OK
Re: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
In reply to Ben LevinskyHello Ben,
Quoted 16 lines · Ben Levinsky
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +16
-0+/** +* zynqmp_r5_release() - ZynqMP R5 device release function +* @dev: pointer to the device struct of ZynqMP R5 +* +* Function to release ZynqMP R5 device. +*/ +static void zynqmp_r5_release (struct device *dev) +{ +struct zynqmp_r5_pdata *pdata; +struct rproc *rproc; +struct sk_buff *skb; + + pdata =dev_get_drvdata (dev); + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc);
drivers/remoteproc/zynqmp_r5_remoteproc.c line 557
Quoted 10 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +10
-0+rproc_free (rproc); + } +if (pdata->tx_chan) +mbox_free_channel (pdata->tx_chan); +if (pdata->rx_chan) +mbox_free_channel (pdata->rx_chan); +/* Discard all SKBs */ +while (!skb_queue_empty (&pdata->tx_mc_skbs)) { + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb);
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).
Quoted 357 lines
drivers/remoteproc/zynqmp_r5_remoteproc.c new file +354
-0+ } + +put_device (dev->parent); +} + +/** +* event_notified_idr_cb() - event notified idr callback +* @id: idr id +* @ptr: pointer to idr private data +* @data: data passed to idr_for_each callback +* +* Pass notification to remoteproc virtio +* +* Return: 0. having return is to satisfy the idr_for_each() function +* pointer input argument requirement. +**/ +static int event_notified_idr_cb (int id,void *ptr,void *data) +{ +struct rproc *rproc = data; + + (void )rproc_vq_interrupt (rproc, id); +return 0 ; +} + +/** +* handle_event_notified() - remoteproc notification work funciton +* @work: pointer to the work structure +* +* It checks each registered remoteproc notify IDs. +*/ +static void handle_event_notified (struct work_struct *work) +{ +struct rproc *rproc; +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(work, struct zynqmp_r5_pdata, mbox_work); + + (void )mbox_send_message (pdata->rx_chan,NULL ); + rproc = pdata->rproc; +/* +* We only use IPI for interrupt. The firmware side may or may +* not write the notifyid when it trigger IPI. +* And thus, we scan through all the registered notifyids. +*/ +idr_for_each (&rproc->notifyids, event_notified_idr_cb, rproc); +} + +/** +* zynqmp_r5_mb_rx_cb() - Receive channel mailbox callback +* @cl: mailbox client +* @mssg: message pointer +* +* It will schedule the R5 notification work. +*/ +static void zynqmp_r5_mb_rx_cb (struct mbox_client *cl,void *mssg) +{ +struct zynqmp_r5_pdata *pdata; + + pdata = container_of(cl, struct zynqmp_r5_pdata, rx_mc); +if (mssg) { +struct zynqmp_ipi_message *ipi_msg, *buf_msg; +size_t len; + + ipi_msg = (struct zynqmp_ipi_message *)mssg; + buf_msg = (struct zynqmp_ipi_message *)pdata->rx_mc_buf; + len = (ipi_msg->len >=IPI_BUF_LEN_MAX ) ? +IPI_BUF_LEN_MAX : ipi_msg->len; + buf_msg->len = len; +memcpy (buf_msg->data, ipi_msg->data, len); + } +schedule_work (&pdata->mbox_work); +} + +/** +* zynqmp_r5_mb_tx_done() - Request has been sent to the remote +* @cl: mailbox client +* @mssg: pointer to the message which has been sent +* @r: status of last TX - OK or error +* +* It will be called by the mailbox framework when the last TX has done. +*/ +static void zynqmp_r5_mb_tx_done (struct mbox_client *cl,void *mssg,int r) +{ +struct zynqmp_r5_pdata *pdata; +struct sk_buff *skb; + +if (!mssg) +return ; + pdata = container_of(cl, struct zynqmp_r5_pdata, tx_mc); + skb =skb_dequeue (&pdata->tx_mc_skbs); +kfree_skb (skb); +} + +/** +* zynqmp_r5_setup_mbox() - Setup mailboxes +* +* @pdata: pointer to the ZynqMP R5 processor platform data +* @node: pointer of the device node +* +* Function to setup mailboxes to talk to RPU. +* +* Return: 0 for success, negative value for failure. +*/ +static int zynqmp_r5_setup_mbox (struct zynqmp_r5_pdata *pdata, +struct device_node *node) +{ +struct device *dev = &pdata->dev; +struct mbox_client *mclient; + +/* Setup TX mailbox channel client */ + mclient = &pdata->tx_mc; + mclient->dev = dev; + mclient->rx_callback =NULL ; + mclient->tx_block = false; + mclient->knows_txdone = false; + mclient->tx_done = zynqmp_r5_mb_tx_done; + +/* Setup TX mailbox channel client */ + mclient = &pdata->rx_mc; + mclient->dev = dev; + mclient->rx_callback = zynqmp_r5_mb_rx_cb; + mclient->tx_block = false; + mclient->knows_txdone = false; + +INIT_WORK (&pdata->mbox_work, handle_event_notified); + +/* Request TX and RX channels */ + pdata->tx_chan =mbox_request_channel_byname (&pdata->tx_mc,"tx" ); +if (IS_ERR (pdata->tx_chan)) { +dev_err (dev,"failed to request mbox tx channel.\n" ); + pdata->tx_chan =NULL ; +return -EINVAL ; + } + pdata->rx_chan =mbox_request_channel_byname (&pdata->rx_mc,"rx" ); +if (IS_ERR (pdata->rx_chan)) { +dev_err (dev,"failed to request mbox rx channel.\n" ); + pdata->rx_chan =NULL ; +return -EINVAL ; + } +skb_queue_head_init (&pdata->tx_mc_skbs); +return 0 ; +} + +/** +* zynqmp_r5_probe() - Probes ZynqMP R5 processor device node +* @pdata: pointer to the ZynqMP R5 processor platform data +* @pdev: parent RPU domain platform device +* @node: pointer of the device node +* +* Function to retrieve the information of the ZynqMP R5 device node. +* +* Return: 0 for success, negative value for failure. +*/ +static int zynqmp_r5_probe (struct zynqmp_r5_pdata *pdata, +struct platform_device *pdev, +struct device_node *node) +{ +struct device *dev = &pdata->dev; +struct rproc *rproc; +struct device_node *nc; +int ret; + +/* Create device for ZynqMP R5 device */ + dev->parent = &pdev->dev; + dev->release = zynqmp_r5_release; + dev->of_node = node; +dev_set_name (dev,"%s" ,of_node_full_name (node)); +dev_set_drvdata (dev, pdata); + ret =device_register (dev); +if (ret) { +dev_err (dev,"failed to register device.\n" ); +return ret; + } +get_device (&pdev->dev); + +/* Allocate remoteproc instance */ + rproc =rproc_alloc (dev,dev_name (dev), &zynqmp_r5_rproc_ops,NULL ,0 ); +if (!rproc) { +dev_err (dev,"rproc allocation failed.\n" ); + ret = -ENOMEM ; + goto error; + } + rproc->auto_boot = autoboot; + pdata->rproc = rproc; + rproc->priv = pdata; + +/* +* The device has not been spawned from a device tree, so +* arch_setup_dma_ops has not been called, thus leaving +* the device with dummy DMA ops. +* Fix this by inheriting the parent's DMA ops and mask. +*/ + rproc->dev.dma_mask = pdev->dev.dma_mask; +set_dma_ops (&rproc->dev,get_dma_ops (&pdev->dev)); + +/* Probe R5 memory devices */ +INIT_LIST_HEAD (&pdata->mems); +for_each_available_child_of_node (node, nc) { + ret =zynqmp_r5_mem_probe (pdata, nc); +if (ret) { +dev_err (dev,"failed to probe memory %s.\n" , +of_node_full_name (nc)); + goto error; + } + } + +/* Set up DMA mask */ + ret =dma_set_coherent_mask (dev,DMA_BIT_MASK (32 )); +if (ret) { +dev_warn (dev,"dma_set_coherent_mask failed: %d\n" , ret); +/* If DMA is not configured yet, try to configure it. */ + ret =of_dma_configure (dev, node, true); +if (ret) { +dev_err (dev,"failed to configure DMA.\n" ); + goto error; + } + } + +/* Get R5 power domain node */ + ret =of_property_read_u32 (node,"pnode-id" , &pdata->pnode_id); +if (ret) { +dev_err (dev,"failed to get power node id.\n" ); + goto error; + } + +/* TODO Check if R5 is running */ + +/* Set up R5 if not already setup */ + ret = pdata->is_r5_mode_set ?0 :r5_set_mode (pdata); +if (ret) { +dev_err (dev,"failed to set R5 operation mode.\n" ); +return ret; + } + +if (!of_get_property (dev->of_node,"mboxes" ,NULL )) { +dev_dbg (dev,"no mailboxes.\n" ); + goto error; + }else { + ret =zynqmp_r5_setup_mbox (pdata, node); +if (ret <0 ) + goto error; + } + +/* Add R5 remoteproc */ + ret =rproc_add (rproc); +if (ret) { +dev_err (dev,"rproc registration failed\n" ); + goto error; + } +return 0 ; +error: +if (pdata->rproc) +rproc_free (pdata->rproc); + pdata->rproc =NULL ; +device_unregister (dev); +put_device (&pdev->dev); +return ret; +} + +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; +struct zynqmp_r5_pdata *pdata; + + pdata =devm_kzalloc (dev,sizeof (*pdata),GFP_KERNEL ); + lockstep_mode =devm_kzalloc (dev,sizeof (u32 *),GFP_KERNEL ); +if (!pdata || !lockstep_mode) +return -ENOMEM ; + +platform_set_drvdata (pdev, pdata); + +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 ; + }else { +dev_err (dev, +"Invalid lockstep-mode mode provided - %x %d\n" , + *lockstep_mode, rpu_mode); +return -EINVAL ; + } +dev_dbg (dev,"RPU configuration: %s\r\n" , + (*lockstep_mode) ?"lockstep" :"split" ); + +for_each_available_child_of_node (dev->of_node, nc) { + ret =zynqmp_r5_probe (&rpus[i], pdev, nc); +if (ret) { +dev_err (dev,"failed to probe rpu %s.\n" , +of_node_full_name (nc)); +return ret; + } + i++; + } + +return 0 ; +} + +static int zynqmp_r5_remoteproc_remove (struct platform_device *pdev) +{ +int i; + +for (i =0 ; i <MAX_RPROCS ; i++) { +struct zynqmp_r5_pdata *pdata = &rpus[i]; +struct rproc *rproc; + + rproc = pdata->rproc; +if (rproc) { +rproc_del (rproc); +rproc_free (rproc); + pdata->rproc =NULL ; + } +if (pdata->tx_chan) { +mbox_free_channel (pdata->tx_chan); + pdata->tx_chan =NULL ; + } +if (pdata->rx_chan) { +mbox_free_channel (pdata->rx_chan); + pdata->rx_chan =NULL ; + } + +device_unregister (&pdata->dev); + } + +return 0 ; +} + +/* Match table for OF platform binding */ +static const struct of_device_id zynqmp_r5_remoteproc_match[] = { + { .compatible ="xlnx,zynqmp-r5-remoteproc-1.0" , }, + {/* end of list */ }, +}; +MODULE_DEVICE_TABLE (of, zynqmp_r5_remoteproc_match); + +static struct platform_driver zynqmp_r5_remoteproc_driver = { + .probe = zynqmp_r5_remoteproc_probe, + .remove = zynqmp_r5_remoteproc_remove, + .driver = { + .name ="zynqmp_r5_remoteproc" , + .of_match_table = zynqmp_r5_remoteproc_match, + }, +}; +module_platform_driver (zynqmp_r5_remoteproc_driver); + +module_param_named (autoboot, autoboot, bool,0444 ); +MODULE_PARM_DESC (autoboot, +"enable | disable autoboot. (default: false)" ); + +MODULE_AUTHOR ("Ben Levinsky <[email protected]>" ); +MODULE_LICENSE ("GPL v2" );Signature · 1 line
2.17.1
Thanks,
Michael
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel