[U-Boot] [PATCH 1/3] drivers: Introduce a simplified remoteproc framework

[Nishanth Menon <nm@ti.com>]

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