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. -- Regards, Nishanth Menon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot