[U-Boot] [PATCH v2 1/3] drivers: Add board uclass
Simon Glass
sjg at chromium.org
Mon Apr 30 23:12:47 UTC 2018
Hi,
On 27 April 2018 at 07:02, Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
>
>> On 27 Apr 2018, at 14:51, Mario Six <mario.six at gdsys.cc> wrote:
>>
>> Since there is no canonical "board device" that can be used in board
>> files, it is difficult to use DM function for board initialization in
>> these cases.
>>
>> Hence, add a uclass that implements a simple "board device", which can
>> hold devices not suitable anywhere else in the device tree, and is also
>> able to read encoded information, e.g. hard-wired GPIOs on a GPIO
>> expander, read-only memory ICs, etc. that carry information about the
>> hardware.
With comments below:
Reviewed-by: Simon Glass <sjg at chromium.org>
>
> A board-uclass should model a board (i.e. provide implementations for
> the same key abstractions that we have in place today: e.g., board_init).
>
> This seems more like a specialised form of a misc-device.
>
>> The devices of this uclass expose methods to read generic data types
>> (integers, strings, booleans) to encode the information provided by the
>> hardware.
>
> Again, this is what a misc-device is intended for… and extending the
> misc-device APIs (or having a convenience layer on top of its ioctl
> interface?) might be more appropriate.
>
> After reviewing the below, the similarities to the misc-device are even
> more apparent: if you need typed access to a key-value pair, this can
> be easily implemented through a common ioctl with semantic sugar
> at the misc-uclass level.
A misc device might be similar in action but it is not the same in
terms of concept. For example it would be very strange to have a misc
device which points to lots of other devices that are on the board.
For example, consider this hypothetical case:
board {
identity-eeprom = <&eeprom>;
id-gpios = <&gpio0 2>, <&gpio0 4>;
ec = <&cros_ec>;
};
Here we point to a few devices which may otherwise require specific
device-tree references to locate. Already we are seeing this sort of
code in U-Boot:
/* find the grf node */
node = fdt_node_offset_by_compatible(blob, -1,
"rockchip,rk3288-grf");
ret = regulator_get_by_platname("vdd_arm", &dev);
if (ret) {
debug("Cannot set regulator name\n");
return ret;
}
These are the sorts of things which suggest that some sort of 'board
directory', pointing to the things that the board needs to init, would
be useful.
>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>> ---
>>
>> v1 -> v2:
>> * Corrected description of dev parameter of devinfo_detect
>> * Added size parameter to devinfo_get_str
>> * Expanded uclass documentation
>> * Added function to get devinfo instance
>> * Renamed the uclass from devinfo to board
>>
>> ---
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/board/Kconfig | 17 +++++++
>> drivers/board/Makefile | 9 ++++
>> drivers/board/board-uclass.c | 61 +++++++++++++++++++++++
>> include/board.h | 115 +++++++++++++++++++++++++++++++++++++++++++
>> include/dm/uclass-id.h | 1 +
>> 7 files changed, 206 insertions(+)
>> create mode 100644 drivers/board/Kconfig
>> create mode 100644 drivers/board/Makefile
>> create mode 100644 drivers/board/board-uclass.c
>> create mode 100644 include/board.h
>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index c2e813f5ad..19c7c448c0 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -22,6 +22,8 @@ source "drivers/ddr/Kconfig"
>>
>> source "drivers/demo/Kconfig"
>>
>> +source "drivers/board/Kconfig"
>> +
>> source "drivers/ddr/fsl/Kconfig"
>>
>> source "drivers/dfu/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 6846d181aa..fecf64021c 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -72,6 +72,7 @@ obj-y += block/
>> obj-$(CONFIG_BOOTCOUNT_LIMIT) += bootcount/
>> obj-$(CONFIG_CPU) += cpu/
>> obj-y += crypto/
>> +obj-y += board/
>> obj-y += firmware/
>> obj-$(CONFIG_FPGA) += fpga/
>> obj-y += misc/
>> diff --git a/drivers/board/Kconfig b/drivers/board/Kconfig
>> new file mode 100644
>> index 0000000000..cc1cf27205
>> --- /dev/null
>> +++ b/drivers/board/Kconfig
>> @@ -0,0 +1,17 @@
>> +menuconfig BOARD
>> + bool "Device Information"
>> + help
>> + Support methods to query hardware configurations from internal
>> + mechanisms (e.g. reading GPIO values, determining the presence of
>> + devices on busses, etc.). This enables the usage of U-Boot with
>> + modular board architectures.
>> +
>> +if BOARD
>> +
>> +
>> +config BOARD_GAZERBEAM
>> + bool "Enable device information for the Gazerbeam board"
>> + help
>> + Support querying device information for the gdsys Gazerbeam board.
>> +
>> +endif
>> diff --git a/drivers/board/Makefile b/drivers/board/Makefile
>> new file mode 100644
>> index 0000000000..397706245a
>> --- /dev/null
>> +++ b/drivers/board/Makefile
>> @@ -0,0 +1,9 @@
>> +#
>> +# (C) Copyright 2017
>> +# Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
>> +#
>> +# SPDX-License-Identifier: GPL-2.0+
>> +#
>> +
>> +obj-$(CONFIG_BOARD) += board-uclass.o
>> +obj-$(CONFIG_BOARD_GAZERBEAM) += gazerbeam.o
>> diff --git a/drivers/board/board-uclass.c b/drivers/board/board-uclass.c
>> new file mode 100644
>> index 0000000000..3137437245
>> --- /dev/null
>> +++ b/drivers/board/board-uclass.c
>> @@ -0,0 +1,61 @@
>> +/*
>> + * (C) Copyright 2017
>> + * Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <board.h>
>> +
>> +int get_board(struct udevice **devp)
board_get() ?
That way we can use a consistent prefix for all functions.
>> +{
>> + return uclass_first_device_err(UCLASS_BOARD, devp);
>> +}
>> +
>> +int board_detect(struct udevice *dev)
>> +{
>> + struct board_ops *ops = board_get_ops(dev);
>> +
>> + if (!ops->detect)
>> + return -ENOSYS;
>> +
>> + return ops->detect(dev);
>> +}
>> +
>> +int board_get_bool(struct udevice *dev, int id, bool *val)
>> +{
>> + struct board_ops *ops = board_get_ops(dev);
>> +
>> + if (!ops->get_bool)
>> + return -ENOSYS;
>> +
>> + return ops->get_bool(dev, id, val);
>> +}
>> +
>> +int board_get_int(struct udevice *dev, int id, int *val)
>> +{
>> + struct board_ops *ops = board_get_ops(dev);
>> +
>> + if (!ops->get_int)
>> + return -ENOSYS;
>> +
>> + return ops->get_int(dev, id, val);
>> +}
>> +
>> +int board_get_str(struct udevice *dev, int id, size_t size, char *val)
>> +{
>> + struct board_ops *ops = board_get_ops(dev);
>> +
>> + if (!ops->get_str)
>> + return -ENOSYS;
>> +
>> + return ops->get_str(dev, id, size, val);
>> +}
>> +
>> +UCLASS_DRIVER(board) = {
>> + .id = UCLASS_BOARD,
>> + .name = "board",
>> + .post_bind = dm_scan_fdt_dev,
>> +};
>> diff --git a/include/board.h b/include/board.h
>> new file mode 100644
>> index 0000000000..0e64a3dfce
>> --- /dev/null
>> +++ b/include/board.h
>> @@ -0,0 +1,115 @@
>> +/*
>> + * (C) Copyright 2017
>> + * Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +/**
>> + * This uclass encapsulates hardware methods to gather information about a
>> + * board or a specific device such as hard-wired GPIOs on GPIO expanders,
>> + * read-only data in flash ICs, or similar.
>> + *
>> + * The interface offers functions to read the usual standard data types (bool,
>> + * int, string) from the device, each of which is identified by a static
>> + * numeric ID (which will usually be defined as a enum in a header file).
>> + *
>> + * If for example dev was a board device representing a read-only serial
>> + * number flash IC, we could call
>> + *
>> + * board_detect(dev);
>> + * board_get_int(dev, ID_SERIAL_NUMBER, &serial);
Please add error checking code.
>> + *
>> + * to read the serial number from the device.
>> + */
>> +
>> +struct board_ops {
>> + /**
>> + * detect() - Run the hardware info detection procedure for this
>> + * device.
>> + *
>> + * @dev: The device containing the information
>> + * @return 0 if OK, -ve on error.
>> + */
>> + int (*detect)(struct udevice *dev);
>
> This doesn’t make any sense, as the probe entry-point is intended to
> probe for a device.
I can see your point, but I feel that it does make some sense.
Probing the board device could be used to simply locate all the
dependent devices and set them up in a data structure. This might be a
fairly fast operation.
Detecting the hardware could take a lot longer:
- reading from an EEPROM
- calling out to an EC to ask for details
- checking for the present of a device on an I2C device
I don't like the idea of probe() taking a really long time (e.g.
50ms). It should be possible to get access to a device without
incurring that delay. This way, we can access the devices without
necessarily knowing exactly what type of board it is.
The comment above definitely needs expanding to explain why this
doesn't happen in probe()
>
>> +
>> + /**
>> + * get_bool() - Read a specific bool data value that describes the
>> + * hardware setup.
>> + *
>> + * @dev: The board instance to gather the data.
>> + * @id: A unique identifier for the bool value to be read.
>> + * @val: Pointer to a buffer that receives the value read.
>> + * @return 0 if OK, -ve on error.
>> + */
>> + int (*get_bool)(struct udevice *dev, int id, bool *val);
>> +
>> + /**
>> + * get_int() - Read a specific int data value that describes the
>> + * hardware setup.
>> + *
>> + * @dev: The board instance to gather the data.
>> + * @id: A unique identifier for the int value to be read.
>> + * @val: Pointer to a buffer that receives the value read.
>> + * @return 0 if OK, -ve on error.
>> + */
>> + int (*get_int)(struct udevice *dev, int id, int *val);
>> +
>> + /**
>> + * get_str() - Read a specific string data value that describes the
>> + * hardware setup.
>> + *
>> + * @dev: The board instance to gather the data.
>> + * @id: A unique identifier for the string value to be read.
>> + * @size: The size of the buffer to receive the string data.
>> + * @val: Pointer to a buffer that receives the value read.
>> + * @return 0 if OK, -ve on error.
>> + */
>> + int (*get_str)(struct udevice *dev, int id, size_t size, char *val);
>
> These could all be handled as ioctl()-calls to a misc-device by
> defining a common protocol and convenience layer.
It could, and perhaps we should add an ioctl() as well. But I don't
see a lot of overhead in adding type-specific methods, and they are
convenient to use in the driver and in the caller. For the caller we
can add a convenience layer of course, but in the driver that is a bit
more of a pain.
>
>> +};
>> +
>> +#define board_get_ops(dev) ((struct board_ops *)(dev)->driver->ops)
>> +
>> +/**
>> + * board_detect() - Run the hardware info detection procedure for this device.
>> + *
>> + * @dev: The device containing the information
>> + * @return 0 if OK, -ve on error.
>> + */
>> +int board_detect(struct udevice *dev);
>> +
>> +/**
>> + * board_get_bool() - Read a specific bool data value that describes the
>> + * hardware setup.
>> + *
>> + * @dev: The board instance to gather the data.
>> + * @id: A unique identifier for the bool value to be read.
>> + * @val: Pointer to a buffer that receives the value read.
>> + * @return 0 if OK, -ve on error.
>> + */
>> +int board_get_bool(struct udevice *dev, int id, bool *val);
>> +
>> +/**
>> + * board_get_int() - Read a specific int data value that describes the
>> + * hardware setup.
>> + *
>> + * @dev: The board instance to gather the data.
>> + * @id: A unique identifier for the int value to be read.
>> + * @val: Pointer to a buffer that receives the value read.
>> + * @return 0 if OK, -ve on error.
>> + */
>> +int board_get_int(struct udevice *dev, int id, int *val);
>> +
>> +/**
>> + * board_get_str() - Read a specific string data value that describes the
>> + * hardware setup.
>> + *
>> + * @dev: The board instance to gather the data.
>> + * @id: A unique identifier for the string value to be read.
>> + * @size: The size of the buffer to receive the string data.
>> + * @val: Pointer to a buffer that receives the value read.
>> + * @return 0 if OK, -ve on error.
>> + */
>> +int board_get_str(struct udevice *dev, int id, size_t size, char *val);
>> +
>> +int get_board(struct udevice **devp);
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index d28fb3e23f..97327194b4 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -30,6 +30,7 @@ enum uclass_id {
>> UCLASS_ADC, /* Analog-to-digital converter */
>> UCLASS_AHCI, /* SATA disk controller */
>> UCLASS_BLK, /* Block device */
>> + UCLASS_BOARD, /* Device information from hardware */
>> UCLASS_CLK, /* Clock source, e.g. used by peripherals */
>> UCLASS_CPU, /* CPU, typically part of an SoC */
>> UCLASS_CROS_EC, /* Chrome OS EC */
>> --
>> 2.16.1
>>
>
Regards,
SImon
More information about the U-Boot
mailing list