[U-Boot] [PATCH v4 0/15] Collected driver model bug-fixes and docs
This series collects some of the patches from the Tegra GPIO conversion to driver model. That work is still in progress, but the bug fixes and iotracing feature should go into this release I think. Also the documentation improvements may as well follow since the existings docs are proven inadequate.
Changes in v4: - Correct typo in CONFIG_CMD_IOTRACE - Remove 64-bit addresses which are not used in U-Boot - Minor spelling fixes
Changes in v3: - Remove use of bool in header file to avoid exynos5 build failure - Create a symlink for each arch to dt-bindings - Fix typo in commit subject - Bring in GPIO bindings for tegra{30,114,124} also - Enable dm command in this patch instead of the next
Changes in v2: - Add a new patch for an I/O tracing feature - Add a new patch to enable iotrace for arm - Add a new patch to enable iotrace for sandbox - Add new patch to support include files for .dts files - Update README to encourage conversion to driver model - Add new patch to use case-insensitive comparison for GPIO banks - Add new patch to add missing header files in lists and root - Add new patch to deal with const-ness of the global_data pointer - Add new patch to allow driver model tests only for sandbox - Add new patch to fix printf() strings in the 'dm' command - Add new patch to bring in Tegra device tree files from linux - Split out a separate patch to enable driver model for tegra - Rename struct device to struct udevice - Adjust docs as per Jon's review
Simon Glass (15): Add an I/O tracing feature arm: Support iotrace feature sandbox: Support iotrace feature Makefile: Support include files for .dts files dm: Rename struct device_id to udevice_id dm: Update README to encourage conversion to driver model dm: Use case-insensitive comparison for GPIO banks dm: Add missing header files in lists and root dm: Cast away the const-ness of the global_data pointer dm: Allow driver model tests only for sandbox dm: Fix printf() strings in the 'dm' command tegra: dts: Bring in GPIO bindings from linux tegra: Enable driver model dm: Tidy up four minor code nits dm: Expand and improve the device lifecycle docs
README | 28 +++ arch/arm/dts/include/dt-bindings | 1 + arch/arm/dts/tegra114.dtsi | 21 +- arch/arm/dts/tegra124.dtsi | 19 +- arch/arm/dts/tegra20.dtsi | 15 +- arch/arm/dts/tegra30.dtsi | 21 +- arch/arm/include/asm/io.h | 3 + arch/microblaze/dts/include/dt-bindings | 1 + arch/sandbox/dts/include/dt-bindings | 1 + arch/sandbox/include/asm/io.h | 10 + arch/x86/dts/include/dt-bindings | 1 + common/Makefile | 2 + common/cmd_iotrace.c | 73 +++++++ common/iotrace.c | 169 ++++++++++++++++ doc/driver-model/README.txt | 217 ++++++++++++++++++++- drivers/core/lists.c | 3 +- drivers/core/root.c | 7 +- drivers/core/uclass.c | 2 +- drivers/demo/demo-shape.c | 2 +- drivers/demo/demo-simple.c | 2 +- drivers/gpio/gpio-uclass.c | 2 +- drivers/gpio/sandbox.c | 2 +- include/configs/sandbox.h | 3 + include/configs/tegra-common.h | 3 + include/dm/device-internal.h | 4 + include/dm/device.h | 8 +- include/dm/lists.h | 20 ++ include/dm/root.h | 2 +- include/dm/uclass.h | 6 +- include/dt-bindings/gpio/gpio.h | 15 ++ include/dt-bindings/gpio/tegra-gpio.h | 51 +++++ include/dt-bindings/interrupt-controller/arm-gic.h | 22 +++ include/dt-bindings/interrupt-controller/irq.h | 19 ++ include/iotrace.h | 104 ++++++++++ scripts/Makefile.lib | 1 + test/dm/Makefile | 2 + test/dm/cmd_dm.c | 19 +- test/dm/test-fdt.c | 2 +- 38 files changed, 824 insertions(+), 59 deletions(-) create mode 120000 arch/arm/dts/include/dt-bindings create mode 120000 arch/microblaze/dts/include/dt-bindings create mode 120000 arch/sandbox/dts/include/dt-bindings create mode 120000 arch/x86/dts/include/dt-bindings create mode 100644 common/cmd_iotrace.c create mode 100644 common/iotrace.c create mode 100644 include/dt-bindings/gpio/gpio.h create mode 100644 include/dt-bindings/gpio/tegra-gpio.h create mode 100644 include/dt-bindings/interrupt-controller/arm-gic.h create mode 100644 include/dt-bindings/interrupt-controller/irq.h create mode 100644 include/iotrace.h
When debugging drivers it is useful to see what I/O accesses were done and in what order.
Even if the individual accesses are of little interest it can be useful to verify that the access pattern is consistent each time an operation is performed. In this case a checksum can be used to characterise the operation of a driver. The checksum can be compared across different runs of the operation to verify that the driver is working properly.
In particular, when performing major refactoring of the driver, where the access pattern should not change, the checksum provides assurance that the refactoring work has not broken the driver.
Add an I/O tracing feature and associated commands to provide this facility. It works by sneaking into the io.h heder for an architecture and redirecting I/O accesses through its tracing mechanism.
For now no commands are provided to examine the trace buffer. The format is fairly simple, so 'md' is a reasonable substitute.
Note: The checksum feature is only useful for I/O regions where the contents do not change outside of software control. Where this is not suitable you can fall back to manually comparing the addresses. It might be useful to enhance tracing to only checksum the accesses and not the data read/written.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: - Remove use of bool in header file to avoid exynos5 build failure
Changes in v2: - Add a new patch for an I/O tracing feature
README | 23 +++++++ common/Makefile | 2 + common/cmd_iotrace.c | 73 ++++++++++++++++++++++ common/iotrace.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/iotrace.h | 104 +++++++++++++++++++++++++++++++ 5 files changed, 371 insertions(+) create mode 100644 common/cmd_iotrace.c create mode 100644 common/iotrace.c create mode 100644 include/iotrace.h
diff --git a/README b/README index 6edb2e7..a53f69a 100644 --- a/README +++ b/README @@ -1000,6 +1000,7 @@ The following options need to be configured: CONFIG_CMD_IMLS List all images found in NOR flash CONFIG_CMD_IMLS_NAND * List all images found in NAND flash CONFIG_CMD_IMMAP * IMMR dump support + CONFIG_CMD_IOTRACE * I/O tracing for debugging CONFIG_CMD_IMPORTENV * import an environment CONFIG_CMD_INI * import data from an ini file into the env CONFIG_CMD_IRQ * irqinfo @@ -1171,6 +1172,28 @@ The following options need to be configured: Note that if the GPIO device uses I2C, then the I2C interface must also be configured. See I2C Support, below.
+- I/O tracing: + When CONFIG_IO_TRACE is selected, U-Boot intercepts all I/O + accesses and can checksum them or write a list of them out + to memory. See the 'iotrace' command for details. This is + useful for testing device drivers since it can confirm that + the driver behaves the same way before and after a code + change. Currently this is supported on sandbox and arm. To + add support for your architecture, add '#include <iotrace.h>' + to the bottom of arch/<arch>/include/asm/io.h and test. + + Example output from the 'iotrace stats' command is below. + Note that if the trace buffer is exhausted, the checksum will + still continue to operate. + + iotrace is enabled + Start: 10000000 (buffer start address) + Size: 00010000 (buffer size) + Offset: 00000120 (current buffer offset) + Output: 10000120 (start + offset) + Count: 00000018 (number of trace records) + CRC32: 9526fb66 (CRC32 of all trace records) + - Timestamp Support:
When CONFIG_TIMESTAMP is selected, the timestamp diff --git a/common/Makefile b/common/Makefile index 391a8d6..7e192c7 100644 --- a/common/Makefile +++ b/common/Makefile @@ -114,6 +114,7 @@ obj-$(CONFIG_CMD_FUSE) += cmd_fuse.o obj-$(CONFIG_CMD_GETTIME) += cmd_gettime.o obj-$(CONFIG_CMD_GPIO) += cmd_gpio.o obj-$(CONFIG_CMD_I2C) += cmd_i2c.o +obj-$(CONFIG_CMD_IOTRACE) += cmd_iotrace.o obj-$(CONFIG_CMD_HASH) += cmd_hash.o obj-$(CONFIG_CMD_IDE) += cmd_ide.o obj-$(CONFIG_CMD_IMMAP) += cmd_immap.o @@ -261,6 +262,7 @@ obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o obj-$(CONFIG_OF_LIBFDT) += image-fdt.o obj-$(CONFIG_FIT) += image-fit.o obj-$(CONFIG_FIT_SIGNATURE) += image-sig.o +obj-$(CONFIG_IO_TRACE) += iotrace.o obj-y += memsize.o obj-y += stdio.o
diff --git a/common/cmd_iotrace.c b/common/cmd_iotrace.c new file mode 100644 index 0000000..f54276d --- /dev/null +++ b/common/cmd_iotrace.c @@ -0,0 +1,73 @@ +/* + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <command.h> +#include <iotrace.h> + +static void do_print_stats(void) +{ + ulong start, size, offset, count; + + printf("iotrace is %sabled\n", iotrace_get_enabled() ? "en" : "dis"); + iotrace_get_buffer(&start, &size, &offset, &count); + printf("Start: %08lx\n", start); + printf("Size: %08lx\n", size); + printf("Offset: %08lx\n", offset); + printf("Output: %08lx\n", start + offset); + printf("Count: %08lx\n", count); + printf("CRC32: %08lx\n", (ulong)iotrace_get_checksum()); +} + +static int do_set_buffer(int argc, char * const argv[]) +{ + ulong addr = 0, size = 0; + + if (argc == 2) { + addr = simple_strtoul(*argv++, NULL, 16); + size = simple_strtoul(*argv++, NULL, 16); + } else if (argc != 0) { + return CMD_RET_USAGE; + } + + iotrace_set_buffer(addr, size); + + return 0; +} + +int do_iotrace(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + const char *cmd = argc < 2 ? NULL : argv[1]; + + if (!cmd) + return cmd_usage(cmdtp); + switch (*cmd) { + case 'b': + return do_set_buffer(argc - 2, argv + 2); + case 'p': + iotrace_set_enabled(0); + break; + case 'r': + iotrace_set_enabled(1); + break; + case 's': + do_print_stats(); + break; + default: + return CMD_RET_USAGE; + } + + return 0; +} + +U_BOOT_CMD( + iotrace, 4, 1, do_iotrace, + "iotrace utility commands", + "stats - display iotrace stats\n" + "iotrace buffer <address> <size> - set iotrace buffer\n" + "iotrace pause - pause tracing\n" + "iotrace resume - resume tracing" +); diff --git a/common/iotrace.c b/common/iotrace.c new file mode 100644 index 0000000..ced426e --- /dev/null +++ b/common/iotrace.c @@ -0,0 +1,169 @@ +/* + * Copyright (c) 2014 Google, Inc. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#define IOTRACE_IMPL + +#include <common.h> +#include <asm/io.h> + +DECLARE_GLOBAL_DATA_PTR; + +/* Support up to the machine word length for now */ +typedef ulong iovalue_t; + +enum iotrace_flags { + IOT_8 = 0, + IOT_16, + IOT_32, + + IOT_READ = 0 << 3, + IOT_WRITE = 1 << 3, +}; + +/** + * struct iotrace_record - Holds a single I/O trace record + * + * @flags: I/O access type + * @addr: Address of access + * @value: Value written or read + */ +struct iotrace_record { + enum iotrace_flags flags; + phys_addr_t addr; + iovalue_t value; +}; + +/** + * struct iotrace - current trace status and checksum + * + * @start: Start address of iotrace buffer + * @size: Size of iotrace buffer in bytes + * @offset: Current write offset into iotrace buffer + * @crc32: Current value of CRC chceksum of trace records + * @enabled: true if enabled, false if disabled + */ +static struct iotrace { + ulong start; + ulong size; + ulong offset; + u32 crc32; + bool enabled; +} iotrace; + +static void add_record(int flags, const void *ptr, ulong value) +{ + struct iotrace_record srec, *rec = &srec; + + /* + * We don't support iotrace before relocation. Since the trace buffer + * is set up by a command, it can't be enabled at present. To change + * this we would need to set the iotrace buffer at build-time. See + * lib/trace.c for how this might be done if you are interested. + */ + if (!(gd->flags & GD_FLG_RELOC) || !iotrace.enabled) + return; + + /* Store it if there is room */ + if (iotrace.offset + sizeof(*rec) < iotrace.size) { + rec = (struct iotrace_record *)map_sysmem( + iotrace.start + iotrace.offset, + sizeof(value)); + } + + rec->flags = flags; + rec->addr = map_to_sysmem(ptr); + rec->value = value; + + /* Update our checksum */ + iotrace.crc32 = crc32(iotrace.crc32, (unsigned char *)rec, + sizeof(*rec)); + + iotrace.offset += sizeof(struct iotrace_record); +} + +u32 iotrace_readl(const void *ptr) +{ + u32 v; + + v = readl(ptr); + add_record(IOT_32 | IOT_READ, ptr, v); + + return v; +} + +void iotrace_writel(ulong value, const void *ptr) +{ + add_record(IOT_32 | IOT_WRITE, ptr, value); + writel(value, ptr); +} + +u16 iotrace_readw(const void *ptr) +{ + u32 v; + + v = readw(ptr); + add_record(IOT_16 | IOT_READ, ptr, v); + + return v; +} + +void iotrace_writew(ulong value, const void *ptr) +{ + add_record(IOT_16 | IOT_WRITE, ptr, value); + writew(value, ptr); +} + +u8 iotrace_readb(const void *ptr) +{ + u32 v; + + v = readb(ptr); + add_record(IOT_8 | IOT_READ, ptr, v); + + return v; +} + +void iotrace_writeb(ulong value, const void *ptr) +{ + add_record(IOT_8 | IOT_WRITE, ptr, value); + writeb(value, ptr); +} + +void iotrace_reset_checksum(void) +{ + iotrace.crc32 = 0; +} + +u32 iotrace_get_checksum(void) +{ + return iotrace.crc32; +} + +void iotrace_set_enabled(int enable) +{ + iotrace.enabled = enable; +} + +int iotrace_get_enabled(void) +{ + return iotrace.enabled; +} + +void iotrace_set_buffer(ulong start, ulong size) +{ + iotrace.start = start; + iotrace.size = size; + iotrace.offset = 0; + iotrace.crc32 = 0; +} + +void iotrace_get_buffer(ulong *start, ulong *size, ulong *offset, ulong *count) +{ + *start = iotrace.start; + *size = iotrace.size; + *offset = iotrace.offset; + *count = iotrace.offset / sizeof(struct iotrace_record); +} diff --git a/include/iotrace.h b/include/iotrace.h new file mode 100644 index 0000000..9bd1f16 --- /dev/null +++ b/include/iotrace.h @@ -0,0 +1,104 @@ +/* + * Copyright (c) 2014 Google, Inc. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef __IOTRACE_H +#define __IOTRACE_H + +#include <linux/types.h> + +/* + * This file is designed to be included in arch/<arch>/include/asm/io.h. + * It redirects all IO access through a tracing/checksumming feature for + * testing purposes. + */ + +#if defined(CONFIG_IO_TRACE) && !defined(IOTRACE_IMPL) && \ + !defined(CONFIG_SPL_BUILD) + +#undef readl +#define readl(addr) iotrace_readl((const void *)(addr)) + +#undef writel +#define writel(val, addr) iotrace_writel(val, (const void *)(addr)) + +#undef readw +#define readw(addr) iotrace_readw((const void *)(addr)) + +#undef writew +#define writew(val, addr) iotrace_writew(val, (const void *)(addr)) + +#undef readb +#define readb(addr) iotrace_readb((const void *)(addr)) + +#undef writeb +#define writeb(val, addr) iotrace_writeb(val, (const void *)(addr)) + +#endif + +/* Tracing functions which mirror their io.h counterparts */ +u32 iotrace_readl(const void *ptr); +void iotrace_writel(ulong value, const void *ptr); +u16 iotrace_readw(const void *ptr); +void iotrace_writew(ulong value, const void *ptr); +u8 iotrace_readb(const void *ptr); +void iotrace_writeb(ulong value, const void *ptr); + +/** + * iotrace_reset_checksum() - Reset the iotrace checksum + */ +void iotrace_reset_checksum(void); + +/** + * iotrace_get_checksum() - Get the current checksum value + * + * @return currect checksum value + */ +u32 iotrace_get_checksum(void); + +/** + * iotrace_set_enabled() - Set whether iotracing is enabled or not + * + * This controls whether the checksum is updated and a trace record added + * for each I/O access. + * + * @enable: true to enable iotracing, false to disable + */ +void iotrace_set_enabled(int enable); + +/** + * iotrace_get_enabled() - Get whether iotracing is enabled or not + * + * @return true if enabled, false if disabled + */ +int iotrace_get_enabled(void); + +/** + * iotrace_set_buffer() - Set position and size of iotrace buffer + * + * Defines where the iotrace buffer goes, and resets the output pointer to + * the start of the buffer. + * + * The buffer can be 0 size in which case the checksum is updated but no + * trace records are writen. If the buffer is exhausted, the offset will + * continue to increase but not new data will be written. + * + * @start: Start address of buffer + * @size: Size of buffer in bytes + */ +void iotrace_set_buffer(ulong start, ulong size); + +/** + * iotrace_get_buffer() - Get buffer information + * + * @start: Returns start address of buffer + * @size: Returns size of buffer in bytes + * @offset: Returns the byte offset where the next output trace record will + * @count: Returns the number of trace records recorded + * be written (or would be if the buffer was large enough) + */ +void iotrace_get_buffer(ulong *start, ulong *size, ulong *offset, ulong *count); + +#endif /* __IOTRACE_H */
Support the iotrace feature for ARM, when enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: - Add a new patch to enable iotrace for arm
arch/arm/include/asm/io.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 6a1f05a..9f35fd6 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -437,4 +437,7 @@ out:
#endif /* __mem_isa */ #endif /* __KERNEL__ */ + +#include <iotrace.h> + #endif /* __ASM_ARM_IO_H */
Support the iotrace feature for sandbox, and enable it, using some dummy I/O access methods.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Correct typo in CONFIG_CMD_IOTRACE
Changes in v3: None Changes in v2: - Add a new patch to enable iotrace for sandbox
arch/sandbox/include/asm/io.h | 10 ++++++++++ include/configs/sandbox.h | 3 +++ 2 files changed, 13 insertions(+)
diff --git a/arch/sandbox/include/asm/io.h b/arch/sandbox/include/asm/io.h index 7956041..895fcb8 100644 --- a/arch/sandbox/include/asm/io.h +++ b/arch/sandbox/include/asm/io.h @@ -40,4 +40,14 @@ static inline void unmap_sysmem(const void *vaddr) /* Map from a pointer to our RAM buffer */ phys_addr_t map_to_sysmem(const void *ptr);
+/* Define nops for sandbox I/O access */ +#define readb(addr) 0 +#define readw(addr) 0 +#define readl(addr) 0 +#define writeb(v, addr) +#define writew(v, addr) +#define writel(v, addr) + +#include <iotrace.h> + #endif diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 6bb2546..fdc8b75 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -16,6 +16,9 @@
#endif
+#define CONFIG_IO_TRACE +#define CONFIG_CMD_IOTRACE + #define CONFIG_SYS_TIMER_RATE 1000000
#define CONFIG_BOOTSTAGE
Linux supports this, and if we are to have compatible device tree files, U-Boot should also.
Avoid giving the device tree files access to U-Boot's include/ directory. Only include/dt-bindings is accessible.
Signed-off-by: Simon Glass sjg@chromium.org Acked-by: Stephen Warren swarren@nvidia.com Reviewed-by: Masahiro Yamada yamada.m@jp.panasonic.com ---
Changes in v4: None Changes in v3: - Create a symlink for each arch to dt-bindings
Changes in v2: - Add new patch to support include files for .dts files
arch/arm/dts/include/dt-bindings | 1 + arch/microblaze/dts/include/dt-bindings | 1 + arch/sandbox/dts/include/dt-bindings | 1 + arch/x86/dts/include/dt-bindings | 1 + scripts/Makefile.lib | 1 + 5 files changed, 5 insertions(+) create mode 120000 arch/arm/dts/include/dt-bindings create mode 120000 arch/microblaze/dts/include/dt-bindings create mode 120000 arch/sandbox/dts/include/dt-bindings create mode 120000 arch/x86/dts/include/dt-bindings
diff --git a/arch/arm/dts/include/dt-bindings b/arch/arm/dts/include/dt-bindings new file mode 120000 index 0000000..0cecb3d --- /dev/null +++ b/arch/arm/dts/include/dt-bindings @@ -0,0 +1 @@ +../../../../include/dt-bindings \ No newline at end of file diff --git a/arch/microblaze/dts/include/dt-bindings b/arch/microblaze/dts/include/dt-bindings new file mode 120000 index 0000000..0cecb3d --- /dev/null +++ b/arch/microblaze/dts/include/dt-bindings @@ -0,0 +1 @@ +../../../../include/dt-bindings \ No newline at end of file diff --git a/arch/sandbox/dts/include/dt-bindings b/arch/sandbox/dts/include/dt-bindings new file mode 120000 index 0000000..0cecb3d --- /dev/null +++ b/arch/sandbox/dts/include/dt-bindings @@ -0,0 +1 @@ +../../../../include/dt-bindings \ No newline at end of file diff --git a/arch/x86/dts/include/dt-bindings b/arch/x86/dts/include/dt-bindings new file mode 120000 index 0000000..0cecb3d --- /dev/null +++ b/arch/x86/dts/include/dt-bindings @@ -0,0 +1 @@ +../../../../include/dt-bindings \ No newline at end of file diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index a04439d..722afed 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -153,6 +153,7 @@ ld_flags = $(LDFLAGS) $(ldflags-y) # Modified for U-Boot dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc \ -I$(srctree)/arch/$(ARCH)/dts \ + -I$(srctree)/arch/$(ARCH)/dts/include \ -undef -D__DTS__
# Finds the multi-part object the current object will be linked into
diff --git a/arch/arm/dts/include/dt-bindings b/arch/arm/dts/include/dt-bindings new file mode 120000 index 0000000..0cecb3d --- /dev/null +++ b/arch/arm/dts/include/dt-bindings @@ -0,0 +1 @@ +../../../../include/dt-bindings \ No newline at end of file
Hmmm... is this right?
jdl
Hi Jon,
On 6 June 2014 14:29, Jon Loeliger loeliger@gmail.com wrote:
diff --git a/arch/arm/dts/include/dt-bindings b/arch/arm/dts/include/dt-bindings new file mode 120000 index 0000000..0cecb3d --- /dev/null +++ b/arch/arm/dts/include/dt-bindings @@ -0,0 +1 @@ +../../../../include/dt-bindings \ No newline at end of file
Hmmm... is this right?
It is supposed to come through as a symlink.
Regards, Simon
It is best to avoid having any occurence of 'struct device' in driver model, so rename to achieve this.
- Add new patch to rename struct device_id to udevice_id
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
doc/driver-model/README.txt | 2 +- drivers/core/lists.c | 2 +- drivers/demo/demo-shape.c | 2 +- drivers/demo/demo-simple.c | 2 +- drivers/gpio/sandbox.c | 2 +- include/dm/device.h | 6 +++--- test/dm/test-fdt.c | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt index dcecb9a..4af89b0 100644 --- a/doc/driver-model/README.txt +++ b/doc/driver-model/README.txt @@ -315,7 +315,7 @@ is little or no 'driver model' code to write. - Moved some data from code into data structure - e.g. store a pointer to the driver operations structure in the driver, rather than passing it to the driver bind function. -- Rename some structures to make them more similar to Linux (struct device +- Rename some structures to make them more similar to Linux (struct udevice instead of struct instance, struct platdata, etc.) - Change the name 'core' to 'uclass', meaning U-Boot class. It seems that this concept relates to a class of drivers (or a subsystem). We shouldn't diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 205b140..9f2917f 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -94,7 +94,7 @@ int lists_bind_drivers(struct udevice *parent) * tree error */ static int driver_check_compatible(const void *blob, int offset, - const struct device_id *of_match) + const struct udevice_id *of_match) { int ret;
diff --git a/drivers/demo/demo-shape.c b/drivers/demo/demo-shape.c index a68cc10..3fa9c59 100644 --- a/drivers/demo/demo-shape.c +++ b/drivers/demo/demo-shape.c @@ -111,7 +111,7 @@ static int shape_ofdata_to_platdata(struct udevice *dev) return 0; }
-static const struct device_id demo_shape_id[] = { +static const struct udevice_id demo_shape_id[] = { { "demo-shape", 0 }, { }, }; diff --git a/drivers/demo/demo-simple.c b/drivers/demo/demo-simple.c index 11def86..2bcb7df 100644 --- a/drivers/demo/demo-simple.c +++ b/drivers/demo/demo-simple.c @@ -32,7 +32,7 @@ static int demo_shape_ofdata_to_platdata(struct udevice *dev) return demo_parse_dt(dev); }
-static const struct device_id demo_shape_id[] = { +static const struct udevice_id demo_shape_id[] = { { "demo-simple", 0 }, { }, }; diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c index 09cebe2..75ada5d 100644 --- a/drivers/gpio/sandbox.c +++ b/drivers/gpio/sandbox.c @@ -239,7 +239,7 @@ static int gpio_sandbox_probe(struct udevice *dev) return 0; }
-static const struct device_id sandbox_gpio_ids[] = { +static const struct udevice_id sandbox_gpio_ids[] = { { .compatible = "sandbox,gpio" }, { } }; diff --git a/include/dm/device.h b/include/dm/device.h index ec04982..19f2039 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -75,11 +75,11 @@ struct udevice { #define device_active(dev) ((dev)->flags & DM_FLAG_ACTIVATED)
/** - * struct device_id - Lists the compatible strings supported by a driver + * struct udevice_id - Lists the compatible strings supported by a driver * @compatible: Compatible string * @data: Data for this compatible string */ -struct device_id { +struct udevice_id { const char *compatible; ulong data; }; @@ -121,7 +121,7 @@ struct device_id { struct driver { char *name; enum uclass_id id; - const struct device_id *of_match; + const struct udevice_id *of_match; int (*bind)(struct udevice *dev); int (*probe)(struct udevice *dev); int (*remove)(struct udevice *dev); diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 6eccf11..98e3936 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -53,7 +53,7 @@ static int testfdt_drv_probe(struct udevice *dev) return 0; }
-static const struct device_id testfdt_ids[] = { +static const struct udevice_id testfdt_ids[] = { { .compatible = "denx,u-boot-fdt-test", .data = DM_TEST_TYPE_FIRST },
Didn't this hit upstream master already? 54c5d08a09e631f88738db54c75395c6457c2157
Or is this subsequent cleanup for straglers in this branch?
Thanks, jdl
On Fri, Jun 6, 2014 at 2:13 PM, Simon Glass sjg@chromium.org wrote:
It is best to avoid having any occurence of 'struct device' in driver model, so rename to achieve this.
- Add new patch to rename struct device_id to udevice_id
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None Changes in v3: None Changes in v2: None
doc/driver-model/README.txt | 2 +- drivers/core/lists.c | 2 +- drivers/demo/demo-shape.c | 2 +- drivers/demo/demo-simple.c | 2 +- drivers/gpio/sandbox.c | 2 +- include/dm/device.h | 6 +++--- test/dm/test-fdt.c | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt index dcecb9a..4af89b0 100644 --- a/doc/driver-model/README.txt +++ b/doc/driver-model/README.txt @@ -315,7 +315,7 @@ is little or no 'driver model' code to write.
- Moved some data from code into data structure - e.g. store a pointer to
the driver operations structure in the driver, rather than passing it to the driver bind function. -- Rename some structures to make them more similar to Linux (struct device +- Rename some structures to make them more similar to Linux (struct udevice instead of struct instance, struct platdata, etc.)
- Change the name 'core' to 'uclass', meaning U-Boot class. It seems that
this concept relates to a class of drivers (or a subsystem). We shouldn't diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 205b140..9f2917f 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -94,7 +94,7 @@ int lists_bind_drivers(struct udevice *parent)
- tree error
*/ static int driver_check_compatible(const void *blob, int offset,
const struct device_id *of_match)
const struct udevice_id *of_match){ int ret;
diff --git a/drivers/demo/demo-shape.c b/drivers/demo/demo-shape.c index a68cc10..3fa9c59 100644 --- a/drivers/demo/demo-shape.c +++ b/drivers/demo/demo-shape.c @@ -111,7 +111,7 @@ static int shape_ofdata_to_platdata(struct udevice *dev) return 0; }
-static const struct device_id demo_shape_id[] = { +static const struct udevice_id demo_shape_id[] = { { "demo-shape", 0 }, { }, }; diff --git a/drivers/demo/demo-simple.c b/drivers/demo/demo-simple.c index 11def86..2bcb7df 100644 --- a/drivers/demo/demo-simple.c +++ b/drivers/demo/demo-simple.c @@ -32,7 +32,7 @@ static int demo_shape_ofdata_to_platdata(struct udevice *dev) return demo_parse_dt(dev); }
-static const struct device_id demo_shape_id[] = { +static const struct udevice_id demo_shape_id[] = { { "demo-simple", 0 }, { }, }; diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c index 09cebe2..75ada5d 100644 --- a/drivers/gpio/sandbox.c +++ b/drivers/gpio/sandbox.c @@ -239,7 +239,7 @@ static int gpio_sandbox_probe(struct udevice *dev) return 0; }
-static const struct device_id sandbox_gpio_ids[] = { +static const struct udevice_id sandbox_gpio_ids[] = { { .compatible = "sandbox,gpio" }, { } }; diff --git a/include/dm/device.h b/include/dm/device.h index ec04982..19f2039 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -75,11 +75,11 @@ struct udevice { #define device_active(dev) ((dev)->flags & DM_FLAG_ACTIVATED)
/**
- struct device_id - Lists the compatible strings supported by a driver
*/
- struct udevice_id - Lists the compatible strings supported by a driver
- @compatible: Compatible string
- @data: Data for this compatible string
-struct device_id { +struct udevice_id { const char *compatible; ulong data; }; @@ -121,7 +121,7 @@ struct device_id { struct driver { char *name; enum uclass_id id;
const struct device_id *of_match;
const struct udevice_id *of_match; int (*bind)(struct udevice *dev); int (*probe)(struct udevice *dev); int (*remove)(struct udevice *dev);diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 6eccf11..98e3936 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -53,7 +53,7 @@ static int testfdt_drv_probe(struct udevice *dev) return 0; }
-static const struct device_id testfdt_ids[] = { +static const struct udevice_id testfdt_ids[] = { { .compatible = "denx,u-boot-fdt-test", .data = DM_TEST_TYPE_FIRST }, -- 2.0.0.526.g5318336
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Gah. Sorry about that top-post.
I hate gmail's collapsed stupid-shit representation.
Sorry.
Hi Jon,
On 6 June 2014 14:26, Jon Loeliger loeliger@gmail.com wrote:
Didn't this hit upstream master already? 54c5d08a09e631f88738db54c75395c6457c2157
Or is this subsequent cleanup for straglers in this branch?
Thanks, jdl
This is a subsequent clean-up, just to keep things consistent.
Regards, Simon
Add a note to encourage people to convert drivers to use driver model.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: - Update README to encourage conversion to driver model
README | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/README b/README index a53f69a..c7b911b 100644 --- a/README +++ b/README @@ -5310,6 +5310,11 @@ Information structure as we define in include/asm-<arch>/u-boot.h, and make sure that your definition of IMAP_ADDR uses the same value as your U-Boot configuration in CONFIG_SYS_IMMR.
+Note that U-Boot now has a driver model, a unified model for drivers. +If you are adding a new driver, plumb it into driver model. If there +is no uclass available, you are encouraged to create one. See +doc/driver-model. +
Configuring the Linux kernel: -----------------------------
We want 'N0' and 'n0' to mean the same thing, so ensure that case is not considered when naming GPIO banks.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: - Add new patch to use case-insensitive comparison for GPIO banks
drivers/gpio/gpio-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index fa2c2fb..f1bbc58 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -58,7 +58,7 @@ int gpio_lookup_name(const char *name, struct udevice **devp, uc_priv = dev->uclass_priv; len = uc_priv->bank_name ? strlen(uc_priv->bank_name) : 0;
- if (!strncmp(name, uc_priv->bank_name, len)) { + if (!strncasecmp(name, uc_priv->bank_name, len)) { if (strict_strtoul(name + len, 10, &offset)) continue; if (devp)
These files don't compile in some architectures. Fix it by adding the missing headers.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: - Add new patch to add missing header files in lists and root
drivers/core/lists.c | 1 + drivers/core/root.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 9f2917f..afb59d1 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -14,6 +14,7 @@ #include <dm/platdata.h> #include <dm/uclass.h> #include <dm/util.h> +#include <fdtdec.h> #include <linux/compiler.h>
struct driver *lists_driver_lookup_name(const char *name) diff --git a/drivers/core/root.c b/drivers/core/root.c index 4977875..f31be72 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -10,6 +10,7 @@ #include <common.h> #include <errno.h> #include <malloc.h> +#include <libfdt.h> #include <dm/device.h> #include <dm/device-internal.h> #include <dm/lists.h>
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: - Fix typo in commit subject
Changes in v2: - Add new patch to deal with const-ness of the global_data pointer
drivers/core/root.c | 6 +++--- drivers/core/uclass.c | 2 +- include/dm/device-internal.h | 4 ++++ 3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/core/root.c b/drivers/core/root.c index f31be72..70ea97f 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -43,9 +43,9 @@ int dm_init(void) dm_warn("Virtual root driver already exists!\n"); return -EINVAL; } - INIT_LIST_HEAD(&gd->uclass_root); + INIT_LIST_HEAD(&DM_UCLASS_ROOT());
- ret = device_bind_by_name(NULL, &root_info, &gd->dm_root); + ret = device_bind_by_name(NULL, &root_info, &DM_ROOT()); if (ret) return ret;
@@ -56,7 +56,7 @@ int dm_scan_platdata(void) { int ret;
- ret = lists_bind_drivers(gd->dm_root); + ret = lists_bind_drivers(DM_ROOT()); if (ret == -ENOENT) { dm_warn("Some drivers were not found\n"); ret = 0; diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index f6867e4..d16d6fc 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -75,7 +75,7 @@ static int uclass_add(enum uclass_id id, struct uclass **ucp) uc->uc_drv = uc_drv; INIT_LIST_HEAD(&uc->sibling_node); INIT_LIST_HEAD(&uc->dev_head); - list_add(&uc->sibling_node, &gd->uclass_root); + list_add(&uc->sibling_node, &DM_UCLASS_ROOT());
if (uc_drv->init) { ret = uc_drv->init(uc); diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index ea3df36..67318fa 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -84,4 +84,8 @@ int device_remove(struct udevice *dev); */ int device_unbind(struct udevice *dev);
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root) + #endif
On Friday, June 06, 2014 at 09:13:26 PM, Simon Glass wrote:
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org
[...]
- ret = device_bind_by_name(NULL, &root_info, &gd->dm_root);
- ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());
[...]
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
Can you implement this "DM_ROOT()" macro as a function instead ? I believe that'd be much nicer , would have typechecking etc., usual stuff.
Best regards, Marek Vasut
Hi Marek,
On 8 June 2014 01:00, Marek Vasut marex@denx.de wrote:
On Friday, June 06, 2014 at 09:13:26 PM, Simon Glass wrote:
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org
[...]
ret = device_bind_by_name(NULL, &root_info, &gd->dm_root);
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());[...]
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
Can you implement this "DM_ROOT()" macro as a function instead ? I believe that'd be much nicer , would have typechecking etc., usual stuff.
I had a look at this. I don't see how I can do it, but you might have ideas.
In this function call I need to pass a pointer without its const bit. I made it const since nothing should change the dm root except driver model itself.
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());
In the above line, I don't think I can make it a function. Similarly one good think about the function is that I can grep for it easily.
So I'm thinking of using something like this:
/* Cast away any volatile pointer */ #define DM_ROOT_NON_CONST (((gd_t *)gd)->dm_root) #define DM_UCLASS_ROOT_NON_CONST (((gd_t *)gd)->uclass_root)
It is no-longer a function, but I can easily find it. Plus it is explicit as to what it is for, with the NON_CONST suffix.
Regards, Simon
Hi Marek,
On 11 June 2014 23:26, Simon Glass sjg@chromium.org wrote:
Hi Marek,
On 8 June 2014 01:00, Marek Vasut marex@denx.de wrote:
On Friday, June 06, 2014 at 09:13:26 PM, Simon Glass wrote:
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org
[...]
ret = device_bind_by_name(NULL, &root_info, &gd->dm_root);
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());[...]
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
Can you implement this "DM_ROOT()" macro as a function instead ? I believe that'd be much nicer , would have typechecking etc., usual stuff.
I had a look at this. I don't see how I can do it, but you might have ideas.
In this function call I need to pass a pointer without its const bit. I made it const since nothing should change the dm root except driver model itself.
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());In the above line, I don't think I can make it a function. Similarly one good think about the function is that I can grep for it easily.
So I'm thinking of using something like this:
/* Cast away any volatile pointer */ #define DM_ROOT_NON_CONST (((gd_t *)gd)->dm_root) #define DM_UCLASS_ROOT_NON_CONST (((gd_t *)gd)->uclass_root)
It is no-longer a function, but I can easily find it. Plus it is explicit as to what it is for, with the NON_CONST suffix.
Also I forgot to mention that type-checking is pretty pointless since it is a hard-coded variable. If someone changed it lots of other code would break. Also this is an internal dm header file so cannot be accessed outside driver model core.
Regards, Simon
On Thursday, June 12, 2014 at 05:26:50 AM, Simon Glass wrote:
Hi Marek,
On 8 June 2014 01:00, Marek Vasut marex@denx.de wrote:
On Friday, June 06, 2014 at 09:13:26 PM, Simon Glass wrote:
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org
[...]
ret = device_bind_by_name(NULL, &root_info, &gd->dm_root);
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());[...]
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
Can you implement this "DM_ROOT()" macro as a function instead ? I believe that'd be much nicer , would have typechecking etc., usual stuff.
I had a look at this. I don't see how I can do it, but you might have ideas.
In this function call I need to pass a pointer without its const bit. I made it const since nothing should change the dm root except driver model itself.
All right, I will just be doing a little guesswork here. Shall you not have some functions to change the DM root then instead of exposing the DM root ? Such functions shall not be exported publicly , but available via separate header file too ?
Sorry, I might be completely wrong.
Best regards, Marek Vasut
Hi Marek,
On 17 June 2014 16:42, Marek Vasut marex@denx.de wrote:
On Thursday, June 12, 2014 at 05:26:50 AM, Simon Glass wrote:
Hi Marek,
On 8 June 2014 01:00, Marek Vasut marex@denx.de wrote:
On Friday, June 06, 2014 at 09:13:26 PM, Simon Glass wrote:
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org
[...]
ret = device_bind_by_name(NULL, &root_info, &gd->dm_root);
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());[...]
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
Can you implement this "DM_ROOT()" macro as a function instead ? I believe that'd be much nicer , would have typechecking etc., usual stuff.
I had a look at this. I don't see how I can do it, but you might have ideas.
In this function call I need to pass a pointer without its const bit. I made it const since nothing should change the dm root except driver model itself.
All right, I will just be doing a little guesswork here. Shall you not have some functions to change the DM root then instead of exposing the DM root ? Such functions shall not be exported publicly , but available via separate header file too ?
Firstly I should retitle the commit - it is of course the volatile that I'm trying to get rid of as it says in the commit message. I might have just been trying to confuse everyone.
I have code like this:
INIT_LIST_HEAD(&gd->uclass_root);
I don't see how I can turn this into a function. I suppose I could init the list head in a temporary variable and then assign it to gd->uclass_root, perhaps with memcpy(), but that doesn't really seem any better to me.
Sorry, I might be completely wrong.
Well I'm not saying it's pretty, so if you have time and come up with a better option please let me know.
Also, this is not exported 'publicly'. It is in device-internal.h
Regards, Simon
Hi Marek,
On 17 June 2014 23:36, Simon Glass sjg@chromium.org wrote:
Hi Marek,
On 17 June 2014 16:42, Marek Vasut marex@denx.de wrote:
On Thursday, June 12, 2014 at 05:26:50 AM, Simon Glass wrote:
Hi Marek,
On 8 June 2014 01:00, Marek Vasut marex@denx.de wrote:
On Friday, June 06, 2014 at 09:13:26 PM, Simon Glass wrote:
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org
[...]
ret = device_bind_by_name(NULL, &root_info, &gd->dm_root);
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());[...]
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
Can you implement this "DM_ROOT()" macro as a function instead ? I believe that'd be much nicer , would have typechecking etc., usual stuff.
I had a look at this. I don't see how I can do it, but you might have ideas.
In this function call I need to pass a pointer without its const bit. I made it const since nothing should change the dm root except driver model itself.
All right, I will just be doing a little guesswork here. Shall you not have some functions to change the DM root then instead of exposing the DM root ? Such functions shall not be exported publicly , but available via separate header file too ?
Firstly I should retitle the commit - it is of course the volatile that I'm trying to get rid of as it says in the commit message. I might have just been trying to confuse everyone.
I have code like this:
INIT_LIST_HEAD(&gd->uclass_root);
I don't see how I can turn this into a function. I suppose I could init the list head in a temporary variable and then assign it to gd->uclass_root, perhaps with memcpy(), but that doesn't really seem any better to me.
Sorry, I might be completely wrong.
Well I'm not saying it's pretty, so if you have time and come up with a better option please let me know.
Also, this is not exported 'publicly'. It is in device-internal.h
Please let me know on this one. If you have a better option please let me know.
Otherwise I'd like to do a dm pull request soon to get these fixes into the release.
Regards, Simon
On Friday, June 20, 2014 at 06:07:02 AM, Simon Glass wrote:
Hi Marek,
On 17 June 2014 23:36, Simon Glass sjg@chromium.org wrote:
Hi Marek,
On 17 June 2014 16:42, Marek Vasut marex@denx.de wrote:
On Thursday, June 12, 2014 at 05:26:50 AM, Simon Glass wrote:
Hi Marek,
On 8 June 2014 01:00, Marek Vasut marex@denx.de wrote:
On Friday, June 06, 2014 at 09:13:26 PM, Simon Glass wrote:
In a very few cases we need to adjust the driver model root device, such as when setting it up at initialisation. Add a macro to make this easier.
Signed-off-by: Simon Glass sjg@chromium.org
[...]
ret = device_bind_by_name(NULL, &root_info, &gd->dm_root);
ret = device_bind_by_name(NULL, &root_info, &DM_ROOT());[...]
+/* Cast away any volatile pointer */ +#define DM_ROOT() (((gd_t *)gd)->dm_root) +#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
Can you implement this "DM_ROOT()" macro as a function instead ? I believe that'd be much nicer , would have typechecking etc., usual stuff.
I had a look at this. I don't see how I can do it, but you might have ideas.
In this function call I need to pass a pointer without its const bit. I made it const since nothing should change the dm root except driver model itself.
All right, I will just be doing a little guesswork here. Shall you not have some functions to change the DM root then instead of exposing the DM root ? Such functions shall not be exported publicly , but available via separate header file too ?
Firstly I should retitle the commit - it is of course the volatile that I'm trying to get rid of as it says in the commit message. I might have just been trying to confuse everyone.
I have code like this:
INIT_LIST_HEAD(&gd->uclass_root);
I don't see how I can turn this into a function. I suppose I could init the list head in a temporary variable and then assign it to gd->uclass_root, perhaps with memcpy(), but that doesn't really seem any better to me.
Sorry, I might be completely wrong.
Well I'm not saying it's pretty, so if you have time and come up with a better option please let me know.
Also, this is not exported 'publicly'. It is in device-internal.h
Please let me know on this one. If you have a better option please let me know.
Otherwise I'd like to do a dm pull request soon to get these fixes into the release.
Ah, please get the PR out. We can tweak this later I'm sure. Apologies for holding you and the PR.
Hi Marek,
On 19 June 2014 22:12, Marek Vasut marex@denx.de wrote:
Otherwise I'd like to do a dm pull request soon to get these fixes into the release.
Ah, please get the PR out. We can tweak this later I'm sure. Apologies for holding you and the PR.
It's OK it is has only been a week and I wondered to leave the patches out there for that long. Thanks.
Applied to u-boot-dm/master.
Regards, Simon
On Friday, June 20, 2014 at 10:34:20 PM, Simon Glass wrote:
Hi Marek,
On 19 June 2014 22:12, Marek Vasut marex@denx.de wrote:
Otherwise I'd like to do a dm pull request soon to get these fixes into the release.
Ah, please get the PR out. We can tweak this later I'm sure. Apologies for holding you and the PR.
It's OK it is has only been a week and I wondered to leave the patches out there for that long. Thanks.
Applied to u-boot-dm/master.
I should start looking into u-boot-dm/master again I think. Thank you!
Best regards, Marek Vasut
The GPIO tests require the sandbox GPIO driver, so cannot be run on other platforms. Similarly for the 'dm test' command.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: - Add new patch to allow driver model tests only for sandbox
test/dm/Makefile | 2 ++ test/dm/cmd_dm.c | 11 +++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/test/dm/Makefile b/test/dm/Makefile index 4e9afe6..c0f2135 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -15,4 +15,6 @@ obj-$(CONFIG_DM_TEST) += ut.o # subsystem you must add sandbox tests here. obj-$(CONFIG_DM_TEST) += core.o obj-$(CONFIG_DM_TEST) += ut.o +ifneq ($(CONFIG_SANDBOX),) obj-$(CONFIG_DM_GPIO) += gpio.o +endif diff --git a/test/dm/cmd_dm.c b/test/dm/cmd_dm.c index 083f15c..180661b 100644 --- a/test/dm/cmd_dm.c +++ b/test/dm/cmd_dm.c @@ -93,16 +93,23 @@ static int do_dm_dump_uclass(cmd_tbl_t *cmdtp, int flag, int argc, return 0; }
+#ifdef CONFIG_DM_TEST static int do_dm_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { return dm_test_main(); } +#define TEST_HELP "\ndm test Run tests" +#else +#define TEST_HELP +#endif
static cmd_tbl_t test_commands[] = { U_BOOT_CMD_MKENT(tree, 0, 1, do_dm_dump_all, "", ""), U_BOOT_CMD_MKENT(uclass, 1, 1, do_dm_dump_uclass, "", ""), +#ifdef CONFIG_DM_TEST U_BOOT_CMD_MKENT(test, 1, 1, do_dm_test, "", ""), +#endif };
static int do_dm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -128,6 +135,6 @@ U_BOOT_CMD( dm, 2, 1, do_dm, "Driver model low level access", "tree Dump driver model tree\n" - "dm uclass Dump list of instances for each uclass\n" - "dm test Run tests" + "dm uclass Dump list of instances for each uclass" + TEST_HELP );
The values here are int, but the map_to_sysmem() call can return a long. Add a cast to deal with this.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: - Add new patch to fix printf() strings in the 'dm' command
test/dm/cmd_dm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/test/dm/cmd_dm.c b/test/dm/cmd_dm.c index 180661b..8019407 100644 --- a/test/dm/cmd_dm.c +++ b/test/dm/cmd_dm.c @@ -23,7 +23,7 @@ static int display_succ(struct udevice *in, char *buf) char local[16]; struct udevice *pos, *n, *prev = NULL;
- printf("%s- %s @ %08x", buf, in->name, map_to_sysmem(in)); + printf("%s- %s @ %08x", buf, in->name, (uint)map_to_sysmem(in)); if (in->flags & DM_FLAG_ACTIVATED) puts(" - activated"); puts("\n"); @@ -62,7 +62,7 @@ static int do_dm_dump_all(cmd_tbl_t *cmdtp, int flag, int argc, struct udevice *root;
root = dm_root(); - printf("ROOT %08x\n", map_to_sysmem(root)); + printf("ROOT %08x\n", (uint)map_to_sysmem(root)); return dm_dump(root); }
@@ -84,8 +84,8 @@ static int do_dm_dump_uclass(cmd_tbl_t *cmdtp, int flag, int argc, for (ret = uclass_first_device(id, &dev); dev; ret = uclass_next_device(&dev)) { - printf(" %s @ %08x:\n", dev->name, - map_to_sysmem(dev)); + printf(" %s @ %08x:\n", dev->name, + (uint)map_to_sysmem(dev)); } puts("\n"); }
On 7 June 2014 07:13, Simon Glass sjg@chromium.org wrote:
The values here are int, but the map_to_sysmem() call can return a long. Add a cast to deal with this.
...
printf("%s- %s @ %08x", buf, in->name, map_to_sysmem(in));
printf("%s- %s @ %08x", buf, in->name, (uint)map_to_sysmem(in));
If the argument is a long, shouldn't it be printed as long, rather than possibly truncated to an int? printf("%s- %s @ %08lx", buf, in->name, map_to_sysmem(in));
Regards, Andre
Hi Andre,
On 8 June 2014 18:46, Andre Renaud andre@bluewatersys.com wrote:
On 7 June 2014 07:13, Simon Glass sjg@chromium.org wrote:
The values here are int, but the map_to_sysmem() call can return a long. Add a cast to deal with this.
...
printf("%s- %s @ %08x", buf, in->name, map_to_sysmem(in));
printf("%s- %s @ %08x", buf, in->name, (uint)map_to_sysmem(in));If the argument is a long, shouldn't it be printed as long, rather than possibly truncated to an int? printf("%s- %s @ %08lx", buf, in->name, map_to_sysmem(in));
I found that this happened on ARM, where sizeof(int) == sizeof(long) and was conscious that all addresses in U-Boot are 32-bits at present - hmmm is that still true?.
But yes that is more correct. I will change it.
Regards, Simon
These files are taken from Linux 3.14.
Signed-off-by: Simon Glass sjg@chromium.org Acked-by: Stephen Warren swarren@nvidia.com ---
Changes in v4: - Remove 64-bit addresses which are not used in U-Boot
Changes in v3: - Bring in GPIO bindings for tegra{30,114,124} also
Changes in v2: - Add new patch to bring in Tegra device tree files from linux
arch/arm/dts/tegra114.dtsi | 21 +++++---- arch/arm/dts/tegra124.dtsi | 19 ++++---- arch/arm/dts/tegra20.dtsi | 15 ++++++- arch/arm/dts/tegra30.dtsi | 21 +++++---- include/dt-bindings/gpio/gpio.h | 15 +++++++ include/dt-bindings/gpio/tegra-gpio.h | 51 ++++++++++++++++++++++ include/dt-bindings/interrupt-controller/arm-gic.h | 22 ++++++++++ include/dt-bindings/interrupt-controller/irq.h | 19 ++++++++ 8 files changed, 155 insertions(+), 28 deletions(-) create mode 100644 include/dt-bindings/gpio/gpio.h create mode 100644 include/dt-bindings/gpio/tegra-gpio.h create mode 100644 include/dt-bindings/interrupt-controller/arm-gic.h create mode 100644 include/dt-bindings/interrupt-controller/irq.h
diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi index f52fcf1..59434e0 100644 --- a/arch/arm/dts/tegra114.dtsi +++ b/arch/arm/dts/tegra114.dtsi @@ -1,3 +1,6 @@ +#include <dt-bindings/gpio/tegra-gpio.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> + #include "skeleton.dtsi"
/ { @@ -46,17 +49,17 @@ 0 143 0x04>; };
- gpio: gpio { + gpio: gpio@6000d000 { compatible = "nvidia,tegra114-gpio", "nvidia,tegra30-gpio"; reg = <0x6000d000 0x1000>; - interrupts = <0 32 0x04 - 0 33 0x04 - 0 34 0x04 - 0 35 0x04 - 0 55 0x04 - 0 87 0x04 - 0 89 0x04 - 0 125 0x04>; + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>; #gpio-cells = <2>; gpio-controller; #interrupt-cells = <2>; diff --git a/arch/arm/dts/tegra124.dtsi b/arch/arm/dts/tegra124.dtsi index 18a8b24..4561c5f 100644 --- a/arch/arm/dts/tegra124.dtsi +++ b/arch/arm/dts/tegra124.dtsi @@ -1,3 +1,6 @@ +#include <dt-bindings/gpio/tegra-gpio.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> + #include "skeleton.dtsi"
/ { @@ -49,14 +52,14 @@ gpio: gpio@6000d000 { compatible = "nvidia,tegra124-gpio", "nvidia,tegra30-gpio"; reg = <0x6000d000 0x1000>; - interrupts = <0 32 0x04 - 0 33 0x04 - 0 34 0x04 - 0 35 0x04 - 0 55 0x04 - 0 87 0x04 - 0 89 0x04 - 0 125 0x04>; + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>; #gpio-cells = <2>; gpio-controller; #interrupt-cells = <2>; diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi index 3805750..a524f6e 100644 --- a/arch/arm/dts/tegra20.dtsi +++ b/arch/arm/dts/tegra20.dtsi @@ -1,3 +1,6 @@ +#include <dt-bindings/gpio/tegra-gpio.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> + #include "skeleton.dtsi"
/ { @@ -139,10 +142,18 @@
gpio: gpio@6000d000 { compatible = "nvidia,tegra20-gpio"; - reg = < 0x6000d000 0x1000 >; - interrupts = < 64 65 66 67 87 119 121 >; + reg = <0x6000d000 0x1000>; + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; #gpio-cells = <2>; gpio-controller; + #interrupt-cells = <2>; + interrupt-controller; };
pinmux: pinmux@70000000 { diff --git a/arch/arm/dts/tegra30.dtsi b/arch/arm/dts/tegra30.dtsi index fee1c36..7be3791 100644 --- a/arch/arm/dts/tegra30.dtsi +++ b/arch/arm/dts/tegra30.dtsi @@ -1,3 +1,6 @@ +#include <dt-bindings/gpio/tegra-gpio.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> + #include "skeleton.dtsi"
/ { @@ -47,17 +50,17 @@ clocks = <&tegra_car 34>; };
- gpio: gpio { + gpio: gpio@6000d000 { compatible = "nvidia,tegra30-gpio"; reg = <0x6000d000 0x1000>; - interrupts = <0 32 0x04 - 0 33 0x04 - 0 34 0x04 - 0 35 0x04 - 0 55 0x04 - 0 87 0x04 - 0 89 0x04 - 0 125 0x04>; + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>; #gpio-cells = <2>; gpio-controller; #interrupt-cells = <2>; diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h new file mode 100644 index 0000000..e6b1e0a --- /dev/null +++ b/include/dt-bindings/gpio/gpio.h @@ -0,0 +1,15 @@ +/* + * This header provides constants for most GPIO bindings. + * + * Most GPIO bindings include a flags cell as part of the GPIO specifier. + * In most cases, the format of the flags cell uses the standard values + * defined in this header. + */ + +#ifndef _DT_BINDINGS_GPIO_GPIO_H +#define _DT_BINDINGS_GPIO_GPIO_H + +#define GPIO_ACTIVE_HIGH 0 +#define GPIO_ACTIVE_LOW 1 + +#endif diff --git a/include/dt-bindings/gpio/tegra-gpio.h b/include/dt-bindings/gpio/tegra-gpio.h new file mode 100644 index 0000000..197dc28 --- /dev/null +++ b/include/dt-bindings/gpio/tegra-gpio.h @@ -0,0 +1,51 @@ +/* + * This header provides constants for binding nvidia,tegra*-gpio. + * + * The first cell in Tegra's GPIO specifier is the GPIO ID. The macros below + * provide names for this. + * + * The second cell contains standard flag values specified in gpio.h. + */ + +#ifndef _DT_BINDINGS_GPIO_TEGRA_GPIO_H +#define _DT_BINDINGS_GPIO_TEGRA_GPIO_H + +#include <dt-bindings/gpio/gpio.h> + +#define TEGRA_GPIO_BANK_ID_A 0 +#define TEGRA_GPIO_BANK_ID_B 1 +#define TEGRA_GPIO_BANK_ID_C 2 +#define TEGRA_GPIO_BANK_ID_D 3 +#define TEGRA_GPIO_BANK_ID_E 4 +#define TEGRA_GPIO_BANK_ID_F 5 +#define TEGRA_GPIO_BANK_ID_G 6 +#define TEGRA_GPIO_BANK_ID_H 7 +#define TEGRA_GPIO_BANK_ID_I 8 +#define TEGRA_GPIO_BANK_ID_J 9 +#define TEGRA_GPIO_BANK_ID_K 10 +#define TEGRA_GPIO_BANK_ID_L 11 +#define TEGRA_GPIO_BANK_ID_M 12 +#define TEGRA_GPIO_BANK_ID_N 13 +#define TEGRA_GPIO_BANK_ID_O 14 +#define TEGRA_GPIO_BANK_ID_P 15 +#define TEGRA_GPIO_BANK_ID_Q 16 +#define TEGRA_GPIO_BANK_ID_R 17 +#define TEGRA_GPIO_BANK_ID_S 18 +#define TEGRA_GPIO_BANK_ID_T 19 +#define TEGRA_GPIO_BANK_ID_U 20 +#define TEGRA_GPIO_BANK_ID_V 21 +#define TEGRA_GPIO_BANK_ID_W 22 +#define TEGRA_GPIO_BANK_ID_X 23 +#define TEGRA_GPIO_BANK_ID_Y 24 +#define TEGRA_GPIO_BANK_ID_Z 25 +#define TEGRA_GPIO_BANK_ID_AA 26 +#define TEGRA_GPIO_BANK_ID_BB 27 +#define TEGRA_GPIO_BANK_ID_CC 28 +#define TEGRA_GPIO_BANK_ID_DD 29 +#define TEGRA_GPIO_BANK_ID_EE 30 +#define TEGRA_GPIO_BANK_ID_FF 31 + +#define TEGRA_GPIO(bank, offset) \ + ((TEGRA_GPIO_BANK_ID_##bank * 8) + offset) + +#endif diff --git a/include/dt-bindings/interrupt-controller/arm-gic.h b/include/dt-bindings/interrupt-controller/arm-gic.h new file mode 100644 index 0000000..1ea1b70 --- /dev/null +++ b/include/dt-bindings/interrupt-controller/arm-gic.h @@ -0,0 +1,22 @@ +/* + * This header provides constants for the ARM GIC. + */ + +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H + +#include <dt-bindings/interrupt-controller/irq.h> + +/* interrupt specific cell 0 */ + +#define GIC_SPI 0 +#define GIC_PPI 1 + +/* + * Interrupt specifier cell 2. + * The flaggs in irq.h are valid, plus those below. + */ +#define GIC_CPU_MASK_RAW(x) ((x) << 8) +#define GIC_CPU_MASK_SIMPLE(num) GIC_CPU_MASK_RAW((1 << (num)) - 1) + +#endif diff --git a/include/dt-bindings/interrupt-controller/irq.h b/include/dt-bindings/interrupt-controller/irq.h new file mode 100644 index 0000000..33a1003 --- /dev/null +++ b/include/dt-bindings/interrupt-controller/irq.h @@ -0,0 +1,19 @@ +/* + * This header provides constants for most IRQ bindings. + * + * Most IRQ bindings include a flags cell as part of the IRQ specifier. + * In most cases, the format of the flags cell uses the standard values + * defined in this header. + */ + +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_IRQ_H +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_IRQ_H + +#define IRQ_TYPE_NONE 0 +#define IRQ_TYPE_EDGE_RISING 1 +#define IRQ_TYPE_EDGE_FALLING 2 +#define IRQ_TYPE_EDGE_BOTH (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING) +#define IRQ_TYPE_LEVEL_HIGH 4 +#define IRQ_TYPE_LEVEL_LOW 8 + +#endif
Enable driver model for Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org Acked-by: Stephen Warren swarren@nvidia.com ---
Changes in v4: None Changes in v3: - Enable dm command in this patch instead of the next
Changes in v2: - Split out a separate patch to enable driver model for tegra
include/configs/tegra-common.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h index 129acf2..3b88a83 100644 --- a/include/configs/tegra-common.h +++ b/include/configs/tegra-common.h @@ -19,6 +19,9 @@
#include <asm/arch/tegra.h> /* get chip and board defs */
+#define CONFIG_DM +#define CONFIG_CMD_DM + #define CONFIG_SYS_TIMER_RATE 1000000 #define CONFIG_SYS_TIMER_COUNTER NV_PA_TMRUS_BASE
There is a spelling mistake and two functions are missing comments altogether. Also the flags declaration is correct, but doesn't follow style. Finally, the uclass_get_device() function has some errors in its documentation.
Fix these problems.
Signed-off-by: Simon Glass sjg@chromium.org Acked-by: Marek Vasut marex@denx.de ---
Changes in v4: None Changes in v3: None Changes in v2: None
include/dm/device.h | 2 +- include/dm/lists.h | 20 ++++++++++++++++++++ include/dm/root.h | 2 +- include/dm/uclass.h | 6 ++++-- 4 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/include/dm/device.h b/include/dm/device.h index 19f2039..ae75a3f 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -21,7 +21,7 @@ struct driver_info; #define DM_FLAG_ACTIVATED (1 << 0)
/* DM is responsible for allocating and freeing platdata */ -#define DM_FLAG_ALLOC_PDATA (2 << 0) +#define DM_FLAG_ALLOC_PDATA (1 << 1)
/** * struct udevice - An instance of a driver diff --git a/include/dm/lists.h b/include/dm/lists.h index 7feba4b..49d87e6 100644 --- a/include/dm/lists.h +++ b/include/dm/lists.h @@ -32,8 +32,28 @@ struct driver *lists_driver_lookup_name(const char *name); */ struct uclass_driver *lists_uclass_lookup(enum uclass_id id);
+/** + * lists_bind_drivers() - search for and bind all drivers to parent + * + * This searches the U_BOOT_DEVICE() structures and creates new devices for + * each one. The devices will have @parent as their parent. + * + * @parent: parent driver (root) + * @early_only: If true, bind only drivers with the DM_INIT_F flag. If false + * bind all drivers. + */ int lists_bind_drivers(struct udevice *parent);
+/** + * lists_bind_fdt() - bind a device tree node + * + * This creates a new device bound to the given device tree node, with + * @parent as its parent. + * + * @parent: parent driver (root) + * @blob: device tree blob + * @offset: offset of this device tree node + */ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset);
#endif diff --git a/include/dm/root.h b/include/dm/root.h index 3018bc8..a4826a6 100644 --- a/include/dm/root.h +++ b/include/dm/root.h @@ -41,7 +41,7 @@ int dm_scan_platdata(void); int dm_scan_fdt(const void *blob);
/** - * dm_init() - Initialize Driver Model structures + * dm_init() - Initialise Driver Model structures * * This function will initialize roots of driver tree and class tree. * This needs to be called before anything uses the DM diff --git a/include/dm/uclass.h b/include/dm/uclass.h index 931d9c0..0df2e5e 100644 --- a/include/dm/uclass.h +++ b/include/dm/uclass.h @@ -96,12 +96,14 @@ int uclass_get(enum uclass_id key, struct uclass **ucp); /** * uclass_get_device() - Get a uclass device based on an ID and index * + * The device is probed to activate it ready for use. + * * id: ID to look up * @index: Device number within that uclass (0=first) - * @ucp: Returns pointer to uclass (there is only one per for each ID) + * devp: Returns pointer to device (there is only one per for each ID) * @return 0 if OK, -ve on error */ -int uclass_get_device(enum uclass_id id, int index, struct udevice **ucp); +int uclass_get_device(enum uclass_id id, int index, struct udevice **devp);
/** * uclass_first_device() - Get the first device in a uclass
On Friday, June 06, 2014 at 09:13:31 PM, Simon Glass wrote:
There is a spelling mistake and two functions are missing comments altogether. Also the flags declaration is correct, but doesn't follow style. Finally, the uclass_get_device() function has some errors in its documentation.
[...]
--- a/include/dm/uclass.h +++ b/include/dm/uclass.h @@ -96,12 +96,14 @@ int uclass_get(enum uclass_id key, struct uclass **ucp); /**
- uclass_get_device() - Get a uclass device based on an ID and index
- The device is probed to activate it ready for use.
- id: ID to look up
- @index: Device number within that uclass (0=first)
- @ucp: Returns pointer to uclass (there is only one per for each ID)
- devp: Returns pointer to device (there is only one per for each ID)
You're missing the leading @ in "devp" here for consistency's sake. [...] Best regards, Marek Vasut
The lifecycle of a device is an important part of driver model. Add to the existing documentation and clarify it.
Reported-by: Jon Loeliger jdl@jdl.com
Signed-off-by: Simon Glass sjg@chromium.org --- Thanks for Jon Loeliger jdl@jdl.com for helping with the text and suggesting improvements.
(Jon please comment/adjust to help clarify things further)
Changes in v4: - Minor spelling fixes
Changes in v3: None Changes in v2: - Rename struct device to struct udevice - Adjust docs as per Jon's review
doc/driver-model/README.txt | 215 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 208 insertions(+), 7 deletions(-)
diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt index 4af89b0..884acef 100644 --- a/doc/driver-model/README.txt +++ b/doc/driver-model/README.txt @@ -222,7 +222,44 @@ device tree) and probe. Platform Data -------------
-Where does the platform data come from? See demo-pdata.c which +Platform data is like Linux platform data, if you are familiar with that. +It provides the board-specific information to start up a device. + +Why is this information not just stored in the device driver itself? The +idea is that the device driver is generic, and can in principle operate on +any board that has that type of device. For example, with modern +highly-complex SoCs it is common for the IP to come from an IP vendor, and +therefore (for example) the MMC controller may be the same on chips from +different vendors. It makes no sense to write independent drivers for the +MMC controller on each vendor's SoC, when they are all almost the same. +Similarly, we may have 6 UARTs in an SoC, all of which are mostly the same, +but lie at different addresses in the address space. + +Using the UART example, we have a single driver and it is instantiated 6 +times by supplying 6 lots of platform data. Each lot of platform data +gives the driver name and a pointer to a structure containing information +about this instance - e.g. the address of the register space. It may be that +one of the UARTS supports RS-485 operation - this can be added as a flag in +the platform data, which is set for this one port and clear for the rest. + +Think of your driver as a generic piece of code which knows how to talk to +a device, but needs to know where it is, any variant/option information and +so on. Platform data provides this link between the generic piece of code +and the specific way it is bound on a particular board. + +Examples of platform data include: + + - The base address of the IP block's register space + - Configuration options, like: + - the SPI polarity and maximum speed for a SPI controller + - the I2C speed to use for an I2C device + - the number of GPIOs available in a GPIO device + +Where does the platform data come from? It is either held in a structure +which is compiled into U-Boot, or it can be parsed from the Device Tree +(see 'Device Tree' below). + +For an example of how it can be compiled in, see demo-pdata.c which sets up a table of driver names and their associated platform data. The data can be interpreted by the drivers however they like - it is basically a communication scheme between the board-specific code and @@ -259,21 +296,30 @@ following device tree fragment: sides = <4>; };
+This means that instead of having lots of U_BOOT_DEVICE() declarations in +the board file, we put these in the device tree. This approach allows a lot +more generality, since the same board file can support many types of boards +(e,g. with the same SoC) just by using different device trees. An added +benefit is that the Linux device tree can be used, thus further simplifying +the task of board-bring up either for U-Boot or Linux devs (whoever gets to +the baord first!).
The easiest way to make this work it to add a few members to the driver:
.platdata_auto_alloc_size = sizeof(struct dm_test_pdata), .ofdata_to_platdata = testfdt_ofdata_to_platdata, - .probe = testfdt_drv_probe,
The 'auto_alloc' feature allowed space for the platdata to be allocated -and zeroed before the driver's ofdata_to_platdata method is called. This -method reads the information out of the device tree and puts it in -dev->platdata. Then the probe method is called to set up the device. +and zeroed before the driver's ofdata_to_platdata() method is called. The +ofdata_to_platdata() method, which the driver write supplies, should parse +the device tree node for this device and place it in dev->platdata. Thus +when the probe method is called later (to set up the device ready for use) +the platform data will be present.
Note that both methods are optional. If you provide an ofdata_to_platdata -method then it wlil be called first (after bind). If you provide a probe -method it will be called next. +method then it wlil be called first (during activation). If you provide a +probe method it will be called next. See Driver Lifecycle below for more +details.
If you don't want to have the platdata automatically allocated then you can leave out platdata_auto_alloc_size. In this case you can use malloc @@ -295,6 +341,161 @@ numbering comes from include/dm/uclass.h. To add a new uclass, add to the end of the enum there, then declare your uclass as above.
+Driver Lifecycle +---------------- + +Here are the stages that a device goes through in driver model. Note that all +methods mentioned here are optional - e.g. if there is no probe() method for +a device then it will not be called. A simple device may have very few +methods actually defined. + +1. Bind stage + +A device and its driver are bound using one of these two methods: + + - Scan the U_BOOT_DEVICE() definitions. U-Boot It looks up the +name specified by each, to find the appropriate driver. It then calls +device_bind() to create a new device and bind' it to its driver. This will +call the device's bind() method. + + - Scan through the device tree definitions. U-Boot looks at top-level +nodes in the the device tree. It looks at the compatible string in each node +and uses the of_match part of the U_BOOT_DRIVER() structure to find the +right driver for each node. It then calls device_bind() to bind the +newly-created device to its driver (thereby creating a device structure). +This will also call the device's bind() method. + +At this point all the devices are known, and bound to their drivers. There +is a 'struct udevice' allocated for all devices. However, nothing has been +activated (except for the root device). Each bound device that was created +from a U_BOOT_DEVICE() declaration will hold the platdata pointer specified +in that declaration. For a bound device created from the device tree, +platdata will be NULL, but of_offset will be the offset of the device tree +node that caused the device to be created. The uclass is set correctly for +the device. + +The device's bind() method is permitted to perform simple actions, but +should not scan the device tree node, not initialise hardware, nor set up +structures or allocate memory. All of these tasks should be left for +the probe() method. + +Note that compared to Linux, U-Boot's driver model has a separate step of +probe/remove which is independent of bind/unbind. This is partly because in +U-Boot it may be expensive to prove devices and we don't want to do it until +they are needed, or perhaps until after relocation. + +2. Activation/probe + +When a device needs to be used, U-Boot activates it, by following these +steps (see device_probe()): + + a. If priv_auto_alloc_size is non-zero, then the device-private space + is allocated for the device and zeroed. It will be accessible as + dev->priv. The driver can put anything it likes in there, but should use + it for run-time information, not platform data (which should be static + and known before the device is probed). + + b. If platdata_auto_alloc_size is non-zero, then the platform data space + is allocated. This is only useful for device tree operation, since + otherwise you would have to specific the platform data in the + U_BOOT_DEVICE() declaration. The space is allocated for the device and + zeroed. It will be accessible as dev->platdata. + + c. If the device's uclass specifies a non-zero per_device_auto_alloc_size, + then this space is allocated and zeroed also. It is allocated for and + stored in the device, but it is uclass data. owned by the uclass driver. + It is possible for the device to access it. + + d. All parent devices are probed. It is not possible to activate a device + unless its parents (all the way up to the root device) are activated. + This means (for example) that an I2C driver will require that its bus + be activated. + + e. If the driver provides a ofdata_to_platdata() method, then this is + called to convert the device tree data into platform data. This should + do various calls like fdtdec_get_int(gd->fdt_blob, dev->of_offset, ...) + to access the node and store the resulting information into dev->platdata. + After this point, the device works the same way whether it was bound + using a device tree node or U_BOOT_DEVICE() structure. In either case, + the platform data is now stored in the platdata structure. Typically you + will use the platdata_auto_alloc_size feature to specify the size of the + platform data structure, and U-Boot will automatically allocate and zero + it for you before entry to ofdata_to_platdata(). But if not, you can + allocate it yourself in ofdata_to_platdata(). Note that it is preferable + to do all the device tree decoding in ofdata_to_platdata() rather than + in probe(). (Apart from the ugliness of mixing configuration and run-time + data, one day it is possible that U-Boot will cache platformat data for + devices which are regularly de/activated). + + f. The device's probe() method is called. This should do anything that + is required by the device to get it going. This could include checking + that the hardware is actually present, setting up clocks for the + hardware and setting up hardware registers to initial values. The code + in probe() can access: + + - platform data in dev->platdata (for configuration) + - private data in dev->priv (for run-time state) + - uclass data in dev->uclass_priv (for things the uclass stores + about this device) + + Note: If you don't use priv_auto_alloc_size then you will need to + allocate the priv space here yourself. The same applies also to + platdata_auto_alloc_size. Remember to free them in the remove() method. + + g. The device is marked 'activated' + + h. The uclass's post_probe() method is called, if one exists. This may + cause the uclass to do some housekeeping to record the device as + activated and 'known' by the uclass. + +3. Running stage + +The device is now activated and can be used. From now until it is removed +all of the above structures are accessible. The device appears in the +uclass's list of devices (so if the device is in UCLASS_GPIO it will appear +as a device in the GPIO uclass). This is the 'running' state of the device. + +4. Removal stage + +When the device is no-longer required, you can call device_remove() to +remove it. This performs the probe steps in reverse: + + a. The uclass's pre_remove() method is called, if one exists. This may + cause the uclass to do some housekeeping to record the device as + deactivated and no-longer 'known' by the uclass. + + b. All the device's children are removed. It is not permitted to have + an active child device with a non-active parent. This means that + device_remove() is called for all the children recursively at this point. + + c. The device's remove() method is called. At this stage nothing has been + deallocated so platform data, private data and the uclass data will all + still be present. This is where the hardware can be shut down. It is + intended that the device be completely inactive at this point, For U-Boot + to be sure that no hardware is running, it should be enough to remove + all devices. + + d. The device memory is freed (platform data, private data, uclass data). + + Note: for a U_BOOT_DEVICE() declaration, the platform data is supplied as + a static pointer and is not allocated. For device tree, the platform + data is allocated during activation and freed during dectivation, + typically automatically using platdata_auto_alloc_size. But if that value + is 0 then U-Boot will not do the allocation/freeing and you will need to + do this yourself in your ofdata_to_platdata() and remove() methods. This + difference is tracked by the device's DM_FLAG_ALLOC_PDATA flag. + + e. The device is marked inactive. Note that it is still bound, so the + device structure itself is not freed at this point. Should the device be + activated again, then the cycle starts again at step 4 above. + +5. Unbind stage + +The device is unbound. This is the step that actually destroys the device. +If a parent has children these will be destroyed first. After this point +the device does not exist and its memory has be deallocated. + + Data Structures ---------------
Hi,
I found some typos in this
On Friday 06 of June 2014 13:13:32 Simon Glass wrote:
The lifecycle of a device is an important part of driver model. Add to the existing documentation and clarify it.
...snip...
+This means that instead of having lots of U_BOOT_DEVICE() declarations in +the board file, we put these in the device tree. This approach allows a lot +more generality, since the same board file can support many types of boards +(e,g. with the same SoC) just by using different device trees. An added +benefit is that the Linux device tree can be used, thus further simplifying +the task of board-bring up either for U-Boot or Linux devs (whoever gets to +the baord first!).
s/baord/board/
...snip...
+Note that compared to Linux, U-Boot's driver model has a separate step of +probe/remove which is independent of bind/unbind. This is partly because in +U-Boot it may be expensive to prove devices and we don't want to do it until +they are needed, or perhaps until after relocation.
s/prove/probe/
...snip...
- d. All parent devices are probed. It is not possible to activate a
device + unless its parents (all the way up to the root device) are activated. + This means (for example) that an I2C driver will require that its bus + be activated.
s/parents/predecessors/ in a tree, a node can have up to one parent. parent of a parent (recursively) is a predecessor.
...snip...
- e. The device is marked inactive. Note that it is still bound, so the
- device structure itself is not freed at this point. Should the device be
- activated again, then the cycle starts again at step 4 above.
if there were no drastic changes since I last checked, this would go to activation/probe(), which is described in step 2
regards Pavel Herrmann
Hi Pawel,
On 6 June 2014 13:52, Pavel Herrmann morpheus.ibis@gmail.com wrote:
Hi,
I found some typos in this
Thanks very much for that, will add these.
Regards, Simon
participants (5)
-
Andre Renaud -
Jon Loeliger -
Marek Vasut -
Pavel Herrmann -
Simon Glass