* [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP
@ 2020-10-05 16:06 Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 1/5] firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration Ben Levinsky
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-05 16:06 UTC (permalink / raw)
To: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
michael.auchter
Cc: devicetree, mathieu.poirier, linux-remoteproc, linux-kernel,
robh+dt, linux-arm-kernel
The driver was tested on Xilinx ZynqMP QEMU
For sake of ease of review, only support ZynqMP. Once accepted, then
add support for Versal platform and R5 loading onto OCM.
v2:
- remove domain struct
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around
function pointer in xilinx eemi ops struct
- update zynqmp_r5 yaml parsing to not raise warnings for extra
information in children of R5 node. The warning "node has a unit
name, but no reg or ranges property" will still be raised though
as this particular node is needed to describe the
'#address-cells' and '#size-cells' information.
v4:
- add default values for enums
- fix formatting as per checkpatch.pl --strict. Note that 1 warning and
1 check are still raised as each is due to fixing the warning
results in that particular line going over 80 characters.
- remove warning '/example-0/rpu@ff9a0000/r5@0:
node has a unit name, but no reg or ranges property'
by adding reg to r5 node.
v5:
- update device tree sample and yaml parsing to not raise any warnings
- description for memory-region in yaml parsing
- compatible string in yaml parsing for TCM
- parse_fw change from use of rproc_of_resm_mem_entry_init to
rproc_mem_entry_init and use of alloc/release
- var's of type zynqmp_r5_pdata all have same local variable name
- use dev_dbg instead of dev_info
v6:
- adding memory carveouts is handled much more similarly.
All mem carveouts are now described in reserved memory as needed.
That is, TCM nodes are not coupled to remoteproc anymore.
This is reflected in the remoteproc R5 driver and the device tree
binding.
- remove mailbox from device tree binding as it is not necessary for elf
loading
v7:
- remove unused headers
- zynqmp_r5_remoteproc_probe:lockstep_mode from u32* to u32
- device-tree binding "lockstep-mode" to "xlnx,cluster-mode"
- remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at
probe()
- remove is_r5_mode_set from zynqmp rpu remote processor private data
- do not error out if no mailbox is provided since mailboxes are optional
- remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as pdata
is handled in zynqmp_r5_remoteproc_remove
v8:
- remove old acks, reviewed-by's in commit message
v9:
- if zynqmp_r5_remoteproc.c pdata->tx_mc_skbs not initialized, then do not
call skb_queue_empty
- update arguments and documentation for zynqmp_pm_set_rpu_mode
- in fn zynqmp_pm_force_powerdown, change arg 'target' to 'node'
- zynqmp_pm_request_wakeup update code style
- edit 3/5 patch commit message
- document zynqmp_pm_set_tcm_config and zynqmp_pm_get_rpu_mode
documentation to include expected return val
- remove unused fn zynqmp_pm_get_node_status
- update 5/5 patch commit message to document supported configurations
and how they are booted by the driver.
- remove copyrights other than SPDX from zynqmp_r5_remoteproc.c
- compilation warnings no longer raised
- remove unused includes from zynqmp_r5_remoteproc.c
- remove unused var autoboot from zynqmp_r5_remoteproc.c
- reorder zynqmp_r5_pdata fpr small mem savings due to alignment
- zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode uses second arg
- zynqmp_r5_remoteproc.c use of zynqmp_pm_set_tcm_config now does not
have output arg
- in tcm handling, unconditionally use &= 0x000fffff mask since all nodes
in this fn are for tcm
- update comments for translating dma field in tcm handling to device
address
- update calls to rproc_mem_entry_init in parse_mem_regions so that there
are only 2 cases for types of carveouts instead of 3
- in parse_mem_regions, check if device tree node is null before using it
- add example device tree nodes used in parse_mem_regions and tcm parsing
- add comment for vring id node length
- add check for string length so that vring id is at least min length
- move tcm nodes from reserved mem to instead own device tree nodes
and only use them if enabled in device tree
- add comment for explaining handling of rproc_elf_load_rsc_table
- remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick
- remove unused field mems in struct zynqmp_r5_pdata
- remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling
is done by zyqmp_r5_pm_request_tcm
- remove obsolete setting of dma_ops and parent device dma_mask
- remove obsolete use of of_dma_configure
- add comment for call to r5_set_mode fn
- make mbox usage optional and gracefully inform user via dev_dbg if not
present
- change lockstep_mode from u32* to u32
- update zynqmp_pm_set_rpu_mode and zynqmp_pm_set_rpu_mode documentation
and remove unused args
v10:
- add include types.h to xlnx-zynqmp.h for compilation
v11:
- update usage of zynqmp_pm_get_rpu_mode to return rpu mode in enum
- update zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode arguments to
remove unused args
- use enums instead of u32 where possible in zynqmp_r5_remoteproc
- zynqmp_r5_remoteproc: update prints to not use carriage return, just
newline
- zynqmp_r5_remoteproc: look up tcm banks via property instead of string
name
- print device tree nodes with %pOF instead of %s with node name field
- update tcm release to unmap VA
- handle r5-1 use case
- device tree binding r5 node to have link to TCMs via
meta-memory-regions property
- fix device tree binding so no warnings from 'make dt_binding_check'
v12:
- update signed off by so that latest developer name is last
- in drivers/firmware/zynqmp.c, update zynqmp_pm_set_rpu_mode so rpu_mode
is only set if no error
- update args for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config fn
arg's to reflect what is expected in the function and the usage in
zynqmp_r5_remoteproc accordingly
v13:
- zynqmp_pm_get_rpu_mode argument zynqmp_pm_get_rpu_mode is
only set if no error
v14:
- rebase off v5.9-rc3 kernel
- concerns were raised about the new property meta-memory-regions.
There is no clear direction so for the moment I kept it in the series
- in device tree binding, place IPC nodes in RAM in the reserved memory section
- zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
property specified in device tree
- use u32 instead of u32* to store in remoteproc memory entry private data
for pnode_id information
- always call r5_set_mode on probe
- remove alloc of zynqmp_r5_pdata in
zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static
allocation already
- error at probe time if lockstep-mode property not present in device tree
- update commit message as per review
- remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is present
- remove unused macros
- update comment ordering of zynqmp_r5_pdata to match struct definition
- zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
- remove obsolete TODOs
- only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
- remove uneven dev_dbg/dev_err fn calls
v15:
- change lockstep mode device tree property from acting as
boolean, to instead being used as, if it is present, then r5 is in lockstep mode. otherwise in split.
- add safeguard for if 2 rpu remoteproc instances are provided then err out
v16:
- replace of_get_property(dev->of_node, "lockstep-mode" with
of_property_read_bool
- propagate rpu mode specified in device tree through functions instead
of holding a global, static var
- check child remoteproc nodes via of_get_available_child_count before
looping through children
- replace check of "pdata->pnode_id == 0" instead by checking rpu's
zynqmp_r5_pdata* if NULL
- remove old, obsolete checks for dma_pools in zynqmp_r5_remoteproc_remove
- change rpus from zynqmp_r5_pdata[] to zynqmp_r5_pdata*[] so that
check for pdata->pnode_id == 0 is not needed
v17:
- remove compatible string from tcm bank nodes
- fix style for bindings
- add boolean type to lockstep mode in binding
- add/update descriptions memory-region, meta-memory-regions,
pnode-id, mbox* properties
- update driver as per kernel-test-robot
v18:
- *really* change zynqmp_r5_remoteproc::rpus and rpu_mode to static
- to more closely mimic other remoteproc drivers, change zynqmp r5 rproc
data from zynqmp_r5_pdata to zynqmp_r5_rproc and pdata local var to
zproc
- remove global vars rpus and rpu_mode
- instantiate device for zynqmp r5 rproc from device set by rproc_alloc
- fix typos
- update to call zynqmp_r5_release from the rproc_alloc-related device and
remove the instantiated device from zynqmp_r5_probe
- remove unneeded call to platform_set_drvdata
- remove driver remove function, as the clean up is handled in release
- remove while (!skb_queue_empty loop and mbox_free_channel calls in
zynqmp_r5_release, and mbox_free_channel
- remove device_unregister call in zynqmp_r5_release
- remove kzalloc for pdata (what is now called z_rproc)
- update conditional in loop to calls of zynqmp_r5_probe
- update example remoteproc zynqmp r5 compat string, remove version
number
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 | 142 ++++
drivers/firmware/xilinx/zynqmp.c | 96 +++
drivers/remoteproc/Kconfig | 8 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/zynqmp_r5_remoteproc.c | 708 ++++++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 60 ++
6 files changed, 1015 insertions(+)
create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v18 1/5] firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration.
2020-10-05 16:06 [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky
@ 2020-10-05 16:06 ` Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 2/5] firmware: xilinx: Add shutdown/wakeup APIs Ben Levinsky
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-05 16:06 UTC (permalink / raw)
To: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
michael.auchter
Cc: devicetree, mathieu.poirier, linux-remoteproc, linux-kernel,
robh+dt, linux-arm-kernel
Add ZynqMP firmware ioctl enums for RPU configuration.
Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
---
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(+)
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 5968df82b991..bb347dfe4ba4 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -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,
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v18 2/5] firmware: xilinx: Add shutdown/wakeup APIs
2020-10-05 16:06 [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 1/5] firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration Ben Levinsky
@ 2020-10-05 16:06 ` Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 3/5] firmware: xilinx: Add RPU configuration APIs Ben Levinsky
` (2 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-05 16:06 UTC (permalink / raw)
To: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
michael.auchter
Cc: devicetree, mathieu.poirier, linux-remoteproc, linux-kernel,
robh+dt, linux-arm-kernel
Add shutdown/wakeup a resource eemi operations to shutdown
or bringup a resource.
Note alignment of args matches convention of other fn's in this file.
The reason being that the long fn name results in aligned args that
otherwise go over 80 chars so shift right to avoid this
Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
---
drivers/firmware/xilinx/zynqmp.c | 35 ++++++++++++++++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 23 ++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 8d1ff2454e2e..a966ee956573 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node)
}
EXPORT_SYMBOL_GPL(zynqmp_pm_release_node);
+/**
+ * zynqmp_pm_force_pwrdwn - PM call to request for another PU or subsystem to
+ * be powered down forcefully
+ * @node: Node ID of the targeted PU or subsystem
+ * @ack: Flag to specify whether acknowledge is requested
+ *
+ * Return: status, either success or error+reason
+ */
+int zynqmp_pm_force_pwrdwn(const u32 node,
+ const enum zynqmp_pm_request_ack ack)
+{
+ return zynqmp_pm_invoke_fn(PM_FORCE_POWERDOWN, node, ack, 0, 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_force_pwrdwn);
+
+/**
+ * zynqmp_pm_request_wake - PM call to wake up selected master or subsystem
+ * @node: Node ID of the master or subsystem
+ * @set_addr: Specifies whether the address argument is relevant
+ * @address: Address from which to resume when woken up
+ * @ack: Flag to specify whether acknowledge requested
+ *
+ * Return: status, either success or error+reason
+ */
+int zynqmp_pm_request_wake(const u32 node,
+ const bool set_addr,
+ const u64 address,
+ const enum zynqmp_pm_request_ack ack)
+{
+ /* set_addr flag is encoded into 1st bit of address */
+ return zynqmp_pm_invoke_fn(PM_REQUEST_WAKEUP, node, address | set_addr,
+ address >> 32, ack, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_request_wake);
+
/**
* zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves
* @node: Node ID of the slave
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index bb347dfe4ba4..6241c5ac51b3 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -12,6 +12,7 @@
#ifndef __FIRMWARE_ZYNQMP_H__
#define __FIRMWARE_ZYNQMP_H__
+#include <linux/types.h>
#define ZYNQMP_PM_VERSION_MAJOR 1
#define ZYNQMP_PM_VERSION_MINOR 0
@@ -64,6 +65,8 @@
enum pm_api_id {
PM_GET_API_VERSION = 1,
+ PM_FORCE_POWERDOWN = 8,
+ PM_REQUEST_WAKEUP = 10,
PM_SYSTEM_SHUTDOWN = 12,
PM_REQUEST_NODE = 13,
PM_RELEASE_NODE,
@@ -376,6 +379,12 @@ int zynqmp_pm_write_pggs(u32 index, u32 value);
int zynqmp_pm_read_pggs(u32 index, u32 *value);
int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype);
int zynqmp_pm_set_boot_health_status(u32 value);
+int zynqmp_pm_force_pwrdwn(const u32 target,
+ const enum zynqmp_pm_request_ack ack);
+int zynqmp_pm_request_wake(const u32 node,
+ const bool set_addr,
+ const u64 address,
+ const enum zynqmp_pm_request_ack ack);
#else
static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
{
@@ -526,6 +535,20 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value)
{
return -ENODEV;
}
+
+static inline int zynqmp_pm_force_pwrdwn(const u32 target,
+ const enum zynqmp_pm_request_ack ack)
+{
+ return -ENODEV;
+}
+
+static inline int zynqmp_pm_request_wake(const u32 node,
+ const bool set_addr,
+ const u64 address,
+ const enum zynqmp_pm_request_ack ack)
+{
+ return -ENODEV;
+}
#endif
#endif /* __FIRMWARE_ZYNQMP_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v18 3/5] firmware: xilinx: Add RPU configuration APIs
2020-10-05 16:06 [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 1/5] firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 2/5] firmware: xilinx: Add shutdown/wakeup APIs Ben Levinsky
@ 2020-10-05 16:06 ` Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Ben Levinsky
4 siblings, 0 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-05 16:06 UTC (permalink / raw)
To: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
michael.auchter
Cc: devicetree, mathieu.poirier, linux-remoteproc, linux-kernel,
robh+dt, linux-arm-kernel
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).
Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
---
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around
function pointer in xilinx eemi ops struct
v4:
- add default values for enums
v9:
- update commit message
- for zynqmp_pm_set_tcm_config and zynqmp_pm_get_rpu_mode update docs for
expected output, arguments as well removing unused args
- remove unused fn zynqmp_pm_get_node_status
v11:
- update usage of zynqmp_pm_get_rpu_mode to return rpu mode in enum
- update zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode arguments to remove unused args
v12:
- in drivers/firmware/zynqmp.c, update zynqmp_pm_set_rpu_mode so rpu_mode
is only set if no error
- update args for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config fn arg's to
reflect what is expected in the function and the usage in
zynqmp_r5_remoteproc accordingly
- zynqmp_pm_get_rpu_mode argument zynqmp_pm_get_rpu_mode is
only set if no error
---
drivers/firmware/xilinx/zynqmp.c | 61 ++++++++++++++++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 18 ++++++++
2 files changed, 79 insertions(+)
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index a966ee956573..b390a00338d0 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -846,6 +846,67 @@ int zynqmp_pm_release_node(const u32 node)
}
EXPORT_SYMBOL_GPL(zynqmp_pm_release_node);
+/**
+ * zynqmp_pm_get_rpu_mode() - Get RPU mode
+ * @node_id: Node ID of the device
+ * @rpu_mode: return by reference value
+ * either split or lockstep
+ *
+ * Return: return 0 on success or error+reason.
+ * if success, then rpu_mode will be set
+ * to current rpu mode.
+ */
+int zynqmp_pm_get_rpu_mode(u32 node_id, enum rpu_oper_mode *rpu_mode)
+{
+ u32 ret_payload[PAYLOAD_ARG_CNT];
+ int ret;
+
+ ret = zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
+ IOCTL_GET_RPU_OPER_MODE, 0, 0, ret_payload);
+
+ /* only set rpu_mode if no error */
+ if (ret == XST_PM_SUCCESS)
+ *rpu_mode = ret_payload[0];
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_get_rpu_mode);
+
+/**
+ * zynqmp_pm_set_rpu_mode() - Set RPU mode
+ * @node_id: Node ID of the device
+ * @rpu_mode: Argument 1 to requested IOCTL call. either split or lockstep
+ *
+ * This function is used to set RPU mode to split or
+ * lockstep
+ *
+ * Return: Returns status, either success or error+reason
+ */
+int zynqmp_pm_set_rpu_mode(u32 node_id, enum rpu_oper_mode rpu_mode)
+{
+ return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
+ IOCTL_SET_RPU_OPER_MODE, (u32)rpu_mode,
+ 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_set_rpu_mode);
+
+/**
+ * zynqmp_pm_set_tcm_config - configure TCM
+ * @tcm_mode: Argument 1 to requested IOCTL call
+ * either PM_RPU_TCM_COMB or PM_RPU_TCM_SPLIT
+ *
+ * This function is used to set RPU mode to split or combined
+ *
+ * Return: status: 0 for success, else failure
+ */
+int zynqmp_pm_set_tcm_config(u32 node_id, enum rpu_tcm_comb tcm_mode)
+{
+ return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
+ IOCTL_TCM_COMB_CONFIG, (u32)tcm_mode, 0,
+ NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_set_tcm_config);
+
/**
* zynqmp_pm_force_pwrdwn - PM call to request for another PU or subsystem to
* be powered down forcefully
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 6241c5ac51b3..79aa2fcbcd54 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -385,6 +385,9 @@ int zynqmp_pm_request_wake(const u32 node,
const bool set_addr,
const u64 address,
const enum zynqmp_pm_request_ack ack);
+int zynqmp_pm_get_rpu_mode(u32 node_id, enum rpu_oper_mode *rpu_mode);
+int zynqmp_pm_set_rpu_mode(u32 node_id, u32 arg1);
+int zynqmp_pm_set_tcm_config(u32 node_id, u32 arg1);
#else
static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
{
@@ -549,6 +552,21 @@ static inline int zynqmp_pm_request_wake(const u32 node,
{
return -ENODEV;
}
+
+static inline int zynqmp_pm_get_rpu_mode(u32 node_id, enum rpu_oper_mode *rpu_mode)
+{
+ return -ENODEV;
+}
+
+static inline int zynqmp_pm_set_rpu_mode(u32 node_id, u32 arg1)
+{
+ return -ENODEV;
+}
+
+static inline int zynqmp_pm_set_tcm_config(u32 node_id, u32 arg1)
+{
+ return -ENODEV;
+}
#endif
#endif /* __FIRMWARE_ZYNQMP_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
2020-10-05 16:06 [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky
` (2 preceding siblings ...)
2020-10-05 16:06 ` [PATCH v18 3/5] firmware: xilinx: Add RPU configuration APIs Ben Levinsky
@ 2020-10-05 16:06 ` Ben Levinsky
2020-10-08 12:37 ` Linus Walleij
2020-10-05 16:06 ` [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Ben Levinsky
4 siblings, 1 reply; 20+ messages in thread
From: Ben Levinsky @ 2020-10-05 16:06 UTC (permalink / raw)
To: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
michael.auchter
Cc: devicetree, mathieu.poirier, linux-remoteproc, linux-kernel,
robh+dt, linux-arm-kernel
Add binding for ZynqMP R5 OpenAMP.
Represent the RPU domain resources in one device node. Each RPU
processor is a subnode of the top RPU domain node.
Signed-off-by: Jason Wu <j.wu@xilinx.com>
Signed-off-by: Wendy Liang <jliang@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
---
v3:
- update zynqmp_r5 yaml parsing to not raise warnings for extra
information in children of R5 node. The warning "node has a unit
name, but no reg or ranges property" will still be raised though
as this particular node is needed to describe the
'#address-cells' and '#size-cells' information.
v4::
- remove warning '/example-0/rpu@ff9a0000/r5@0:
node has a unit name, but no reg or ranges property'
by adding reg to r5 node.
v5:
- update device tree sample and yaml parsing to not raise any warnings
- description for memory-region in yaml parsing
- compatible string in yaml parsing for TCM
v6:
- remove coupling TCM nodes with remoteproc
- remove mailbox as it is optional not needed
v7:
- change lockstep-mode to xlnx,cluster-mode
v9:
- show example IPC nodes and tcm bank nodes
v11:
- add property meta-memory-regions to illustrate link
between r5 and TCM banks
- update so no warnings from 'make dt_binding_check'
v14:
- concerns were raised about the new property meta-memory-regions.
There is no clear direction so for the moment I kept it in the series
- place IPC nodes in RAM in the reserved memory section
v15:
- change lockstep-mode prop as follows: if present, then RPU cluster is in
lockstep mode. if not present, cluster is in split mode.
v17:
- remove compatible string from tcm bank nodes
- fix style for bindings
- add boolean type to lockstep mode in binding
- add/update descriptions memory-region, meta-memory-regions,
pnode-id, mbox* properties
v18:
- update example remoteproc zynqmp r5 compat string, remove version
number
---
.../xilinx,zynqmp-r5-remoteproc.yaml | 142 ++++++++++++++++++
1 file changed, 142 insertions(+)
create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
diff --git a/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
new file mode 100644
index 000000000000..c202dca3b6d0
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
@@ -0,0 +1,142 @@
+# 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 <ed.mooring@xilinx.com>
+ - Ben Levinsky <ben.levinsky@xilinx.com>
+
+properties:
+ compatible:
+ const: xlnx,zynqmp-r5-remoteproc
+
+ lockstep-mode:
+ description:
+ If this property is present, then the configuration is lock-step.
+ Otherwise RPU is split.
+ type: boolean
+ 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. each carveout in this case should be in DDR, not
+ chip-specific memory. In Xilinx case, this is TCM, OCM, BRAM, etc.
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+
+ meta-memory-regions:
+ description:
+ collection of memories that are not present in the top level memory
+ nodes' mapping. For example, R5s' TCM banks. These banks are needed
+ for R5 firmware meta data such as the R5 firmware's heap and stack.
+ To be more precise, this is on-chip reserved SRAM regions, e.g. TCM,
+ BRAM, OCM, etc.
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+
+ pnode-id:
+ maxItems: 1
+ description:
+ power node id that is used to uniquely identify the node for Xilinx
+ Power Management. The value is then passed to Xilinx platform
+ manager for power on/off and access.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ mboxes:
+ description:
+ array of phandles that describe the rx and tx for xilinx zynqmp
+ mailbox driver. order of rx and tx is described by the mbox-names
+ property. This will be used for communication with remote
+ processor.
+ maxItems: 2
+
+ mbox-names:
+ description:
+ array of strings that denote which item in the mboxes property array
+ are the rx and tx for xilinx zynqmp mailbox driver
+ maxItems: 2
+ $ref: /schemas/types.yaml#/definitions/string-array
+
+
+examples:
+ - |
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ elf_load: rproc@3ed000000 {
+ no-map;
+ reg = <0x3ed00000 0x40000>;
+ };
+
+ rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
+ no-map;
+ reg = <0x3ed40000 0x4000>;
+ };
+ rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
+ no-map;
+ reg = <0x3ed44000 0x4000>;
+ };
+ rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
+ no-map;
+ reg = <0x3ed48000 0x100000>;
+ };
+
+ };
+
+ /*
+ * Below nodes are required if using TCM to load R5 firmware
+ * if not, then either do not provide nodes are label as disabled in
+ * status property
+ */
+ tcm0a: tcm_0a@ffe00000 {
+ reg = <0xffe00000 0x10000>;
+ pnode-id = <0xf>;
+ no-map;
+ status = "okay";
+ phandle = <0x40>;
+ };
+ tcm0b: tcm_1a@ffe20000 {
+ reg = <0xffe20000 0x10000>;
+ pnode-id = <0x10>;
+ no-map;
+ status = "okay";
+ phandle = <0x41>;
+ };
+
+ rpu {
+ compatible = "xlnx,zynqmp-r5-remoteproc";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ lockstep-mode;
+ r5_0 {
+ ranges;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ memory-region = <&elf_load>,
+ <&rpu0vdev0vring0>,
+ <&rpu0vdev0vring1>,
+ <&rpu0vdev0buffer>;
+ meta-memory-regions = <&tcm_0a>, <&tcm_0b>;
+ pnode-id = <0x7>;
+ };
+ };
+
+...
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-05 16:06 [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky
` (3 preceding siblings ...)
2020-10-05 16:06 ` [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings Ben Levinsky
@ 2020-10-05 16:06 ` Ben Levinsky
2020-10-05 19:34 ` Michael Auchter
2020-10-19 20:43 ` Stefano Stabellini
4 siblings, 2 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-05 16:06 UTC (permalink / raw)
To: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
michael.auchter
Cc: devicetree, mathieu.poirier, linux-remoteproc, linux-kernel,
robh+dt, linux-arm-kernel
R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
remotproc driver, we can boot the R5 sub-system in different 2
configurations -
* split
* lock-step
The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
Platform Management Unit that handles the R5 configuration, memory access
and R5 lifecycle management. The interface to this manager is done in this
driver via zynqmp_pm_* function calls.
Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Ed Mooring <ed.mooring@xilinx.com>
Signed-off-by: Jason Wu <j.wu@xilinx.com>
Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
---
v2:
- remove domain struct as per review from Mathieu
v3:
- add xilinx-related platform mgmt fn's instead of wrapping around
function pointer in xilinx eemi ops struct
v4:
- add default values for enums
- fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1 check
are still raised as each is due to fixing the warning results in that
particular line going over 80 characters.
v5:
- parse_fw change from use of rproc_of_resm_mem_entry_init to
rproc_mem_entry_init and use of alloc/release
- var's of type zynqmp_r5_pdata all have same local variable name
- use dev_dbg instead of dev_info
v6:
- adding memory carveouts is handled much more similarly. All mem
carveouts are
now described in reserved memory as needed. That is, TCM nodes are not
coupled to remoteproc anymore. This is reflected in the remoteproc R5
driver
and the device tree binding.
- remove mailbox from device tree binding as it is not necessary for elf
loading
- use lockstep-mode property for configuring RPU
v7:
- remove unused headers
- change u32 *lockstep_mode -> u32 lockstep_mode;
- change device-tree binding "lockstep-mode" to xlnx,cluster-mode
- remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at
remoteproc-probe time
- remove is_r5_mode_set from zynqmp rpu remote processor private data
- do not error out if no mailbox is provided
- remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as
pdata is
handled in zynqmp_r5_remoteproc_remove
v8:
- remove old acks, reviewed-by's in commit message
v9:
- as mboxes are now optional, if pdata->tx_mc_skbs not initialized then
do not call skb_queue_empty
- update usage for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config and
zynqmp_pm_get_rpu_mode
- update 5/5 patch commit message to document supported configurations
and how they are booted by the driver.
- remove copyrights other than SPDX from zynqmp_r5_remoteproc.c
- compilation warnings no longer raised
- remove unused includes from zynqmp_r5_remoteproc.c
- remove unused var autoboot from zynqmp_r5_remoteproc.c
- reorder zynqmp_r5_pdata fpr small mem savings due to alignment
- use of zynqmp_pm_set_tcm_config now does not have
output arg
- in tcm handling, unconditionally use &= 0x000fffff mask since all nodes
in this fn are for tcm
- update comments for translating dma field in tcm handling to device
address
- update calls to rproc_mem_entry_init in parse_mem_regions so that there
are only 2 cases for types of carveouts instead of 3
- in parse_mem_regions, check if device tree node is null before using it
- add example device tree nodes used in parse_mem_regions and tcm parsing
- add comment for vring id node length
- add check for string length so that vring id is at least min length
- move tcm nodes from reserved mem to instead own device tree nodes
and only use them if enabled in device tree
- add comment for explaining handling of rproc_elf_load_rsc_table
- remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick
- remove unused field mems in struct zynqmp_r5_pdata
- remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling
is done by zyqmp_r5_pm_request_tcm
- remove obsolete setting of dma_ops and parent device dma_mask
- remove obsolete use of of_dma_configure
- add comment for call to r5_set_mode fn
- make mbox usage optional and gracefully inform user via dev_dbg if not
present
- change var lockstep_mode from u32* to u32
v11:
- use enums instead of u32 where possible in zynqmp_r5_remoteproc
- update usage of zynqmp_pm_set/get_rpu_mode and zynqmp_pm_set_tcm_config
- update prints to not use carriage return, just newline
- look up tcm banks via property in r5 node instead of string name
- print device tree nodes with %pOF instead of %s with node name field
- update tcm release to unmap VA
- handle r5-1 use case
v12:
- update signed off by so that latest developer name is last
- do not cast enums to u32s for zynqmp_pm* functions
v14:
- change zynqmp_r5_remoteproc::rpus and rpu_mode to static
- fix typo
- zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
property specified in device tree
- use u32 instead of u32* to store in remoteproc memory entry private data
for pnode_id information
- always call r5_set_mode on probe
- remove alloc of zynqmp_r5_pdata in
zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static
allocation already
- error at probe time if lockstep-mode property not present in device tree
- update commit message as per review
- remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is present
- remove unused macros
- update comment ordering of zynqmp_r5_pdata to match struct definition
- zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
- remove obsolete TODOs
- only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
- remove uneven dev_dbg/dev_err fn calls
v15:
- if lockstep mode prop is present, then RPU cluster is in lockstep mode.
if not present, cluster is in split mode.
- if 2 RPUs provided but one is lockstep then error out as this is invalid
configuration
v16:
- replace of_get_property(dev->of_node, "lockstep-mode" with
of_property_read_bool
- propagate rpu mode specified in device tree through functions instead
of holding a global, static var
- check child remoteproc nodes via of_get_available_child_count before
looping through children
- replace check of "pdata->pnode_id == 0" instead by checking rpu's
zynqmp_r5_pdata* if NULL
- remove old, obsolete checks for dma_pools in zynqmp_r5_remoteproc_remove
- change rpus from zynqmp_r5_pdata[] to zynqmp_r5_pdata*[] so that
check for pdata->pnode_id == 0 is not needed
v17:
- fix style as per kernel test bot
v18:
- to more closely mimic other remoteproc drivers, change zynqmp r5 rproc
data from zynqmp_r5_pdata to zynqmp_r5_rproc and pdata local var to
zproc
- remove global vars rpus and rpu_mode
- instantiate device for zynqmp r5 rproc from device set by rproc_alloc
- fix typos
- update to call zynqmp_r5_release from the rproc_alloc-related device and
remove the instantiated device from zynqmp_r5_probe
- remove unneeded call to platform_set_drvdata
- remove driver remove function, as the clean up is handled in release
- remove while (!skb_queue_empty loop and mbox_free_channel calls in
zynqmp_r5_release, and mbox_free_channel
- remove device_unregister call in zynqmp_r5_release
- remove kzalloc for pdata (what is now called z_rproc)
- update conditional in loop to calls of zynqmp_r5_probe
---
drivers/remoteproc/Kconfig | 8 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/zynqmp_r5_remoteproc.c | 707 ++++++++++++++++++++++
3 files changed, 716 insertions(+)
create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c6659dfea7c7..68e567c5375c 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -275,6 +275,14 @@ config TI_K3_DSP_REMOTEPROC
It's safe to say N here if you're not interested in utilizing
the DSP slave processors.
+config ZYNQMP_R5_REMOTEPROC
+ tristate "ZynqMP_R5 remoteproc support"
+ depends on PM && ARCH_ZYNQMP
+ select RPMSG_VIRTIO
+ select ZYNQMP_IPI_MBOX
+ help
+ Say y or m here to support ZynqMP R5 remote processors via the remote
+ processor framework.
endif # REMOTEPROC
endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 3dfa28e6c701..ef1abff654c2 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
+obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o
diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c b/drivers/remoteproc/zynqmp_r5_remoteproc.c
new file mode 100644
index 000000000000..37bd76252ff2
--- /dev/null
+++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c
@@ -0,0 +1,707 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Zynq R5 Remote Processor driver
+ *
+ * Based on origin OMAP and Zynq Remote Processor driver
+ *
+ */
+
+#include <linux/firmware/xlnx-zynqmp.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_platform.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+#include <linux/skbuff.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 BANK_LIST_PROP "meta-memory-regions"
+
+/* IPI buffer MAX length */
+#define IPI_BUF_LEN_MAX 32U
+/* RX mailbox client buffer max length */
+#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
+ sizeof(struct zynqmp_ipi_message))
+
+/**
+ * struct zynqmp_r5_mem - zynqmp rpu memory data
+ * @pnode_id: TCM power domain ids
+ * @res: memory resource
+ * @node: list node
+ */
+struct zynqmp_r5_mem {
+ u32 pnode_id[MAX_MEM_PNODES];
+ struct resource res;
+ struct list_head node;
+};
+
+/**
+ * struct zynqmp_r5_rproc - zynqmp rpu remote processor state
+ * @rx_mc_buf: rx mailbox client buffer to save the rx message
+ * @tx_mc: tx mailbox client
+ * @rx_mc: rx mailbox client * @dev: device of RPU instance
+ * @mbox_work: mbox_work for the RPU remoteproc
+ * @tx_mc_skbs: socket buffers for tx mailbox client
+ * @dev: device of RPU instance
+ * @rproc: rproc handle
+ * @tx_chan: tx mailbox channel
+ * @rx_chan: rx mailbox channel
+ * @pnode_id: RPU CPU power domain id
+ */
+struct zynqmp_r5_rproc {
+ unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
+ struct mbox_client tx_mc;
+ struct mbox_client rx_mc;
+ struct work_struct mbox_work;
+ struct sk_buff_head tx_mc_skbs;
+ struct device dev;
+ struct rproc *rproc;
+ struct mbox_chan *tx_chan;
+ struct mbox_chan *rx_chan;
+ u32 pnode_id;
+};
+
+/*
+ * r5_set_mode - set RPU operation mode
+ * @z_rproc: Remote processor private data
+ *
+ * set RPU operation mode
+ *
+ * Return: 0 for success, negative value for failure
+ */
+static int r5_set_mode(struct zynqmp_r5_rproc *z_rproc,
+ enum rpu_oper_mode rpu_mode)
+{
+ enum rpu_tcm_comb tcm_mode;
+ enum rpu_oper_mode cur_rpu_mode;
+ int ret;
+
+ ret = zynqmp_pm_get_rpu_mode(z_rproc->pnode_id, &cur_rpu_mode);
+ if (ret < 0)
+ return ret;
+
+ if (rpu_mode != cur_rpu_mode) {
+ ret = zynqmp_pm_set_rpu_mode(z_rproc->pnode_id,
+ rpu_mode);
+ if (ret < 0)
+ return ret;
+ }
+
+ tcm_mode = (rpu_mode == PM_RPU_MODE_LOCKSTEP) ?
+ PM_RPU_TCM_COMB : PM_RPU_TCM_SPLIT;
+ return zynqmp_pm_set_tcm_config(z_rproc->pnode_id, tcm_mode);
+}
+
+/*
+ * ZynqMP R5 remoteproc memory release function
+ */
+static int tcm_mem_release(struct rproc *rproc, struct rproc_mem_entry *mem)
+{
+ u32 pnode_id = (u64)mem->priv;
+
+ if (pnode_id <= 0)
+ return -EINVAL;
+
+ iounmap(mem->va);
+ return zynqmp_pm_release_node(pnode_id);
+}
+
+/*
+ * ZynqMP R5 remoteproc operations
+ */
+static int zynqmp_r5_rproc_start(struct rproc *rproc)
+{
+ struct device *dev = rproc->dev.parent;
+ struct zynqmp_r5_rproc *z_rproc = rproc->priv;
+ enum rpu_boot_mem bootmem;
+
+ bootmem = (rproc->bootaddr & 0xF0000000) == 0xF0000000 ?
+ PM_RPU_BOOTMEM_HIVEC : PM_RPU_BOOTMEM_LOVEC;
+
+ dev_dbg(dev, "RPU boot from %s.",
+ bootmem == PM_RPU_BOOTMEM_HIVEC ? "OCM" : "TCM");
+
+ return zynqmp_pm_request_wake(z_rproc->pnode_id, 1,
+ bootmem, ZYNQMP_PM_REQUEST_ACK_NO);
+}
+
+static int zynqmp_r5_rproc_stop(struct rproc *rproc)
+{
+ struct zynqmp_r5_rproc *z_rproc = rproc->priv;
+ struct sk_buff *skb;
+
+ if (z_rproc->tx_chan)
+ mbox_free_channel(z_rproc->tx_chan);
+ if (z_rproc->rx_chan)
+ mbox_free_channel(z_rproc->rx_chan);
+
+ return zynqmp_pm_force_pwrdwn(z_rproc->pnode_id,
+ ZYNQMP_PM_REQUEST_ACK_BLOCKING);
+}
+
+static int zynqmp_r5_rproc_mem_alloc(struct rproc *rproc,
+ struct rproc_mem_entry *mem)
+{
+ void *va;
+
+ va = ioremap_wc(mem->dma, mem->len);
+ if (IS_ERR_OR_NULL(va))
+ 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)
+{
+ iounmap(mem->va);
+ return 0;
+}
+
+static int parse_mem_regions(struct rproc *rproc)
+{
+ int num_mems, i;
+ struct zynqmp_r5_rproc *z_rproc = rproc->priv;
+ struct device *dev = &z_rproc->dev;
+ struct device_node *np = dev->of_node;
+ 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);
+ if (!node)
+ return -EINVAL;
+
+ rmem = of_reserved_mem_lookup(node);
+ if (!rmem)
+ return -EINVAL;
+
+ if (strstr(node->name, "vdev0vring")) {
+ int vring_id;
+ char name[16];
+
+ /*
+ * expecting form of "rpuXvdev0vringX as documented
+ * in xilinx remoteproc device tree binding
+ */
+ if (strlen(node->name) < 14) {
+ dev_err(dev, "%pOF is less than 14 chars",
+ node);
+ return -EINVAL;
+ }
+
+ /*
+ * can be 1 of multiple vring IDs per IPC channel
+ * e.g. 'vdev0vring0' and 'vdev0vring1'
+ */
+ vring_id = node->name[14] - '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);
+ } else {
+ /* Register DMA region */
+ int (*alloc)(struct rproc *r,
+ struct rproc_mem_entry *rme);
+ int (*release)(struct rproc *r,
+ struct rproc_mem_entry *rme);
+ char name[20];
+
+ if (strstr(node->name, "vdev0buffer")) {
+ alloc = NULL;
+ release = NULL;
+ strcpy(name, "vdev0buffer");
+ } else {
+ alloc = zynqmp_r5_rproc_mem_alloc;
+ release = zynqmp_r5_rproc_mem_release;
+ strcpy(name, node->name);
+ }
+
+ mem = rproc_mem_entry_init(dev, NULL,
+ (dma_addr_t)rmem->base,
+ rmem->size, rmem->base,
+ alloc, release, name);
+ }
+ if (!mem)
+ return -ENOMEM;
+
+ rproc_add_carveout(rproc, mem);
+ }
+
+ return 0;
+}
+
+/* call Xilinx Platform manager to request access to TCM bank */
+static int zynqmp_r5_pm_request_tcm(struct device_node *tcm_node,
+ struct device *dev, u32 *pnode_id)
+{
+ int ret;
+
+ ret = of_property_read_u32(tcm_node, "pnode-id", pnode_id);
+ if (ret)
+ return ret;
+
+ return zynqmp_pm_request_node(*pnode_id, ZYNQMP_PM_CAPABILITY_ACCESS, 0,
+ ZYNQMP_PM_REQUEST_ACK_BLOCKING);
+}
+
+/* Given tcm bank entry,
+ * this callback will set device address for R5 running on TCM
+ * and also setup virtual address for tcm bank remoteproc carveout
+ */
+static int tcm_mem_alloc(struct rproc *rproc,
+ struct rproc_mem_entry *mem)
+{
+ void *va;
+ struct device *dev = rproc->dev.parent;
+
+ va = ioremap_wc(mem->dma, mem->len);
+ if (IS_ERR_OR_NULL(va))
+ return -ENOMEM;
+
+ /* Update memory entry va */
+ mem->va = va;
+
+ va = devm_ioremap_wc(dev, mem->da, mem->len);
+ if (!va)
+ return -ENOMEM;
+ /* As R5 is 32 bit, wipe out extra high bits */
+ mem->da &= 0x000fffff;
+ /*
+ * handle tcm banks 1 a and b (0xffe90000 and oxffeb0000)
+ * As both of these the only common bit found not in tcm bank0 a or b
+ * is at 0x80000 use this mask to suss it out
+ */
+ if (mem->da & 0x80000)
+ /*
+ * need to do more to further translate
+ * tcm banks 1a and 1b at 0xffe90000 and oxffeb0000
+ * respectively to 0x0 and 0x20000
+ */
+ mem->da -= 0x90000;
+
+ return 0;
+}
+
+/*
+ * Given R5 node in remoteproc instance,
+ * allocate remoteproc carveout for TCM memory
+ * needed for firmware to be loaded
+ */
+static int parse_tcm_banks(struct rproc *rproc)
+{
+ int i, num_banks;
+ struct zynqmp_r5_rproc *z_rproc = rproc->priv;
+ struct device *dev = &z_rproc->dev;
+ struct device_node *r5_node = dev->of_node;
+
+ /* go through tcm banks for r5 node */
+ num_banks = of_count_phandle_with_args(r5_node, BANK_LIST_PROP, NULL);
+ if (num_banks <= 0) {
+ dev_err(dev, "need to specify TCM banks\n");
+ return -EINVAL;
+ }
+
+ for (i = 0; i < num_banks; i++) {
+ struct resource rsc;
+ resource_size_t size;
+ struct device_node *dt_node;
+ struct rproc_mem_entry *mem;
+ int ret;
+ u32 pnode_id; /* zynqmp_pm* fn's expect u32 */
+
+ dt_node = of_parse_phandle(r5_node, BANK_LIST_PROP, i);
+ if (!dt_node)
+ return -EINVAL;
+
+ if (of_device_is_available(dt_node)) {
+ ret = of_address_to_resource(dt_node, 0, &rsc);
+ if (ret < 0)
+ return ret;
+
+ ret = zynqmp_r5_pm_request_tcm(dt_node, dev, &pnode_id);
+ if (ret < 0)
+ return ret;
+
+ /* add carveout */
+ size = resource_size(&rsc);
+ mem = rproc_mem_entry_init(dev, NULL, rsc.start,
+ (int)size, rsc.start,
+ tcm_mem_alloc,
+ tcm_mem_release,
+ rsc.name);
+ if (!mem)
+ return -ENOMEM;
+
+ mem->priv = (void *)(u64)pnode_id;
+ 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_rproc *z_rproc = rproc->priv;
+ struct device *dev = &z_rproc->dev;
+
+ ret = parse_tcm_banks(rproc);
+ if (ret)
+ return ret;
+
+ ret = parse_mem_regions(rproc);
+ if (ret)
+ return ret;
+
+ ret = rproc_elf_load_rsc_table(rproc, fw);
+ if (ret == -EINVAL) {
+ /*
+ * resource table only required for IPC.
+ * if not present, this is not necessarily an error;
+ * for example, loading r5 hello world application
+ * so simply inform user and keep going.
+ */
+ dev_info(dev, "no resource table found.\n");
+ ret = 0;
+ }
+ return ret;
+}
+
+/* kick a firmware */
+static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
+{
+ struct sk_buff *skb;
+ unsigned int skb_len;
+ struct zynqmp_ipi_message *mb_msg;
+ int ret;
+
+ struct device *dev = rproc->dev.parent;
+ struct zynqmp_r5_rproc *z_rproc = rproc->priv;
+
+ skb_len = (unsigned int)(sizeof(vqid) + sizeof(mb_msg));
+ skb = alloc_skb(skb_len, GFP_ATOMIC);
+ if (!skb)
+ 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(&z_rproc->tx_mc_skbs, skb);
+ ret = mbox_send_message(z_rproc->tx_chan, mb_msg);
+ if (ret < 0) {
+ dev_warn(dev, "Failed to kick remote.\n");
+ skb_dequeue_tail(&z_rproc->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_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_rproc *z_rproc;
+ struct rproc *rproc;
+
+ z_rproc = dev_get_drvdata(dev);
+ rproc = z_rproc->rproc;
+ if (rproc) {
+ rproc_del(rproc);
+ rproc_free(rproc);
+ }
+}
+
+/**
+ * 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_rproc *z_rproc;
+
+ z_rproc = container_of(work, struct zynqmp_r5_rproc, mbox_work);
+
+ (void)mbox_send_message(z_rproc->rx_chan, NULL);
+ rproc = z_rproc->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_rproc *z_rproc;
+
+ z_rproc = container_of(cl, struct zynqmp_r5_rproc, 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 *)z_rproc->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(&z_rproc->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_rproc *z_rproc;
+ struct sk_buff *skb;
+
+ if (!mssg)
+ return;
+ z_rproc = container_of(cl, struct zynqmp_r5_rproc, tx_mc);
+ skb = skb_dequeue(&z_rproc->tx_mc_skbs);
+ kfree_skb(skb);
+}
+
+/**
+ * zynqmp_r5_setup_mbox() - Setup mailboxes
+ *
+ * @z_rproc: 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_rproc *z_rproc,
+ struct device_node *node)
+{
+ struct device *dev = &z_rproc->dev;
+ struct mbox_client *mclient;
+
+ dev->of_node = node;
+
+ /* Setup TX mailbox channel client */
+ mclient = &z_rproc->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 = &z_rproc->rx_mc;
+ mclient->dev = dev;
+ mclient->rx_callback = zynqmp_r5_mb_rx_cb;
+ mclient->tx_block = false;
+ mclient->knows_txdone = false;
+
+ INIT_WORK(&z_rproc->mbox_work, handle_event_notified);
+
+ /* Request TX and RX channels */
+ z_rproc->tx_chan = mbox_request_channel_byname(&z_rproc->tx_mc, "tx");
+ if (IS_ERR(z_rproc->tx_chan)) {
+ dev_err(dev, "failed to request mbox tx channel.\n");
+ z_rproc->tx_chan = NULL;
+ return -EINVAL;
+ }
+ z_rproc->rx_chan = mbox_request_channel_byname(&z_rproc->rx_mc, "rx");
+ if (IS_ERR(z_rproc->rx_chan)) {
+ dev_err(dev, "failed to request mbox rx channel.\n");
+ z_rproc->rx_chan = NULL;
+ return -EINVAL;
+ }
+ skb_queue_head_init(&z_rproc->tx_mc_skbs);
+
+ return 0;
+}
+
+/**
+ * zynqmp_r5_probe() - Probes ZynqMP R5 processor device node
+ * @z_rproc: pointer to the ZynqMP R5 processor platform data
+ * @pdev: parent RPU domain platform device
+ * @node: pointer of the device node
+ * @rpu_mode: rpu config set by DT
+ *
+ * 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 platform_device *pdev,
+ struct device_node *node,
+ enum rpu_oper_mode rpu_mode)
+{
+ struct rproc *rproc;
+ int ret;
+ struct zynqmp_r5_rproc *z_rproc;
+ struct device *dev = &pdev->dev;
+
+ /* Allocate remoteproc instance */
+ rproc = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops, NULL, sizeof(*z_rproc));
+ if (!rproc) {
+ ret = -ENOMEM;
+ goto error;
+ }
+ z_rproc = rproc->priv;
+ z_rproc->dev.release = zynqmp_r5_release;
+
+ /* Set up DMA mask */
+ ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
+ if (ret)
+ goto error;
+ /* Get R5 power domain node */
+ ret = of_property_read_u32(node, "pnode-id", &z_rproc->pnode_id);
+ if (ret)
+ goto error;
+
+ ret = r5_set_mode(z_rproc, rpu_mode);
+ if (ret)
+ return ret;
+
+ if (of_property_read_bool(node, "mboxes")) {
+ ret = zynqmp_r5_setup_mbox(z_rproc, node);
+ if (ret)
+ goto error;
+ }
+ /* Add R5 remoteproc */
+ ret = rproc_add(rproc);
+ if (ret)
+ goto error;
+
+ return 0;
+error:
+ if (z_rproc->rproc)
+ rproc_free(z_rproc->rproc);
+ z_rproc->rproc = NULL;
+ return ret;
+}
+
+static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
+{
+ int ret, i;
+ struct device *dev = &pdev->dev;
+ struct device_node *nc;
+ enum rpu_oper_mode rpu_mode;
+
+ rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode") ?
+ PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;
+ dev_dbg(dev, "RPU configuration: %s\n",
+ rpu_mode == PM_RPU_MODE_LOCKSTEP ? "lockstep" : "split");
+
+ /*
+ * if 2 RPUs provided but one is lockstep, then we have an
+ * invalid configuration.
+ */
+ i = of_get_available_child_count(dev->of_node);
+ if ((rpu_mode == PM_RPU_MODE_LOCKSTEP && i != 1) || i > MAX_RPROCS)
+ return -EINVAL;
+
+ i = 0;
+ for_each_available_child_of_node(dev->of_node, nc) {
+ /* only call zynqmp_r5_probe if proper # of rpu's */
+ ret = zynqmp_r5_probe(pdev, nc, rpu_mode);
+ dev_dbg(dev, "%s to probe rpu %pOF\n",
+ ret ? "Failed" : "Able",
+ nc);
+ if (ret)
+ return ret;
+ i++;
+ }
+
+ return 0;
+}
+
+/* Match table for OF platform binding */
+static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
+ { .compatible = "xlnx,zynqmp-r5-remoteproc", },
+ { /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match);
+
+static struct platform_driver zynqmp_r5_remoteproc_driver = {
+ .probe = zynqmp_r5_remoteproc_probe,
+ .driver = {
+ .name = "zynqmp_r5_remoteproc",
+ .of_match_table = zynqmp_r5_remoteproc_match,
+ },
+};
+module_platform_driver(zynqmp_r5_remoteproc_driver);
+
+MODULE_AUTHOR("Ben Levinsky <ben.levinsky@xilinx.com>");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-05 16:06 ` [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Ben Levinsky
@ 2020-10-05 19:34 ` Michael Auchter
2020-10-06 19:15 ` Ben Levinsky
2020-10-19 20:43 ` Stefano Stabellini
1 sibling, 1 reply; 20+ messages in thread
From: Michael Auchter @ 2020-10-05 19:34 UTC (permalink / raw)
To: Ben Levinsky
Cc: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
devicetree, mathieu.poirier, linux-remoteproc, linux-kernel,
robh+dt, linux-arm-kernel
Hey Ben,
On Mon, Oct 05, 2020 at 09:06:14AM -0700, Ben Levinsky wrote:
> R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> remotproc driver, we can boot the R5 sub-system in different 2
> configurations -
> * split
> * lock-step
>
> The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
> Platform Management Unit that handles the R5 configuration, memory access
> and R5 lifecycle management. The interface to this manager is done in this
> driver via zynqmp_pm_* function calls.
>
> Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Ed Mooring <ed.mooring@xilinx.com>
> Signed-off-by: Jason Wu <j.wu@xilinx.com>
> Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> ---
> v2:
> - remove domain struct as per review from Mathieu
> v3:
> - add xilinx-related platform mgmt fn's instead of wrapping around
> function pointer in xilinx eemi ops struct
> v4:
> - add default values for enums
> - fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1 check
> are still raised as each is due to fixing the warning results in that
> particular line going over 80 characters.
> v5:
> - parse_fw change from use of rproc_of_resm_mem_entry_init to
> rproc_mem_entry_init and use of alloc/release
> - var's of type zynqmp_r5_pdata all have same local variable name
> - use dev_dbg instead of dev_info
> v6:
> - adding memory carveouts is handled much more similarly. All mem
> carveouts are
> now described in reserved memory as needed. That is, TCM nodes are not
> coupled to remoteproc anymore. This is reflected in the remoteproc R5
> driver
> and the device tree binding.
> - remove mailbox from device tree binding as it is not necessary for elf
> loading
> - use lockstep-mode property for configuring RPU
> v7:
> - remove unused headers
> - change u32 *lockstep_mode -> u32 lockstep_mode;
> - change device-tree binding "lockstep-mode" to xlnx,cluster-mode
> - remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at
> remoteproc-probe time
> - remove is_r5_mode_set from zynqmp rpu remote processor private data
> - do not error out if no mailbox is provided
> - remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as
> pdata is
> handled in zynqmp_r5_remoteproc_remove
> v8:
> - remove old acks, reviewed-by's in commit message
> v9:
> - as mboxes are now optional, if pdata->tx_mc_skbs not initialized then
> do not call skb_queue_empty
> - update usage for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config and
> zynqmp_pm_get_rpu_mode
> - update 5/5 patch commit message to document supported configurations
> and how they are booted by the driver.
> - remove copyrights other than SPDX from zynqmp_r5_remoteproc.c
> - compilation warnings no longer raised
> - remove unused includes from zynqmp_r5_remoteproc.c
> - remove unused var autoboot from zynqmp_r5_remoteproc.c
> - reorder zynqmp_r5_pdata fpr small mem savings due to alignment
> - use of zynqmp_pm_set_tcm_config now does not have
> output arg
> - in tcm handling, unconditionally use &= 0x000fffff mask since all nodes
> in this fn are for tcm
> - update comments for translating dma field in tcm handling to device
> address
> - update calls to rproc_mem_entry_init in parse_mem_regions so that there
> are only 2 cases for types of carveouts instead of 3
> - in parse_mem_regions, check if device tree node is null before using it
> - add example device tree nodes used in parse_mem_regions and tcm parsing
> - add comment for vring id node length
> - add check for string length so that vring id is at least min length
> - move tcm nodes from reserved mem to instead own device tree nodes
> and only use them if enabled in device tree
> - add comment for explaining handling of rproc_elf_load_rsc_table
> - remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick
> - remove unused field mems in struct zynqmp_r5_pdata
> - remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling
> is done by zyqmp_r5_pm_request_tcm
> - remove obsolete setting of dma_ops and parent device dma_mask
> - remove obsolete use of of_dma_configure
> - add comment for call to r5_set_mode fn
> - make mbox usage optional and gracefully inform user via dev_dbg if not
> present
> - change var lockstep_mode from u32* to u32
> v11:
> - use enums instead of u32 where possible in zynqmp_r5_remoteproc
> - update usage of zynqmp_pm_set/get_rpu_mode and zynqmp_pm_set_tcm_config
> - update prints to not use carriage return, just newline
> - look up tcm banks via property in r5 node instead of string name
> - print device tree nodes with %pOF instead of %s with node name field
> - update tcm release to unmap VA
> - handle r5-1 use case
> v12:
> - update signed off by so that latest developer name is last
> - do not cast enums to u32s for zynqmp_pm* functions
> v14:
> - change zynqmp_r5_remoteproc::rpus and rpu_mode to static
> - fix typo
> - zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
> property specified in device tree
> - use u32 instead of u32* to store in remoteproc memory entry private data
> for pnode_id information
> - always call r5_set_mode on probe
> - remove alloc of zynqmp_r5_pdata in
> zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static
> allocation already
> - error at probe time if lockstep-mode property not present in device tree
> - update commit message as per review
> - remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is present
> - remove unused macros
> - update comment ordering of zynqmp_r5_pdata to match struct definition
> - zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
> - remove obsolete TODOs
> - only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
> - remove uneven dev_dbg/dev_err fn calls
> v15:
> - if lockstep mode prop is present, then RPU cluster is in lockstep mode.
> if not present, cluster is in split mode.
> - if 2 RPUs provided but one is lockstep then error out as this is invalid
> configuration
> v16:
> - replace of_get_property(dev->of_node, "lockstep-mode" with
> of_property_read_bool
> - propagate rpu mode specified in device tree through functions instead
> of holding a global, static var
> - check child remoteproc nodes via of_get_available_child_count before
> looping through children
> - replace check of "pdata->pnode_id == 0" instead by checking rpu's
> zynqmp_r5_pdata* if NULL
> - remove old, obsolete checks for dma_pools in zynqmp_r5_remoteproc_remove
> - change rpus from zynqmp_r5_pdata[] to zynqmp_r5_pdata*[] so that
> check for pdata->pnode_id == 0 is not needed
> v17:
> - fix style as per kernel test bot
> v18:
> - to more closely mimic other remoteproc drivers, change zynqmp r5 rproc
> data from zynqmp_r5_pdata to zynqmp_r5_rproc and pdata local var to
> zproc
> - remove global vars rpus and rpu_mode
> - instantiate device for zynqmp r5 rproc from device set by rproc_alloc
> - fix typos
> - update to call zynqmp_r5_release from the rproc_alloc-related device and
> remove the instantiated device from zynqmp_r5_probe
> - remove unneeded call to platform_set_drvdata
> - remove driver remove function, as the clean up is handled in release
> - remove while (!skb_queue_empty loop and mbox_free_channel calls in
> zynqmp_r5_release, and mbox_free_channel
> - remove device_unregister call in zynqmp_r5_release
> - remove kzalloc for pdata (what is now called z_rproc)
> - update conditional in loop to calls of zynqmp_r5_probe
>
> ---
> drivers/remoteproc/Kconfig | 8 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/zynqmp_r5_remoteproc.c | 707 ++++++++++++++++++++++
> 3 files changed, 716 insertions(+)
> create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c6659dfea7c7..68e567c5375c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -275,6 +275,14 @@ config TI_K3_DSP_REMOTEPROC
> It's safe to say N here if you're not interested in utilizing
> the DSP slave processors.
>
> +config ZYNQMP_R5_REMOTEPROC
> + tristate "ZynqMP_R5 remoteproc support"
> + depends on PM && ARCH_ZYNQMP
> + select RPMSG_VIRTIO
> + select ZYNQMP_IPI_MBOX
> + help
> + Say y or m here to support ZynqMP R5 remote processors via the remote
> + processor framework.
> endif # REMOTEPROC
>
> endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 3dfa28e6c701..ef1abff654c2 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o
> diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> new file mode 100644
> index 000000000000..37bd76252ff2
> --- /dev/null
> +++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> @@ -0,0 +1,707 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Zynq R5 Remote Processor driver
> + *
> + * Based on origin OMAP and Zynq Remote Processor driver
> + *
> + */
> +
> +#include <linux/firmware/xlnx-zynqmp.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_platform.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/skbuff.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 BANK_LIST_PROP "meta-memory-regions"
> +
> +/* IPI buffer MAX length */
> +#define IPI_BUF_LEN_MAX 32U
> +/* RX mailbox client buffer max length */
> +#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
> + sizeof(struct zynqmp_ipi_message))
> +
> +/**
> + * struct zynqmp_r5_mem - zynqmp rpu memory data
> + * @pnode_id: TCM power domain ids
> + * @res: memory resource
> + * @node: list node
> + */
> +struct zynqmp_r5_mem {
> + u32 pnode_id[MAX_MEM_PNODES];
> + struct resource res;
> + struct list_head node;
> +};
> +
> +/**
> + * struct zynqmp_r5_rproc - zynqmp rpu remote processor state
> + * @rx_mc_buf: rx mailbox client buffer to save the rx message
> + * @tx_mc: tx mailbox client
> + * @rx_mc: rx mailbox client * @dev: device of RPU instance
> + * @mbox_work: mbox_work for the RPU remoteproc
> + * @tx_mc_skbs: socket buffers for tx mailbox client
> + * @dev: device of RPU instance
> + * @rproc: rproc handle
> + * @tx_chan: tx mailbox channel
> + * @rx_chan: rx mailbox channel
> + * @pnode_id: RPU CPU power domain id
> + */
> +struct zynqmp_r5_rproc {
> + unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
> + struct mbox_client tx_mc;
> + struct mbox_client rx_mc;
> + struct work_struct mbox_work;
> + struct sk_buff_head tx_mc_skbs;
> + struct device dev;
> + struct rproc *rproc;
> + struct mbox_chan *tx_chan;
> + struct mbox_chan *rx_chan;
> + u32 pnode_id;
> +};
> +
> +/*
> + * r5_set_mode - set RPU operation mode
> + * @z_rproc: Remote processor private data
> + *
> + * set RPU operation mode
> + *
> + * Return: 0 for success, negative value for failure
> + */
> +static int r5_set_mode(struct zynqmp_r5_rproc *z_rproc,
> + enum rpu_oper_mode rpu_mode)
> +{
> + enum rpu_tcm_comb tcm_mode;
> + enum rpu_oper_mode cur_rpu_mode;
> + int ret;
> +
> + ret = zynqmp_pm_get_rpu_mode(z_rproc->pnode_id, &cur_rpu_mode);
> + if (ret < 0)
> + return ret;
> +
> + if (rpu_mode != cur_rpu_mode) {
> + ret = zynqmp_pm_set_rpu_mode(z_rproc->pnode_id,
> + rpu_mode);
> + if (ret < 0)
> + return ret;
> + }
> +
> + tcm_mode = (rpu_mode == PM_RPU_MODE_LOCKSTEP) ?
> + PM_RPU_TCM_COMB : PM_RPU_TCM_SPLIT;
> + return zynqmp_pm_set_tcm_config(z_rproc->pnode_id, tcm_mode);
> +}
> +
> +/*
> + * ZynqMP R5 remoteproc memory release function
> + */
> +static int tcm_mem_release(struct rproc *rproc, struct rproc_mem_entry *mem)
> +{
> + u32 pnode_id = (u64)mem->priv;
> +
> + if (pnode_id <= 0)
> + return -EINVAL;
> +
> + iounmap(mem->va);
> + return zynqmp_pm_release_node(pnode_id);
> +}
> +
> +/*
> + * ZynqMP R5 remoteproc operations
> + */
> +static int zynqmp_r5_rproc_start(struct rproc *rproc)
> +{
> + struct device *dev = rproc->dev.parent;
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> + enum rpu_boot_mem bootmem;
> +
> + bootmem = (rproc->bootaddr & 0xF0000000) == 0xF0000000 ?
> + PM_RPU_BOOTMEM_HIVEC : PM_RPU_BOOTMEM_LOVEC;
> +
> + dev_dbg(dev, "RPU boot from %s.",
> + bootmem == PM_RPU_BOOTMEM_HIVEC ? "OCM" : "TCM");
> +
> + return zynqmp_pm_request_wake(z_rproc->pnode_id, 1,
> + bootmem, ZYNQMP_PM_REQUEST_ACK_NO);
> +}
> +
> +static int zynqmp_r5_rproc_stop(struct rproc *rproc)
> +{
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> + struct sk_buff *skb;
> +
> + if (z_rproc->tx_chan)
> + mbox_free_channel(z_rproc->tx_chan);
> + if (z_rproc->rx_chan)
> + mbox_free_channel(z_rproc->rx_chan);
This looks incorrect: these are requested during probe, so I would
expect these to be free'd during remove.
It's legal to call stop, then start again, then stop again, etc.
Consider what would happen here in that case.
> + return zynqmp_pm_force_pwrdwn(z_rproc->pnode_id,
> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> +}
> +
> +static int zynqmp_r5_rproc_mem_alloc(struct rproc *rproc,
> + struct rproc_mem_entry *mem)
> +{
> + void *va;
> +
> + va = ioremap_wc(mem->dma, mem->len);
> + if (IS_ERR_OR_NULL(va))
> + 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)
> +{
> + iounmap(mem->va);
> + return 0;
> +}
> +
> +static int parse_mem_regions(struct rproc *rproc)
> +{
> + int num_mems, i;
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> + struct device *dev = &z_rproc->dev;
> + struct device_node *np = dev->of_node;
> + 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);
> + if (!node)
> + return -EINVAL;
> +
> + rmem = of_reserved_mem_lookup(node);
> + if (!rmem)
> + return -EINVAL;
> +
> + if (strstr(node->name, "vdev0vring")) {
> + int vring_id;
> + char name[16];
> +
> + /*
> + * expecting form of "rpuXvdev0vringX as documented
> + * in xilinx remoteproc device tree binding
> + */
> + if (strlen(node->name) < 14) {
> + dev_err(dev, "%pOF is less than 14 chars",
> + node);
> + return -EINVAL;
> + }
> +
> + /*
> + * can be 1 of multiple vring IDs per IPC channel
> + * e.g. 'vdev0vring0' and 'vdev0vring1'
> + */
> + vring_id = node->name[14] - '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);
> + } else {
> + /* Register DMA region */
> + int (*alloc)(struct rproc *r,
> + struct rproc_mem_entry *rme);
> + int (*release)(struct rproc *r,
> + struct rproc_mem_entry *rme);
> + char name[20];
> +
> + if (strstr(node->name, "vdev0buffer")) {
> + alloc = NULL;
> + release = NULL;
> + strcpy(name, "vdev0buffer");
> + } else {
> + alloc = zynqmp_r5_rproc_mem_alloc;
> + release = zynqmp_r5_rproc_mem_release;
> + strcpy(name, node->name);
> + }
> +
> + mem = rproc_mem_entry_init(dev, NULL,
> + (dma_addr_t)rmem->base,
> + rmem->size, rmem->base,
> + alloc, release, name);
> + }
> + if (!mem)
> + return -ENOMEM;
> +
> + rproc_add_carveout(rproc, mem);
> + }
> +
> + return 0;
> +}
> +
> +/* call Xilinx Platform manager to request access to TCM bank */
> +static int zynqmp_r5_pm_request_tcm(struct device_node *tcm_node,
> + struct device *dev, u32 *pnode_id)
> +{
> + int ret;
> +
> + ret = of_property_read_u32(tcm_node, "pnode-id", pnode_id);
> + if (ret)
> + return ret;
> +
> + return zynqmp_pm_request_node(*pnode_id, ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> +}
> +
> +/* Given tcm bank entry,
> + * this callback will set device address for R5 running on TCM
> + * and also setup virtual address for tcm bank remoteproc carveout
> + */
> +static int tcm_mem_alloc(struct rproc *rproc,
> + struct rproc_mem_entry *mem)
> +{
> + void *va;
> + struct device *dev = rproc->dev.parent;
> +
> + va = ioremap_wc(mem->dma, mem->len);
> + if (IS_ERR_OR_NULL(va))
> + return -ENOMEM;
> +
> + /* Update memory entry va */
> + mem->va = va;
> +
> + va = devm_ioremap_wc(dev, mem->da, mem->len);
> + if (!va)
> + return -ENOMEM;
> + /* As R5 is 32 bit, wipe out extra high bits */
> + mem->da &= 0x000fffff;
> + /*
> + * handle tcm banks 1 a and b (0xffe90000 and oxffeb0000)
> + * As both of these the only common bit found not in tcm bank0 a or b
> + * is at 0x80000 use this mask to suss it out
> + */
> + if (mem->da & 0x80000)
> + /*
> + * need to do more to further translate
> + * tcm banks 1a and 1b at 0xffe90000 and oxffeb0000
> + * respectively to 0x0 and 0x20000
> + */
> + mem->da -= 0x90000;
> +
> + return 0;
> +}
> +
> +/*
> + * Given R5 node in remoteproc instance,
> + * allocate remoteproc carveout for TCM memory
> + * needed for firmware to be loaded
> + */
> +static int parse_tcm_banks(struct rproc *rproc)
> +{
> + int i, num_banks;
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> + struct device *dev = &z_rproc->dev;
> + struct device_node *r5_node = dev->of_node;
> +
> + /* go through tcm banks for r5 node */
> + num_banks = of_count_phandle_with_args(r5_node, BANK_LIST_PROP, NULL);
> + if (num_banks <= 0) {
> + dev_err(dev, "need to specify TCM banks\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < num_banks; i++) {
> + struct resource rsc;
> + resource_size_t size;
> + struct device_node *dt_node;
> + struct rproc_mem_entry *mem;
> + int ret;
> + u32 pnode_id; /* zynqmp_pm* fn's expect u32 */
> +
> + dt_node = of_parse_phandle(r5_node, BANK_LIST_PROP, i);
> + if (!dt_node)
> + return -EINVAL;
> +
> + if (of_device_is_available(dt_node)) {
> + ret = of_address_to_resource(dt_node, 0, &rsc);
> + if (ret < 0)
> + return ret;
> +
> + ret = zynqmp_r5_pm_request_tcm(dt_node, dev, &pnode_id);
> + if (ret < 0)
> + return ret;
> +
> + /* add carveout */
> + size = resource_size(&rsc);
> + mem = rproc_mem_entry_init(dev, NULL, rsc.start,
> + (int)size, rsc.start,
> + tcm_mem_alloc,
> + tcm_mem_release,
> + rsc.name);
> + if (!mem)
> + return -ENOMEM;
> +
> + mem->priv = (void *)(u64)pnode_id;
> + 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_rproc *z_rproc = rproc->priv;
> + struct device *dev = &z_rproc->dev;
> +
> + ret = parse_tcm_banks(rproc);
> + if (ret)
> + return ret;
> +
> + ret = parse_mem_regions(rproc);
> + if (ret)
> + return ret;
> +
> + ret = rproc_elf_load_rsc_table(rproc, fw);
> + if (ret == -EINVAL) {
> + /*
> + * resource table only required for IPC.
> + * if not present, this is not necessarily an error;
> + * for example, loading r5 hello world application
> + * so simply inform user and keep going.
> + */
> + dev_info(dev, "no resource table found.\n");
> + ret = 0;
> + }
> + return ret;
> +}
> +
> +/* kick a firmware */
> +static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
> +{
> + struct sk_buff *skb;
> + unsigned int skb_len;
> + struct zynqmp_ipi_message *mb_msg;
> + int ret;
> +
> + struct device *dev = rproc->dev.parent;
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> +
> + skb_len = (unsigned int)(sizeof(vqid) + sizeof(mb_msg));
> + skb = alloc_skb(skb_len, GFP_ATOMIC);
> + if (!skb)
> + 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(&z_rproc->tx_mc_skbs, skb);
> + ret = mbox_send_message(z_rproc->tx_chan, mb_msg);
> + if (ret < 0) {
> + dev_warn(dev, "Failed to kick remote.\n");
> + skb_dequeue_tail(&z_rproc->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_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_rproc *z_rproc;
> + struct rproc *rproc;
> +
> + z_rproc = dev_get_drvdata(dev);
> + rproc = z_rproc->rproc;
> + if (rproc) {
> + rproc_del(rproc);
> + rproc_free(rproc);
> + }
> +}
> +
> +/**
> + * 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_rproc *z_rproc;
> +
> + z_rproc = container_of(work, struct zynqmp_r5_rproc, mbox_work);
> +
> + (void)mbox_send_message(z_rproc->rx_chan, NULL);
> + rproc = z_rproc->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_rproc *z_rproc;
> +
> + z_rproc = container_of(cl, struct zynqmp_r5_rproc, 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 *)z_rproc->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(&z_rproc->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_rproc *z_rproc;
> + struct sk_buff *skb;
> +
> + if (!mssg)
> + return;
> + z_rproc = container_of(cl, struct zynqmp_r5_rproc, tx_mc);
> + skb = skb_dequeue(&z_rproc->tx_mc_skbs);
> + kfree_skb(skb);
> +}
> +
> +/**
> + * zynqmp_r5_setup_mbox() - Setup mailboxes
> + *
> + * @z_rproc: 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_rproc *z_rproc,
> + struct device_node *node)
> +{
> + struct device *dev = &z_rproc->dev;
> + struct mbox_client *mclient;
> +
> + dev->of_node = node;
> +
> + /* Setup TX mailbox channel client */
> + mclient = &z_rproc->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 = &z_rproc->rx_mc;
> + mclient->dev = dev;
> + mclient->rx_callback = zynqmp_r5_mb_rx_cb;
> + mclient->tx_block = false;
> + mclient->knows_txdone = false;
> +
> + INIT_WORK(&z_rproc->mbox_work, handle_event_notified);
> +
> + /* Request TX and RX channels */
> + z_rproc->tx_chan = mbox_request_channel_byname(&z_rproc->tx_mc, "tx");
> + if (IS_ERR(z_rproc->tx_chan)) {
> + dev_err(dev, "failed to request mbox tx channel.\n");
> + z_rproc->tx_chan = NULL;
> + return -EINVAL;
> + }
> + z_rproc->rx_chan = mbox_request_channel_byname(&z_rproc->rx_mc, "rx");
> + if (IS_ERR(z_rproc->rx_chan)) {
> + dev_err(dev, "failed to request mbox rx channel.\n");
> + z_rproc->rx_chan = NULL;
> + return -EINVAL;
> + }
> + skb_queue_head_init(&z_rproc->tx_mc_skbs);
> +
> + return 0;
> +}
> +
> +/**
> + * zynqmp_r5_probe() - Probes ZynqMP R5 processor device node
> + * @z_rproc: pointer to the ZynqMP R5 processor platform data
> + * @pdev: parent RPU domain platform device
> + * @node: pointer of the device node
> + * @rpu_mode: rpu config set by DT
> + *
> + * 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 platform_device *pdev,
> + struct device_node *node,
> + enum rpu_oper_mode rpu_mode)
> +{
> + struct rproc *rproc;
> + int ret;
> + struct zynqmp_r5_rproc *z_rproc;
> + struct device *dev = &pdev->dev;
> +
> + /* Allocate remoteproc instance */
> + rproc = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops, NULL, sizeof(*z_rproc));
> + if (!rproc) {
> + ret = -ENOMEM;
> + goto error;
> + }
Should be just:
if (!rproc)
return -ENOMEM;
As this is, if the allocation fails, z_rproc is uninitialized and the
z_rproc->rproc deref below the error label is going to be problematic.
> + z_rproc = rproc->priv;
> + z_rproc->dev.release = zynqmp_r5_release;
This is the only field of z_rproc->dev that's actually initialized, and
this device is not registered with the core at all, so zynqmp_r5_release
will never be called.
Since it doesn't look like there's a need to create this additional
device, I'd suggest:
- Dropping the struct device from struct zynqmp_r5_rproc
- Performing the necessary cleanup in the driver remove
callback instead of trying to tie it to device release
> +
> + /* Set up DMA mask */
> + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> + if (ret)
> + goto error;
> + /* Get R5 power domain node */
> + ret = of_property_read_u32(node, "pnode-id", &z_rproc->pnode_id);
> + if (ret)
> + goto error;
> +
> + ret = r5_set_mode(z_rproc, rpu_mode);
> + if (ret)
> + return ret;
> +
> + if (of_property_read_bool(node, "mboxes")) {
> + ret = zynqmp_r5_setup_mbox(z_rproc, node);
> + if (ret)
> + goto error;
> + }
> + /* Add R5 remoteproc */
> + ret = rproc_add(rproc);
> + if (ret)
> + goto error;
> +
> + return 0;
> +error:
> + if (z_rproc->rproc)
> + rproc_free(z_rproc->rproc);
> + z_rproc->rproc = NULL;
> + return ret;
> +}
> +
> +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
> +{
> + int ret, i;
> + struct device *dev = &pdev->dev;
> + struct device_node *nc;
> + enum rpu_oper_mode rpu_mode;
> +
> + rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode") ?
> + PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;
> + dev_dbg(dev, "RPU configuration: %s\n",
> + rpu_mode == PM_RPU_MODE_LOCKSTEP ? "lockstep" : "split");
> +
> + /*
> + * if 2 RPUs provided but one is lockstep, then we have an
> + * invalid configuration.
> + */
> + i = of_get_available_child_count(dev->of_node);
> + if ((rpu_mode == PM_RPU_MODE_LOCKSTEP && i != 1) || i > MAX_RPROCS)
> + return -EINVAL;
> +
> + i = 0;
> + for_each_available_child_of_node(dev->of_node, nc) {
> + /* only call zynqmp_r5_probe if proper # of rpu's */
> + ret = zynqmp_r5_probe(pdev, nc, rpu_mode);
> + dev_dbg(dev, "%s to probe rpu %pOF\n",
> + ret ? "Failed" : "Able",
> + nc);
> + if (ret)
> + return ret;
> + i++;
> + }
> +
> + return 0;
> +}
> +
> +/* Match table for OF platform binding */
> +static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
> + { .compatible = "xlnx,zynqmp-r5-remoteproc", },
> + { /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match);
> +
> +static struct platform_driver zynqmp_r5_remoteproc_driver = {
> + .probe = zynqmp_r5_remoteproc_probe,
> + .driver = {
> + .name = "zynqmp_r5_remoteproc",
> + .of_match_table = zynqmp_r5_remoteproc_match,
> + },
> +};
> +module_platform_driver(zynqmp_r5_remoteproc_driver);
> +
> +MODULE_AUTHOR("Ben Levinsky <ben.levinsky@xilinx.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-05 19:34 ` Michael Auchter
@ 2020-10-06 19:15 ` Ben Levinsky
2020-10-06 21:31 ` Michael Auchter
0 siblings, 1 reply; 20+ messages in thread
From: Ben Levinsky @ 2020-10-06 19:15 UTC (permalink / raw)
To: Michael Auchter
Cc: Ed T. Mooring, sunnyliangjy, punit1.agrawal, Stefano Stabellini,
Michal Simek, devicetree, mathieu.poirier, linux-remoteproc,
linux-kernel, robh+dt, linux-arm-kernel
Hi Michael,
Thanks for the review
> -----Original Message-----
> From: Michael Auchter <michael.auchter@ni.com>
> Sent: Monday, October 5, 2020 12:35 PM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; sunnyliangjy@gmail.com;
> punit1.agrawal@toshiba.co.jp; Stefano Stabellini <stefanos@xilinx.com>;
> Michal Simek <michals@xilinx.com>; devicetree@vger.kernel.org;
> mathieu.poirier@linaro.org; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; robh+dt@kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> driver
>
> Hey Ben,
>
> On Mon, Oct 05, 2020 at 09:06:14AM -0700, Ben Levinsky wrote:
> > R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> > remotproc driver, we can boot the R5 sub-system in different 2
> > configurations -
> > * split
> > * lock-step
> >
> > The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
> > Platform Management Unit that handles the R5 configuration, memory
> access
> > and R5 lifecycle management. The interface to this manager is done in this
> > driver via zynqmp_pm_* function calls.
> >
> > Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Signed-off-by: Ed Mooring <ed.mooring@xilinx.com>
> > Signed-off-by: Jason Wu <j.wu@xilinx.com>
> > Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> > ---
> > v2:
> > - remove domain struct as per review from Mathieu
> > v3:
> > - add xilinx-related platform mgmt fn's instead of wrapping around
> > function pointer in xilinx eemi ops struct
> > v4:
> > - add default values for enums
> > - fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1
> check
> > are still raised as each is due to fixing the warning results in that
> > particular line going over 80 characters.
> > v5:
> > - parse_fw change from use of rproc_of_resm_mem_entry_init to
> > rproc_mem_entry_init and use of alloc/release
> > - var's of type zynqmp_r5_pdata all have same local variable name
> > - use dev_dbg instead of dev_info
> > v6:
> > - adding memory carveouts is handled much more similarly. All mem
> > carveouts are
> > now described in reserved memory as needed. That is, TCM nodes are not
> > coupled to remoteproc anymore. This is reflected in the remoteproc R5
> > driver
> > and the device tree binding.
> > - remove mailbox from device tree binding as it is not necessary for elf
> > loading
> > - use lockstep-mode property for configuring RPU
> > v7:
> > - remove unused headers
> > - change u32 *lockstep_mode -> u32 lockstep_mode;
> > - change device-tree binding "lockstep-mode" to xlnx,cluster-mode
> > - remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at
> > remoteproc-probe time
> > - remove is_r5_mode_set from zynqmp rpu remote processor private data
> > - do not error out if no mailbox is provided
> > - remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as
> > pdata is
> > handled in zynqmp_r5_remoteproc_remove
> > v8:
> > - remove old acks, reviewed-by's in commit message
> > v9:
> > - as mboxes are now optional, if pdata->tx_mc_skbs not initialized then
> > do not call skb_queue_empty
> > - update usage for zynqmp_pm_set_rpu_mode,
> zynqmp_pm_set_tcm_config and
> > zynqmp_pm_get_rpu_mode
> > - update 5/5 patch commit message to document supported configurations
> > and how they are booted by the driver.
> > - remove copyrights other than SPDX from zynqmp_r5_remoteproc.c
> > - compilation warnings no longer raised
> > - remove unused includes from zynqmp_r5_remoteproc.c
> > - remove unused var autoboot from zynqmp_r5_remoteproc.c
> > - reorder zynqmp_r5_pdata fpr small mem savings due to alignment
> > - use of zynqmp_pm_set_tcm_config now does not have
> > output arg
> > - in tcm handling, unconditionally use &= 0x000fffff mask since all nodes
> > in this fn are for tcm
> > - update comments for translating dma field in tcm handling to device
> > address
> > - update calls to rproc_mem_entry_init in parse_mem_regions so that there
> > are only 2 cases for types of carveouts instead of 3
> > - in parse_mem_regions, check if device tree node is null before using it
> > - add example device tree nodes used in parse_mem_regions and tcm
> parsing
> > - add comment for vring id node length
> > - add check for string length so that vring id is at least min length
> > - move tcm nodes from reserved mem to instead own device tree nodes
> > and only use them if enabled in device tree
> > - add comment for explaining handling of rproc_elf_load_rsc_table
> > - remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick
> > - remove unused field mems in struct zynqmp_r5_pdata
> > - remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling
> > is done by zyqmp_r5_pm_request_tcm
> > - remove obsolete setting of dma_ops and parent device dma_mask
> > - remove obsolete use of of_dma_configure
> > - add comment for call to r5_set_mode fn
> > - make mbox usage optional and gracefully inform user via dev_dbg if not
> > present
> > - change var lockstep_mode from u32* to u32
> > v11:
> > - use enums instead of u32 where possible in zynqmp_r5_remoteproc
> > - update usage of zynqmp_pm_set/get_rpu_mode and
> zynqmp_pm_set_tcm_config
> > - update prints to not use carriage return, just newline
> > - look up tcm banks via property in r5 node instead of string name
> > - print device tree nodes with %pOF instead of %s with node name field
> > - update tcm release to unmap VA
> > - handle r5-1 use case
> > v12:
> > - update signed off by so that latest developer name is last
> > - do not cast enums to u32s for zynqmp_pm* functions
> > v14:
> > - change zynqmp_r5_remoteproc::rpus and rpu_mode to static
> > - fix typo
> > - zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
> > property specified in device tree
> > - use u32 instead of u32* to store in remoteproc memory entry private data
> > for pnode_id information
> > - always call r5_set_mode on probe
> > - remove alloc of zynqmp_r5_pdata in
> > zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static
> > allocation already
> > - error at probe time if lockstep-mode property not present in device tree
> > - update commit message as per review
> > - remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is
> present
> > - remove unused macros
> > - update comment ordering of zynqmp_r5_pdata to match struct definition
> > - zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
> > - remove obsolete TODOs
> > - only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
> > - remove uneven dev_dbg/dev_err fn calls
> > v15:
> > - if lockstep mode prop is present, then RPU cluster is in lockstep mode.
> > if not present, cluster is in split mode.
> > - if 2 RPUs provided but one is lockstep then error out as this is invalid
> > configuration
> > v16:
> > - replace of_get_property(dev->of_node, "lockstep-mode" with
> > of_property_read_bool
> > - propagate rpu mode specified in device tree through functions instead
> > of holding a global, static var
> > - check child remoteproc nodes via of_get_available_child_count before
> > looping through children
> > - replace check of "pdata->pnode_id == 0" instead by checking rpu's
> > zynqmp_r5_pdata* if NULL
> > - remove old, obsolete checks for dma_pools in
> zynqmp_r5_remoteproc_remove
> > - change rpus from zynqmp_r5_pdata[] to zynqmp_r5_pdata*[] so that
> > check for pdata->pnode_id == 0 is not needed
> > v17:
> > - fix style as per kernel test bot
> > v18:
> > - to more closely mimic other remoteproc drivers, change zynqmp r5 rproc
> > data from zynqmp_r5_pdata to zynqmp_r5_rproc and pdata local var to
> > zproc
> > - remove global vars rpus and rpu_mode
> > - instantiate device for zynqmp r5 rproc from device set by rproc_alloc
> > - fix typos
> > - update to call zynqmp_r5_release from the rproc_alloc-related device and
> > remove the instantiated device from zynqmp_r5_probe
> > - remove unneeded call to platform_set_drvdata
> > - remove driver remove function, as the clean up is handled in release
> > - remove while (!skb_queue_empty loop and mbox_free_channel calls in
> > zynqmp_r5_release, and mbox_free_channel
> > - remove device_unregister call in zynqmp_r5_release
> > - remove kzalloc for pdata (what is now called z_rproc)
> > - update conditional in loop to calls of zynqmp_r5_probe
> >
> > ---
> > drivers/remoteproc/Kconfig | 8 +
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/zynqmp_r5_remoteproc.c | 707
> ++++++++++++++++++++++
> > 3 files changed, 716 insertions(+)
> > create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
> >
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index c6659dfea7c7..68e567c5375c 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -275,6 +275,14 @@ config TI_K3_DSP_REMOTEPROC
> > It's safe to say N here if you're not interested in utilizing
> > the DSP slave processors.
> >
> > +config ZYNQMP_R5_REMOTEPROC
> > + tristate "ZynqMP_R5 remoteproc support"
> > + depends on PM && ARCH_ZYNQMP
> > + select RPMSG_VIRTIO
> > + select ZYNQMP_IPI_MBOX
> > + help
> > + Say y or m here to support ZynqMP R5 remote processors via the
> remote
> > + processor framework.
> > endif # REMOTEPROC
> >
> > endmenu
> > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > index 3dfa28e6c701..ef1abff654c2 100644
> > --- a/drivers/remoteproc/Makefile
> > +++ b/drivers/remoteproc/Makefile
> > @@ -33,3 +33,4 @@ obj-$(CONFIG_ST_REMOTEPROC) +=
> st_remoteproc.o
> > obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> > obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> > obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> > +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o
> > diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c
> b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> > new file mode 100644
> > index 000000000000..37bd76252ff2
> > --- /dev/null
> > +++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> > @@ -0,0 +1,707 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Zynq R5 Remote Processor driver
> > + *
> > + * Based on origin OMAP and Zynq Remote Processor driver
> > + *
> > + */
> > +
> > +#include <linux/firmware/xlnx-zynqmp.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_platform.h>
> > +#include <linux/of_reserved_mem.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/skbuff.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 BANK_LIST_PROP "meta-memory-regions"
> > +
> > +/* IPI buffer MAX length */
> > +#define IPI_BUF_LEN_MAX 32U
> > +/* RX mailbox client buffer max length */
> > +#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
> > + sizeof(struct zynqmp_ipi_message))
> > +
> > +/**
> > + * struct zynqmp_r5_mem - zynqmp rpu memory data
> > + * @pnode_id: TCM power domain ids
> > + * @res: memory resource
> > + * @node: list node
> > + */
> > +struct zynqmp_r5_mem {
> > + u32 pnode_id[MAX_MEM_PNODES];
> > + struct resource res;
> > + struct list_head node;
> > +};
> > +
> > +/**
> > + * struct zynqmp_r5_rproc - zynqmp rpu remote processor state
> > + * @rx_mc_buf: rx mailbox client buffer to save the rx message
> > + * @tx_mc: tx mailbox client
> > + * @rx_mc: rx mailbox client * @dev: device of RPU instance
> > + * @mbox_work: mbox_work for the RPU remoteproc
> > + * @tx_mc_skbs: socket buffers for tx mailbox client
> > + * @dev: device of RPU instance
> > + * @rproc: rproc handle
> > + * @tx_chan: tx mailbox channel
> > + * @rx_chan: rx mailbox channel
> > + * @pnode_id: RPU CPU power domain id
> > + */
> > +struct zynqmp_r5_rproc {
> > + unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
> > + struct mbox_client tx_mc;
> > + struct mbox_client rx_mc;
> > + struct work_struct mbox_work;
> > + struct sk_buff_head tx_mc_skbs;
> > + struct device dev;
> > + struct rproc *rproc;
> > + struct mbox_chan *tx_chan;
> > + struct mbox_chan *rx_chan;
> > + u32 pnode_id;
> > +};
> > +
> > +/*
> > + * r5_set_mode - set RPU operation mode
> > + * @z_rproc: Remote processor private data
> > + *
> > + * set RPU operation mode
> > + *
> > + * Return: 0 for success, negative value for failure
> > + */
> > +static int r5_set_mode(struct zynqmp_r5_rproc *z_rproc,
> > + enum rpu_oper_mode rpu_mode)
> > +{
> > + enum rpu_tcm_comb tcm_mode;
> > + enum rpu_oper_mode cur_rpu_mode;
> > + int ret;
> > +
> > + ret = zynqmp_pm_get_rpu_mode(z_rproc->pnode_id,
> &cur_rpu_mode);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (rpu_mode != cur_rpu_mode) {
> > + ret = zynqmp_pm_set_rpu_mode(z_rproc->pnode_id,
> > + rpu_mode);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + tcm_mode = (rpu_mode == PM_RPU_MODE_LOCKSTEP) ?
> > + PM_RPU_TCM_COMB : PM_RPU_TCM_SPLIT;
> > + return zynqmp_pm_set_tcm_config(z_rproc->pnode_id, tcm_mode);
> > +}
> > +
> > +/*
> > + * ZynqMP R5 remoteproc memory release function
> > + */
> > +static int tcm_mem_release(struct rproc *rproc, struct rproc_mem_entry
> *mem)
> > +{
> > + u32 pnode_id = (u64)mem->priv;
> > +
> > + if (pnode_id <= 0)
> > + return -EINVAL;
> > +
> > + iounmap(mem->va);
> > + return zynqmp_pm_release_node(pnode_id);
> > +}
> > +
> > +/*
> > + * ZynqMP R5 remoteproc operations
> > + */
> > +static int zynqmp_r5_rproc_start(struct rproc *rproc)
> > +{
> > + struct device *dev = rproc->dev.parent;
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > + enum rpu_boot_mem bootmem;
> > +
> > + bootmem = (rproc->bootaddr & 0xF0000000) == 0xF0000000 ?
> > + PM_RPU_BOOTMEM_HIVEC : PM_RPU_BOOTMEM_LOVEC;
> > +
> > + dev_dbg(dev, "RPU boot from %s.",
> > + bootmem == PM_RPU_BOOTMEM_HIVEC ? "OCM" : "TCM");
> > +
> > + return zynqmp_pm_request_wake(z_rproc->pnode_id, 1,
> > + bootmem,
> ZYNQMP_PM_REQUEST_ACK_NO);
> > +}
> > +
> > +static int zynqmp_r5_rproc_stop(struct rproc *rproc)
> > +{
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > + struct sk_buff *skb;
> > +
> > + if (z_rproc->tx_chan)
> > + mbox_free_channel(z_rproc->tx_chan);
> > + if (z_rproc->rx_chan)
> > + mbox_free_channel(z_rproc->rx_chan);
>
> This looks incorrect: these are requested during probe, so I would
> expect these to be free'd during remove.
>
> It's legal to call stop, then start again, then stop again, etc.
> Consider what would happen here in that case.
>
> > + return zynqmp_pm_force_pwrdwn(z_rproc->pnode_id,
> > + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> > +}
> > +
> > +static int zynqmp_r5_rproc_mem_alloc(struct rproc *rproc,
> > + struct rproc_mem_entry *mem)
> > +{
> > + void *va;
> > +
> > + va = ioremap_wc(mem->dma, mem->len);
> > + if (IS_ERR_OR_NULL(va))
> > + 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)
> > +{
> > + iounmap(mem->va);
> > + return 0;
> > +}
> > +
> > +static int parse_mem_regions(struct rproc *rproc)
> > +{
> > + int num_mems, i;
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > + struct device *dev = &z_rproc->dev;
> > + struct device_node *np = dev->of_node;
> > + 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);
> > + if (!node)
> > + return -EINVAL;
> > +
> > + rmem = of_reserved_mem_lookup(node);
> > + if (!rmem)
> > + return -EINVAL;
> > +
> > + if (strstr(node->name, "vdev0vring")) {
> > + int vring_id;
> > + char name[16];
> > +
> > + /*
> > + * expecting form of "rpuXvdev0vringX as documented
> > + * in xilinx remoteproc device tree binding
> > + */
> > + if (strlen(node->name) < 14) {
> > + dev_err(dev, "%pOF is less than 14 chars",
> > + node);
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * can be 1 of multiple vring IDs per IPC channel
> > + * e.g. 'vdev0vring0' and 'vdev0vring1'
> > + */
> > + vring_id = node->name[14] - '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);
> > + } else {
> > + /* Register DMA region */
> > + int (*alloc)(struct rproc *r,
> > + struct rproc_mem_entry *rme);
> > + int (*release)(struct rproc *r,
> > + struct rproc_mem_entry *rme);
> > + char name[20];
> > +
> > + if (strstr(node->name, "vdev0buffer")) {
> > + alloc = NULL;
> > + release = NULL;
> > + strcpy(name, "vdev0buffer");
> > + } else {
> > + alloc = zynqmp_r5_rproc_mem_alloc;
> > + release = zynqmp_r5_rproc_mem_release;
> > + strcpy(name, node->name);
> > + }
> > +
> > + mem = rproc_mem_entry_init(dev, NULL,
> > + (dma_addr_t)rmem->base,
> > + rmem->size, rmem->base,
> > + alloc, release, name);
> > + }
> > + if (!mem)
> > + return -ENOMEM;
> > +
> > + rproc_add_carveout(rproc, mem);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* call Xilinx Platform manager to request access to TCM bank */
> > +static int zynqmp_r5_pm_request_tcm(struct device_node *tcm_node,
> > + struct device *dev, u32 *pnode_id)
> > +{
> > + int ret;
> > +
> > + ret = of_property_read_u32(tcm_node, "pnode-id", pnode_id);
> > + if (ret)
> > + return ret;
> > +
> > + return zynqmp_pm_request_node(*pnode_id,
> ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> > + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> > +}
> > +
> > +/* Given tcm bank entry,
> > + * this callback will set device address for R5 running on TCM
> > + * and also setup virtual address for tcm bank remoteproc carveout
> > + */
> > +static int tcm_mem_alloc(struct rproc *rproc,
> > + struct rproc_mem_entry *mem)
> > +{
> > + void *va;
> > + struct device *dev = rproc->dev.parent;
> > +
> > + va = ioremap_wc(mem->dma, mem->len);
> > + if (IS_ERR_OR_NULL(va))
> > + return -ENOMEM;
> > +
> > + /* Update memory entry va */
> > + mem->va = va;
> > +
> > + va = devm_ioremap_wc(dev, mem->da, mem->len);
> > + if (!va)
> > + return -ENOMEM;
> > + /* As R5 is 32 bit, wipe out extra high bits */
> > + mem->da &= 0x000fffff;
> > + /*
> > + * handle tcm banks 1 a and b (0xffe90000 and oxffeb0000)
> > + * As both of these the only common bit found not in tcm bank0 a or
> b
> > + * is at 0x80000 use this mask to suss it out
> > + */
> > + if (mem->da & 0x80000)
> > + /*
> > + * need to do more to further translate
> > + * tcm banks 1a and 1b at 0xffe90000 and oxffeb0000
> > + * respectively to 0x0 and 0x20000
> > + */
> > + mem->da -= 0x90000;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Given R5 node in remoteproc instance,
> > + * allocate remoteproc carveout for TCM memory
> > + * needed for firmware to be loaded
> > + */
> > +static int parse_tcm_banks(struct rproc *rproc)
> > +{
> > + int i, num_banks;
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > + struct device *dev = &z_rproc->dev;
> > + struct device_node *r5_node = dev->of_node;
> > +
> > + /* go through tcm banks for r5 node */
> > + num_banks = of_count_phandle_with_args(r5_node,
> BANK_LIST_PROP, NULL);
> > + if (num_banks <= 0) {
> > + dev_err(dev, "need to specify TCM banks\n");
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < num_banks; i++) {
> > + struct resource rsc;
> > + resource_size_t size;
> > + struct device_node *dt_node;
> > + struct rproc_mem_entry *mem;
> > + int ret;
> > + u32 pnode_id; /* zynqmp_pm* fn's expect u32 */
> > +
> > + dt_node = of_parse_phandle(r5_node, BANK_LIST_PROP, i);
> > + if (!dt_node)
> > + return -EINVAL;
> > +
> > + if (of_device_is_available(dt_node)) {
> > + ret = of_address_to_resource(dt_node, 0, &rsc);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = zynqmp_r5_pm_request_tcm(dt_node, dev,
> &pnode_id);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* add carveout */
> > + size = resource_size(&rsc);
> > + mem = rproc_mem_entry_init(dev, NULL, rsc.start,
> > + (int)size, rsc.start,
> > + tcm_mem_alloc,
> > + tcm_mem_release,
> > + rsc.name);
> > + if (!mem)
> > + return -ENOMEM;
> > +
> > + mem->priv = (void *)(u64)pnode_id;
> > + 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_rproc *z_rproc = rproc->priv;
> > + struct device *dev = &z_rproc->dev;
> > +
> > + ret = parse_tcm_banks(rproc);
> > + if (ret)
> > + return ret;
> > +
> > + ret = parse_mem_regions(rproc);
> > + if (ret)
> > + return ret;
> > +
> > + ret = rproc_elf_load_rsc_table(rproc, fw);
> > + if (ret == -EINVAL) {
> > + /*
> > + * resource table only required for IPC.
> > + * if not present, this is not necessarily an error;
> > + * for example, loading r5 hello world application
> > + * so simply inform user and keep going.
> > + */
> > + dev_info(dev, "no resource table found.\n");
> > + ret = 0;
> > + }
> > + return ret;
> > +}
> > +
> > +/* kick a firmware */
> > +static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
> > +{
> > + struct sk_buff *skb;
> > + unsigned int skb_len;
> > + struct zynqmp_ipi_message *mb_msg;
> > + int ret;
> > +
> > + struct device *dev = rproc->dev.parent;
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > +
> > + skb_len = (unsigned int)(sizeof(vqid) + sizeof(mb_msg));
> > + skb = alloc_skb(skb_len, GFP_ATOMIC);
> > + if (!skb)
> > + 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(&z_rproc->tx_mc_skbs, skb);
> > + ret = mbox_send_message(z_rproc->tx_chan, mb_msg);
> > + if (ret < 0) {
> > + dev_warn(dev, "Failed to kick remote.\n");
> > + skb_dequeue_tail(&z_rproc->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_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_rproc *z_rproc;
> > + struct rproc *rproc;
> > +
> > + z_rproc = dev_get_drvdata(dev);
> > + rproc = z_rproc->rproc;
> > + if (rproc) {
> > + rproc_del(rproc);
> > + rproc_free(rproc);
> > + }
> > +}
> > +
> > +/**
> > + * 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_rproc *z_rproc;
> > +
> > + z_rproc = container_of(work, struct zynqmp_r5_rproc, mbox_work);
> > +
> > + (void)mbox_send_message(z_rproc->rx_chan, NULL);
> > + rproc = z_rproc->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_rproc *z_rproc;
> > +
> > + z_rproc = container_of(cl, struct zynqmp_r5_rproc, 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 *)z_rproc->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(&z_rproc->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_rproc *z_rproc;
> > + struct sk_buff *skb;
> > +
> > + if (!mssg)
> > + return;
> > + z_rproc = container_of(cl, struct zynqmp_r5_rproc, tx_mc);
> > + skb = skb_dequeue(&z_rproc->tx_mc_skbs);
> > + kfree_skb(skb);
> > +}
> > +
> > +/**
> > + * zynqmp_r5_setup_mbox() - Setup mailboxes
> > + *
> > + * @z_rproc: 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_rproc *z_rproc,
> > + struct device_node *node)
> > +{
> > + struct device *dev = &z_rproc->dev;
> > + struct mbox_client *mclient;
> > +
> > + dev->of_node = node;
> > +
> > + /* Setup TX mailbox channel client */
> > + mclient = &z_rproc->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 = &z_rproc->rx_mc;
> > + mclient->dev = dev;
> > + mclient->rx_callback = zynqmp_r5_mb_rx_cb;
> > + mclient->tx_block = false;
> > + mclient->knows_txdone = false;
> > +
> > + INIT_WORK(&z_rproc->mbox_work, handle_event_notified);
> > +
> > + /* Request TX and RX channels */
> > + z_rproc->tx_chan = mbox_request_channel_byname(&z_rproc-
> >tx_mc, "tx");
> > + if (IS_ERR(z_rproc->tx_chan)) {
> > + dev_err(dev, "failed to request mbox tx channel.\n");
> > + z_rproc->tx_chan = NULL;
> > + return -EINVAL;
> > + }
> > + z_rproc->rx_chan = mbox_request_channel_byname(&z_rproc-
> >rx_mc, "rx");
> > + if (IS_ERR(z_rproc->rx_chan)) {
> > + dev_err(dev, "failed to request mbox rx channel.\n");
> > + z_rproc->rx_chan = NULL;
> > + return -EINVAL;
> > + }
> > + skb_queue_head_init(&z_rproc->tx_mc_skbs);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * zynqmp_r5_probe() - Probes ZynqMP R5 processor device node
> > + * @z_rproc: pointer to the ZynqMP R5 processor platform data
> > + * @pdev: parent RPU domain platform device
> > + * @node: pointer of the device node
> > + * @rpu_mode: rpu config set by DT
> > + *
> > + * 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 platform_device *pdev,
> > + struct device_node *node,
> > + enum rpu_oper_mode rpu_mode)
> > +{
> > + struct rproc *rproc;
> > + int ret;
> > + struct zynqmp_r5_rproc *z_rproc;
> > + struct device *dev = &pdev->dev;
> > +
> > + /* Allocate remoteproc instance */
> > + rproc = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops,
> NULL, sizeof(*z_rproc));
> > + if (!rproc) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
>
> Should be just:
>
> if (!rproc)
> return -ENOMEM;
>
> As this is, if the allocation fails, z_rproc is uninitialized and the
> z_rproc->rproc deref below the error label is going to be problematic.
>
> > + z_rproc = rproc->priv;
> > + z_rproc->dev.release = zynqmp_r5_release;
>
> This is the only field of z_rproc->dev that's actually initialized, and
> this device is not registered with the core at all, so zynqmp_r5_release
> will never be called.
>
> Since it doesn't look like there's a need to create this additional
> device, I'd suggest:
> - Dropping the struct device from struct zynqmp_r5_rproc
> - Performing the necessary cleanup in the driver remove
> callback instead of trying to tie it to device release
For the most part I agree. I believe the device is still needed for the mailbox client setup.
As the call to mbox_request_channel_byname() requires its own device that has the corresponding child node with the corresponding mbox-related properties.
With that in mind, is it still ok to keep the device node?
Thanks
Ben
> > +
> > + /* Set up DMA mask */
> > + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> > + if (ret)
> > + goto error;
> > + /* Get R5 power domain node */
> > + ret = of_property_read_u32(node, "pnode-id", &z_rproc->pnode_id);
> > + if (ret)
> > + goto error;
> > +
> > + ret = r5_set_mode(z_rproc, rpu_mode);
> > + if (ret)
> > + return ret;
> > +
> > + if (of_property_read_bool(node, "mboxes")) {
> > + ret = zynqmp_r5_setup_mbox(z_rproc, node);
> > + if (ret)
> > + goto error;
> > + }
> > + /* Add R5 remoteproc */
> > + ret = rproc_add(rproc);
> > + if (ret)
> > + goto error;
> > +
> > + return 0;
> > +error:
> > + if (z_rproc->rproc)
> > + rproc_free(z_rproc->rproc);
> > + z_rproc->rproc = NULL;
> > + return ret;
> > +}
> > +
> > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
> > +{
> > + int ret, i;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *nc;
> > + enum rpu_oper_mode rpu_mode;
> > +
> > + rpu_mode = of_property_read_bool(dev->of_node, "lockstep-mode")
> ?
> > + PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT;
> > + dev_dbg(dev, "RPU configuration: %s\n",
> > + rpu_mode == PM_RPU_MODE_LOCKSTEP ? "lockstep" :
> "split");
> > +
> > + /*
> > + * if 2 RPUs provided but one is lockstep, then we have an
> > + * invalid configuration.
> > + */
> > + i = of_get_available_child_count(dev->of_node);
> > + if ((rpu_mode == PM_RPU_MODE_LOCKSTEP && i != 1) || i >
> MAX_RPROCS)
> > + return -EINVAL;
> > +
> > + i = 0;
> > + for_each_available_child_of_node(dev->of_node, nc) {
> > + /* only call zynqmp_r5_probe if proper # of rpu's */
> > + ret = zynqmp_r5_probe(pdev, nc, rpu_mode);
> > + dev_dbg(dev, "%s to probe rpu %pOF\n",
> > + ret ? "Failed" : "Able",
> > + nc);
> > + if (ret)
> > + return ret;
> > + i++;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* Match table for OF platform binding */
> > +static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
> > + { .compatible = "xlnx,zynqmp-r5-remoteproc", },
> > + { /* end of list */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match);
> > +
> > +static struct platform_driver zynqmp_r5_remoteproc_driver = {
> > + .probe = zynqmp_r5_remoteproc_probe,
> > + .driver = {
> > + .name = "zynqmp_r5_remoteproc",
> > + .of_match_table = zynqmp_r5_remoteproc_match,
> > + },
> > +};
> > +module_platform_driver(zynqmp_r5_remoteproc_driver);
> > +
> > +MODULE_AUTHOR("Ben Levinsky <ben.levinsky@xilinx.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-06 19:15 ` Ben Levinsky
@ 2020-10-06 21:31 ` Michael Auchter
2020-10-06 21:46 ` Ben Levinsky
0 siblings, 1 reply; 20+ messages in thread
From: Michael Auchter @ 2020-10-06 21:31 UTC (permalink / raw)
To: Ben Levinsky
Cc: Ed T. Mooring, sunnyliangjy, punit1.agrawal, Stefano Stabellini,
Michal Simek, devicetree, mathieu.poirier, linux-remoteproc,
linux-kernel, robh+dt, linux-arm-kernel
On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote:
>
> Hi Michael,
>
> Thanks for the review
>
< ... snip ... >
> > > + z_rproc = rproc->priv;
> > > + z_rproc->dev.release = zynqmp_r5_release;
> >
> > This is the only field of z_rproc->dev that's actually initialized, and
> > this device is not registered with the core at all, so zynqmp_r5_release
> > will never be called.
> >
> > Since it doesn't look like there's a need to create this additional
> > device, I'd suggest:
> > - Dropping the struct device from struct zynqmp_r5_rproc
> > - Performing the necessary cleanup in the driver remove
> > callback instead of trying to tie it to device release
>
> For the most part I agree. I believe the device is still needed for
> the mailbox client setup.
>
> As the call to mbox_request_channel_byname() requires its own device
> that has the corresponding child node with the corresponding
> mbox-related properties.
>
> With that in mind, is it still ok to keep the device node?
Ah, I see. Thanks for the clarification!
Instead of manually dealing with the device node creation for the
individual processors, perhaps it makes more sense to use
devm_of_platform_populate() to create them. This is also consistent with
the way the TI K3 R5F remoteproc driver does things.
Cheers,
Michael
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-06 21:31 ` Michael Auchter
@ 2020-10-06 21:46 ` Ben Levinsky
2020-10-06 22:20 ` Michael Auchter
0 siblings, 1 reply; 20+ messages in thread
From: Ben Levinsky @ 2020-10-06 21:46 UTC (permalink / raw)
To: Michael Auchter
Cc: Ed T. Mooring, Stefano Stabellini, Michal Simek, devicetree,
mathieu.poirier, linux-remoteproc, linux-kernel, robh+dt,
linux-arm-kernel
> -----Original Message-----
> From: Michael Auchter <michael.auchter@ni.com>
> Sent: Tuesday, October 6, 2020 2:32 PM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; sunnyliangjy@gmail.com;
> punit1.agrawal@toshiba.co.jp; Stefano Stabellini <stefanos@xilinx.com>;
> Michal Simek <michals@xilinx.com>; devicetree@vger.kernel.org;
> mathieu.poirier@linaro.org; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; robh+dt@kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> remoteproc driver
>
> On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote:
> >
> > Hi Michael,
> >
> > Thanks for the review
> >
>
> < ... snip ... >
>
> > > > + z_rproc = rproc->priv;
> > > > + z_rproc->dev.release = zynqmp_r5_release;
> > >
> > > This is the only field of z_rproc->dev that's actually initialized, and
> > > this device is not registered with the core at all, so zynqmp_r5_release
> > > will never be called.
> > >
> > > Since it doesn't look like there's a need to create this additional
> > > device, I'd suggest:
> > > - Dropping the struct device from struct zynqmp_r5_rproc
> > > - Performing the necessary cleanup in the driver remove
> > > callback instead of trying to tie it to device release
> >
> > For the most part I agree. I believe the device is still needed for
> > the mailbox client setup.
> >
> > As the call to mbox_request_channel_byname() requires its own device
> > that has the corresponding child node with the corresponding
> > mbox-related properties.
> >
> > With that in mind, is it still ok to keep the device node?
>
> Ah, I see. Thanks for the clarification!
>
> Instead of manually dealing with the device node creation for the
> individual processors, perhaps it makes more sense to use
> devm_of_platform_populate() to create them. This is also consistent with
> the way the TI K3 R5F remoteproc driver does things.
>
> Cheers,
> Michael
I've been working on this today for a way around it and found one that I think works with your initial suggestion,
- in z_rproc, change dev from struct device to struct device*
^ the above is shown the usage thereof below. It is there for the mailbox setup.
- in driver probe:
- add list_head to keep track of each core's z_rproc and for the driver remove clean up
- in each core's probe (zynqmp_r5_probe) dothe following:
rproc_ptr = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops,
NULL, sizeof(struct zynqmp_r5_rproc));
if (!rproc_ptr)
return -ENOMEM;
z_rproc = rproc_ptr->priv;
z_rproc->dt_node = node;
z_rproc->rproc = rproc_ptr;
z_rproc->dev = &rproc_ptr->dev;
z_rproc->dev->of_node = node;
where node is the specific R5 core's of_node/ Device tree node.
the above preserves most of the mailbox setup code.
With this, I have already successfully done the following in a v19 patch
- move all the previous driver release code to remove
- able to probe, start/stop r5, driver remove repeatedly
Also, this mimics the TI R5 driver code as each core's rproc has a list_head and they have a structure for the cluster which among other things maintains a linked list of the cores' specific rproc information.
Thanks
Ben
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-06 21:46 ` Ben Levinsky
@ 2020-10-06 22:20 ` Michael Auchter
2020-10-07 14:31 ` Ben Levinsky
2020-10-15 18:31 ` Ben Levinsky
0 siblings, 2 replies; 20+ messages in thread
From: Michael Auchter @ 2020-10-06 22:20 UTC (permalink / raw)
To: Ben Levinsky
Cc: Ed T. Mooring, Stefano Stabellini, Michal Simek, devicetree,
mathieu.poirier, linux-remoteproc, linux-kernel, robh+dt,
linux-arm-kernel
On Tue, Oct 06, 2020 at 09:46:38PM +0000, Ben Levinsky wrote:
>
>
> > -----Original Message-----
> > From: Michael Auchter <michael.auchter@ni.com>
> > Sent: Tuesday, October 6, 2020 2:32 PM
> > To: Ben Levinsky <BLEVINSK@xilinx.com>
> > Cc: Ed T. Mooring <emooring@xilinx.com>; sunnyliangjy@gmail.com;
> > punit1.agrawal@toshiba.co.jp; Stefano Stabellini <stefanos@xilinx.com>;
> > Michal Simek <michals@xilinx.com>; devicetree@vger.kernel.org;
> > mathieu.poirier@linaro.org; linux-remoteproc@vger.kernel.org; linux-
> > kernel@vger.kernel.org; robh+dt@kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> > remoteproc driver
> >
> > On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote:
> > >
> > > Hi Michael,
> > >
> > > Thanks for the review
> > >
> >
> > < ... snip ... >
> >
> > > > > + z_rproc = rproc->priv;
> > > > > + z_rproc->dev.release = zynqmp_r5_release;
> > > >
> > > > This is the only field of z_rproc->dev that's actually initialized, and
> > > > this device is not registered with the core at all, so zynqmp_r5_release
> > > > will never be called.
> > > >
> > > > Since it doesn't look like there's a need to create this additional
> > > > device, I'd suggest:
> > > > - Dropping the struct device from struct zynqmp_r5_rproc
> > > > - Performing the necessary cleanup in the driver remove
> > > > callback instead of trying to tie it to device release
> > >
> > > For the most part I agree. I believe the device is still needed for
> > > the mailbox client setup.
> > >
> > > As the call to mbox_request_channel_byname() requires its own device
> > > that has the corresponding child node with the corresponding
> > > mbox-related properties.
> > >
> > > With that in mind, is it still ok to keep the device node?
> >
> > Ah, I see. Thanks for the clarification!
> >
> > Instead of manually dealing with the device node creation for the
> > individual processors, perhaps it makes more sense to use
> > devm_of_platform_populate() to create them. This is also consistent with
> > the way the TI K3 R5F remoteproc driver does things.
> >
> > Cheers,
> > Michael
>
> I've been working on this today for a way around it and found one that I think works with your initial suggestion,
> - in z_rproc, change dev from struct device to struct device*
> ^ the above is shown the usage thereof below. It is there for the mailbox setup.
> - in driver probe:
> - add list_head to keep track of each core's z_rproc and for the driver remove clean up
> - in each core's probe (zynqmp_r5_probe) dothe following:
>
>
> rproc_ptr = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops,
> NULL, sizeof(struct zynqmp_r5_rproc));
> if (!rproc_ptr)
> return -ENOMEM;
> z_rproc = rproc_ptr->priv;
> z_rproc->dt_node = node;
> z_rproc->rproc = rproc_ptr;
> z_rproc->dev = &rproc_ptr->dev;
> z_rproc->dev->of_node = node;
> where node is the specific R5 core's of_node/ Device tree node.
>
> the above preserves most of the mailbox setup code.
I see how this works, but it feels a bit weird to me to be overriding
the remoteproc dev's of_node ptr. Personally I find the
devm_of_platform_populate() approach a bit less confusing.
But, it's also not my call to make ;). Perhaps a remoteproc maintainer
can chime in here.
>
>
> With this, I have already successfully done the following in a v19 patch
> - move all the previous driver release code to remove
> - able to probe, start/stop r5, driver remove repeatedly
>
> Also, this mimics the TI R5 driver code as each core's rproc has a list_head and they have a structure for the cluster which among other things maintains a linked list of the cores' specific rproc information.
>
> Thanks
> Ben
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-06 22:20 ` Michael Auchter
@ 2020-10-07 14:31 ` Ben Levinsky
2020-10-15 18:31 ` Ben Levinsky
1 sibling, 0 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-07 14:31 UTC (permalink / raw)
To: Michael Auchter
Cc: Ed T. Mooring, Stefano Stabellini, Michal Simek, devicetree,
mathieu.poirier, linux-remoteproc, linux-kernel, robh+dt,
linux-arm-kernel
> -----Original Message-----
> From: Michael Auchter <michael.auchter@ni.com>
> Sent: Tuesday, October 6, 2020 3:21 PM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; Stefano Stabellini
> <stefanos@xilinx.com>; Michal Simek <michals@xilinx.com>;
> devicetree@vger.kernel.org; mathieu.poirier@linaro.org; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org;
> robh+dt@kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> remoteproc driver
>
> On Tue, Oct 06, 2020 at 09:46:38PM +0000, Ben Levinsky wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michael Auchter <michael.auchter@ni.com>
> > > Sent: Tuesday, October 6, 2020 2:32 PM
> > > To: Ben Levinsky <BLEVINSK@xilinx.com>
> > > Cc: Ed T. Mooring <emooring@xilinx.com>; sunnyliangjy@gmail.com;
> > > punit1.agrawal@toshiba.co.jp; Stefano Stabellini <stefanos@xilinx.com>;
> > > Michal Simek <michals@xilinx.com>; devicetree@vger.kernel.org;
> > > mathieu.poirier@linaro.org; linux-remoteproc@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; robh+dt@kernel.org; linux-arm-
> > > kernel@lists.infradead.org
> > > Subject: Re: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> > > remoteproc driver
> > >
> > > On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote:
> > > >
> > > > Hi Michael,
> > > >
> > > > Thanks for the review
> > > >
> > >
> > > < ... snip ... >
> > >
> > > > > > + z_rproc = rproc->priv;
> > > > > > + z_rproc->dev.release = zynqmp_r5_release;
> > > > >
> > > > > This is the only field of z_rproc->dev that's actually initialized, and
> > > > > this device is not registered with the core at all, so zynqmp_r5_release
> > > > > will never be called.
> > > > >
> > > > > Since it doesn't look like there's a need to create this additional
> > > > > device, I'd suggest:
> > > > > - Dropping the struct device from struct zynqmp_r5_rproc
> > > > > - Performing the necessary cleanup in the driver remove
> > > > > callback instead of trying to tie it to device release
> > > >
> > > > For the most part I agree. I believe the device is still needed for
> > > > the mailbox client setup.
> > > >
> > > > As the call to mbox_request_channel_byname() requires its own device
> > > > that has the corresponding child node with the corresponding
> > > > mbox-related properties.
> > > >
> > > > With that in mind, is it still ok to keep the device node?
> > >
> > > Ah, I see. Thanks for the clarification!
> > >
> > > Instead of manually dealing with the device node creation for the
> > > individual processors, perhaps it makes more sense to use
> > > devm_of_platform_populate() to create them. This is also consistent with
> > > the way the TI K3 R5F remoteproc driver does things.
> > >
> > > Cheers,
> > > Michael
> >
> > I've been working on this today for a way around it and found one that I
> think works with your initial suggestion,
> > - in z_rproc, change dev from struct device to struct device*
> > ^ the above is shown the usage thereof below. It is there for the
> mailbox setup.
> > - in driver probe:
> > - add list_head to keep track of each core's z_rproc and for the driver
> remove clean up
> > - in each core's probe (zynqmp_r5_probe) dothe following:
> >
> >
> > rproc_ptr = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops,
> > NULL, sizeof(struct zynqmp_r5_rproc));
> > if (!rproc_ptr)
> > return -ENOMEM;
> > z_rproc = rproc_ptr->priv;
> > z_rproc->dt_node = node;
> > z_rproc->rproc = rproc_ptr;
> > z_rproc->dev = &rproc_ptr->dev;
> > z_rproc->dev->of_node = node;
> > where node is the specific R5 core's of_node/ Device tree node.
> >
> > the above preserves most of the mailbox setup code.
>
> I see how this works, but it feels a bit weird to me to be overriding
> the remoteproc dev's of_node ptr. Personally I find the
> devm_of_platform_populate() approach a bit less confusing.
>
> But, it's also not my call to make ;). Perhaps a remoteproc maintainer
> can chime in here.
>
Fair enough. The way I see this, there is still a need for a struct device* in the zynqmp R5 rproc structure.
If we look at the TI K3 R5 remoteproc patch, https://patchwork.kernel.org/patch/11763795/ ,
there is a call to devm_of_platform_populate in k3_r5_probe similar to your suggestion. I can look to emulate it similarly in the Xilinx remoteproc driver.
Even still there is a usage of the struct device * in the TI K3 R5 remoteproc structure for the same reason of setting up mailbox for each core as detailed below (can be found in the same link I posted):
** here is where the device* is stored at probe
- k3_r5_probe calls k3_r5_cluster_rproc_init
static int k3_r5_cluster_rproc_init(struct platform_device *pdev) {
struct k3_r5_rproc *kproc;
struct device *cdev;
......<snip>...
core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem);
list_for_each_entry(core, &cluster->cores, elem) {
cdev = core->dev;
......<snip>...
kproc->dev = cdev;
...
}
and then back in the mailbox initialization:
static int k3_r5_rproc_start(struct rproc *rproc) {
...
struct k3_r5_rproc *kproc = rproc->priv;
struct device *dev = kproc->dev;
...
client->dev = dev; <--- this is needed when requesting the mailbox
This needs to be device with the corresponding of_node that has the mbox-related properties in it
This is the example I based my usage of the struct device* field in the Soc Specific remoteproc structure off of.
Given that I can still proceed and update with the other suggestions?
Thanks
Ben
> >
> >
> > With this, I have already successfully done the following in a v19 patch
> > - move all the previous driver release code to remove
> > - able to probe, start/stop r5, driver remove repeatedly
> >
> > Also, this mimics the TI R5 driver code as each core's rproc has a list_head
> and they have a structure for the cluster which among other things maintains
> a linked list of the cores' specific rproc information.
> >
> > Thanks
> > Ben
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
2020-10-05 16:06 ` [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings Ben Levinsky
@ 2020-10-08 12:37 ` Linus Walleij
2020-10-08 14:21 ` Ben Levinsky
0 siblings, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2020-10-08 12:37 UTC (permalink / raw)
To: Ben Levinsky, Catalin Marinas
Cc: ed.mooring, sunnyliangjy, Punit Agrawal, stefanos, michals,
michael.auchter,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mathieu Poirier, linux-remoteproc, linux-kernel, Rob Herring,
Linux ARM
Hi Ben,
thanks for your patch! I noticed this today and pay some interest
because in the past I used with implementing the support for
TCM memory on ARM32.
On Mon, Oct 5, 2020 at 6:06 PM Ben Levinsky <ben.levinsky@xilinx.com> wrote:
> Add binding for ZynqMP R5 OpenAMP.
>
> Represent the RPU domain resources in one device node. Each RPU
> processor is a subnode of the top RPU domain node.
>
> Signed-off-by: Jason Wu <j.wu@xilinx.com>
> Signed-off-by: Wendy Liang <jliang@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
(...)
> +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.
... firmwares for the on-board Cortex R5 of the Zynqmp .. (etc)
> +
> + 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.
Please do not refer to Linux in bindings, they are also for other
operating systems.
Isn't that spelled out "Tightly Coupled Memory" (please expand the acronym).
I had a hard time to parse this description, do you mean:
"The Tightly Coupled Memory (an on-chip SRAM) used by the Cortex R5
is double-ported and visible in both the physical memory space of the
Cortex A5 and the memory space of the main ZynqMP processor
cluster. This is visible in the address space of the ZynqMP processor
at the address indicated here."
That would make sense, but please confirm/update.
> + memory-region:
> + description:
> + collection of memory carveouts used for elf-loading and inter-processor
> + communication. each carveout in this case should be in DDR, not
> + chip-specific memory. In Xilinx case, this is TCM, OCM, BRAM, etc.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
This is nice, you're reusing the infrastructure we already
have for these carveouts, good design!
> + meta-memory-regions:
> + description:
> + collection of memories that are not present in the top level memory
> + nodes' mapping. For example, R5s' TCM banks. These banks are needed
> + for R5 firmware meta data such as the R5 firmware's heap and stack.
> + To be more precise, this is on-chip reserved SRAM regions, e.g. TCM,
> + BRAM, OCM, etc.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
Is this in the memory space of the main CPU cluster?
It sure looks like that.
> + /*
> + * Below nodes are required if using TCM to load R5 firmware
> + * if not, then either do not provide nodes are label as disabled in
> + * status property
> + */
> + tcm0a: tcm_0a@ffe00000 {
> + reg = <0xffe00000 0x10000>;
> + pnode-id = <0xf>;
> + no-map;
> + status = "okay";
> + phandle = <0x40>;
> + };
> + tcm0b: tcm_1a@ffe20000 {
> + reg = <0xffe20000 0x10000>;
> + pnode-id = <0x10>;
> + no-map;
> + status = "okay";
> + phandle = <0x41>;
> + };
All right so this looks suspicious to me. Please explain
what we are seeing in those reg entries? Is this the address
seen by the main CPU cluster?
Does it mean that the main CPU see the memory of the
R5 as "some kind of TCM" and that TCM is physically
mapped at 0xffe00000 (ITCM) and 0xffe20000 (DTCM)?
If the first is ITCM and the second DTCM that is pretty
important to point out, since this reflects the harvard
architecture properties of these two memory areas.
The phandle = thing I do not understand at all, but
maybe there is generic documentation for it that
I've missed?
Last time I checked (which was on the ARM32) the physical
address of the ITCM and DTCM could be changed at
runtime with CP15 instructions. I might be wrong
about this, but if that (or something similar) is still the case
you can't just say hardcode these addresses here, the
CPU can move that physical address somewhere else.
See the code in
arch/arm/kernel/tcm.c
It appears the ARM64 Linux kernel does not have any
TCM handling today, but that could change.
So is this just regular ARM TCM memory (as seen by
the main ARM64 cluster)?
If this is the case, you should probably add back the
compatible string, add a separate device tree binding
for TCM memories along the lines of
compatible = "arm,itcm";
compatible = "arm,dtcm";
The reg address should then ideally be interpreted by
the ARM64 kernel and assigned to the I/DTCM.
I'm paging Catalin on this because I do not know if
ARM64 really has [I|D]TCM or if this is some invention
of Xilinx's.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
2020-10-08 12:37 ` Linus Walleij
@ 2020-10-08 14:21 ` Ben Levinsky
2020-10-08 16:45 ` Ben Levinsky
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-08 14:21 UTC (permalink / raw)
To: Linus Walleij, Catalin Marinas, Stefano Stabellini, Ed T. Mooring
Cc: Ed T. Mooring, sunnyliangjy, Punit Agrawal, Michal Simek,
michael.auchter,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mathieu Poirier, linux-remoteproc, linux-kernel, Rob Herring,
Linux ARM
Hi Linus,
Thanks for the review
Please see my comments inline
>
> Hi Ben,
>
> thanks for your patch! I noticed this today and pay some interest
> because in the past I used with implementing the support for
> TCM memory on ARM32.
>
> On Mon, Oct 5, 2020 at 6:06 PM Ben Levinsky <ben.levinsky@xilinx.com>
> wrote:
>
> > Add binding for ZynqMP R5 OpenAMP.
> >
> > Represent the RPU domain resources in one device node. Each RPU
> > processor is a subnode of the top RPU domain node.
> >
> > Signed-off-by: Jason Wu <j.wu@xilinx.com>
> > Signed-off-by: Wendy Liang <jliang@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> (...)
>
> > +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.
>
> ... firmwares for the on-board Cortex R5 of the Zynqmp .. (etc)
>
Will fix
> > +
> > + 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.
>
> Please do not refer to Linux in bindings, they are also for other
> operating systems.
>
> Isn't that spelled out "Tightly Coupled Memory" (please expand the acronym).
>
> I had a hard time to parse this description, do you mean:
>
> "The Tightly Coupled Memory (an on-chip SRAM) used by the Cortex R5
> is double-ported and visible in both the physical memory space of the
> Cortex A5 and the memory space of the main ZynqMP processor
> cluster. This is visible in the address space of the ZynqMP processor
> at the address indicated here."
>
> That would make sense, but please confirm/update.
>
Yes the TCM address space as noted in the TCM device tree nodes (e.g. 0xffe00000) is visible at that address on the A53 cluster. Will fix
> > + memory-region:
> > + description:
> > + collection of memory carveouts used for elf-loading and inter-processor
> > + communication. each carveout in this case should be in DDR, not
> > + chip-specific memory. In Xilinx case, this is TCM, OCM, BRAM, etc.
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
>
> This is nice, you're reusing the infrastructure we already
> have for these carveouts, good design!
>
> > + meta-memory-regions:
> > + description:
> > + collection of memories that are not present in the top level memory
> > + nodes' mapping. For example, R5s' TCM banks. These banks are needed
> > + for R5 firmware meta data such as the R5 firmware's heap and stack.
> > + To be more precise, this is on-chip reserved SRAM regions, e.g. TCM,
> > + BRAM, OCM, etc.
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
>
> Is this in the memory space of the main CPU cluster?
>
> It sure looks like that.
>
Yes this is in the memory space of the A53 cluster
Will fix to comment as such. Thank you
> > + /*
> > + * Below nodes are required if using TCM to load R5 firmware
> > + * if not, then either do not provide nodes are label as disabled in
> > + * status property
> > + */
> > + tcm0a: tcm_0a@ffe00000 {
> > + reg = <0xffe00000 0x10000>;
> > + pnode-id = <0xf>;
> > + no-map;
> > + status = "okay";
> > + phandle = <0x40>;
> > + };
> > + tcm0b: tcm_1a@ffe20000 {
> > + reg = <0xffe20000 0x10000>;
> > + pnode-id = <0x10>;
> > + no-map;
> > + status = "okay";
> > + phandle = <0x41>;
> > + };
>
> All right so this looks suspicious to me. Please explain
> what we are seeing in those reg entries? Is this the address
> seen by the main CPU cluster?
>
> Does it mean that the main CPU see the memory of the
> R5 as "some kind of TCM" and that TCM is physically
> mapped at 0xffe00000 (ITCM) and 0xffe20000 (DTCM)?
>
> If the first is ITCM and the second DTCM that is pretty
> important to point out, since this reflects the harvard
> architecture properties of these two memory areas.
>
> The phandle = thing I do not understand at all, but
> maybe there is generic documentation for it that
> I've missed?
>
> Last time I checked (which was on the ARM32) the physical
> address of the ITCM and DTCM could be changed at
> runtime with CP15 instructions. I might be wrong
> about this, but if that (or something similar) is still the case
> you can't just say hardcode these addresses here, the
> CPU can move that physical address somewhere else.
> See the code in
> arch/arm/kernel/tcm.c
>
> It appears the ARM64 Linux kernel does not have any
> TCM handling today, but that could change.
>
> So is this just regular ARM TCM memory (as seen by
> the main ARM64 cluster)?
>
> If this is the case, you should probably add back the
> compatible string, add a separate device tree binding
> for TCM memories along the lines of
> compatible = "arm,itcm";
> compatible = "arm,dtcm";
> The reg address should then ideally be interpreted by
> the ARM64 kernel and assigned to the I/DTCM.
>
> I'm paging Catalin on this because I do not know if
> ARM64 really has [I|D]TCM or if this is some invention
> of Xilinx's.
>
> Yours,
> Linus Walleij
As you said, this is just regular ARM TCM memory (as seen by the main ARM64 cluster). Yes I can add back the compatible string, though maybe just "tcm" or "xlnx,tcm" though there is also tcm compat string for the TI remoteproc driver later listed below
So I think I answered this above, but the APU (a53 cluster) sees the TCM banks (referred to on Zynq UltraScale+) at TCM banks 0A and 0B as 0xffe00000 and 0xffe20000 respectively and TCM banks 1A and 1B at 0xffe90000 and 0xffeb0000. Also it is similar to the TI k3 R5 Remoteproc driver binding for reference:
- https://patchwork.kernel.org/cover/11763783/
The phandle array here is to serve as a later for these nodes so that later on, in the Remoteproc R5 driver, these can be referenced to get some information via the pnode-id property.
The reason we have the compatible, reg, etc. is for System DT effort + @Stefano Stabellini
That being said, we can change this around to couple the TCM bank nodes into the R5 as we have In our present, internal implementation at
- https://github.com/Xilinx/linux-xlnx/blob/master/Documentation/devicetree/bindings/remoteproc/xilinx%2Czynqmp-r5-remoteproc.txt
- https://github.com/Xilinx/linux-xlnx/blob/master/drivers/remoteproc/zynqmp_r5_remoteproc.c
the TCM nodes are coupled in the R5 but after some previous review on this list, it was moved to have the TCM nodes decoupled from the R5 node
I am not sure what you mean on the Arm64 handling of TCM memory. Via the architecture of the SoC https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf I know that the A53 cluster can see the absolute addresses of the R5 cluster so the translation is *I think* done as you describe with the CP15 instructions you listed.
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
2020-10-08 14:21 ` Ben Levinsky
@ 2020-10-08 16:45 ` Ben Levinsky
2020-10-08 20:22 ` Stefano Stabellini
2020-10-08 20:54 ` Linus Walleij
2 siblings, 0 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-08 16:45 UTC (permalink / raw)
To: Linus Walleij, Catalin Marinas, Ed T. Mooring
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-remoteproc, linux-kernel, Linux ARM
Hi Sorry there were some typos and errors in my response so I'll correct them below:
> -----Original Message-----
> From: Ben Levinsky <BLEVINSK@xilinx.com>
> Sent: Thursday, October 8, 2020 7:21 AM
> To: Linus Walleij <linus.walleij@linaro.org>; Catalin Marinas
> <catalin.marinas@arm.com>; Stefano Stabellini <stefanos@xilinx.com>; Ed T.
> Mooring <emooring@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; sunnyliangjy@gmail.com; Punit
> Agrawal <punit1.agrawal@toshiba.co.jp>; Michal Simek
> <michals@xilinx.com>; michael.auchter@ni.com; open list:OPEN FIRMWARE
> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> Mathieu Poirier <mathieu.poirier@linaro.org>; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; Rob Herring
> <robh+dt@kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>
> Subject: RE: [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for
> ZynqMP R5 rproc bindings
>
> Hi Linus,
>
> Thanks for the review
>
> Please see my comments inline
>
> >
> > Hi Ben,
> >
> > thanks for your patch! I noticed this today and pay some interest
> > because in the past I used with implementing the support for
> > TCM memory on ARM32.
> >
> > On Mon, Oct 5, 2020 at 6:06 PM Ben Levinsky <ben.levinsky@xilinx.com>
> > wrote:
> >
> > > Add binding for ZynqMP R5 OpenAMP.
> > >
> > > Represent the RPU domain resources in one device node. Each RPU
> > > processor is a subnode of the top RPU domain node.
> > >
> > > Signed-off-by: Jason Wu <j.wu@xilinx.com>
> > > Signed-off-by: Wendy Liang <jliang@xilinx.com>
> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > > Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> > (...)
> >
> > > +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.
> >
> > ... firmwares for the on-board Cortex R5 of the Zynqmp .. (etc)
> >
> Will fix
> > > +
> > > + 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.
> >
> > Please do not refer to Linux in bindings, they are also for other
> > operating systems.
> >
> > Isn't that spelled out "Tightly Coupled Memory" (please expand the
> acronym).
> >
> > I had a hard time to parse this description, do you mean:
> >
> > "The Tightly Coupled Memory (an on-chip SRAM) used by the Cortex R5
> > is double-ported and visible in both the physical memory space of the
> > Cortex A5 and the memory space of the main ZynqMP processor
> > cluster. This is visible in the address space of the ZynqMP processor
> > at the address indicated here."
> >
> > That would make sense, but please confirm/update.
> >
>
> Yes the TCM address space as noted in the TCM device tree nodes (e.g.
> 0xffe00000) is visible at that address on the A53 cluster. Will fix
>
> > > + memory-region:
> > > + description:
> > > + collection of memory carveouts used for elf-loading and inter-
> processor
> > > + communication. each carveout in this case should be in DDR, not
> > > + chip-specific memory. In Xilinx case, this is TCM, OCM, BRAM, etc.
> > > + $ref: /schemas/types.yaml#/definitions/phandle-array
> >
> > This is nice, you're reusing the infrastructure we already
> > have for these carveouts, good design!
> >
> > > + meta-memory-regions:
> > > + description:
> > > + collection of memories that are not present in the top level memory
> > > + nodes' mapping. For example, R5s' TCM banks. These banks are
> needed
> > > + for R5 firmware meta data such as the R5 firmware's heap and stack.
> > > + To be more precise, this is on-chip reserved SRAM regions, e.g. TCM,
> > > + BRAM, OCM, etc.
> > > + $ref: /schemas/types.yaml#/definitions/phandle-array
> >
> > Is this in the memory space of the main CPU cluster?
> >
> > It sure looks like that.
> >
>
> Yes this is in the memory space of the A53 cluster
>
> Will fix to comment as such. Thank you
>
> > > + /*
> > > + * Below nodes are required if using TCM to load R5 firmware
> > > + * if not, then either do not provide nodes are label as disabled in
> > > + * status property
> > > + */
> > > + tcm0a: tcm_0a@ffe00000 {
> > > + reg = <0xffe00000 0x10000>;
> > > + pnode-id = <0xf>;
> > > + no-map;
> > > + status = "okay";
> > > + phandle = <0x40>;
> > > + };
> > > + tcm0b: tcm_1a@ffe20000 {
> > > + reg = <0xffe20000 0x10000>;
> > > + pnode-id = <0x10>;
> > > + no-map;
> > > + status = "okay";
> > > + phandle = <0x41>;
> > > + };
> >
> > All right so this looks suspicious to me. Please explain
> > what we are seeing in those reg entries? Is this the address
> > seen by the main CPU cluster?
> >
> > Does it mean that the main CPU see the memory of the
> > R5 as "some kind of TCM" and that TCM is physically
> > mapped at 0xffe00000 (ITCM) and 0xffe20000 (DTCM)?
> >
> > If the first is ITCM and the second DTCM that is pretty
> > important to point out, since this reflects the harvard
> > architecture properties of these two memory areas.
> >
> > The phandle = thing I do not understand at all, but
> > maybe there is generic documentation for it that
> > I've missed?
> >
> > Last time I checked (which was on the ARM32) the physical
> > address of the ITCM and DTCM could be changed at
> > runtime with CP15 instructions. I might be wrong
> > about this, but if that (or something similar) is still the case
> > you can't just say hardcode these addresses here, the
> > CPU can move that physical address somewhere else.
> > See the code in
> > arch/arm/kernel/tcm.c
> >
> > It appears the ARM64 Linux kernel does not have any
> > TCM handling today, but that could change.
> >
> > So is this just regular ARM TCM memory (as seen by
> > the main ARM64 cluster)?
> >
> > If this is the case, you should probably add back the
> > compatible string, add a separate device tree binding
> > for TCM memories along the lines of
> > compatible = "arm,itcm";
> > compatible = "arm,dtcm";
> > The reg address should then ideally be interpreted by
> > the ARM64 kernel and assigned to the I/DTCM.
> >
> > I'm paging Catalin on this because I do not know if
> > ARM64 really has [I|D]TCM or if this is some invention
> > of Xilinx's.
> >
> > Yours,
> > Linus Walleij
>
> As you said, this is just regular ARM TCM memory (as seen by the main
> ARM64 cluster). Yes I can add back the compatible string, though maybe just
> "tcm" or "xlnx,tcm" though there is also tcm compat string for the TI
> remoteproc driver later listed below
>
> So I think I answered this above, but the APU (a53 cluster) sees the TCM
> banks (referred to on Zynq UltraScale+) at TCM banks 0A and 0B as 0xffe00000
> and 0xffe20000 respectively and TCM banks 1A and 1B at 0xffe90000 and
> 0xffeb0000. Also it is similar to the TI k3 R5 Remoteproc driver binding for
> reference:
> - https://patchwork.kernel.org/cover/11763783/
>
> The phandle array here is to serve as a later for these nodes so that later on,
> in the Remoteproc R5 driver, these can be referenced to get some information
> via the pnode-id property.
>
> The reason we have the compatible, reg, etc. is for System DT effort +
> @Stefano Stabellini
>
> That being said, we can change this around to couple the TCM bank nodes
> into the R5 as we have In our present, internal implementation at
> - https://github.com/Xilinx/linux-
> xlnx/blob/master/Documentation/devicetree/bindings/remoteproc/xilinx%2C
> zynqmp-r5-remoteproc.txt
> - https://github.com/Xilinx/linux-
> xlnx/blob/master/drivers/remoteproc/zynqmp_r5_remoteproc.c
> the TCM nodes are coupled in the R5 but after some previous review on this
> list, it was moved to have the TCM nodes decoupled from the R5 node
>
> I am not sure what you mean on the Arm64 handling of TCM memory. Via the
> architecture of the SoC
> https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-
> ultrascale-trm.pdf I know that the A53 cluster can see the absolute addresses
> of the R5 cluster so the translation is *I think* done as you describe with the
> CP15 instructions you listed.
>
Sorry the above is unclear! editing this for clarity:
The phandle in the TCM node is referenced in the "meta-memory-regions" property. It is used as TCM, BRAM, OCM or other SoC-specific on-chip memories may be used for firmware loading other than the generic DDR.
The TCM nodes' originally had a compatible string, but was removed after a comment from the device tree maintainer Rob H. commented that TCM was not specific to Xilinx. With that in mind, I am not sure if the "arm,itcm" or "arm,dtcm" are appropriate.
The addresses for the TCM banks are addressable with these values provided in the reg via a physical bus not an instruction AFAIK so the CP15 instruction is not used.
Apologies again for the previously ambiguous response and hope this addresses your concerns,
Ben
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
2020-10-08 14:21 ` Ben Levinsky
2020-10-08 16:45 ` Ben Levinsky
@ 2020-10-08 20:22 ` Stefano Stabellini
2020-10-08 20:54 ` Linus Walleij
2 siblings, 0 replies; 20+ messages in thread
From: Stefano Stabellini @ 2020-10-08 20:22 UTC (permalink / raw)
To: Ben Levinsky, linus.walleij
Cc: Catalin Marinas, Stefano Stabellini, Ed T. Mooring, sunnyliangjy,
Punit Agrawal, Michal Simek, michael.auchter,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mathieu Poirier, linux-remoteproc, linux-kernel, Rob Herring,
Linux ARM
On Thu, 8 Oct 2020, Ben Levinsky wrote:
> > Does it mean that the main CPU see the memory of the
> > R5 as "some kind of TCM" and that TCM is physically
> > mapped at 0xffe00000 (ITCM) and 0xffe20000 (DTCM)?
> >
> > If the first is ITCM and the second DTCM that is pretty
> > important to point out, since this reflects the harvard
> > architecture properties of these two memory areas.
Hi Linus,
I don't think Xilinx TCMs are split in ITCM and DTCM in the way you
describe here: https://www.kernel.org/doc/Documentation/arm/tcm.txt.
Either TCM could be used for anything. See
https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
at page 82.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
2020-10-08 14:21 ` Ben Levinsky
2020-10-08 16:45 ` Ben Levinsky
2020-10-08 20:22 ` Stefano Stabellini
@ 2020-10-08 20:54 ` Linus Walleij
2 siblings, 0 replies; 20+ messages in thread
From: Linus Walleij @ 2020-10-08 20:54 UTC (permalink / raw)
To: Ben Levinsky
Cc: Catalin Marinas, Stefano Stabellini, Ed T. Mooring, sunnyliangjy,
Punit Agrawal, Michal Simek, michael.auchter,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mathieu Poirier, linux-remoteproc, linux-kernel, Rob Herring,
Linux ARM
On Thu, Oct 8, 2020 at 4:21 PM Ben Levinsky <BLEVINSK@xilinx.com> wrote:
> As you said, this is just regular ARM TCM memory (as seen by the main ARM64 cluster).
> Yes I can add back the compatible string, though maybe just "tcm" or "xlnx,tcm"
I mean that if it is an ARM standard feature it should be prefixed "arm,"
> That being said, we can change this around to couple the TCM bank nodes into the R5 as we have In our present, internal implementation at
> - https://github.com/Xilinx/linux-xlnx/blob/master/Documentation/devicetree/bindings/remoteproc/xilinx%2Czynqmp-r5-remoteproc.txt
> - https://github.com/Xilinx/linux-xlnx/blob/master/drivers/remoteproc/zynqmp_r5_remoteproc.c
> the TCM nodes are coupled in the R5 but after some previous review on this list, it was moved to have the TCM nodes decoupled from the R5 node
>
> I am not sure what you mean on the Arm64 handling of TCM memory. Via the architecture of the SoC https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf I know that the A53 cluster can see the absolute addresses of the R5 cluster so the translation is *I think* done as you describe with the CP15 instructions you listed.
It seems like the TCM memories are von Neumann type (either can
contain both code and
data) which removes one of my worries. According to this data sheet
they are also nothing
ARM-provided but a Xilinx invention, and just happen to be hardcoded
at the same address
as TCM memories in ARM32, what a coincidence.
So I take it this is the correct view and you can proceed like this,
sorry for the fuzz.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-06 22:20 ` Michael Auchter
2020-10-07 14:31 ` Ben Levinsky
@ 2020-10-15 18:31 ` Ben Levinsky
1 sibling, 0 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-15 18:31 UTC (permalink / raw)
To: linux-remoteproc
Cc: Ed T. Mooring, Stefano Stabellini, Michal Simek, devicetree,
michael.auchter, mathieu.poirier, linux-kernel, Rob Herring,
linux-arm-kernel
Hi All,
> -----Original Message-----
> From: Michael Auchter <michael.auchter@ni.com>
> Sent: Tuesday, October 6, 2020 3:21 PM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; Stefano Stabellini
> <stefanos@xilinx.com>; Michal Simek <michals@xilinx.com>;
> devicetree@vger.kernel.org; mathieu.poirier@linaro.org; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org;
> robh+dt@kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> remoteproc driver
>
> On Tue, Oct 06, 2020 at 09:46:38PM +0000, Ben Levinsky wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michael Auchter <michael.auchter@ni.com>
> > > Sent: Tuesday, October 6, 2020 2:32 PM
> > > To: Ben Levinsky <BLEVINSK@xilinx.com>
> > > Cc: Ed T. Mooring <emooring@xilinx.com>; sunnyliangjy@gmail.com;
> > > punit1.agrawal@toshiba.co.jp; Stefano Stabellini <stefanos@xilinx.com>;
> > > Michal Simek <michals@xilinx.com>; devicetree@vger.kernel.org;
> > > mathieu.poirier@linaro.org; linux-remoteproc@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; robh+dt@kernel.org; linux-arm-
> > > kernel@lists.infradead.org
> > > Subject: Re: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> > > remoteproc driver
> > >
> > > On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote:
> > > >
> > > > Hi Michael,
> > > >
> > > > Thanks for the review
> > > >
> > >
> > > < ... snip ... >
> > >
> > > > > > + z_rproc = rproc->priv;
> > > > > > + z_rproc->dev.release = zynqmp_r5_release;
> > > > >
> > > > > This is the only field of z_rproc->dev that's actually initialized, and
> > > > > this device is not registered with the core at all, so zynqmp_r5_release
> > > > > will never be called.
> > > > >
> > > > > Since it doesn't look like there's a need to create this additional
> > > > > device, I'd suggest:
> > > > > - Dropping the struct device from struct zynqmp_r5_rproc
> > > > > - Performing the necessary cleanup in the driver remove
> > > > > callback instead of trying to tie it to device release
> > > >
> > > > For the most part I agree. I believe the device is still needed for
> > > > the mailbox client setup.
> > > >
> > > > As the call to mbox_request_channel_byname() requires its own device
> > > > that has the corresponding child node with the corresponding
> > > > mbox-related properties.
> > > >
> > > > With that in mind, is it still ok to keep the device node?
> > >
> > > Ah, I see. Thanks for the clarification!
> > >
> > > Instead of manually dealing with the device node creation for the
> > > individual processors, perhaps it makes more sense to use
> > > devm_of_platform_populate() to create them. This is also consistent with
> > > the way the TI K3 R5F remoteproc driver does things.
> > >
> > > Cheers,
> > > Michael
> >
> > I've been working on this today for a way around it and found one that I
> think works with your initial suggestion,
> > - in z_rproc, change dev from struct device to struct device*
> > ^ the above is shown the usage thereof below. It is there for the
> mailbox setup.
> > - in driver probe:
> > - add list_head to keep track of each core's z_rproc and for the driver
> remove clean up
> > - in each core's probe (zynqmp_r5_probe) dothe following:
> >
> >
> > rproc_ptr = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops,
> > NULL, sizeof(struct zynqmp_r5_rproc));
> > if (!rproc_ptr)
> > return -ENOMEM;
> > z_rproc = rproc_ptr->priv;
> > z_rproc->dt_node = node;
> > z_rproc->rproc = rproc_ptr;
> > z_rproc->dev = &rproc_ptr->dev;
> > z_rproc->dev->of_node = node;
> > where node is the specific R5 core's of_node/ Device tree node.
> >
> > the above preserves most of the mailbox setup code.
>
> I see how this works, but it feels a bit weird to me to be overriding
> the remoteproc dev's of_node ptr. Personally I find the
> devm_of_platform_populate() approach a bit less confusing.
>
> But, it's also not my call to make ;). Perhaps a remoteproc maintainer
> can chime in here.
>
> >
Ping for comments here.
I looked at the TI R5 remoteproc driver and from what I can see, it seems the crux of the line:
z_rproc->dev->of_node = node;
is as follows:
the TI driver only has 1 R5-related remoteproc node. But in this it has information for both cores so
the rproc_alloc's device that is passed in is sufficient for subsequent mailbox calls. This is because the device
here also has a device_node that has the mbox information.
The Xilinx driver differs in that while there is a cluster device tree node that has the remoteproc-related
Information, it ALSO has child R5 cores that have their own TCM bank and mbox information.
As a result of this difference the use of devm_of_populate would not remove the use of the line of code in question because the mailbox setup calls later on still require the device field to have a corresponding device tree node that
Has the mailbox information.
If it is desired to see the use of devm_of_populate and more close alignment to the TI driver that has been merged then the Xilinx R5 driver bindings can instead have the TCM bank info, memory-regions, meta-memory-regions into R5 core-specific lists which resembles how the TI R5 driver has R5 core-specific properties. At this point just trying to suss out some direction in this patch series.
Your feedback and review is much appreciated,
Ben
> >
> > With this, I have already successfully done the following in a v19 patch
> > - move all the previous driver release code to remove
> > - able to probe, start/stop r5, driver remove repeatedly
> >
> > Also, this mimics the TI R5 driver code as each core's rproc has a list_head
> and they have a structure for the cluster which among other things maintains
> a linked list of the cores' specific rproc information.
> >
> > Thanks
> > Ben
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-05 16:06 ` [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Ben Levinsky
2020-10-05 19:34 ` Michael Auchter
@ 2020-10-19 20:43 ` Stefano Stabellini
2020-10-19 21:33 ` Ben Levinsky
1 sibling, 1 reply; 20+ messages in thread
From: Stefano Stabellini @ 2020-10-19 20:43 UTC (permalink / raw)
To: Ben Levinsky
Cc: ed.mooring, sunnyliangjy, punit1.agrawal, stefanos, michals,
michael.auchter, devicetree, mathieu.poirier, linux-remoteproc,
linux-kernel, robh+dt, linux-arm-kernel
On Mon, 5 Oct 2020, Ben Levinsky wrote:
> R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> remotproc driver, we can boot the R5 sub-system in different 2
> configurations -
> * split
> * lock-step
>
> The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
> Platform Management Unit that handles the R5 configuration, memory access
> and R5 lifecycle management. The interface to this manager is done in this
> driver via zynqmp_pm_* function calls.
Mostly minor comments left
> Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Ed Mooring <ed.mooring@xilinx.com>
> Signed-off-by: Jason Wu <j.wu@xilinx.com>
> Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> ---
> v2:
> - remove domain struct as per review from Mathieu
> v3:
> - add xilinx-related platform mgmt fn's instead of wrapping around
> function pointer in xilinx eemi ops struct
> v4:
> - add default values for enums
> - fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1 check
> are still raised as each is due to fixing the warning results in that
> particular line going over 80 characters.
> v5:
> - parse_fw change from use of rproc_of_resm_mem_entry_init to
> rproc_mem_entry_init and use of alloc/release
> - var's of type zynqmp_r5_pdata all have same local variable name
> - use dev_dbg instead of dev_info
> v6:
> - adding memory carveouts is handled much more similarly. All mem
> carveouts are
> now described in reserved memory as needed. That is, TCM nodes are not
> coupled to remoteproc anymore. This is reflected in the remoteproc R5
> driver
> and the device tree binding.
> - remove mailbox from device tree binding as it is not necessary for elf
> loading
> - use lockstep-mode property for configuring RPU
> v7:
> - remove unused headers
> - change u32 *lockstep_mode -> u32 lockstep_mode;
> - change device-tree binding "lockstep-mode" to xlnx,cluster-mode
> - remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at
> remoteproc-probe time
> - remove is_r5_mode_set from zynqmp rpu remote processor private data
> - do not error out if no mailbox is provided
> - remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as
> pdata is
> handled in zynqmp_r5_remoteproc_remove
> v8:
> - remove old acks, reviewed-by's in commit message
> v9:
> - as mboxes are now optional, if pdata->tx_mc_skbs not initialized then
> do not call skb_queue_empty
> - update usage for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config and
> zynqmp_pm_get_rpu_mode
> - update 5/5 patch commit message to document supported configurations
> and how they are booted by the driver.
> - remove copyrights other than SPDX from zynqmp_r5_remoteproc.c
> - compilation warnings no longer raised
> - remove unused includes from zynqmp_r5_remoteproc.c
> - remove unused var autoboot from zynqmp_r5_remoteproc.c
> - reorder zynqmp_r5_pdata fpr small mem savings due to alignment
> - use of zynqmp_pm_set_tcm_config now does not have
> output arg
> - in tcm handling, unconditionally use &= 0x000fffff mask since all nodes
> in this fn are for tcm
> - update comments for translating dma field in tcm handling to device
> address
> - update calls to rproc_mem_entry_init in parse_mem_regions so that there
> are only 2 cases for types of carveouts instead of 3
> - in parse_mem_regions, check if device tree node is null before using it
> - add example device tree nodes used in parse_mem_regions and tcm parsing
> - add comment for vring id node length
> - add check for string length so that vring id is at least min length
> - move tcm nodes from reserved mem to instead own device tree nodes
> and only use them if enabled in device tree
> - add comment for explaining handling of rproc_elf_load_rsc_table
> - remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick
> - remove unused field mems in struct zynqmp_r5_pdata
> - remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling
> is done by zyqmp_r5_pm_request_tcm
> - remove obsolete setting of dma_ops and parent device dma_mask
> - remove obsolete use of of_dma_configure
> - add comment for call to r5_set_mode fn
> - make mbox usage optional and gracefully inform user via dev_dbg if not
> present
> - change var lockstep_mode from u32* to u32
> v11:
> - use enums instead of u32 where possible in zynqmp_r5_remoteproc
> - update usage of zynqmp_pm_set/get_rpu_mode and zynqmp_pm_set_tcm_config
> - update prints to not use carriage return, just newline
> - look up tcm banks via property in r5 node instead of string name
> - print device tree nodes with %pOF instead of %s with node name field
> - update tcm release to unmap VA
> - handle r5-1 use case
> v12:
> - update signed off by so that latest developer name is last
> - do not cast enums to u32s for zynqmp_pm* functions
> v14:
> - change zynqmp_r5_remoteproc::rpus and rpu_mode to static
> - fix typo
> - zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
> property specified in device tree
> - use u32 instead of u32* to store in remoteproc memory entry private data
> for pnode_id information
> - always call r5_set_mode on probe
> - remove alloc of zynqmp_r5_pdata in
> zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static
> allocation already
> - error at probe time if lockstep-mode property not present in device tree
> - update commit message as per review
> - remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is present
> - remove unused macros
> - update comment ordering of zynqmp_r5_pdata to match struct definition
> - zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
> - remove obsolete TODOs
> - only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
> - remove uneven dev_dbg/dev_err fn calls
> v15:
> - if lockstep mode prop is present, then RPU cluster is in lockstep mode.
> if not present, cluster is in split mode.
> - if 2 RPUs provided but one is lockstep then error out as this is invalid
> configuration
> v16:
> - replace of_get_property(dev->of_node, "lockstep-mode" with
> of_property_read_bool
> - propagate rpu mode specified in device tree through functions instead
> of holding a global, static var
> - check child remoteproc nodes via of_get_available_child_count before
> looping through children
> - replace check of "pdata->pnode_id == 0" instead by checking rpu's
> zynqmp_r5_pdata* if NULL
> - remove old, obsolete checks for dma_pools in zynqmp_r5_remoteproc_remove
> - change rpus from zynqmp_r5_pdata[] to zynqmp_r5_pdata*[] so that
> check for pdata->pnode_id == 0 is not needed
> v17:
> - fix style as per kernel test bot
> v18:
> - to more closely mimic other remoteproc drivers, change zynqmp r5 rproc
> data from zynqmp_r5_pdata to zynqmp_r5_rproc and pdata local var to
> zproc
> - remove global vars rpus and rpu_mode
> - instantiate device for zynqmp r5 rproc from device set by rproc_alloc
> - fix typos
> - update to call zynqmp_r5_release from the rproc_alloc-related device and
> remove the instantiated device from zynqmp_r5_probe
> - remove unneeded call to platform_set_drvdata
> - remove driver remove function, as the clean up is handled in release
> - remove while (!skb_queue_empty loop and mbox_free_channel calls in
> zynqmp_r5_release, and mbox_free_channel
> - remove device_unregister call in zynqmp_r5_release
> - remove kzalloc for pdata (what is now called z_rproc)
> - update conditional in loop to calls of zynqmp_r5_probe
>
> ---
> drivers/remoteproc/Kconfig | 8 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/zynqmp_r5_remoteproc.c | 707 ++++++++++++++++++++++
> 3 files changed, 716 insertions(+)
> create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c6659dfea7c7..68e567c5375c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -275,6 +275,14 @@ config TI_K3_DSP_REMOTEPROC
> It's safe to say N here if you're not interested in utilizing
> the DSP slave processors.
>
> +config ZYNQMP_R5_REMOTEPROC
> + tristate "ZynqMP_R5 remoteproc support"
> + depends on PM && ARCH_ZYNQMP
> + select RPMSG_VIRTIO
> + select ZYNQMP_IPI_MBOX
> + help
> + Say y or m here to support ZynqMP R5 remote processors via the remote
> + processor framework.
> endif # REMOTEPROC
>
> endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 3dfa28e6c701..ef1abff654c2 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o
> diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> new file mode 100644
> index 000000000000..37bd76252ff2
> --- /dev/null
> +++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> @@ -0,0 +1,707 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Zynq R5 Remote Processor driver
> + *
> + * Based on origin OMAP and Zynq Remote Processor driver
> + *
> + */
> +
> +#include <linux/firmware/xlnx-zynqmp.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_platform.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/skbuff.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 BANK_LIST_PROP "meta-memory-regions"
> +
> +/* IPI buffer MAX length */
> +#define IPI_BUF_LEN_MAX 32U
> +/* RX mailbox client buffer max length */
> +#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
> + sizeof(struct zynqmp_ipi_message))
> +
> +/**
> + * struct zynqmp_r5_mem - zynqmp rpu memory data
> + * @pnode_id: TCM power domain ids
> + * @res: memory resource
> + * @node: list node
> + */
> +struct zynqmp_r5_mem {
> + u32 pnode_id[MAX_MEM_PNODES];
> + struct resource res;
> + struct list_head node;
> +};
> +
> +/**
> + * struct zynqmp_r5_rproc - zynqmp rpu remote processor state
> + * @rx_mc_buf: rx mailbox client buffer to save the rx message
> + * @tx_mc: tx mailbox client
> + * @rx_mc: rx mailbox client * @dev: device of RPU instance
> + * @mbox_work: mbox_work for the RPU remoteproc
> + * @tx_mc_skbs: socket buffers for tx mailbox client
> + * @dev: device of RPU instance
> + * @rproc: rproc handle
> + * @tx_chan: tx mailbox channel
> + * @rx_chan: rx mailbox channel
> + * @pnode_id: RPU CPU power domain id
> + */
> +struct zynqmp_r5_rproc {
> + unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
> + struct mbox_client tx_mc;
> + struct mbox_client rx_mc;
> + struct work_struct mbox_work;
> + struct sk_buff_head tx_mc_skbs;
> + struct device dev;
> + struct rproc *rproc;
> + struct mbox_chan *tx_chan;
> + struct mbox_chan *rx_chan;
> + u32 pnode_id;
> +};
> +
> +/*
> + * r5_set_mode - set RPU operation mode
> + * @z_rproc: Remote processor private data
> + *
> + * set RPU operation mode
> + *
> + * Return: 0 for success, negative value for failure
> + */
> +static int r5_set_mode(struct zynqmp_r5_rproc *z_rproc,
> + enum rpu_oper_mode rpu_mode)
> +{
> + enum rpu_tcm_comb tcm_mode;
> + enum rpu_oper_mode cur_rpu_mode;
> + int ret;
> +
> + ret = zynqmp_pm_get_rpu_mode(z_rproc->pnode_id, &cur_rpu_mode);
> + if (ret < 0)
> + return ret;
> +
> + if (rpu_mode != cur_rpu_mode) {
> + ret = zynqmp_pm_set_rpu_mode(z_rproc->pnode_id,
> + rpu_mode);
> + if (ret < 0)
> + return ret;
> + }
> +
> + tcm_mode = (rpu_mode == PM_RPU_MODE_LOCKSTEP) ?
> + PM_RPU_TCM_COMB : PM_RPU_TCM_SPLIT;
> + return zynqmp_pm_set_tcm_config(z_rproc->pnode_id, tcm_mode);
> +}
> +
> +/*
> + * ZynqMP R5 remoteproc memory release function
> + */
> +static int tcm_mem_release(struct rproc *rproc, struct rproc_mem_entry *mem)
> +{
> + u32 pnode_id = (u64)mem->priv;
> +
> + if (pnode_id <= 0)
pnode_id is a u32, so checks for it to be negative don't make a lot of
sense
> + return -EINVAL;
> +
> + iounmap(mem->va);
> + return zynqmp_pm_release_node(pnode_id);
> +}
> +
> +/*
> + * ZynqMP R5 remoteproc operations
> + */
> +static int zynqmp_r5_rproc_start(struct rproc *rproc)
> +{
> + struct device *dev = rproc->dev.parent;
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> + enum rpu_boot_mem bootmem;
> +
> + bootmem = (rproc->bootaddr & 0xF0000000) == 0xF0000000 ?
> + PM_RPU_BOOTMEM_HIVEC : PM_RPU_BOOTMEM_LOVEC;
> +
> + dev_dbg(dev, "RPU boot from %s.",
> + bootmem == PM_RPU_BOOTMEM_HIVEC ? "OCM" : "TCM");
> +
> + return zynqmp_pm_request_wake(z_rproc->pnode_id, 1,
> + bootmem, ZYNQMP_PM_REQUEST_ACK_NO);
> +}
> +
> +static int zynqmp_r5_rproc_stop(struct rproc *rproc)
> +{
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> + struct sk_buff *skb;
> +
> + if (z_rproc->tx_chan)
> + mbox_free_channel(z_rproc->tx_chan);
> + if (z_rproc->rx_chan)
> + mbox_free_channel(z_rproc->rx_chan);
> +
> + return zynqmp_pm_force_pwrdwn(z_rproc->pnode_id,
> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> +}
> +
> +static int zynqmp_r5_rproc_mem_alloc(struct rproc *rproc,
> + struct rproc_mem_entry *mem)
> +{
> + void *va;
> +
> + va = ioremap_wc(mem->dma, mem->len);
> + if (IS_ERR_OR_NULL(va))
> + 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)
> +{
> + iounmap(mem->va);
> + return 0;
> +}
> +
> +static int parse_mem_regions(struct rproc *rproc)
> +{
> + int num_mems, i;
> + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> + struct device *dev = &z_rproc->dev;
> + struct device_node *np = dev->of_node;
> + 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);
> + if (!node)
> + return -EINVAL;
> +
> + rmem = of_reserved_mem_lookup(node);
> + if (!rmem)
> + return -EINVAL;
> +
> + if (strstr(node->name, "vdev0vring")) {
> + int vring_id;
> + char name[16];
> +
> + /*
> + * expecting form of "rpuXvdev0vringX as documented
> + * in xilinx remoteproc device tree binding
> + */
> + if (strlen(node->name) < 14) {
> + dev_err(dev, "%pOF is less than 14 chars",
> + node);
> + return -EINVAL;
> + }
> +
> + /*
> + * can be 1 of multiple vring IDs per IPC channel
> + * e.g. 'vdev0vring0' and 'vdev0vring1'
> + */
> + vring_id = node->name[14] - '0';
If you are going to use a direct access to node->name[14], then the
strlen check above should cover it, which means we should check for at
least strlen(node->name) < 15.
> + 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);
> + } else {
> + /* Register DMA region */
> + int (*alloc)(struct rproc *r,
> + struct rproc_mem_entry *rme);
> + int (*release)(struct rproc *r,
> + struct rproc_mem_entry *rme);
> + char name[20];
> +
> + if (strstr(node->name, "vdev0buffer")) {
> + alloc = NULL;
> + release = NULL;
> + strcpy(name, "vdev0buffer");
> + } else {
> + alloc = zynqmp_r5_rproc_mem_alloc;
> + release = zynqmp_r5_rproc_mem_release;
> + strcpy(name, node->name);
> + }
> +
> + mem = rproc_mem_entry_init(dev, NULL,
> + (dma_addr_t)rmem->base,
> + rmem->size, rmem->base,
> + alloc, release, name);
> + }
> + if (!mem)
> + return -ENOMEM;
> +
> + rproc_add_carveout(rproc, mem);
> + }
> +
> + return 0;
> +}
> +
> +/* call Xilinx Platform manager to request access to TCM bank */
> +static int zynqmp_r5_pm_request_tcm(struct device_node *tcm_node,
> + struct device *dev, u32 *pnode_id)
> +{
> + int ret;
> +
> + ret = of_property_read_u32(tcm_node, "pnode-id", pnode_id);
> + if (ret)
> + return ret;
> +
> + return zynqmp_pm_request_node(*pnode_id, ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> +}
> +
> +/* Given tcm bank entry,
I think checkpatch.pl would complain for this comment format
> + * this callback will set device address for R5 running on TCM
> + * and also setup virtual address for tcm bank remoteproc carveout
> + */
> +static int tcm_mem_alloc(struct rproc *rproc,
> + struct rproc_mem_entry *mem)
> +{
> + void *va;
> + struct device *dev = rproc->dev.parent;
> +
> + va = ioremap_wc(mem->dma, mem->len);
> + if (IS_ERR_OR_NULL(va))
> + return -ENOMEM;
> +
> + /* Update memory entry va */
> + mem->va = va;
> +
> + va = devm_ioremap_wc(dev, mem->da, mem->len);
> + if (!va)
> + return -ENOMEM;
> + /* As R5 is 32 bit, wipe out extra high bits */
> + mem->da &= 0x000fffff;
> + /*
> + * handle tcm banks 1 a and b (0xffe90000 and oxffeb0000)
> + * As both of these the only common bit found not in tcm bank0 a or b
> + * is at 0x80000 use this mask to suss it out
> + */
> + if (mem->da & 0x80000)
> + /*
> + * need to do more to further translate
> + * tcm banks 1a and 1b at 0xffe90000 and oxffeb0000
^typo
> + * respectively to 0x0 and 0x20000
> + */
> + mem->da -= 0x90000;
I understand now why we do "mem->da -= 0x90000" and the in-code comment
explains it. However, why the "if (mem->da & 0x80000)" check?
If we want to make sure to do this "translation" only for 0xffe90000 and
0xffeb0000, wouldn't it be better to call them out explicitly, like:
if (mem->da == 0x90000 || mem->da == 0xB0000)
Also if this if check fails, should we print an error? Or is it a
possible handled condition?
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
2020-10-19 20:43 ` Stefano Stabellini
@ 2020-10-19 21:33 ` Ben Levinsky
0 siblings, 0 replies; 20+ messages in thread
From: Ben Levinsky @ 2020-10-19 21:33 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Ed T. Mooring, sunnyliangjy, punit1.agrawal, Stefano Stabellini,
Michal Simek, michael.auchter, devicetree, mathieu.poirier,
linux-remoteproc, linux-kernel, robh+dt, linux-arm-kernel
Hi Stefano,
Thanks for the review.
> -----Original Message-----
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Sent: Monday, October 19, 2020 1:44 PM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; sunnyliangjy@gmail.com;
> punit1.agrawal@toshiba.co.jp; Stefano Stabellini <stefanos@xilinx.com>;
> Michal Simek <michals@xilinx.com>; michael.auchter@ni.com;
> devicetree@vger.kernel.org; mathieu.poirier@linaro.org; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org;
> robh+dt@kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> driver
>
> On Mon, 5 Oct 2020, Ben Levinsky wrote:
> > R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> > remotproc driver, we can boot the R5 sub-system in different 2
> > configurations -
> > * split
> > * lock-step
> >
> > The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx
> > Platform Management Unit that handles the R5 configuration, memory
> access
> > and R5 lifecycle management. The interface to this manager is done in this
> > driver via zynqmp_pm_* function calls.
>
> Mostly minor comments left
>
>
> > Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Signed-off-by: Ed Mooring <ed.mooring@xilinx.com>
> > Signed-off-by: Jason Wu <j.wu@xilinx.com>
> > Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> > ---
> > v2:
> > - remove domain struct as per review from Mathieu
> > v3:
> > - add xilinx-related platform mgmt fn's instead of wrapping around
> > function pointer in xilinx eemi ops struct
> > v4:
> > - add default values for enums
> > - fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1
> check
> > are still raised as each is due to fixing the warning results in that
> > particular line going over 80 characters.
> > v5:
> > - parse_fw change from use of rproc_of_resm_mem_entry_init to
> > rproc_mem_entry_init and use of alloc/release
> > - var's of type zynqmp_r5_pdata all have same local variable name
> > - use dev_dbg instead of dev_info
> > v6:
> > - adding memory carveouts is handled much more similarly. All mem
> > carveouts are
> > now described in reserved memory as needed. That is, TCM nodes are not
> > coupled to remoteproc anymore. This is reflected in the remoteproc R5
> > driver
> > and the device tree binding.
> > - remove mailbox from device tree binding as it is not necessary for elf
> > loading
> > - use lockstep-mode property for configuring RPU
> > v7:
> > - remove unused headers
> > - change u32 *lockstep_mode -> u32 lockstep_mode;
> > - change device-tree binding "lockstep-mode" to xlnx,cluster-mode
> > - remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at
> > remoteproc-probe time
> > - remove is_r5_mode_set from zynqmp rpu remote processor private data
> > - do not error out if no mailbox is provided
> > - remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as
> > pdata is
> > handled in zynqmp_r5_remoteproc_remove
> > v8:
> > - remove old acks, reviewed-by's in commit message
> > v9:
> > - as mboxes are now optional, if pdata->tx_mc_skbs not initialized then
> > do not call skb_queue_empty
> > - update usage for zynqmp_pm_set_rpu_mode,
> zynqmp_pm_set_tcm_config and
> > zynqmp_pm_get_rpu_mode
> > - update 5/5 patch commit message to document supported configurations
> > and how they are booted by the driver.
> > - remove copyrights other than SPDX from zynqmp_r5_remoteproc.c
> > - compilation warnings no longer raised
> > - remove unused includes from zynqmp_r5_remoteproc.c
> > - remove unused var autoboot from zynqmp_r5_remoteproc.c
> > - reorder zynqmp_r5_pdata fpr small mem savings due to alignment
> > - use of zynqmp_pm_set_tcm_config now does not have
> > output arg
> > - in tcm handling, unconditionally use &= 0x000fffff mask since all nodes
> > in this fn are for tcm
> > - update comments for translating dma field in tcm handling to device
> > address
> > - update calls to rproc_mem_entry_init in parse_mem_regions so that there
> > are only 2 cases for types of carveouts instead of 3
> > - in parse_mem_regions, check if device tree node is null before using it
> > - add example device tree nodes used in parse_mem_regions and tcm
> parsing
> > - add comment for vring id node length
> > - add check for string length so that vring id is at least min length
> > - move tcm nodes from reserved mem to instead own device tree nodes
> > and only use them if enabled in device tree
> > - add comment for explaining handling of rproc_elf_load_rsc_table
> > - remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick
> > - remove unused field mems in struct zynqmp_r5_pdata
> > - remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling
> > is done by zyqmp_r5_pm_request_tcm
> > - remove obsolete setting of dma_ops and parent device dma_mask
> > - remove obsolete use of of_dma_configure
> > - add comment for call to r5_set_mode fn
> > - make mbox usage optional and gracefully inform user via dev_dbg if not
> > present
> > - change var lockstep_mode from u32* to u32
> > v11:
> > - use enums instead of u32 where possible in zynqmp_r5_remoteproc
> > - update usage of zynqmp_pm_set/get_rpu_mode and
> zynqmp_pm_set_tcm_config
> > - update prints to not use carriage return, just newline
> > - look up tcm banks via property in r5 node instead of string name
> > - print device tree nodes with %pOF instead of %s with node name field
> > - update tcm release to unmap VA
> > - handle r5-1 use case
> > v12:
> > - update signed off by so that latest developer name is last
> > - do not cast enums to u32s for zynqmp_pm* functions
> > v14:
> > - change zynqmp_r5_remoteproc::rpus and rpu_mode to static
> > - fix typo
> > - zynqmp_r5_remoteproc::r5_set_mode set rpu mode from
> > property specified in device tree
> > - use u32 instead of u32* to store in remoteproc memory entry private data
> > for pnode_id information
> > - always call r5_set_mode on probe
> > - remove alloc of zynqmp_r5_pdata in
> > zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static
> > allocation already
> > - error at probe time if lockstep-mode property not present in device tree
> > - update commit message as per review
> > - remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is
> present
> > - remove unused macros
> > - update comment ordering of zynqmp_r5_pdata to match struct definition
> > - zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid
> > - remove obsolete TODOs
> > - only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid
> > - remove uneven dev_dbg/dev_err fn calls
> > v15:
> > - if lockstep mode prop is present, then RPU cluster is in lockstep mode.
> > if not present, cluster is in split mode.
> > - if 2 RPUs provided but one is lockstep then error out as this is invalid
> > configuration
> > v16:
> > - replace of_get_property(dev->of_node, "lockstep-mode" with
> > of_property_read_bool
> > - propagate rpu mode specified in device tree through functions instead
> > of holding a global, static var
> > - check child remoteproc nodes via of_get_available_child_count before
> > looping through children
> > - replace check of "pdata->pnode_id == 0" instead by checking rpu's
> > zynqmp_r5_pdata* if NULL
> > - remove old, obsolete checks for dma_pools in
> zynqmp_r5_remoteproc_remove
> > - change rpus from zynqmp_r5_pdata[] to zynqmp_r5_pdata*[] so that
> > check for pdata->pnode_id == 0 is not needed
> > v17:
> > - fix style as per kernel test bot
> > v18:
> > - to more closely mimic other remoteproc drivers, change zynqmp r5 rproc
> > data from zynqmp_r5_pdata to zynqmp_r5_rproc and pdata local var to
> > zproc
> > - remove global vars rpus and rpu_mode
> > - instantiate device for zynqmp r5 rproc from device set by rproc_alloc
> > - fix typos
> > - update to call zynqmp_r5_release from the rproc_alloc-related device and
> > remove the instantiated device from zynqmp_r5_probe
> > - remove unneeded call to platform_set_drvdata
> > - remove driver remove function, as the clean up is handled in release
> > - remove while (!skb_queue_empty loop and mbox_free_channel calls in
> > zynqmp_r5_release, and mbox_free_channel
> > - remove device_unregister call in zynqmp_r5_release
> > - remove kzalloc for pdata (what is now called z_rproc)
> > - update conditional in loop to calls of zynqmp_r5_probe
> >
> > ---
> > drivers/remoteproc/Kconfig | 8 +
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/zynqmp_r5_remoteproc.c | 707
> ++++++++++++++++++++++
> > 3 files changed, 716 insertions(+)
> > create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c
> >
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index c6659dfea7c7..68e567c5375c 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -275,6 +275,14 @@ config TI_K3_DSP_REMOTEPROC
> > It's safe to say N here if you're not interested in utilizing
> > the DSP slave processors.
> >
> > +config ZYNQMP_R5_REMOTEPROC
> > + tristate "ZynqMP_R5 remoteproc support"
> > + depends on PM && ARCH_ZYNQMP
> > + select RPMSG_VIRTIO
> > + select ZYNQMP_IPI_MBOX
> > + help
> > + Say y or m here to support ZynqMP R5 remote processors via the
> remote
> > + processor framework.
> > endif # REMOTEPROC
> >
> > endmenu
> > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > index 3dfa28e6c701..ef1abff654c2 100644
> > --- a/drivers/remoteproc/Makefile
> > +++ b/drivers/remoteproc/Makefile
> > @@ -33,3 +33,4 @@ obj-$(CONFIG_ST_REMOTEPROC) +=
> st_remoteproc.o
> > obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> > obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> > obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> > +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o
> > diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c
> b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> > new file mode 100644
> > index 000000000000..37bd76252ff2
> > --- /dev/null
> > +++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c
> > @@ -0,0 +1,707 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Zynq R5 Remote Processor driver
> > + *
> > + * Based on origin OMAP and Zynq Remote Processor driver
> > + *
> > + */
> > +
> > +#include <linux/firmware/xlnx-zynqmp.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_platform.h>
> > +#include <linux/of_reserved_mem.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/skbuff.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 BANK_LIST_PROP "meta-memory-regions"
> > +
> > +/* IPI buffer MAX length */
> > +#define IPI_BUF_LEN_MAX 32U
> > +/* RX mailbox client buffer max length */
> > +#define RX_MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
> > + sizeof(struct zynqmp_ipi_message))
> > +
> > +/**
> > + * struct zynqmp_r5_mem - zynqmp rpu memory data
> > + * @pnode_id: TCM power domain ids
> > + * @res: memory resource
> > + * @node: list node
> > + */
> > +struct zynqmp_r5_mem {
> > + u32 pnode_id[MAX_MEM_PNODES];
> > + struct resource res;
> > + struct list_head node;
> > +};
> > +
> > +/**
> > + * struct zynqmp_r5_rproc - zynqmp rpu remote processor state
> > + * @rx_mc_buf: rx mailbox client buffer to save the rx message
> > + * @tx_mc: tx mailbox client
> > + * @rx_mc: rx mailbox client * @dev: device of RPU instance
> > + * @mbox_work: mbox_work for the RPU remoteproc
> > + * @tx_mc_skbs: socket buffers for tx mailbox client
> > + * @dev: device of RPU instance
> > + * @rproc: rproc handle
> > + * @tx_chan: tx mailbox channel
> > + * @rx_chan: rx mailbox channel
> > + * @pnode_id: RPU CPU power domain id
> > + */
> > +struct zynqmp_r5_rproc {
> > + unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
> > + struct mbox_client tx_mc;
> > + struct mbox_client rx_mc;
> > + struct work_struct mbox_work;
> > + struct sk_buff_head tx_mc_skbs;
> > + struct device dev;
> > + struct rproc *rproc;
> > + struct mbox_chan *tx_chan;
> > + struct mbox_chan *rx_chan;
> > + u32 pnode_id;
> > +};
> > +
> > +/*
> > + * r5_set_mode - set RPU operation mode
> > + * @z_rproc: Remote processor private data
> > + *
> > + * set RPU operation mode
> > + *
> > + * Return: 0 for success, negative value for failure
> > + */
> > +static int r5_set_mode(struct zynqmp_r5_rproc *z_rproc,
> > + enum rpu_oper_mode rpu_mode)
> > +{
> > + enum rpu_tcm_comb tcm_mode;
> > + enum rpu_oper_mode cur_rpu_mode;
> > + int ret;
> > +
> > + ret = zynqmp_pm_get_rpu_mode(z_rproc->pnode_id,
> &cur_rpu_mode);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (rpu_mode != cur_rpu_mode) {
> > + ret = zynqmp_pm_set_rpu_mode(z_rproc->pnode_id,
> > + rpu_mode);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + tcm_mode = (rpu_mode == PM_RPU_MODE_LOCKSTEP) ?
> > + PM_RPU_TCM_COMB : PM_RPU_TCM_SPLIT;
> > + return zynqmp_pm_set_tcm_config(z_rproc->pnode_id, tcm_mode);
> > +}
> > +
> > +/*
> > + * ZynqMP R5 remoteproc memory release function
> > + */
> > +static int tcm_mem_release(struct rproc *rproc, struct rproc_mem_entry
> *mem)
> > +{
> > + u32 pnode_id = (u64)mem->priv;
> > +
> > + if (pnode_id <= 0)
>
> pnode_id is a u32, so checks for it to be negative don't make a lot of
> sense
>
>
Will fix in next rev
> > + return -EINVAL;
> > +
> > + iounmap(mem->va);
> > + return zynqmp_pm_release_node(pnode_id);
> > +}
> > +
> > +/*
> > + * ZynqMP R5 remoteproc operations
> > + */
> > +static int zynqmp_r5_rproc_start(struct rproc *rproc)
> > +{
> > + struct device *dev = rproc->dev.parent;
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > + enum rpu_boot_mem bootmem;
> > +
> > + bootmem = (rproc->bootaddr & 0xF0000000) == 0xF0000000 ?
> > + PM_RPU_BOOTMEM_HIVEC : PM_RPU_BOOTMEM_LOVEC;
> > +
> > + dev_dbg(dev, "RPU boot from %s.",
> > + bootmem == PM_RPU_BOOTMEM_HIVEC ? "OCM" : "TCM");
> > +
> > + return zynqmp_pm_request_wake(z_rproc->pnode_id, 1,
> > + bootmem,
> ZYNQMP_PM_REQUEST_ACK_NO);
> > +}
> > +
> > +static int zynqmp_r5_rproc_stop(struct rproc *rproc)
> > +{
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > + struct sk_buff *skb;
> > +
> > + if (z_rproc->tx_chan)
> > + mbox_free_channel(z_rproc->tx_chan);
> > + if (z_rproc->rx_chan)
> > + mbox_free_channel(z_rproc->rx_chan);
> > +
> > + return zynqmp_pm_force_pwrdwn(z_rproc->pnode_id,
> > + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> > +}
> > +
> > +static int zynqmp_r5_rproc_mem_alloc(struct rproc *rproc,
> > + struct rproc_mem_entry *mem)
> > +{
> > + void *va;
> > +
> > + va = ioremap_wc(mem->dma, mem->len);
> > + if (IS_ERR_OR_NULL(va))
> > + 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)
> > +{
> > + iounmap(mem->va);
> > + return 0;
> > +}
> > +
> > +static int parse_mem_regions(struct rproc *rproc)
> > +{
> > + int num_mems, i;
> > + struct zynqmp_r5_rproc *z_rproc = rproc->priv;
> > + struct device *dev = &z_rproc->dev;
> > + struct device_node *np = dev->of_node;
> > + 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);
> > + if (!node)
> > + return -EINVAL;
> > +
> > + rmem = of_reserved_mem_lookup(node);
> > + if (!rmem)
> > + return -EINVAL;
> > +
> > + if (strstr(node->name, "vdev0vring")) {
> > + int vring_id;
> > + char name[16];
> > +
> > + /*
> > + * expecting form of "rpuXvdev0vringX as documented
> > + * in xilinx remoteproc device tree binding
> > + */
> > + if (strlen(node->name) < 14) {
> > + dev_err(dev, "%pOF is less than 14 chars",
> > + node);
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * can be 1 of multiple vring IDs per IPC channel
> > + * e.g. 'vdev0vring0' and 'vdev0vring1'
> > + */
> > + vring_id = node->name[14] - '0';
>
> If you are going to use a direct access to node->name[14], then the
> strlen check above should cover it, which means we should check for at
> least strlen(node->name) < 15.
>
Will fix in next rev
>
> > + 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);
> > + } else {
> > + /* Register DMA region */
> > + int (*alloc)(struct rproc *r,
> > + struct rproc_mem_entry *rme);
> > + int (*release)(struct rproc *r,
> > + struct rproc_mem_entry *rme);
> > + char name[20];
> > +
> > + if (strstr(node->name, "vdev0buffer")) {
> > + alloc = NULL;
> > + release = NULL;
> > + strcpy(name, "vdev0buffer");
> > + } else {
> > + alloc = zynqmp_r5_rproc_mem_alloc;
> > + release = zynqmp_r5_rproc_mem_release;
> > + strcpy(name, node->name);
> > + }
> > +
> > + mem = rproc_mem_entry_init(dev, NULL,
> > + (dma_addr_t)rmem->base,
> > + rmem->size, rmem->base,
> > + alloc, release, name);
> > + }
> > + if (!mem)
> > + return -ENOMEM;
> > +
> > + rproc_add_carveout(rproc, mem);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* call Xilinx Platform manager to request access to TCM bank */
> > +static int zynqmp_r5_pm_request_tcm(struct device_node *tcm_node,
> > + struct device *dev, u32 *pnode_id)
> > +{
> > + int ret;
> > +
> > + ret = of_property_read_u32(tcm_node, "pnode-id", pnode_id);
> > + if (ret)
> > + return ret;
> > +
> > + return zynqmp_pm_request_node(*pnode_id,
> ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> > + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> > +}
> > +
> > +/* Given tcm bank entry,
>
> I think checkpatch.pl would complain for this comment format
>
Will fix in next rev
>
> > + * this callback will set device address for R5 running on TCM
> > + * and also setup virtual address for tcm bank remoteproc carveout
> > + */
> > +static int tcm_mem_alloc(struct rproc *rproc,
> > + struct rproc_mem_entry *mem)
> > +{
> > + void *va;
> > + struct device *dev = rproc->dev.parent;
> > +
> > + va = ioremap_wc(mem->dma, mem->len);
> > + if (IS_ERR_OR_NULL(va))
> > + return -ENOMEM;
> > +
> > + /* Update memory entry va */
> > + mem->va = va;
> > +
> > + va = devm_ioremap_wc(dev, mem->da, mem->len);
> > + if (!va)
> > + return -ENOMEM;
> > + /* As R5 is 32 bit, wipe out extra high bits */
> > + mem->da &= 0x000fffff;
> > + /*
> > + * handle tcm banks 1 a and b (0xffe90000 and oxffeb0000)
> > + * As both of these the only common bit found not in tcm bank0 a or
> b
> > + * is at 0x80000 use this mask to suss it out
> > + */
> > + if (mem->da & 0x80000)
> > + /*
> > + * need to do more to further translate
> > + * tcm banks 1a and 1b at 0xffe90000 and oxffeb0000
> ^typo
>
>
> > + * respectively to 0x0 and 0x20000
> > + */
> > + mem->da -= 0x90000;
>
> I understand now why we do "mem->da -= 0x90000" and the in-code
> comment
> explains it. However, why the "if (mem->da & 0x80000)" check?
>
> If we want to make sure to do this "translation" only for 0xffe90000 and
> 0xffeb0000, wouldn't it be better to call them out explicitly, like:
>
> if (mem->da == 0x90000 || mem->da == 0xB0000)
>
Good suggestion, will change if as suggest
>
> Also if this if check fails, should we print an error? Or is it a
> possible handled condition?
Previously this is otherwise handled as TCM banks 0A and 0B. For completeness I will add error checking if it is not in this range either and if so, raise dev_err and EINVAL.
Thanks
Ben
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-10-19 21:33 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 16:06 [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 1/5] firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 2/5] firmware: xilinx: Add shutdown/wakeup APIs Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 3/5] firmware: xilinx: Add RPU configuration APIs Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings Ben Levinsky
2020-10-08 12:37 ` Linus Walleij
2020-10-08 14:21 ` Ben Levinsky
2020-10-08 16:45 ` Ben Levinsky
2020-10-08 20:22 ` Stefano Stabellini
2020-10-08 20:54 ` Linus Walleij
2020-10-05 16:06 ` [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Ben Levinsky
2020-10-05 19:34 ` Michael Auchter
2020-10-06 19:15 ` Ben Levinsky
2020-10-06 21:31 ` Michael Auchter
2020-10-06 21:46 ` Ben Levinsky
2020-10-06 22:20 ` Michael Auchter
2020-10-07 14:31 ` Ben Levinsky
2020-10-15 18:31 ` Ben Levinsky
2020-10-19 20:43 ` Stefano Stabellini
2020-10-19 21:33 ` Ben Levinsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).