* [U-Boot] [RFC PATCH v2 01/13] Add an I/O tracing feature
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
@ 2014-05-05 16:08 ` Simon Glass
2014-05-05 16:08 ` [U-Boot] [RFC PATCH v2 02/13] arm: Support iotrace feature Simon Glass
` (11 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:08 UTC (permalink / raw)
To: u-boot
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 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 | 102 +++++++++++++++++++++++++++++++
5 files changed, 369 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 12758dc..6b41630 100644
--- a/README
+++ b/README
@@ -987,6 +987,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
@@ -1158,6 +1159,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 7c853ae..0dccb90 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -96,6 +96,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
@@ -240,6 +241,7 @@ obj-y += image.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..a8ac749
--- /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@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(bool enable)
+{
+ iotrace.enabled = enable;
+}
+
+bool 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..f80a89b
--- /dev/null
+++ b/include/iotrace.h
@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2014 Google, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#ifndef __IOTRACE_H
+#define __IOTRACE_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(bool enable);
+
+/**
+ * iotrace_get_enabled() - Get whether iotracing is enabled or not
+ *
+ * @return true if enabled, false if disabled
+ */
+bool 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 */
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 02/13] arm: Support iotrace feature
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
2014-05-05 16:08 ` [U-Boot] [RFC PATCH v2 01/13] Add an I/O tracing feature Simon Glass
@ 2014-05-05 16:08 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 03/13] sandbox: " Simon Glass
` (10 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:08 UTC (permalink / raw)
To: u-boot
Support the iotrace feature for ARM, when enabled.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
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 */
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 03/13] sandbox: Support iotrace feature
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
2014-05-05 16:08 ` [U-Boot] [RFC PATCH v2 01/13] Add an I/O tracing feature Simon Glass
2014-05-05 16:08 ` [U-Boot] [RFC PATCH v2 02/13] arm: Support iotrace feature Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 04/13] Makefile: Support include files for .dts files Simon Glass
` (9 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
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 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 fa62cb6..8ca2a55 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -16,6 +16,9 @@
#endif
+#define CONFIG_IO_TRACE
+#define CONFIG_CMD_IO_TRACE
+
#define CONFIG_SYS_TIMER_RATE 1000000
#define CONFIG_BOOTSTAGE
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 04/13] Makefile: Support include files for .dts files
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (2 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 03/13] sandbox: " Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:54 ` Stephen Warren
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 05/13] tegra: dts: Bring in GPIO bindings from linux Simon Glass
` (8 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
Linux supports this, and if we are to have compatible device tree files,
U-Boot should also.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add new patch to support include files for .dts files
scripts/Makefile.lib | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index a04439d..10358ba 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)/include \
-undef -D__DTS__
# Finds the multi-part object the current object will be linked into
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 04/13] Makefile: Support include files for .dts files
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 04/13] Makefile: Support include files for .dts files Simon Glass
@ 2014-05-05 16:54 ` Stephen Warren
2014-05-07 2:15 ` Masahiro Yamada
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Warren @ 2014-05-05 16:54 UTC (permalink / raw)
To: u-boot
On 05/05/2014 10:09 AM, Simon Glass wrote:
> Linux supports this, and if we are to have compatible device tree files,
> U-Boot should also.
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> # Modified for U-Boot
> dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc \
> -I$(srctree)/arch/$(ARCH)/dts \
> + -I$(srctree)/include \
> -undef -D__DTS__
I don't think we should add the top-level include/ directory to the DT
include path. That is something I very specifically avoided in the
kernel Makefiles. If we did allow this, then DTs could start including
arbitrary U-Boot header files, rather than just header files intended to
be used in DT bindings, and that would then make the DT files used in
U-Boot not portable to the Linux kernel, or any standalone DT file
repository which may appear.
Instead, let's create a standalone root directory for the DT include
files, and add that to the DT header path. We can add this DT-specific
include path to the include patch for U-Boot C code if needed.
Perhaps we can create a top-level dt/include/ or device-tree/include
directory for this?
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 04/13] Makefile: Support include files for .dts files
2014-05-05 16:54 ` Stephen Warren
@ 2014-05-07 2:15 ` Masahiro Yamada
2014-05-07 22:37 ` Simon Glass
0 siblings, 1 reply; 31+ messages in thread
From: Masahiro Yamada @ 2014-05-07 2:15 UTC (permalink / raw)
To: u-boot
Hi Simon, Stephen,
On Mon, 05 May 2014 10:54:52 -0600
Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/05/2014 10:09 AM, Simon Glass wrote:
> > Linux supports this, and if we are to have compatible device tree files,
> > U-Boot should also.
>
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>
> > # Modified for U-Boot
> > dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc \
> > -I$(srctree)/arch/$(ARCH)/dts \
> > + -I$(srctree)/include \
> > -undef -D__DTS__
>
> I don't think we should add the top-level include/ directory to the DT
> include path. That is something I very specifically avoided in the
> kernel Makefiles. If we did allow this, then DTs could start including
> arbitrary U-Boot header files, rather than just header files intended to
> be used in DT bindings, and that would then make the DT files used in
> U-Boot not portable to the Linux kernel, or any standalone DT file
> repository which may appear.
>
> Instead, let's create a standalone root directory for the DT include
> files, and add that to the DT header path. We can add this DT-specific
> include path to the include patch for U-Boot C code if needed.
>
> Perhaps we can create a top-level dt/include/ or device-tree/include
> directory for this?
I agree with Stephen.
I hesitate to add the top-level include/ to the path.
If we try to follow the Linux style,
we can add the include path "-I$(srctree)/arch/$(ARCH)/dts/include
and create symbolic links to include/dt-bindings.
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 04/13] Makefile: Support include files for .dts files
2014-05-07 2:15 ` Masahiro Yamada
@ 2014-05-07 22:37 ` Simon Glass
0 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-07 22:37 UTC (permalink / raw)
To: u-boot
Hi Stephen and Masahiro
On 6 May 2014 20:15, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon, Stephen,
>
> On Mon, 05 May 2014 10:54:52 -0600
> Stephen Warren <swarren@wwwdotorg.org> wrote:
>
> > On 05/05/2014 10:09 AM, Simon Glass wrote:
> > > Linux supports this, and if we are to have compatible device tree
> files,
> > > U-Boot should also.
> >
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> >
> > > # Modified for U-Boot
> > > dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc
> \
> > > -I$(srctree)/arch/$(ARCH)/dts \
> > > + -I$(srctree)/include \
> > > -undef -D__DTS__
> >
> > I don't think we should add the top-level include/ directory to the DT
> > include path. That is something I very specifically avoided in the
> > kernel Makefiles. If we did allow this, then DTs could start including
> > arbitrary U-Boot header files, rather than just header files intended to
> > be used in DT bindings, and that would then make the DT files used in
> > U-Boot not portable to the Linux kernel, or any standalone DT file
> > repository which may appear.
> >
> > Instead, let's create a standalone root directory for the DT include
> > files, and add that to the DT header path. We can add this DT-specific
> > include path to the include patch for U-Boot C code if needed.
> >
> > Perhaps we can create a top-level dt/include/ or device-tree/include
> > directory for this?
>
> I agree with Stephen.
> I hesitate to add the top-level include/ to the path.
>
> If we try to follow the Linux style,
> we can add the include path "-I$(srctree)/arch/$(ARCH)/dts/include
> and create symbolic links to include/dt-bindings.
>
Yes that's fine. So long as we can use include files it is OK with me.
Regards,
Simon
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 05/13] tegra: dts: Bring in GPIO bindings from linux
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (3 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 04/13] Makefile: Support include files for .dts files Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:57 ` Stephen Warren
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 06/13] dm: Update README to encourage conversion to driver model Simon Glass
` (7 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
These files are taken from Linux 3.14.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add new patch to bring in Tegra device tree files from linux
arch/arm/dts/tegra20.dtsi | 15 ++++++-
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 ++++++++
5 files changed, 120 insertions(+), 2 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/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 at 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 at 70000000 {
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
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 06/13] dm: Update README to encourage conversion to driver model
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (4 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 05/13] tegra: dts: Bring in GPIO bindings from linux Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 07/13] dm: Use case-insensitive comparison for GPIO banks Simon Glass
` (6 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
Add a note to encourage people to convert drivers to use driver model.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
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 6b41630..5be8e8c 100644
--- a/README
+++ b/README
@@ -5246,6 +5246,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:
-----------------------------
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 07/13] dm: Use case-insensitive comparison for GPIO banks
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (5 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 06/13] dm: Update README to encourage conversion to driver model Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 08/13] dm: Add missing header files in lists and root Simon Glass
` (5 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
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 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 56bfd11..ec605dc 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 device **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)
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 08/13] dm: Add missing header files in lists and root
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (6 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 07/13] dm: Use case-insensitive comparison for GPIO banks Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 09/13] dm: Case away the const-ness of the global_data pointer Simon Glass
` (4 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
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 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 4f2c126..6a53362 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 407bc0d..88d2f45 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>
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 09/13] dm: Case away the const-ness of the global_data pointer
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (7 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 08/13] dm: Add missing header files in lists and root Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 10/13] dm: Allow driver model tests only for sandbox Simon Glass
` (3 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
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 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 88d2f45..4427b81 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 4df5a8b..afb5fdc 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 c026e8e..e95b7a9 100644
--- a/include/dm/device-internal.h
+++ b/include/dm/device-internal.h
@@ -84,4 +84,8 @@ int device_remove(struct device *dev);
*/
int device_unbind(struct device *dev);
+/* Cast away any volatile pointer */
+#define DM_ROOT() (((gd_t *)gd)->dm_root)
+#define DM_UCLASS_ROOT() (((gd_t *)gd)->uclass_root)
+
#endif
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 10/13] dm: Allow driver model tests only for sandbox
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (8 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 09/13] dm: Case away the const-ness of the global_data pointer Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 11/13] dm: Fix printf() strings in the 'dm' command Simon Glass
` (2 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
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 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 a03fe20..62eccd6 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
);
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 11/13] dm: Fix printf() strings in the 'dm' command
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (9 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 10/13] dm: Allow driver model tests only for sandbox Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 12/13] tegra: Enable driver model Simon Glass
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use " Simon Glass
12 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
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 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 62eccd6..c6b2eb8 100644
--- a/test/dm/cmd_dm.c
+++ b/test/dm/cmd_dm.c
@@ -23,7 +23,7 @@ static int display_succ(struct device *in, char *buf)
char local[16];
struct device *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 device *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");
}
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 12/13] tegra: Enable driver model
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (10 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 11/13] dm: Fix printf() strings in the 'dm' command Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 16:58 ` Stephen Warren
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use " Simon Glass
12 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
Enable driver model for Tegra boards.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Split out a separate patch to enable driver model for tegra
include/configs/tegra-common.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
index ae786cf..3943249 100644
--- a/include/configs/tegra-common.h
+++ b/include/configs/tegra-common.h
@@ -19,6 +19,8 @@
#include <asm/arch/tegra.h> /* get chip and board defs */
+#define CONFIG_DM
+
#define CONFIG_SYS_TIMER_RATE 1000000
#define CONFIG_SYS_TIMER_COUNTER NV_PA_TMRUS_BASE
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 12/13] tegra: Enable driver model
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 12/13] tegra: Enable driver model Simon Glass
@ 2014-05-05 16:58 ` Stephen Warren
2014-05-05 19:01 ` Simon Glass
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Warren @ 2014-05-05 16:58 UTC (permalink / raw)
To: u-boot
On 05/05/2014 10:09 AM, Simon Glass wrote:
> Enable driver model for Tegra boards.
I have no objection to this, but I'm curious if it brings any changes to
the binary size?
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 12/13] tegra: Enable driver model
2014-05-05 16:58 ` Stephen Warren
@ 2014-05-05 19:01 ` Simon Glass
0 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-05 19:01 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 5 May 2014 10:58, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/05/2014 10:09 AM, Simon Glass wrote:
>> Enable driver model for Tegra boards.
>
> I have no objection to this, but I'm curious if it brings any changes to
> the binary size?
Yes, it depends on how much of driver model you use. Here are the
results for this commit for trimslice (./tools/buildman/buildman -b
us-gpio4 trimslice -sS)
13: tegra: Enable driver model
arm: (for 1/1 boards) all +2863.0 bss +4.0 data +92.0
rodata +551.0 spl/u-boot-spl:all +8.0 spl/u-boot-spl:data +8.0 text
+2216.0
Regards,
Simon
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 16:08 [U-Boot] [RFC PATCH v2 0/13] Enable driver model for GPIOs on Tegra Simon Glass
` (11 preceding siblings ...)
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 12/13] tegra: Enable driver model Simon Glass
@ 2014-05-05 16:09 ` Simon Glass
2014-05-05 17:29 ` Stephen Warren
12 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-05 16:09 UTC (permalink / raw)
To: u-boot
This is an implementation of GPIOs for Tegra that uses driver model. It has
been tested on trimslice and also using the new iotrace feature.
The implementation uses a top-level GPIO device (which has no actual GPIOS).
Under this all the banks are created as separate GPIO devices.
The GPIOs are named as per the Tegra datasheet/header files: A0..A7, B0..B7,
..., Z0..Z7, AA0..AA7, etc.
Since driver model is not yet available before relocation, or in SPL, a
special function is provided for seaboard's SPL code.
This series is marked RFC since the Tegra driver may need further discussion.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Split out driver model changes into separate patches
- Correct bugs found during testing
arch/arm/include/asm/arch-tegra/gpio.h | 14 +-
board/nvidia/seaboard/seaboard.c | 2 +-
drivers/gpio/tegra_gpio.c | 393 +++++++++++++++++++++++++--------
include/configs/tegra-common.h | 2 +
4 files changed, 309 insertions(+), 102 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/gpio.h b/arch/arm/include/asm/arch-tegra/gpio.h
index d97190d..80e4a73 100644
--- a/arch/arm/include/asm/arch-tegra/gpio.h
+++ b/arch/arm/include/asm/arch-tegra/gpio.h
@@ -6,6 +6,8 @@
#ifndef _TEGRA_GPIO_H_
#define _TEGRA_GPIO_H_
+#define TEGRA_GPIOS_PER_PORT 8
+#define TEGRA_PORTS_PER_BANK 4
#define MAX_NUM_GPIOS (TEGRA_GPIO_PORTS * TEGRA_GPIO_BANKS * 8)
#define GPIO_NAME_SIZE 20 /* gpio_request max label len */
@@ -14,11 +16,13 @@
#define GPIO_FULLPORT(x) ((x) >> 3)
#define GPIO_BIT(x) ((x) & 0x7)
-/*
- * Tegra-specific GPIO API
+/**
+ * tegra_spl_gpio_direction_output() - set the output value of a GPIO
+ *
+ * This function is only used from SPL on seaboard, which needs to enable a
+ * GPIO to get the UART running. It could be done in U-Boot rather than SPL,
+ * but for now, this gets it working
*/
+int tegra_spl_gpio_direction_output(int gpio, int value);
-void gpio_info(void);
-
-#define gpio_status() gpio_info()
#endif /* TEGRA_GPIO_H_ */
diff --git a/board/nvidia/seaboard/seaboard.c b/board/nvidia/seaboard/seaboard.c
index ef4e481..e27efcd 100644
--- a/board/nvidia/seaboard/seaboard.c
+++ b/board/nvidia/seaboard/seaboard.c
@@ -22,7 +22,7 @@ void gpio_early_init_uart(void)
#ifndef CONFIG_SPL_BUILD
gpio_request(GPIO_PI3, NULL);
#endif
- gpio_direction_output(GPIO_PI3, 0);
+ tegra_spl_gpio_direction_output(GPIO_PI3, 0);
}
#endif
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
index 82b30d5..4cca266 100644
--- a/drivers/gpio/tegra_gpio.c
+++ b/drivers/gpio/tegra_gpio.c
@@ -12,10 +12,17 @@
*/
#include <common.h>
+#include <dm.h>
+#include <malloc.h>
+#include <errno.h>
+#include <fdtdec.h>
#include <asm/io.h>
#include <asm/bitops.h>
#include <asm/arch/tegra.h>
#include <asm/gpio.h>
+#include <dm/device-internal.h>
+
+DECLARE_GLOBAL_DATA_PTR;
enum {
TEGRA_CMD_INFO,
@@ -24,24 +31,30 @@ enum {
TEGRA_CMD_INPUT,
};
-static struct gpio_names {
- char name[GPIO_NAME_SIZE];
-} gpio_names[MAX_NUM_GPIOS];
+struct tegra_gpio_platdata {
+ struct gpio_ctlr_bank *bank;
+ const char *port_name; /* Name of port, e.g. "B" */
+ int base_port; /* Port number for this port (0, 1,.., n-1) */
+};
-static char *get_name(int i)
-{
- return *gpio_names[i].name ? gpio_names[i].name : "UNKNOWN";
-}
+/* Information about each port at run-time */
+struct tegra_port_info {
+ char label[TEGRA_GPIOS_PER_PORT][GPIO_NAME_SIZE];
+ struct gpio_ctlr_bank *bank;
+ int base_port; /* Port number for this port (0, 1,.., n-1) */
+};
+
+#define GPIO_NUM(state, offset) \
+ (((state)->base_port * TEGRA_GPIOS_PER_PORT) + (offset))
/* Return config of pin 'gpio' as GPIO (1) or SFPIO (0) */
-static int get_config(unsigned gpio)
+static int get_config(struct tegra_port_info *state, int offset)
{
- struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
- struct gpio_ctlr_bank *bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
+ int gpio = GPIO_NUM(state, offset);
u32 u;
int type;
- u = readl(&bank->gpio_config[GPIO_PORT(gpio)]);
+ u = readl(&state->bank->gpio_config[GPIO_PORT(gpio)]);
type = (u >> GPIO_BIT(gpio)) & 1;
debug("get_config: port = %d, bit = %d is %s\n",
@@ -51,32 +64,32 @@ static int get_config(unsigned gpio)
}
/* Config pin 'gpio' as GPIO or SFPIO, based on 'type' */
-static void set_config(unsigned gpio, int type)
+static void set_config(struct tegra_port_info *state, int offset, int type)
{
- struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
- struct gpio_ctlr_bank *bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
+ int gpio = GPIO_NUM(state, offset);
u32 u;
debug("set_config: port = %d, bit = %d, %s\n",
- GPIO_FULLPORT(gpio), GPIO_BIT(gpio), type ? "GPIO" : "SFPIO");
+ GPIO_FULLPORT(gpio),
+ GPIO_BIT(gpio),
+ type ? "GPIO" : "SFPIO");
- u = readl(&bank->gpio_config[GPIO_PORT(gpio)]);
+ u = readl(&state->bank->gpio_config[GPIO_PORT(gpio)]);
if (type) /* GPIO */
u |= 1 << GPIO_BIT(gpio);
else
u &= ~(1 << GPIO_BIT(gpio));
- writel(u, &bank->gpio_config[GPIO_PORT(gpio)]);
+ writel(u, &state->bank->gpio_config[GPIO_PORT(gpio)]);
}
/* Return GPIO pin 'gpio' direction - 0 = input or 1 = output */
-static int get_direction(unsigned gpio)
+static int get_direction(struct tegra_port_info *state, int offset)
{
- struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
- struct gpio_ctlr_bank *bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
+ int gpio = GPIO_NUM(state, offset);
u32 u;
int dir;
- u = readl(&bank->gpio_dir_out[GPIO_PORT(gpio)]);
+ u = readl(&state->bank->gpio_dir_out[GPIO_PORT(gpio)]);
dir = (u >> GPIO_BIT(gpio)) & 1;
debug("get_direction: port = %d, bit = %d, %s\n",
@@ -86,161 +99,349 @@ static int get_direction(unsigned gpio)
}
/* Config GPIO pin 'gpio' as input or output (OE) as per 'output' */
-static void set_direction(unsigned gpio, int output)
+static void set_direction(struct tegra_port_info *state, int offset,
+ int output)
{
- struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
- struct gpio_ctlr_bank *bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
+ int gpio = GPIO_NUM(state, offset);
u32 u;
- debug("set_direction: port = %d, bit = %d, %s\n",
- GPIO_FULLPORT(gpio), GPIO_BIT(gpio), output ? "OUT" : "IN");
+ debug("%s: port = %d, bit = %d, %s\n", __func__,
+ GPIO_FULLPORT(gpio), GPIO_BIT(gpio), output ? "OUT" : "IN");
- u = readl(&bank->gpio_dir_out[GPIO_PORT(gpio)]);
+ u = readl(&state->bank->gpio_dir_out[GPIO_PORT(gpio)]);
if (output)
u |= 1 << GPIO_BIT(gpio);
else
u &= ~(1 << GPIO_BIT(gpio));
- writel(u, &bank->gpio_dir_out[GPIO_PORT(gpio)]);
+ writel(u, &state->bank->gpio_dir_out[GPIO_PORT(gpio)]);
}
/* set GPIO pin 'gpio' output bit as 0 or 1 as per 'high' */
-static void set_level(unsigned gpio, int high)
+static void set_level(struct tegra_port_info *state, int offset, int high)
{
- struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
- struct gpio_ctlr_bank *bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
+ int gpio = GPIO_NUM(state, offset);
u32 u;
- debug("set_level: port = %d, bit %d == %d\n",
- GPIO_FULLPORT(gpio), GPIO_BIT(gpio), high);
+ debug("%s: port = %d, bit %d == %d\n", __func__,
+ GPIO_FULLPORT(gpio), GPIO_BIT(gpio), high);
- u = readl(&bank->gpio_out[GPIO_PORT(gpio)]);
+ u = readl(&state->bank->gpio_out[GPIO_PORT(gpio)]);
if (high)
u |= 1 << GPIO_BIT(gpio);
else
u &= ~(1 << GPIO_BIT(gpio));
- writel(u, &bank->gpio_out[GPIO_PORT(gpio)]);
+ writel(u, &state->bank->gpio_out[GPIO_PORT(gpio)]);
+}
+
+/* read GPIO OUT value of pin 'gpio' */
+static int get_output_value(struct tegra_port_info *state, int offset)
+{
+ int gpio = GPIO_NUM(state, offset);
+ int val;
+
+ debug("gpio_get_output_value: pin = %d (port %d:bit %d)\n",
+ gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio));
+
+ val = readl(&state->bank->gpio_out[GPIO_PORT(gpio)]);
+
+ return (val >> GPIO_BIT(gpio)) & 1;
}
+#ifdef CONFIG_SPL
+/* set GPIO pin 'gpio' as an output, with polarity 'value' */
+int tegra_spl_gpio_direction_output(int gpio, int value)
+{
+ struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
+ struct tegra_port_info state;
+
+ state.bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
+ state.base_port = GPIO_FULLPORT(gpio);
+
+ /* Configure GPIO output value. */
+ set_level(&state, GPIO_BIT(gpio), value);
+
+ /* Configure GPIO direction as output. */
+ set_direction(&state, GPIO_BIT(gpio), value);
+
+ return 0;
+}
+#endif /* CONFIG_SPL */
+
/*
* Generic_GPIO primitives.
*/
-int gpio_request(unsigned gpio, const char *label)
+static int check_reserved(struct device *dev, unsigned offset,
+ const char *func)
{
- if (gpio >= MAX_NUM_GPIOS)
- return -1;
+ struct tegra_port_info *state = dev_get_priv(dev);
+ struct gpio_dev_priv *uc_priv = dev->uclass_priv;
- if (label != NULL) {
- strncpy(gpio_names[gpio].name, label, GPIO_NAME_SIZE);
- gpio_names[gpio].name[GPIO_NAME_SIZE - 1] = '\0';
+ if (!*state->label[offset]) {
+ printf("tegra_gpio: %s: error: gpio %s%d not reserved\n",
+ func, uc_priv->bank_name, offset);
+ return -EPERM;
}
- /* Configure as a GPIO */
- set_config(gpio, 1);
-
return 0;
}
-int gpio_free(unsigned gpio)
+static int tegra_gpio_request(struct device *dev, unsigned offset,
+ const char *label)
{
- if (gpio >= MAX_NUM_GPIOS)
- return -1;
+ struct tegra_port_info *state = dev_get_priv(dev);
+
+ if (*state->label[offset])
+ return -EBUSY;
+
+ strncpy(state->label[offset], label, GPIO_NAME_SIZE);
+ state->label[offset][GPIO_NAME_SIZE - 1] = '\0';
+
+ /* Configure as a GPIO */
+ set_config(state, offset, 1);
- gpio_names[gpio].name[0] = '\0';
- /* Do not configure as input or change pin mux here */
return 0;
}
-/* read GPIO OUT value of pin 'gpio' */
-static int gpio_get_output_value(unsigned gpio)
+static int tegra_gpio_free(struct device *dev, unsigned offset)
{
- struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
- struct gpio_ctlr_bank *bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
- int val;
-
- debug("gpio_get_output_value: pin = %d (port %d:bit %d)\n",
- gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio));
+ struct tegra_port_info *state = dev_get_priv(dev);
+ int ret;
- val = readl(&bank->gpio_out[GPIO_PORT(gpio)]);
+ ret = check_reserved(dev, offset, __func__);
+ if (ret)
+ return ret;
+ state->label[offset][0] = '\0';
- return (val >> GPIO_BIT(gpio)) & 1;
+ return 0;
}
/* set GPIO pin 'gpio' as an input */
-int gpio_direction_input(unsigned gpio)
+static int tegra_gpio_direction_input(struct device *dev, unsigned offset)
{
- debug("gpio_direction_input: pin = %d (port %d:bit %d)\n",
- gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio));
+ struct tegra_port_info *state = dev_get_priv(dev);
+ int ret;
+
+ ret = check_reserved(dev, offset, __func__);
+ if (ret)
+ return ret;
/* Configure GPIO direction as input. */
- set_direction(gpio, 0);
+ set_direction(state, offset, 0);
return 0;
}
/* set GPIO pin 'gpio' as an output, with polarity 'value' */
-int gpio_direction_output(unsigned gpio, int value)
+static int tegra_gpio_direction_output(struct device *dev, unsigned offset,
+ int value)
{
- debug("gpio_direction_output: pin = %d (port %d:bit %d) = %s\n",
- gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio),
- value ? "HIGH" : "LOW");
+ struct tegra_port_info *state = dev_get_priv(dev);
+ int ret;
+
+ ret = check_reserved(dev, offset, __func__);
+ if (ret)
+ return ret;
/* Configure GPIO output value. */
- set_level(gpio, value);
+ set_level(state, offset, value);
/* Configure GPIO direction as output. */
- set_direction(gpio, 1);
+ set_direction(state, offset, 1);
return 0;
}
/* read GPIO IN value of pin 'gpio' */
-int gpio_get_value(unsigned gpio)
+static int tegra_gpio_get_value(struct device *dev, unsigned offset)
{
- struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
- struct gpio_ctlr_bank *bank = &ctlr->gpio_bank[GPIO_BANK(gpio)];
+ struct tegra_port_info *state = dev_get_priv(dev);
+ int gpio = GPIO_NUM(state, offset);
+ int ret;
int val;
- debug("gpio_get_value: pin = %d (port %d:bit %d)\n",
- gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio));
+ ret = check_reserved(dev, offset, __func__);
+ if (ret)
+ return ret;
+
+ debug("%s: pin = %d (port %d:bit %d)\n", __func__,
+ gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio));
- val = readl(&bank->gpio_in[GPIO_PORT(gpio)]);
+ val = readl(&state->bank->gpio_in[GPIO_PORT(gpio)]);
return (val >> GPIO_BIT(gpio)) & 1;
}
/* write GPIO OUT value to pin 'gpio' */
-int gpio_set_value(unsigned gpio, int value)
+static int tegra_gpio_set_value(struct device *dev, unsigned offset, int value)
{
+ struct tegra_port_info *state = dev_get_priv(dev);
+ int gpio = GPIO_NUM(state, offset);
+ int ret;
+
+ ret = check_reserved(dev, offset, __func__);
+ if (ret)
+ return ret;
+
debug("gpio_set_value: pin = %d (port %d:bit %d), value = %d\n",
- gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio), value);
+ gpio, GPIO_FULLPORT(gpio), GPIO_BIT(gpio), value);
/* Configure GPIO output value. */
- set_level(gpio, value);
+ set_level(state, offset, value);
return 0;
}
-/*
- * Display Tegra GPIO information
+static int tegra_gpio_get_state(struct device *dev, unsigned int offset,
+ char *buf, int bufsize)
+{
+ struct gpio_dev_priv *uc_priv = dev->uclass_priv;
+ struct tegra_port_info *state = dev_get_priv(dev);
+ const char *label;
+ int is_output;
+ int is_gpio;
+ int size;
+
+ label = state->label[offset];
+ is_gpio = get_config(state, offset); /* GPIO, not SFPIO */
+ size = snprintf(buf, bufsize, "%s%d: ",
+ uc_priv->bank_name ? uc_priv->bank_name : "", offset);
+ buf += size;
+ bufsize -= size;
+ if (is_gpio) {
+ is_output = get_direction(state, offset);
+
+ snprintf(buf, bufsize, "%s: %d [%c]%s%s",
+ is_output ? "out" : " in",
+ is_output ?
+ get_output_value(state, offset) :
+ tegra_gpio_get_value(dev, offset),
+ *label ? 'x' : ' ',
+ *label ? " " : "",
+ label);
+ } else {
+ snprintf(buf, bufsize, "sfpio");
+ }
+
+ return 0;
+}
+
+static const struct dm_gpio_ops gpio_tegra_ops = {
+ .request = tegra_gpio_request,
+ .free = tegra_gpio_free,
+ .direction_input = tegra_gpio_direction_input,
+ .direction_output = tegra_gpio_direction_output,
+ .get_value = tegra_gpio_get_value,
+ .set_value = tegra_gpio_set_value,
+ .get_state = tegra_gpio_get_state,
+};
+
+/**
+ * Returns the name of a GPIO port
+ *
+ * GPIOs are named A, B, C, ..., Z, AA, BB, CC, ...
+ *
+ * @base_port: Base port number (0, 1..n-1)
+ * @return allocated string containing the name
*/
-void gpio_info(void)
+static char *gpio_port_name(int base_port)
{
- unsigned c;
- int type;
+ char *name, *s;
+
+ name = malloc(3);
+ if (name) {
+ s = name;
+ *s++ = 'A' + (base_port % 26);
+ if (base_port >= 26)
+ *s++ = *name;
+ *s = '\0';
+ }
+
+ return name;
+}
+
+static const struct device_id tegra_gpio_ids[] = {
+ { .compatible = "nvidia,tegra30-gpio" },
+ { .compatible = "nvidia,tegra20-gpio" },
+ { }
+};
+
+static int gpio_tegra_probe(struct device *dev)
+{
+ struct gpio_dev_priv *uc_priv = dev->uclass_priv;
+ struct tegra_port_info *priv = dev->priv;
+ struct tegra_gpio_platdata *plat = dev->platdata;
+
+ /* Only child devices have ports */
+ if (!plat)
+ return 0;
+
+ priv->bank = plat->bank;
+ priv->base_port = plat->base_port;
- for (c = 0; c < MAX_NUM_GPIOS; c++) {
- type = get_config(c); /* GPIO, not SFPIO */
- if (type) {
- printf("GPIO_%d:\t%s is an %s, ", c,
- get_name(c),
- get_direction(c) ? "OUTPUT" : "INPUT");
- if (get_direction(c))
- printf("value = %d", gpio_get_output_value(c));
- else
- printf("value = %d", gpio_get_value(c));
- printf("\n");
- } else
- continue;
+ uc_priv->gpio_count = TEGRA_GPIOS_PER_PORT;
+ uc_priv->bank_name = plat->port_name;
+
+ return 0;
+}
+
+/**
+ * We have a top-level GPIO device with no actual GPIOs. It has a child
+ * device for each Tegra port.
+ */
+static int gpio_tegra_bind(struct device *parent)
+{
+ struct tegra_gpio_platdata *plat = parent->platdata;
+ struct gpio_ctlr *ctlr;
+ int bank_count;
+ int bank;
+ int ret;
+ int len;
+
+ /* If this is a child device, there is nothing to do here */
+ if (plat)
+ return 0;
+
+ /*
+ * This driver does not make use of interrupts, other than to figure
+ * out the number of GPIO banks
+ */
+ if (!fdt_getprop(gd->fdt_blob, parent->of_offset, "interrupts", &len))
+ return -EINVAL;
+ bank_count = len / 3 / sizeof(u32);
+ ctlr = (struct gpio_ctlr *)fdtdec_get_addr(gd->fdt_blob,
+ parent->of_offset, "reg");
+ for (bank = 0; bank < bank_count; bank++) {
+ int port;
+
+ for (port = 0; port < TEGRA_PORTS_PER_BANK; port++) {
+ struct tegra_gpio_platdata *plat;
+ struct device *dev;
+
+ plat = calloc(1, sizeof(*plat));
+ if (!plat)
+ return -ENOMEM;
+ plat->bank = &ctlr->gpio_bank[bank];
+ plat->base_port = bank * TEGRA_PORTS_PER_BANK + port;
+ plat->port_name = gpio_port_name(plat->base_port);
+
+ ret = device_bind(parent, parent->driver,
+ plat->port_name, plat, -1, &dev);
+ if (ret)
+ return ret;
+ dev->of_offset = parent->of_offset;
+ }
}
+
+ return 0;
}
+
+U_BOOT_DRIVER(gpio_tegra) = {
+ .name = "gpio_tegra",
+ .id = UCLASS_GPIO,
+ .of_match = tegra_gpio_ids,
+ .bind = gpio_tegra_bind,
+ .probe = gpio_tegra_probe,
+ .priv_auto_alloc_size = sizeof(struct tegra_port_info),
+ .ops = &gpio_tegra_ops,
+};
diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
index 3943249..7222ba6 100644
--- a/include/configs/tegra-common.h
+++ b/include/configs/tegra-common.h
@@ -20,6 +20,8 @@
#include <asm/arch/tegra.h> /* get chip and board defs */
#define CONFIG_DM
+#define CONFIG_DM_GPIO
+#define CONFIG_CMD_DM
#define CONFIG_SYS_TIMER_RATE 1000000
#define CONFIG_SYS_TIMER_COUNTER NV_PA_TMRUS_BASE
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 16:09 ` [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use " Simon Glass
@ 2014-05-05 17:29 ` Stephen Warren
2014-05-05 19:53 ` Simon Glass
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Warren @ 2014-05-05 17:29 UTC (permalink / raw)
To: u-boot
On 05/05/2014 10:09 AM, Simon Glass wrote:
> This is an implementation of GPIOs for Tegra that uses driver model. It has
> been tested on trimslice and also using the new iotrace feature.
>
> The implementation uses a top-level GPIO device (which has no actual GPIOS).
> Under this all the banks are created as separate GPIO devices.
I don't think that dividing the GPIOs into banks is appropriate. While
the textual naming of Tegra GPIOs is based on the bank concept, I think
that division should only apply to the textual naming, and not any data
structures or device registration, etc. In fact, I often refer to Tegra
GPIOs using their integer ID rather than their name, and dividing the
controller into separate banks probably technically disallows this,
since there would be no architected guarantee that the numbering of the
banks would be sequential. Contiguous numbering of all GPIOs within the
controller is also useful for correlation with pinmux numbering of pins.
...
> Since driver model is not yet available before relocation, or in SPL, a
> special function is provided for seaboard's SPL code.
Perhaps we should just remove that code from Seaboard, since sharing the
UART/SPI pins really isn't a useful feature.
Still, we will need to maintain some low-level APIs so that the SPL can
initialize GPIOs at boot, since doing so is part of Tegra's HW-defined
pinmux initialization mechanism. For reference, see the series I posted:
https://www.mail-archive.com/u-boot at lists.denx.de/msg136607.html
In particular:
ARM: tegra: add GPIO initialization table function
ARM: tegra: make use of GPIO init table on Jetson TK1
> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
> +struct tegra_gpio_platdata {
> + struct gpio_ctlr_bank *bank;
> + const char *port_name; /* Name of port, e.g. "B" */
> + int base_port; /* Port number for this port (0, 1,.., n-1) */
> +};
I'm not sure what "platdata" means. Would "tegra_gpio_bank" be a better
name?
Oh, looking later in the patch, this seems to be about passing data to
probe(). It would be better to get rid of the bank concept, have a
single device, and just pass the GPIO count or SoC type to probe.
> +/* Information about each port at run-time */
> +struct tegra_port_info {
> + char label[TEGRA_GPIOS_PER_PORT][GPIO_NAME_SIZE];
> + struct gpio_ctlr_bank *bank;
> + int base_port; /* Port number for this port (0, 1,.., n-1) */
> +};
It seems odd to have two data-structures with Tegra-specific data
related to a struct gpio_ctrl_bank. Perhaps these can be combined into one?
> +#define GPIO_NUM(state, offset) \
> + (((state)->base_port * TEGRA_GPIOS_PER_PORT) + (offset))
It's not clear whether "state" is supposed to be a "tegra_gpio_platdata"
or a "tegra_port_info". I guess the macro works with either, but that
seems a bit dangerous.
> /* Return config of pin 'gpio' as GPIO (1) or SFPIO (0) */
> +static int get_config(struct tegra_port_info *state, int offset)
Hmm. I guess the name "state" for the GPIO_NUM() macro parameter comes
from the fact that the functions in this file have a parameter named
"state". That seems nasty; it'd be better to name that port_info so it
represents that type it contains. Still, that's a pre-existing issue
unrelated to this patch.
That said, naming a macro parameter "state" and using it in functions
that typically have a parameter named "state" seems dangerous. Perhaps
prefix and suffix the macro parameter name with _ to make the
distinction obvious - i.e.. _state_. That's typical good practice for
macros.
> +static int tegra_gpio_get_state(struct device *dev, unsigned int offset,
> + char *buf, int bufsize)
> + is_gpio = get_config(state, offset); /* GPIO, not SFPIO */
Typo in SFIO there.
> + size = snprintf(buf, bufsize, "%s%d: ",
> + uc_priv->bank_name ? uc_priv->bank_name : "", offset);
> + buf += size;
> + bufsize -= size;
size can be larger than bufsize if the string would have overflowed. Do
the later calls to snprintf() handle negative size arguments correctly?
> +static char *gpio_port_name(int base_port)
> {
> + char *name, *s;
> +
> + name = malloc(3);
It seems like this should be statically allocated (e.g. as part of *plat
in gpio_tegra_bind() or something?). Still, if we stop splitting things
into banks, we can completely get rid of this.
> +static int gpio_tegra_bind(struct device *parent)
> + /*
> + * This driver does not make use of interrupts, other than to figure
> + * out the number of GPIO banks
> + */
> + if (!fdt_getprop(gd->fdt_blob, parent->of_offset, "interrupts", &len))
> + return -EINVAL;
> + bank_count = len / 3 / sizeof(u32);
> + ctlr = (struct gpio_ctlr *)fdtdec_get_addr(gd->fdt_blob,
> + parent->of_offset, "reg");
It's dangerous to assume that #interrupt-cells of the GIC is 3. The GIC
driver is the only thing that can define/parse GIC IRQ specifiers...
> diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
> #define CONFIG_DM
> +#define CONFIG_DM_GPIO
> +#define CONFIG_CMD_DM
I think the second of those lines should be part of patch 12/13
shouldn't it?
^ permalink raw reply [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 17:29 ` Stephen Warren
@ 2014-05-05 19:53 ` Simon Glass
2014-05-05 21:14 ` Stephen Warren
0 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-05 19:53 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 5 May 2014 11:29, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/05/2014 10:09 AM, Simon Glass wrote:
>> This is an implementation of GPIOs for Tegra that uses driver model. It has
>> been tested on trimslice and also using the new iotrace feature.
>>
>> The implementation uses a top-level GPIO device (which has no actual GPIOS).
>> Under this all the banks are created as separate GPIO devices.
>
> I don't think that dividing the GPIOs into banks is appropriate. While
> the textual naming of Tegra GPIOs is based on the bank concept, I think
> that division should only apply to the textual naming, and not any data
> structures or device registration, etc. In fact, I often refer to Tegra
> GPIOs using their integer ID rather than their name, and dividing the
> controller into separate banks probably technically disallows this,
> since there would be no architected guarantee that the numbering of the
> banks would be sequential. Contiguous numbering of all GPIOs within the
> controller is also useful for correlation with pinmux numbering of pins.
This is how the GPIO uclass works at present. Do you think that should
change also? It would mean enhancing the uclass to support multiple
GPIO bank names, with each having a number of GPIOs attached. This is
the sort of complexity that adds to code size. On the other hand it's
something that we may want to do anyway as more SOC's GPIO drivers are
converted.
An alternative might be to leave most the Tegra driver (the static
functions at the top) using an absolute GPIO number, but have a
separate device for each bank. That would be easy enough and would not
add to code.
>
> ...
>> Since driver model is not yet available before relocation, or in SPL, a
>> special function is provided for seaboard's SPL code.
>
> Perhaps we should just remove that code from Seaboard, since sharing the
> UART/SPI pins really isn't a useful feature.
saveenv to SPI won't work without it though.
>
> Still, we will need to maintain some low-level APIs so that the SPL can
> initialize GPIOs at boot, since doing so is part of Tegra's HW-defined
> pinmux initialization mechanism. For reference, see the series I posted:
>
> https://www.mail-archive.com/u-boot at lists.denx.de/msg136607.html
>
> In particular:
> ARM: tegra: add GPIO initialization table function
> ARM: tegra: make use of GPIO init table on Jetson TK1
OK.
>
>> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
>
>> +struct tegra_gpio_platdata {
>> + struct gpio_ctlr_bank *bank;
>> + const char *port_name; /* Name of port, e.g. "B" */
>> + int base_port; /* Port number for this port (0, 1,.., n-1) */
>> +};
>
> I'm not sure what "platdata" means. Would "tegra_gpio_bank" be a better
> name?
>
> Oh, looking later in the patch, this seems to be about passing data to
> probe(). It would be better to get rid of the bank concept, have a
> single device, and just pass the GPIO count or SoC type to probe.
Probably better to improve the name - I wanted to distinguish between
platform data and run-time data.
>
>> +/* Information about each port at run-time */
>> +struct tegra_port_info {
>> + char label[TEGRA_GPIOS_PER_PORT][GPIO_NAME_SIZE];
>> + struct gpio_ctlr_bank *bank;
>> + int base_port; /* Port number for this port (0, 1,.., n-1) */
>> +};
>
> It seems odd to have two data-structures with Tegra-specific data
> related to a struct gpio_ctrl_bank. Perhaps these can be combined into one?
One is static platform data (not supposed to be changed). It is
created before the device is bound. The other is the run-time
information, that only exists when the device is active / probed.
>
>> +#define GPIO_NUM(state, offset) \
>> + (((state)->base_port * TEGRA_GPIOS_PER_PORT) + (offset))
>
> It's not clear whether "state" is supposed to be a "tegra_gpio_platdata"
> or a "tegra_port_info". I guess the macro works with either, but that
> seems a bit dangerous.
Should probably be a function.
>
>> /* Return config of pin 'gpio' as GPIO (1) or SFPIO (0) */
>> +static int get_config(struct tegra_port_info *state, int offset)
>
> Hmm. I guess the name "state" for the GPIO_NUM() macro parameter comes
> from the fact that the functions in this file have a parameter named
> "state". That seems nasty; it'd be better to name that port_info so it
> represents that type it contains. Still, that's a pre-existing issue
> unrelated to this patch.
>
> That said, naming a macro parameter "state" and using it in functions
> that typically have a parameter named "state" seems dangerous. Perhaps
> prefix and suffix the macro parameter name with _ to make the
> distinction obvious - i.e.. _state_. That's typical good practice for
> macros.
OK
>
>> +static int tegra_gpio_get_state(struct device *dev, unsigned int offset,
>> + char *buf, int bufsize)
>
>> + is_gpio = get_config(state, offset); /* GPIO, not SFPIO */
>
> Typo in SFIO there.
>
>> + size = snprintf(buf, bufsize, "%s%d: ",
>> + uc_priv->bank_name ? uc_priv->bank_name : "", offset);
>> + buf += size;
>> + bufsize -= size;
>
> size can be larger than bufsize if the string would have overflowed. Do
> the later calls to snprintf() handle negative size arguments correctly?
Should do - I checked that when I upstreamed that code.
>
>> +static char *gpio_port_name(int base_port)
>> {
>> + char *name, *s;
>> +
>> + name = malloc(3);
>
> It seems like this should be statically allocated (e.g. as part of *plat
> in gpio_tegra_bind() or something?). Still, if we stop splitting things
> into banks, we can completely get rid of this.
No, we still need the name so that 'gpio input T3' works corrrectly.
>
>> +static int gpio_tegra_bind(struct device *parent)
>
>> + /*
>> + * This driver does not make use of interrupts, other than to figure
>> + * out the number of GPIO banks
>> + */
>> + if (!fdt_getprop(gd->fdt_blob, parent->of_offset, "interrupts", &len))
>> + return -EINVAL;
>> + bank_count = len / 3 / sizeof(u32);
>> + ctlr = (struct gpio_ctlr *)fdtdec_get_addr(gd->fdt_blob,
>> + parent->of_offset, "reg");
>
> It's dangerous to assume that #interrupt-cells of the GIC is 3. The GIC
> driver is the only thing that can define/parse GIC IRQ specifiers...
Which GIC driver would that be? :-)
>
>> diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
>
>> #define CONFIG_DM
>> +#define CONFIG_DM_GPIO
>> +#define CONFIG_CMD_DM
>
> I think the second of those lines should be part of patch 12/13
> shouldn't it?
>
Yes, will move it.
Thanks for looking at this. BTW if we decide to change the bank
approach later I'm happy to adjust this driver.
Regards,
Simon
^ permalink raw reply [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 19:53 ` Simon Glass
@ 2014-05-05 21:14 ` Stephen Warren
2014-05-05 21:30 ` Simon Glass
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Warren @ 2014-05-05 21:14 UTC (permalink / raw)
To: u-boot
On 05/05/2014 01:53 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 5 May 2014 11:29, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/05/2014 10:09 AM, Simon Glass wrote:
>>> This is an implementation of GPIOs for Tegra that uses driver model. It has
>>> been tested on trimslice and also using the new iotrace feature.
>>>
>>> The implementation uses a top-level GPIO device (which has no actual GPIOS).
>>> Under this all the banks are created as separate GPIO devices.
>>
>> I don't think that dividing the GPIOs into banks is appropriate. While
>> the textual naming of Tegra GPIOs is based on the bank concept, I think
>> that division should only apply to the textual naming, and not any data
>> structures or device registration, etc. In fact, I often refer to Tegra
>> GPIOs using their integer ID rather than their name, and dividing the
>> controller into separate banks probably technically disallows this,
>> since there would be no architected guarantee that the numbering of the
>> banks would be sequential. Contiguous numbering of all GPIOs within the
>> controller is also useful for correlation with pinmux numbering of pins.
>
> This is how the GPIO uclass works at present. Do you think that should
> change also? It would mean enhancing the uclass to support multiple
> GPIO bank names, with each having a number of GPIOs attached. This is
> the sort of complexity that adds to code size. On the other hand it's
> something that we may want to do anyway as more SOC's GPIO drivers are
> converted.
Surely this means simplifying the core to completely remove any concept
of GPIO banks. I really don't see any need for a 2-level hierarchy. Just
have one struct/object/... that represents a GPIO
controller/module/chip/... Each of those has N GPIOs numbered 0..N.
Linux has managed to get away without a 2-level controller/bank
approach, so there's certainly evidence it works in practice.
>> ...
>>> Since driver model is not yet available before relocation, or in SPL, a
>>> special function is provided for seaboard's SPL code.
>>
>> Perhaps we should just remove that code from Seaboard, since sharing the
>> UART/SPI pins really isn't a useful feature.
>
> saveenv to SPI won't work without it though.
The environment is stored in eMMC.
SPI simply doesn't work sanely on Seaboard, and we should just rip out
any support for it at all. On Seaboard itself, people can just set the
jumpers to ignore SPI completely. On Springbank, something similar can
be done with the pull-up/down resistors. I've been using my Springbank
without any of the resistors present, while disables SPI completely, for
years without issue...
>>> +static char *gpio_port_name(int base_port)
>>> {
>>> + char *name, *s;
>>> +
>>> + name = malloc(3);
>>
>> It seems like this should be statically allocated (e.g. as part of *plat
>> in gpio_tegra_bind() or something?). Still, if we stop splitting things
>> into banks, we can completely get rid of this.
>
> No, we still need the name so that 'gpio input T3' works corrrectly.
Why do we need GPIO names at all? "gpio input 155" works just as well,
and to be honest is often a lot more convenient that trying to maps
things back to a name.
^ permalink raw reply [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 21:14 ` Stephen Warren
@ 2014-05-05 21:30 ` Simon Glass
2014-05-05 22:07 ` Stephen Warren
0 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-05 21:30 UTC (permalink / raw)
To: u-boot
HI Stephen,
On 5 May 2014 15:14, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/05/2014 01:53 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On 5 May 2014 11:29, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 05/05/2014 10:09 AM, Simon Glass wrote:
>>>> This is an implementation of GPIOs for Tegra that uses driver model. It has
>>>> been tested on trimslice and also using the new iotrace feature.
>>>>
>>>> The implementation uses a top-level GPIO device (which has no actual GPIOS).
>>>> Under this all the banks are created as separate GPIO devices.
>>>
>>> I don't think that dividing the GPIOs into banks is appropriate. While
>>> the textual naming of Tegra GPIOs is based on the bank concept, I think
>>> that division should only apply to the textual naming, and not any data
>>> structures or device registration, etc. In fact, I often refer to Tegra
>>> GPIOs using their integer ID rather than their name, and dividing the
>>> controller into separate banks probably technically disallows this,
>>> since there would be no architected guarantee that the numbering of the
>>> banks would be sequential. Contiguous numbering of all GPIOs within the
>>> controller is also useful for correlation with pinmux numbering of pins.
>>
>> This is how the GPIO uclass works at present. Do you think that should
>> change also? It would mean enhancing the uclass to support multiple
>> GPIO bank names, with each having a number of GPIOs attached. This is
>> the sort of complexity that adds to code size. On the other hand it's
>> something that we may want to do anyway as more SOC's GPIO drivers are
>> converted.
>
> Surely this means simplifying the core to completely remove any concept
> of GPIO banks. I really don't see any need for a 2-level hierarchy. Just
> have one struct/object/... that represents a GPIO
> controller/module/chip/... Each of those has N GPIOs numbered 0..N.
> Linux has managed to get away without a 2-level controller/bank
> approach, so there's certainly evidence it works in practice.
I think you have it backwards...the current implementation has a
single level of hierarchy. Each driver handles one bank (or 'port' in
the case of Tegra). What you are talking about is having a single
driver handle multiple banks, thus requiring that the driver have a
second level to deal with banks, over and above the device. We might
end up with that, but I would prefer to move to it when we have
evidence that it is a general need.
>
>>> ...
>>>> Since driver model is not yet available before relocation, or in SPL, a
>>>> special function is provided for seaboard's SPL code.
>>>
>>> Perhaps we should just remove that code from Seaboard, since sharing the
>>> UART/SPI pins really isn't a useful feature.
>>
>> saveenv to SPI won't work without it though.
>
> The environment is stored in eMMC.
>
> SPI simply doesn't work sanely on Seaboard, and we should just rip out
> any support for it at all. On Seaboard itself, people can just set the
> jumpers to ignore SPI completely. On Springbank, something similar can
> be done with the pull-up/down resistors. I've been using my Springbank
> without any of the resistors present, while disables SPI completely, for
> years without issue...
OK, how about you submit a patch to do that, and then I can drop my
special case (or maybe not if your other patch goes in)?
>
>>>> +static char *gpio_port_name(int base_port)
>>>> {
>>>> + char *name, *s;
>>>> +
>>>> + name = malloc(3);
>>>
>>> It seems like this should be statically allocated (e.g. as part of *plat
>>> in gpio_tegra_bind() or something?). Still, if we stop splitting things
>>> into banks, we can completely get rid of this.
>>
>> No, we still need the name so that 'gpio input T3' works corrrectly.
>
> Why do we need GPIO names at all? "gpio input 155" works just as well,
> and to be honest is often a lot more convenient that trying to maps
> things back to a name.
Eh? We need to support named GPIOs in U-Boot. 155 is a meaningless
number which drivers people back and forth to the datasheets, their
calculators, a long table, etc. Even the Tegra device tree has moved
away from numbers to GPIO names, I notice.
Regards,
Simon
^ permalink raw reply [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 21:30 ` Simon Glass
@ 2014-05-05 22:07 ` Stephen Warren
2014-05-05 23:00 ` Simon Glass
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Warren @ 2014-05-05 22:07 UTC (permalink / raw)
To: u-boot
On 05/05/2014 03:30 PM, Simon Glass wrote:
...
> I think you have it backwards...the current implementation has a
> single level of hierarchy. Each driver handles one bank (or 'port' in
> the case of Tegra). What you are talking about is having a single
> driver handle multiple banks, thus requiring that the driver have a
> second level to deal with banks, over and above the device. We might
> end up with that, but I would prefer to move to it when we have
> evidence that it is a general need.
Sigh. This is getting silly. All the APIs in SW need to just take a GPIO
ID in the flat range 0..N (N==223 for Tegra20) and deal with it.
Anything that expose banks anywhere, either as a parameter to public
functions exported from the GPIO controller driver, or as the existence
of separate drivers for separate banks, or as a command-line argument
that the user sees, or ..., whether it be the U-Boot GPIO core or the
Tegra GPIO driver itself that causes this, is just pointless.
Please take a look at what the Linux driver does, and just model that.
It's very trivial. The following is the entire implementation of gpio_set():
> // helper macros used by all register accesses:
> #define GPIO_BANK(x) ((x) >> 5)
> #define GPIO_PORT(x) (((x) >> 3) & 0x3)
> #define GPIO_BIT(x) ((x) & 0x7)
>
> #define GPIO_REG(x) (GPIO_BANK(x) * tegra_gpio_bank_stride + \
> GPIO_PORT(x) * 4)
>
> // one define per register
> #define GPIO_MSK_OUT(x) (GPIO_REG(x) + tegra_gpio_upper_offset + 0x20)
>
> // helper used by a lot of set/clear functions
> static void tegra_gpio_mask_write(u32 reg, int gpio, int value)
> {
> u32 val;
>
> val = 0x100 << GPIO_BIT(gpio);
> if (value)
> val |= 1 << GPIO_BIT(gpio);
> tegra_gpio_writel(val, reg);
> }
>
> static void tegra_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> {
> tegra_gpio_mask_write(GPIO_MSK_OUT(offset), offset, value);
> }
i.e. a single register write with a calculated register address, using
just a few shifts/masks.
>>>> It seems like this should be statically allocated (e.g. as part of *plat
>>>> in gpio_tegra_bind() or something?). Still, if we stop splitting things
>>>> into banks, we can completely get rid of this.
>>>
>>> No, we still need the name so that 'gpio input T3' works corrrectly.
>>
>> Why do we need GPIO names at all? "gpio input 155" works just as well,
>> and to be honest is often a lot more convenient that trying to maps
>> things back to a name.
>
> Eh? We need to support named GPIOs in U-Boot. 155 is a meaningless
> number which drivers people back and forth to the datasheets, their
> calculators, a long table, etc. Even the Tegra device tree has moved
> away from numbers to GPIO names, I notice.
The GPIO names are meaningless. I say this because all the Tegra
schematics (and documentation that drives them) use the pin/pad name,
which is almost always entirely different from the GPIO name. You have
to map the pin name back to either the GPIO name or ID using a lookup
table (such as the kernel's drivers/pinctrl/pinctrl-tegra20.c). Given
the need for a lookup table, we should just use the simpler GPIO ID and
not worry about GPIO names. There's no point screwing around with text
names when we can just use simple numbers.
In DT, GPIOs are specified by integer ID too. Admittedly we have macros
that calculate those IDs from the bank/port/offset, but that was
probably a mistake, since the bank/port/offset names aren't meaningful.
^ permalink raw reply [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 22:07 ` Stephen Warren
@ 2014-05-05 23:00 ` Simon Glass
2014-05-06 17:34 ` Stephen Warren
0 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-05 23:00 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 5 May 2014 16:07, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/05/2014 03:30 PM, Simon Glass wrote:
> ...
> > I think you have it backwards...the current implementation has a
> > single level of hierarchy. Each driver handles one bank (or 'port' in
> > the case of Tegra). What you are talking about is having a single
> > driver handle multiple banks, thus requiring that the driver have a
> > second level to deal with banks, over and above the device. We might
> > end up with that, but I would prefer to move to it when we have
> > evidence that it is a general need.
>
> Sigh. This is getting silly. All the APIs in SW need to just take a GPIO
> ID in the flat range 0..N (N==223 for Tegra20) and deal with it.
> Anything that expose banks anywhere, either as a parameter to public
> functions exported from the GPIO controller driver, or as the existence
> of separate drivers for separate banks, or as a command-line argument
> that the user sees, or ..., whether it be the U-Boot GPIO core or the
> Tegra GPIO driver itself that causes this, is just pointless.
>
For exynos, the banks are not contiguous and there is quite a bit of
fiddling to make this all work. You may have seen the series going in at
the moment to number the GPIOs properly.
I understand that Tegra is very straightforward though.
> Please take a look at what the Linux driver does, and just model that.
> It's very trivial. The following is the entire implementation of
> gpio_set():
>
> > // helper macros used by all register accesses:
> > #define GPIO_BANK(x) ((x) >> 5)
> > #define GPIO_PORT(x) (((x) >> 3) & 0x3)
> > #define GPIO_BIT(x) ((x) & 0x7)
> >
> > #define GPIO_REG(x) (GPIO_BANK(x) * tegra_gpio_bank_stride +
> \
> > GPIO_PORT(x) * 4)
> >
> > // one define per register
> > #define GPIO_MSK_OUT(x) (GPIO_REG(x) + tegra_gpio_upper_offset +
> 0x20)
> >
> > // helper used by a lot of set/clear functions
> > static void tegra_gpio_mask_write(u32 reg, int gpio, int value)
> > {
> > u32 val;
> >
> > val = 0x100 << GPIO_BIT(gpio);
> > if (value)
> > val |= 1 << GPIO_BIT(gpio);
> > tegra_gpio_writel(val, reg);
> > }
> >
> > static void tegra_gpio_set(struct gpio_chip *chip, unsigned offset, int
> value)
> > {
> > tegra_gpio_mask_write(GPIO_MSK_OUT(offset), offset, value);
> > }
>
> i.e. a single register write with a calculated register address, using
> just a few shifts/masks.
>
> >>>> It seems like this should be statically allocated (e.g. as part of
> *plat
> >>>> in gpio_tegra_bind() or something?). Still, if we stop splitting
> things
> >>>> into banks, we can completely get rid of this.
> >>>
> >>> No, we still need the name so that 'gpio input T3' works corrrectly.
> >>
> >> Why do we need GPIO names at all? "gpio input 155" works just as well,
> >> and to be honest is often a lot more convenient that trying to maps
> >> things back to a name.
> >
> > Eh? We need to support named GPIOs in U-Boot. 155 is a meaningless
> > number which drivers people back and forth to the datasheets, their
> > calculators, a long table, etc. Even the Tegra device tree has moved
> > away from numbers to GPIO names, I notice.
>
> The GPIO names are meaningless. I say this because all the Tegra
> schematics (and documentation that drives them) use the pin/pad name,
> which is almost always entirely different from the GPIO name. You have
> to map the pin name back to either the GPIO name or ID using a lookup
> table (such as the kernel's drivers/pinctrl/pinctrl-tegra20.c). Given
> the need for a lookup table, we should just use the simpler GPIO ID and
> not worry about GPIO names. There's no point screwing around with text
> names when we can just use simple numbers.
>
> In DT, GPIOs are specified by integer ID too. Admittedly we have macros
> that calculate those IDs from the bank/port/offset, but that was
> probably a mistake, since the bank/port/offset names aren't meaningful.
>
U-Boot provides a friendly named interface for GPIOs. I see that as a
requirement for driver model too. As someone who has spent a lot of time at
the command line fiddling with hardware, I don't want to go backwards in
the driver model conversion. Similarly, using numbers in the DT is very
unfriendly and painful IMO.
If we can agree on the friendly names, then let's talk about how to
simplify things.
Regards,
Simon
^ permalink raw reply [flat|nested] 31+ messages in thread* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-05 23:00 ` Simon Glass
@ 2014-05-06 17:34 ` Stephen Warren
2014-05-06 20:28 ` Simon Glass
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Warren @ 2014-05-06 17:34 UTC (permalink / raw)
To: u-boot
On 05/05/2014 05:00 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 5 May 2014 16:07, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
>
> On 05/05/2014 03:30 PM, Simon Glass wrote:
> ...
> > I think you have it backwards...the current implementation has a
> > single level of hierarchy. Each driver handles one bank (or 'port' in
> > the case of Tegra). What you are talking about is having a single
> > driver handle multiple banks, thus requiring that the driver have a
> > second level to deal with banks, over and above the device. We might
> > end up with that, but I would prefer to move to it when we have
> > evidence that it is a general need.
>
> Sigh. This is getting silly. All the APIs in SW need to just take a GPIO
> ID in the flat range 0..N (N==223 for Tegra20) and deal with it.
> Anything that expose banks anywhere, either as a parameter to public
> functions exported from the GPIO controller driver, or as the existence
> of separate drivers for separate banks, or as a command-line argument
> that the user sees, or ..., whether it be the U-Boot GPIO core or the
> Tegra GPIO driver itself that causes this, is just pointless.
>
>
> For exynos, the banks are not contiguous and there is quite a bit of
> fiddling to make this all work. You may have seen the series going in at
> the moment to number the GPIOs properly.
>
> I understand that Tegra is very straightforward though.
Sure, HW that truly is designed as separate banks should be represented
as such. However, the existence of such HW shouldn't force *other* HW to
artificially expose banks of GPIOs when there's no need/desire to.
> > Eh? We need to support named GPIOs in U-Boot. 155 is a meaningless
> > number which drivers people back and forth to the datasheets, their
> > calculators, a long table, etc. Even the Tegra device tree has moved
> > away from numbers to GPIO names, I notice.
>
> The GPIO names are meaningless. I say this because all the Tegra
> schematics (and documentation that drives them) use the pin/pad name,
> which is almost always entirely different from the GPIO name. You have
> to map the pin name back to either the GPIO name or ID using a lookup
> table (such as the kernel's drivers/pinctrl/pinctrl-tegra20.c). Given
> the need for a lookup table, we should just use the simpler GPIO ID and
> not worry about GPIO names. There's no point screwing around with text
> names when we can just use simple numbers.
>
> In DT, GPIOs are specified by integer ID too. Admittedly we have macros
> that calculate those IDs from the bank/port/offset, but that was
> probably a mistake, since the bank/port/offset names aren't meaningful.
>
>
> U-Boot provides a friendly named interface for GPIOs. I see that as a
> requirement for driver model too. As someone who has spent a lot of time
> at the command line fiddling with hardware, I don't want to go backwards
> in the driver model conversion. Similarly, using numbers in the DT is
> very unfriendly and painful IMO.
>
> If we can agree on the friendly names, then let's talk about how to
> simplify things.
To be honest, I disagree that meaningless names are friendly. Almost
everything else deals with numbers. The values in DT are numbers. The
debugfs files in Linux are numbers. The sysfs ABI in Linux is numbers.
The current GPIO interface in U-Boot is numbers. The correlation with
pinctrl pins is numbers.
If the following happens, then I could live with a (part-time)
name-based API:
* The U-Boot commands accept either a name or a number. That would allow
people to use what they want.
* The API implemented by U-Boot GPIO drivers uses numbers exclusively.
Note that when I say numbers above, all the numbering should be relative
to a particular controller. So, I don't mean something like:
gpio set 1056
... where 1056 is 1000 (Tegra GPIO controller base) + 56 (Tegra GPIO
ID). Instead, I would expect the command-line interface to be:
gpio set tegra 56
gpio set tegra PH0
... where tegra is the name of the GPIO controller instance, and 56/PH0
is the GPIO ID within that one GPIO controller.
The controller name could be pca9555-0 (0th instance of a pca9555
device) or i2c-0-56 (I2C bus 0 device address 0x56) or whatever naming
style you want.
To support this, I would expect the GPIO driver API to contain just two
APIs that know about the GPIO names:
int name_to_gpio_id(const char *gpio_name);
const char *gpio_id_to_name(int gpio_id);
All the other GPIO driver APIs would take "int gpio_id"
(controller-relative integer ID).
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-06 17:34 ` Stephen Warren
@ 2014-05-06 20:28 ` Simon Glass
2014-05-06 20:37 ` Stephen Warren
0 siblings, 1 reply; 31+ messages in thread
From: Simon Glass @ 2014-05-06 20:28 UTC (permalink / raw)
To: u-boot
HI Stephen,
On 6 May 2014 11:34, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/05/2014 05:00 PM, Simon Glass wrote:
> > Hi Stephen,
> >
> > On 5 May 2014 16:07, Stephen Warren <swarren@wwwdotorg.org
> > <mailto:swarren@wwwdotorg.org>> wrote:
> >
> > On 05/05/2014 03:30 PM, Simon Glass wrote:
> > ...
> > > I think you have it backwards...the current implementation has a
> > > single level of hierarchy. Each driver handles one bank (or 'port'
> in
> > > the case of Tegra). What you are talking about is having a single
> > > driver handle multiple banks, thus requiring that the driver have a
> > > second level to deal with banks, over and above the device. We
> might
> > > end up with that, but I would prefer to move to it when we have
> > > evidence that it is a general need.
> >
> > Sigh. This is getting silly. All the APIs in SW need to just take a
> GPIO
> > ID in the flat range 0..N (N==223 for Tegra20) and deal with it.
> > Anything that expose banks anywhere, either as a parameter to public
> > functions exported from the GPIO controller driver, or as the
> existence
> > of separate drivers for separate banks, or as a command-line argument
> > that the user sees, or ..., whether it be the U-Boot GPIO core or the
> > Tegra GPIO driver itself that causes this, is just pointless.
> >
> >
> > For exynos, the banks are not contiguous and there is quite a bit of
> > fiddling to make this all work. You may have seen the series going in at
> > the moment to number the GPIOs properly.
> >
> > I understand that Tegra is very straightforward though.
>
> Sure, HW that truly is designed as separate banks should be represented
> as such. However, the existence of such HW shouldn't force *other* HW to
> artificially expose banks of GPIOs when there's no need/desire to.
>
OK, well that's mostly an implementation issue, will take a look.
>
> > > Eh? We need to support named GPIOs in U-Boot. 155 is a meaningless
> > > number which drivers people back and forth to the datasheets, their
> > > calculators, a long table, etc. Even the Tegra device tree has
> moved
> > > away from numbers to GPIO names, I notice.
> >
> > The GPIO names are meaningless. I say this because all the Tegra
> > schematics (and documentation that drives them) use the pin/pad name,
> > which is almost always entirely different from the GPIO name. You
> have
> > to map the pin name back to either the GPIO name or ID using a lookup
> > table (such as the kernel's drivers/pinctrl/pinctrl-tegra20.c). Given
> > the need for a lookup table, we should just use the simpler GPIO ID
> and
> > not worry about GPIO names. There's no point screwing around with
> text
> > names when we can just use simple numbers.
> >
> > In DT, GPIOs are specified by integer ID too. Admittedly we have
> macros
> > that calculate those IDs from the bank/port/offset, but that was
> > probably a mistake, since the bank/port/offset names aren't
> meaningful.
> >
> >
> > U-Boot provides a friendly named interface for GPIOs. I see that as a
> > requirement for driver model too. As someone who has spent a lot of time
> > at the command line fiddling with hardware, I don't want to go backwards
> > in the driver model conversion. Similarly, using numbers in the DT is
> > very unfriendly and painful IMO.
> >
> > If we can agree on the friendly names, then let's talk about how to
> > simplify things.
>
> To be honest, I disagree that meaningless names are friendly. Almost
> everything else deals with numbers. The values in DT are numbers. The
> debugfs files in Linux are numbers. The sysfs ABI in Linux is numbers.
> The current GPIO interface in U-Boot is numbers. The correlation with
> pinctrl pins is numbers.
>
> If the following happens, then I could live with a (part-time)
> name-based API:
>
> * The U-Boot commands accept either a name or a number. That would allow
> people to use what they want.
>
> * The API implemented by U-Boot GPIO drivers uses numbers exclusively.
>
> Note that when I say numbers above, all the numbering should be relative
> to a particular controller. So, I don't mean something like:
>
> gpio set 1056
>
> ... where 1056 is 1000 (Tegra GPIO controller base) + 56 (Tegra GPIO
> ID). Instead, I would expect the command-line interface to be:
>
> gpio set tegra 56
> gpio set tegra PH0
>
> ... where tegra is the name of the GPIO controller instance, and 56/PH0
> is the GPIO ID within that one GPIO controller.
>
> The controller name could be pca9555-0 (0th instance of a pca9555
> device) or i2c-0-56 (I2C bus 0 device address 0x56) or whatever naming
> style you want.
>
> To support this, I would expect the GPIO driver API to contain just two
> APIs that know about the GPIO names:
>
> int name_to_gpio_id(const char *gpio_name);
> const char *gpio_id_to_name(int gpio_id);
>
> All the other GPIO driver APIs would take "int gpio_id"
> (controller-relative integer ID).
>
See gpio_lookup_name() which is where this lives.
The GPIO uclass does sequentially number GPIOs, but be aware that on
platforms with multiple GPIO controllers (e.g. an I2C GPIO extender) you
might hit a problem where the tegra GPIOs are not first, so might start at
8 or 16, for example. However I think that probably can be resolved when we
come to it.
It was always my intention to support numbered GPIOs, but the current
function doesn't do that except for anonymous banks - I'll update that as a
separate patch.
OK?
Regards,
Simon
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-06 20:28 ` Simon Glass
@ 2014-05-06 20:37 ` Stephen Warren
2014-05-06 20:41 ` Simon Glass
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Warren @ 2014-05-06 20:37 UTC (permalink / raw)
To: u-boot
On 05/06/2014 02:28 PM, Simon Glass wrote:
...
> The GPIO uclass does sequentially number GPIOs, but be aware that on
> platforms with multiple GPIO controllers (e.g. an I2C GPIO extender) you
> might hit a problem where the tegra GPIOs are not first, so might start
> at 8 or 16, for example. However I think that probably can be resolved
> when we come to it.
That fact shouldn't be exposed to the user. If all the GPIO IDs are
relative to a specific named GPIO device, then the user will never see
the internal offset. Indeed, the GPIO driver for a particular GPIO HW
module/chip wouldn't ever see the offset either. In fact, we shouldn't
have/introduce a flat/global GPIO numbering space at all. The Linux
community has learned that doesn't work very well, and is moving away
from it in the more recent "gpiod" API for example. Everything should be
identified as a tuple (GPIO controller handle, GPIO ID within that GPIO
controller).
^ permalink raw reply [flat|nested] 31+ messages in thread
* [U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model
2014-05-06 20:37 ` Stephen Warren
@ 2014-05-06 20:41 ` Simon Glass
0 siblings, 0 replies; 31+ messages in thread
From: Simon Glass @ 2014-05-06 20:41 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 6 May 2014 14:37, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/06/2014 02:28 PM, Simon Glass wrote:
> ...
> > The GPIO uclass does sequentially number GPIOs, but be aware that on
> > platforms with multiple GPIO controllers (e.g. an I2C GPIO extender) you
> > might hit a problem where the tegra GPIOs are not first, so might start
> > at 8 or 16, for example. However I think that probably can be resolved
> > when we come to it.
>
> That fact shouldn't be exposed to the user. If all the GPIO IDs are
> relative to a specific named GPIO device, then the user will never see
> the internal offset. Indeed, the GPIO driver for a particular GPIO HW
> module/chip wouldn't ever see the offset either. In fact, we shouldn't
> have/introduce a flat/global GPIO numbering space at all. The Linux
> community has learned that doesn't work very well, and is moving away
> from it in the more recent "gpiod" API for example. Everything should be
> identified as a tuple (GPIO controller handle, GPIO ID within that GPIO
> controller).
>
>
Well that would be fairly easy to add my modifying the gpio command.
Nothing we have talked about here precludes that, and driver model of
course supports it. We should be able to make it backwards compatible too.
Regards,
Simon
^ permalink raw reply [flat|nested] 31+ messages in thread