[U-Boot] [PATCH 1/3] drivers: Introduce a simplified remoteproc framework
Nishanth Menon
nm at ti.com
Tue Aug 25 17:44:26 CEST 2015
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
More information about the U-Boot
mailing list