On 08/25/2015 12:04 AM, Simon Glass wrote: [...]
index 000000000000..e8fdb124e251 --- /dev/null +++ b/common/cmd_remoteproc.c @@ -0,0 +1,224 @@ +/*
- (C) Copyright 2015
- Texas Instruments Incorporated - http://www.ti.com/
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <command.h> +#include <remoteproc.h> +#include <dm.h> +#include <errno.h> +#include <malloc.h>
nit: can you please sort those includes?
Yes - Sorry abotu that , I missed.
[...]
+static int print_remoteproc_list(void) +{
struct udevice *dev;struct uclass *uc;int ret;char *type;ret = uclass_get(UCLASS_RPROC, &uc);if (ret) {printf("Cannot find Remote processor class\n");return ret;}uclass_foreach_dev(dev, uc) {struct dm_rproc_uclass_pdata *uc_pdata;const struct dm_rproc_ops *ops = device_get_ops(dev);Can you create a rproc_get_ops() static inline as is done with other uclasses, and use that?
Will do. thanks on the hint.
uc_pdata = dev_get_uclass_platdata(dev);if (!uc_pdata) {debug("%s: no uclass_platdata?\n", dev->name);return -ENXIO;}That can never happen so you can remove this check.
OK. will remove elsewhere as well.
diff --git a/doc/device-tree-bindings/remoteproc/remoteproc.txt b/doc/device-tree-bindings/remoteproc/remoteproc.txt new file mode 100644 index 000000000000..1a98a2e3a03c --- /dev/null +++ b/doc/device-tree-bindings/remoteproc/remoteproc.txt @@ -0,0 +1,14 @@ +Remote Processor uClass
uclass
Thanks. will do.
+Binding:
+Remoteproc devices shall have compatible corresponding to thier +drivers. However the following generic properties will be supported
+Optional Properties: +- remoteproc-name: a string, used if provided to describe the processor.
- This must be unique in an operational system.
+- remoteproc-internal-memory-mapped: a bool, indicates that the remote
- processor has internal memory that it uses to execute code and store
- data. Such a device is not expected to have a MMU. If no type property
- is provided, the device is assumed to map to such a model.
Perhaps you could also specific a fallback compatible string so that it is possible to have both that and the specific string. Then it is easy to see what type this device is.
That assumes a standard compatible is available for all devices -> but with remoteproc devices, we cannot really do that, correct?.
Also does this correspond to any existing device tree binding in (e.g.) Linux?
As of v4.2-rc8, only binding we have in upstream kernel is Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt
which is not really helpful here.
diff --git a/doc/driver-model/remoteproc-framework.txt b/doc/driver-model/remoteproc-framework.txt new file mode 100644 index 000000000000..32cb40242e62 --- /dev/null +++ b/doc/driver-model/remoteproc-framework.txt @@ -0,0 +1,163 @@ +# +# (C) Copyright 2015 +# Texas Instruments Incorporated - http://www.ti.com/ +# SPDX-License-Identifier: GPL-2.0+ +#
+Remote Processor Framework +========================== +TOC: +1. Introduction +2. How does it work - The driver +3. Describing the device using platform data +4. Describing the device using device tree
+1. Introduction +===============
+This is an introduction to driver-model for Remote Processors found +on various System on Chip(SoCs). The term remote processor is used to +indicate that this is not the processor on which u-boot is operating
U-Boot
thanks.
[...]
index 092bc02b304e..24bd51269602 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -60,6 +60,8 @@ source "drivers/crypto/Kconfig"
source "drivers/thermal/Kconfig"
+source "drivers/remoteproc/Kconfig"
Please sort these, so remoteproc should go up above thermal.
will do, thanks. [..]
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig new file mode 100644 index 000000000000..700f52caf1dc --- /dev/null +++ b/drivers/remoteproc/Kconfig @@ -0,0 +1,15 @@ +# +# (C) Copyright 2015 +# Texas Instruments Incorporated - http://www.ti.com/ +# SPDX-License-Identifier: GPL-2.0+ +#
+menu "Remote Processor drivers"
+# DM_REMOTEPROC gets selected by drivers as needed +# All users should depend on DM +config DM_REMOTEPROC
Can we use USE REMOTEPROC? The DM_ prefix indicates that driver model is an optional feature for that subsystem, and when everything is converted we intend to remove all the DM_<subsystem> options.
Aaaah... thanks for clarifying.. I had gotten confused on the naming.. i had used without the "DM" prefix originally, will go back.
[...]
diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c new file mode 100644 index 000000000000..cafc293f78f3 --- /dev/null +++ b/drivers/remoteproc/rproc-uclass.c @@ -0,0 +1,406 @@ +/*
- (C) Copyright 2015
- Texas Instruments Incorporated - http://www.ti.com/
- SPDX-License-Identifier: GPL-2.0+
- */
+#define pr_fmt(fmt) "%s: " fmt, __func__ +#include <common.h> +#include <dm.h> +#include <dm/uclass.h> +#include <dm/uclass-internal.h> +#include <dm/device-internal.h> +#include <errno.h> +#include <fdtdec.h> +#include <malloc.h> +#include <asm/io.h> +#include <remoteproc.h>
This is the ordering I'm keen on:
+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <fdtdec.h> +#include <malloc.h> +#include <remoteproc.h> +#include <asm/io.h> +#include <dm/uclass.h> +#include <dm/uclass-internal.h> +#include <dm/device-internal.h>
alright. will do.
+DECLARE_GLOBAL_DATA_PTR;
function comment please
ok. will update all. I had stuck to providing doc for public functions alone.. but it is better to document all of them. [...]
+static struct udevice *rproc_find_dev_for_id(int id) +{
struct udevice *dev;int ret;for (ret = uclass_find_first_device(UCLASS_RPROC, &dev); dev;ret = uclass_find_next_device(&dev)) {if (ret)continue;if (dev->seq == id)return dev;}Can you not use uclass_get_device_by_seq()?
Arrghh.. ofcourse! Thanks.
+static int rproc_pre_probe(struct udevice *dev) +{
struct dm_rproc_uclass_pdata *uc_pdata;const struct dm_rproc_ops *ops;uc_pdata = dev_get_uclass_platdata(dev);if (!uc_pdata) {debug("%s: no uclass_platdata?\n", dev->name);return -ENXIO;}No need - it cannot happen.
Will drop it. Here and elsewhere.
+UCLASS_DRIVER(rproc) = {
/* *INDENT-OFF* */What is that? ^^
Sorry - I should have removed it -> indent messes up this section of code and the comment keeps it out of my hair..
.id = UCLASS_RPROC,.name = "remoteproc",.flags = DM_UC_FLAG_SEQ_ALIAS,.pre_probe = rproc_pre_probe,.post_probe = rproc_post_probe,.per_device_platdata_auto_alloc_size =sizeof(struct dm_rproc_uclass_pdata),/* *INDENT-ON* */+};
+/* exported functions */
But this is not exported is it?
Agreed. will rephrase. how about "remoteproc subsystem access functions" ? [...]
Can you instead check each device individually and drop this flag? In general I would like drivers to avoid declaring any static data.
ok. will try and do that.
+/**
- rproc_load() - load binary to a remote processor
- @id: id of the remote processor
- @addr: address in memory where the binary image is located
- @size: size of the binary image
- */
+int rproc_load(int id, ulong addr, ulong size) +{
struct udevice *dev = rproc_find_dev_for_id(id);struct dm_rproc_uclass_pdata *uc_pdata;const struct dm_rproc_ops *ops;if (!dev) {printf("Unknown remote processor id '%d' requested\n", id);debug()? We should not print out messages in drivers
OK.
debug("%s: data corruption?? mandatory function is missing!\n",dev->name);return -EINVAL;-ENOSYS, which means that a method is missing.
yep. thanks. [...]
printf("%s %s...\n", op_str, uc_pdata->name);debug()? This is a driver.
Here and elsewhere -> will fix.
[...]
+/**
- rproc_start() - Start a remote processor
- @id: id of the remote processor
Please document the return value in these functions
OK. will do. [...]
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index c744044bb8aa..a2958c234db4 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -47,6 +47,7 @@ enum uclass_id { UCLASS_PMIC, /* PMIC I/O device */ UCLASS_REGULATOR, /* Regulator device */ UCLASS_RESET, /* Reset device */
UCLASS_RPROC, /* Remote Processor device */Maybe UCLASS_REMOTEPROC would be better to be consistent and more descriptive?
ok.
UCLASS_RTC, /* Real time clock device */ UCLASS_SERIAL, /* Serial UART */ UCLASS_SPI, /* SPI bus */diff --git a/include/remoteproc.h b/include/remoteproc.h new file mode 100644 index 000000000000..b92d40e0ca2e --- /dev/null +++ b/include/remoteproc.h @@ -0,0 +1,81 @@ +/*
- (C) Copyright 2015
- Texas Instruments Incorporated - http://www.ti.com/
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef _RPROC_H_ +#define _RPROC_H_
+#include <dm/platdata.h> /* For platform data support - non dt world */
Does this need to be supported for a new uclass?
we do have platforms that are not using DT yet... they will need to pass platform data.
+/**
- enum rproc_mem_type - What type of memory model does the rproc use
- @RPROC_INTERNAL: Remote processor uses own memory and is memory mapped
to the host processor over an address range.
- Please note that this is an enumeration of memory model of different types
- of remote processors. Few of the remote processors do have own internal
- memories, while others use external memory for instruction and data.
- */
+enum rproc_mem_type {
RPROC_INTERNAL_MEMORY_MAPPED = 0,+};
+/**
- struct dm_rproc_uclass_pdata - platform data for a CPU
- This can be accessed with dev_get_platdata() for any UCLASS_RPROC
- device.
- @name: Platform-specific way of naming the Remote proc
- @mem_type: one of 'enum rproc_mem_type'
- @driver_data: driver specific platform data that may be needed.
The comment names do not match the struct.
uggh.. sorry about that. will fix.
- */
+struct dm_rproc_uclass_pdata {
const char *name;enum rproc_mem_type mem_type;void *driver_plat_data;+};
+/**
- struct dm_rproc_ops - Operations that are provided by remote proc driver
- @init: Initialize the remoteproc device invoked after probe (optional)
- @load: Load the remoteproc device using data provided(mandatory)
- @start: Start the remoteproc device (mandatory)
- @stop: Stop the remoteproc device (optional)
- @reset: Reset the remote proc device (optional)
- @is_running: Check if the remote processor is running(optional)
- @ping: Ping the remote device for basic communication check(optional)
You should document the return value (0 for success, -ve on error?). For is_running(), what return value means what?
agreed - will try to do better in the next rev.
- */
+struct dm_rproc_ops {
int (*init)(struct udevice *dev);int (*load)(struct udevice *dev, ulong addr, ulong size);document args
ok. will try to do that. [...]
+static inline int rproc_init(void) { return -EINVAL; }
-ENOSYS
here and else where - will update.
+static inline int rproc_is_initialized(void) { return false; } +static inline int rproc_load(int id, ulong addr, ulong size) { return -EINVAL; } +static inline int rproc_start(int id) { return -EINVAL; } +static inline int rproc_stop(int id) { return -EINVAL; } +static inline int rproc_reset(int id) { return -EINVAL; } +static inline int rproc_ping(int id) { return -EINVAL; } +static inline int rproc_is_running(int id) { return -EINVAL; } +#endif
Thanks for the detailed review. will drop in an updated rev soon.