* [PATCH v5 03/11] drivers: of: add initialization code for dynamic reserved memory
From: Marek Szyprowski @ 2014-02-21 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, linaro-mm-sig, devicetree,
linux-doc
Cc: Marek Szyprowski, Benjamin Herrenschmidt, Arnd Bergmann,
Michal Nazarewicz, Grant Likely, Tomasz Figa, Sascha Hauer,
Laura Abbott, Rob Herring, Olof Johansson, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Tomasz Figa,
Kumar Gala, Nishanth Peethambaran, Marc, Josh Cartwright,
Catalin Marinas, Will Deacon, Paul Mackerras
In-Reply-To: <1392985527-6260-1-git-send-email-m.szyprowski@samsung.com>
This patch adds support for dynamically allocated reserved memory regions
declared in device tree. Such regions are defined by 'size', 'alignment'
and 'alloc-ranges' properties.
Based on previous code provided by Josh Cartwright <joshc@codeaurora.org>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/of/Kconfig | 6 ++
drivers/of/Makefile | 1 +
drivers/of/fdt.c | 13 ++-
drivers/of/of_reserved_mem.c | 188 +++++++++++++++++++++++++++++++++++++++
include/linux/of_reserved_mem.h | 21 +++++
5 files changed, 227 insertions(+), 2 deletions(-)
create mode 100644 drivers/of/of_reserved_mem.c
create mode 100644 include/linux/of_reserved_mem.h
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index c6973f101a3e..30a7d87a8077 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -75,4 +75,10 @@ config OF_MTD
depends on MTD
def_bool y
+config OF_RESERVED_MEM
+ depends on OF_EARLY_FLATTREE
+ bool
+ help
+ Helpers to allow for reservation of memory regions
+
endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index efd05102c405..ed9660adad77 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
obj-$(CONFIG_OF_PCI) += of_pci.o
obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
obj-$(CONFIG_OF_MTD) += of_mtd.o
+obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 12809e20ef71..eafe5805257a 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_fdt.h>
+#include <linux/of_reserved_mem.h>
#include <linux/string.h>
#include <linux/errno.h>
#include <linux/slab.h>
@@ -449,7 +450,7 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
phys_addr_t base, size;
unsigned long len;
__be32 *prop;
- int nomap;
+ int nomap, first = 1;
prop = of_get_flat_dt_prop(node, "reg", &len);
if (!prop)
@@ -476,6 +477,10 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
uname, &base, (unsigned long)size / SZ_1M);
len -= t_len;
+ if (first) {
+ fdt_reserved_mem_save_node(node, uname, base, size);
+ first = 0;
+ }
}
return 0;
}
@@ -506,6 +511,7 @@ static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
{
static int found;
const char *status;
+ int err;
if (!found && depth == 1 && strcmp(uname, "reserved-memory") == 0) {
if (__reserved_mem_check_root(node) != 0) {
@@ -528,7 +534,9 @@ static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
if (status && strcmp(status, "okay") != 0 && strcmp(status, "ok") != 0)
return 0;
- __reserved_mem_reserve_reg(node, uname);
+ err = __reserved_mem_reserve_reg(node, uname);
+ if (err == -ENOENT && of_get_flat_dt_prop(node, "size", NULL))
+ fdt_reserved_mem_save_node(node, uname, 0, 0);
/* scan next node */
return 0;
@@ -544,6 +552,7 @@ static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
void __init early_init_fdt_scan_reserved_mem(void)
{
of_scan_flat_dt(__fdt_scan_reserved_mem, NULL);
+ fdt_init_reserved_mem();
}
/**
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
new file mode 100644
index 000000000000..c7ca6a4a42d1
--- /dev/null
+++ b/drivers/of/of_reserved_mem.c
@@ -0,0 +1,188 @@
+/*
+ * Device tree based initialization code for reserved memory.
+ *
+ * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
+ * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ * Author: Marek Szyprowski <m.szyprowski@samsung.com>
+ * Author: Josh Cartwright <joshc@codeaurora.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License or (at your optional) any later version of the license.
+ */
+
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/of_platform.h>
+#include <linux/mm.h>
+#include <linux/sizes.h>
+#include <linux/of_reserved_mem.h>
+
+#define MAX_RESERVED_REGIONS 16
+static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
+static int reserved_mem_count;
+
+#if defined(CONFIG_HAVE_MEMBLOCK)
+#include <linux/memblock.h>
+int __init __weak early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
+ phys_addr_t align, phys_addr_t start, phys_addr_t end, bool nomap,
+ phys_addr_t *res_base)
+{
+ /*
+ * We use __memblock_alloc_base() because memblock_alloc_base()
+ * panic()s on allocation failure.
+ */
+ phys_addr_t base = __memblock_alloc_base(size, align, end);
+ if (!base)
+ return -ENOMEM;
+
+ /*
+ * Check if the allocated region fits in to start..end window
+ */
+ if (base < start) {
+ memblock_free(base, size);
+ return -ENOMEM;
+ }
+
+ *res_base = base;
+ if (nomap)
+ return memblock_remove(base, size);
+ return 0;
+}
+#else
+int __init __weak early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
+ phys_addr_t align, phys_addr_t start, phys_addr_t end, bool nomap,
+ phys_addr_t *res_base)
+{
+ pr_error("Reserved memory not supported, ignoring region 0x%llx%s\n",
+ size, nomap ? " (nomap)" : "");
+ return -ENOSYS;
+}
+#endif
+
+/**
+ * res_mem_save_node() - save fdt node for second pass initialization
+ */
+void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
+ phys_addr_t base, phys_addr_t size)
+{
+ struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
+
+ if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) {
+ pr_err("Reserved memory: not enough space all defined regions.\n");
+ return;
+ }
+
+ rmem->fdt_node = node;
+ rmem->name = uname;
+ rmem->base = base;
+ rmem->size = size;
+
+ reserved_mem_count++;
+ return;
+}
+
+/**
+ * res_mem_alloc_size() - allocate reserved memory described by 'size', 'align'
+ * and 'alloc-ranges' properties
+ */
+static int __init __reserved_mem_alloc_size(unsigned long node,
+ const char *uname, phys_addr_t *res_base, phys_addr_t *res_size)
+{
+ int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+ phys_addr_t start = 0, end = 0;
+ phys_addr_t base = 0, align = 0, size;
+ unsigned long len;
+ __be32 *prop;
+ int nomap;
+ int ret;
+
+ prop = of_get_flat_dt_prop(node, "size", &len);
+ if (!prop)
+ return -EINVAL;
+
+ if (len != dt_root_size_cells * sizeof(__be32)) {
+ pr_err("Reserved memory: invalid size property in '%s' node.\n",
+ uname);
+ return -EINVAL;
+ }
+ size = dt_mem_next_cell(dt_root_size_cells, &prop);
+
+ nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
+
+ prop = of_get_flat_dt_prop(node, "alignment", &len);
+ if (prop) {
+ if (len != dt_root_addr_cells * sizeof(__be32)) {
+ pr_err("Reserved memory: invalid alignment property in '%s' node.\n",
+ uname);
+ return -EINVAL;
+ }
+ align = dt_mem_next_cell(dt_root_addr_cells, &prop);
+ }
+
+ prop = of_get_flat_dt_prop(node, "alloc-ranges", &len);
+ if (prop) {
+
+ if (len % t_len != 0) {
+ pr_err("Reserved memory: invalid alloc-ranges property in '%s', skipping node.\n",
+ uname);
+ return -EINVAL;
+ }
+
+ base = 0;
+
+ while (len > 0) {
+ start = dt_mem_next_cell(dt_root_addr_cells, &prop);
+ end = start + dt_mem_next_cell(dt_root_size_cells,
+ &prop);
+
+ ret = early_init_dt_alloc_reserved_memory_arch(size,
+ align, start, end, nomap, &base);
+ if (ret == 0) {
+ pr_debug("Reserved memory: allocated memory for '%s' node: base %pa, size %ld MiB\n",
+ uname, &base,
+ (unsigned long)size / SZ_1M);
+ break;
+ }
+ len -= t_len;
+ }
+
+ } else {
+ ret = early_init_dt_alloc_reserved_memory_arch(size, align,
+ 0, 0, nomap, &base);
+ if (ret == 0)
+ pr_debug("Reserved memory: allocated memory for '%s' node: base %pa, size %ld MiB\n",
+ uname, &base, (unsigned long)size / SZ_1M);
+ }
+
+ if (base == 0) {
+ pr_info("Reserved memory: failed to allocate memory for node '%s'\n",
+ uname);
+ return -ENOMEM;
+ }
+
+ *res_base = base;
+ *res_size = size;
+
+ return 0;
+}
+
+/**
+ * fdt_init_reserved_mem - allocate and init all saved reserved memory regions
+ */
+void __init fdt_init_reserved_mem(void)
+{
+ int i;
+ for (i = 0; i < reserved_mem_count; i++) {
+ struct reserved_mem *rmem = &reserved_mem[i];
+ unsigned long node = rmem->fdt_node;
+ int err = 0;
+
+ if (rmem->size == 0)
+ err = __reserved_mem_alloc_size(node, rmem->name,
+ &rmem->base, &rmem->size);
+ }
+}
diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
new file mode 100644
index 000000000000..89226ed7d954
--- /dev/null
+++ b/include/linux/of_reserved_mem.h
@@ -0,0 +1,21 @@
+#ifndef __OF_RESERVED_MEM_H
+#define __OF_RESERVED_MEM_H
+
+struct reserved_mem {
+ const char *name;
+ unsigned long fdt_node;
+ phys_addr_t base;
+ phys_addr_t size;
+};
+
+#ifdef CONFIG_OF_RESERVED_MEM
+void fdt_init_reserved_mem(void);
+void fdt_reserved_mem_save_node(unsigned long node, const char *uname,
+ phys_addr_t base, phys_addr_t size);
+#else
+static inline void fdt_init_reserved_mem(void) { }
+static inline void fdt_reserved_mem_save_node(unsigned long node,
+ const char *uname, phys_addr_t base, phys_addr_t size) { }
+#endif
+
+#endif /* __OF_RESERVED_MEM_H */
--
1.7.9.5
^ permalink raw reply related
* [PATCH v5 02/11] drivers: of: add initialization code for static reserved memory
From: Marek Szyprowski @ 2014-02-21 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, linaro-mm-sig, devicetree,
linux-doc
Cc: Mark Rutland, Benjamin Herrenschmidt, Tomasz Figa, Will Deacon,
Tomasz Figa, Paul Mackerras, Marek Szyprowski, Arnd Bergmann,
Josh Cartwright, Catalin Marinas, Grant Likely, Laura Abbott,
Ian Campbell, Pawel Moll, Stephen Warren, Sascha Hauer,
Michal Nazarewicz, Marc, Nishanth Peethambaran, Rob Herring,
Kumar Gala, Olof Johansson
In-Reply-To: <1392985527-6260-1-git-send-email-m.szyprowski@samsung.com>
This patch adds support for static (defined by 'reg' property) reserved
memory regions declared in device tree.
Memory blocks can be reliably reserved only during early boot. This must
happen before the whole memory management subsystem is initialized,
because we need to ensure that the given contiguous blocks are not yet
allocated by kernel. Also it must happen before kernel mappings for the
whole low memory are created, to ensure that there will be no mappings
(for reserved blocks). Typically, all this happens before device tree
structures are unflattened, so we need to get reserved memory layout
directly from fdt.
Based on previous code provided by Josh Cartwright <joshc@codeaurora.org>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/of/fdt.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_fdt.h | 3 ++
2 files changed, 128 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 758b4f8b30b7..12809e20ef71 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -440,6 +440,113 @@ struct boot_param_header *initial_boot_params;
#ifdef CONFIG_OF_EARLY_FLATTREE
/**
+ * res_mem_reserve_reg() - reserve all memory described in 'reg' property
+ */
+static int __init __reserved_mem_reserve_reg(unsigned long node,
+ const char *uname)
+{
+ int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+ phys_addr_t base, size;
+ unsigned long len;
+ __be32 *prop;
+ int nomap;
+
+ prop = of_get_flat_dt_prop(node, "reg", &len);
+ if (!prop)
+ return -ENOENT;
+
+ if (len && len % t_len != 0) {
+ pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
+ uname);
+ return -EINVAL;
+ }
+
+ nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
+
+ while (len > 0) {
+ base = dt_mem_next_cell(dt_root_addr_cells, &prop);
+ size = dt_mem_next_cell(dt_root_size_cells, &prop);
+
+ if (base && size &&
+ early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
+ pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %ld MiB\n",
+ uname, &base, (unsigned long)size / SZ_1M);
+ else
+ pr_info("Reserved memory: failed to reserve memory for node '%s': base %pa, size %ld MiB\n",
+ uname, &base, (unsigned long)size / SZ_1M);
+
+ len -= t_len;
+ }
+ return 0;
+}
+
+static int __reserved_mem_check_root(unsigned long node)
+{
+ __be32 *prop;
+
+ prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
+ if (prop && be32_to_cpup(prop) != dt_root_size_cells)
+ return -EINVAL;
+
+ prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
+ if (prop && be32_to_cpup(prop) != dt_root_addr_cells)
+ return -EINVAL;
+
+ prop = of_get_flat_dt_prop(node, "ranges", NULL);
+ if (!prop)
+ return -EINVAL;
+ return 0;
+}
+
+/**
+ * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory
+ */
+static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
+ int depth, void *data)
+{
+ static int found;
+ const char *status;
+
+ if (!found && depth == 1 && strcmp(uname, "reserved-memory") == 0) {
+ if (__reserved_mem_check_root(node) != 0) {
+ pr_err("Reserved memory: unsupported node format, ignoring\n");
+ /* break scan */
+ return 1;
+ }
+ found = 1;
+ /* scan next node */
+ return 0;
+ } else if (!found) {
+ /* scan next node */
+ return 0;
+ } else if (found && depth < 2) {
+ /* scanning of /reserved-memory has been finished */
+ return 1;
+ }
+
+ status = of_get_flat_dt_prop(node, "status", NULL);
+ if (status && strcmp(status, "okay") != 0 && strcmp(status, "ok") != 0)
+ return 0;
+
+ __reserved_mem_reserve_reg(node, uname);
+
+ /* scan next node */
+ return 0;
+}
+
+/**
+ * early_init_fdt_scan_reserved_mem() - create reserved memory regions
+ *
+ * This function grabs memory from early allocator for device exclusive use
+ * defined in device tree structures. It should be called by arch specific code
+ * once the early allocator (i.e. memblock) has been fully activated.
+ */
+void __init early_init_fdt_scan_reserved_mem(void)
+{
+ of_scan_flat_dt(__fdt_scan_reserved_mem, NULL);
+}
+
+/**
* of_scan_flat_dt - scan flattened tree blob and call callback on each.
* @it: callback function
* @data: context data pointer
@@ -856,6 +963,16 @@ void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size)
memblock_add(base, size);
}
+int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
+ phys_addr_t size, bool nomap)
+{
+ if (memblock_is_region_reserved(base, size))
+ return -EBUSY;
+ if (nomap)
+ return memblock_remove(base, size);
+ return memblock_reserve(base, size);
+}
+
/*
* called from unflatten_device_tree() to bootstrap devicetree itself
* Architectures can override this definition if memblock isn't used
@@ -864,6 +981,14 @@ void * __init __weak early_init_dt_alloc_memory_arch(u64 size, u64 align)
{
return __va(memblock_alloc(size, align));
}
+#else
+int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
+ phys_addr_t size, bool nomap)
+{
+ pr_error("Reserved memory not supported, ignoring range 0x%llx - 0x%llx%s\n",
+ base, size, nomap ? " (nomap)" : "");
+ return -ENOSYS;
+}
#endif
bool __init early_init_dt_scan(void *params)
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 2b77058a7335..8610ad8d77d2 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -98,7 +98,10 @@ extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
int depth, void *data);
extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
int depth, void *data);
+extern void early_init_fdt_scan_reserved_mem(void);
extern void early_init_dt_add_memory_arch(u64 base, u64 size);
+extern int early_init_dt_reserve_memory_arch(phys_addr_t base, phys_addr_t size,
+ bool no_map);
extern void * early_init_dt_alloc_memory_arch(u64 size, u64 align);
extern u64 dt_mem_next_cell(int s, __be32 **cellp);
--
1.7.9.5
^ permalink raw reply related
* [PATCH v5 01/11] of: document bindings for reserved-memory nodes
From: Marek Szyprowski @ 2014-02-21 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, linaro-mm-sig, devicetree,
linux-doc
Cc: Marek Szyprowski, Benjamin Herrenschmidt, Arnd Bergmann,
Michal Nazarewicz, Grant Likely, Tomasz Figa, Sascha Hauer,
Laura Abbott, Rob Herring, Olof Johansson, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Tomasz Figa,
Kumar Gala, Nishanth Peethambaran, Marc, Josh Cartwright,
Catalin Marinas, Will Deacon, Paul Mackerras
In-Reply-To: <1392985527-6260-1-git-send-email-m.szyprowski@samsung.com>
From: Grant Likely <grant.likely@linaro.org>
Reserved memory nodes allow for the reservation of static (fixed
address) regions, or dynamically allocated regions for a specific
purpose.
Signed-off-by: Grant Likely <grant.likely@linaro.org>
[joshc: Based on binding document proposed (in non-patch form) here:
http://lkml.kernel.org/g/20131030134702.19B57C402A0@trevor.secretlab.ca
adapted to support #memory-region-cells]
Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
.../bindings/reserved-memory/reserved-memory.txt | 138 ++++++++++++++++++++
1 file changed, 138 insertions(+)
create mode 100644 Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
new file mode 100644
index 000000000000..a606ce90c9c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
@@ -0,0 +1,138 @@
+*** Reserved memory regions ***
+
+Reserved memory is specified as a node under the /reserved-memory node.
+The operating system shall exclude reserved memory from normal usage
+one can create child nodes describing particular reserved (excluded from
+normal use) memory regions. Such memory regions are usually designed for
+the special usage by various device drivers.
+
+Parameters for each memory region can be encoded into the device tree
+with the following nodes:
+
+/reserved-memory node
+---------------------
+#address-cells, #size-cells (required) - standard definition
+ - Should use the same values as the root node
+#memory-region-cells (required) - dictates number of cells used in the child
+ nodes memory-region specifier
+ranges (required) - standard definition
+ - Should be empty
+
+/reserved-memory/ child nodes
+-----------------------------
+Each child of the reserved-memory node specifies one or more regions of
+reserved memory. Each child node may either use a 'reg' property to
+specify a specific range of reserved memory, or a 'size' property with
+optional constraints to request a dynamically allocated block of memory.
+
+Following the generic-names recommended practice, node names should
+reflect the purpose of the node (ie. "framebuffer" or "dma-pool"). Unit
+address (@<address>) should be appended to the name if the node is a
+static allocation.
+
+Properties:
+Requires either a) or b) below.
+a) static allocation
+ reg (required) - standard definition
+b) dynamic allocation
+ size (required) - length based on parent's #size-cells
+ - Size in bytes of memory to reserve.
+ alignment (optional) - length based on parent's #size-cells
+ - Address boundary for alignment of allocation.
+ alloc-ranges (optional) - prop-encoded-array (address, length pairs).
+ - Specifies regions of memory that are
+ acceptable to allocate from.
+
+If both reg and size are present, then the reg property takes precedence
+and size is ignored.
+
+Additional properties:
+compatible (optional) - standard definition
+ - may contain the following strings:
+ - shared-dma-pool: This indicates a region of memory meant to be
+ used as a shared pool of DMA buffers for a set of devices. It can
+ be used by an operating system to instanciate the necessary pool
+ management subsystem if necessary.
+ - vendor specific string in the form <vendor>,[<device>-]<usage>
+no-map (optional) - empty property
+ - Indicates the operating system must not create a virtual mapping
+ of the region as part of its standard mapping of system memory,
+ nor permit speculative access to it under any circumstances other
+ than under the control of the device driver using the region.
+reusable (optional) - empty property
+ - The operating system can use the memory in this region with the
+ limitation that the device driver(s) owning the region need to be
+ able to reclaim it back. Typically that means that the operating
+ system can use that region to store volatile or cached data that
+ can be otherwise regenerated or migrated elsewhere.
+
+Linux implementation note:
+- If a "linux,cma-default" property is present, then Linux will use the
+ region for the default pool of the contiguous memory allocator.
+
+Device node references to reserved memory
+-----------------------------------------
+Regions in the /reserved-memory node may be referenced by other device
+nodes by adding a memory-region property to the device node.
+
+memory-region (optional) - phandle, specifier pairs to children of /reserved-memory
+
+Example
+-------
+This example defines 3 contiguous regions are defined for Linux kernel:
+one default of all device drivers (named linux,cma@72000000 and 64MiB in size),
+one dedicated to the framebuffer device (named framebuffer@78000000, 8MiB), and
+one for multimedia processing (named multimedia-memory@77000000, 64MiB).
+
+/ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ memory {
+ reg = <0x40000000 0x40000000>;
+ };
+
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ /* global autoconfigured region for contiguous allocations */
+ linux,cma {
+ compatible = "shared-dma-pool";
+ reusable;
+ #memory-region-cells = <0>;
+ size = <0x4000000>;
+ alignment = <0x2000>;
+ linux,cma-default;
+ };
+
+ display_reserved: framebuffer@78000000 {
+ #memory-region-cells = <0>;
+ reg = <0x78000000 0x800000>;
+ };
+
+ multimedia_reserved: multimedia@77000000 {
+ compatible = "acme,multimedia-memory";
+ #memory-region-cells = <1>;
+ reg = <0x77000000 0x4000000>;
+ };
+ };
+
+ /* ... */
+
+ fb0: video@12300000 {
+ memory-region = <&display_reserved>;
+ /* ... */
+ };
+
+ scaler: scaler@12500000 {
+ memory-region = <&multimedia_reserved 0xdeadbeef>;
+ /* ... */
+ };
+
+ codec: codec@12600000 {
+ memory-region = <&multimedia_reserved 0xfeebdaed>;
+ /* ... */
+ };
+};
--
1.7.9.5
^ permalink raw reply related
* [PATCH v5 00/11] reserved-memory regions/CMA in devicetree, again
From: Marek Szyprowski @ 2014-02-21 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, linaro-mm-sig, devicetree,
linux-doc
Cc: Marek Szyprowski, Benjamin Herrenschmidt, Arnd Bergmann,
Michal Nazarewicz, Grant Likely, Tomasz Figa, Sascha Hauer,
Laura Abbott, Rob Herring, Olof Johansson, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Tomasz Figa,
Kumar Gala, Nishanth Peethambaran, Marc, Josh Cartwright,
Catalin Marinas, Will Deacon, Paul Mackerras
Hi all!
Ok, I hope that this is the last update of the patches which add basic
support for dynamic allocation of memory reserved regions defined in
device tree.
This time I've mainly sliced the main patch into several smaller pieces
to make the changes easier to understand and fixes some minor coding
style issues.
The initial code for this feature were posted here [1], merged as commit
9d8eab7af79cb4ce2de5de39f82c455b1f796963 ("drivers: of: add
initialization code for dma reserved memory") and later reverted by
commit 1931ee143b0ab72924944bc06e363d837ba05063. For more information,
see [2]. Finally a new bindings has been proposed [3] and Josh
Cartwright a few days ago prepared some code which implements those
bindings [4]. This finally pushed me again to find some time to finish
this task and review the code. Josh agreed to give me the ownership of
this series to continue preparing them for mainline inclusion.
For more information please refer to the changlelog and links below.
[1]: http://lkml.kernel.org/g/1377527959-5080-1-git-send-email-m.szyprowski@samsung.com
[2]: http://lkml.kernel.org/g/1381476448-14548-1-git-send-email-m.szyprowski@samsung.com
[3]: http://lkml.kernel.org/g/20131030134702.19B57C402A0@trevor.secretlab.ca
[4]: http://thread.gmane.org/gmane.linux.documentation/19579
Changelog:
v5:
- sliced main patch into several smaller patches on Grant's request
- fixed coding style issues pointed by Grant
- use node->phandle value directly instead of parsing properties manually
v4: https://lkml.org/lkml/2014/2/20/150
- dynamic allocations are processed after all static reservations has been
done
- moved code for handling static reservations to drivers/of/fdt.c
- removed node matching by string comparison, now phandle values are used
directly
- moved code for DMA and CMA handling directly to
drivers/base/dma-{coherent,contiguous}.c
- added checks for proper #size-cells, #address-cells, ranges properties
in /reserved-memory node
- even more code cleanup
- added init code for ARM64 and PowerPC
v3: http://article.gmane.org/gmane.linux.documentation/20169/
- refactored memory reservation code, created common code to parse reg, size,
align, alloc-ranges properties
- added support for multiple tuples in 'reg' property
- memory is reserved regardless of presence of the driver for its compatible
- prepared arch specific hooks for memory reservation (defaults use memblock
calls)
- removed node matching by string during device initialization
- CMA init code: added checks for required region alignment
- more code cleanup here and there
v2: http://thread.gmane.org/gmane.linux.documentation/19870/
- removed copying of the node name
- split shared-dma-pool handling into separate files (one for CMA and one
for dma_declare_coherent based implementations) for making the code easier
to understand
- added support for AMBA devices, changed prototypes to use struct decice
instead of struct platform_device
- renamed some functions to better match other names used in drivers/of/
- restructured the rest of the code a bit for better readability
- added 'reusable' property to exmaple linux,cma node in documentation
- exclusive dma (dma_coherent) is used for only handling 'shared-dma-pool'
regions without 'reusable' property and CMA is used only for handling
'shared-dma-pool' regions with 'reusable' property.
v1: http://thread.gmane.org/gmane.linux.documentation/19579
- initial version prepared by Josh Cartwright
Summary:
Grant Likely (1):
of: document bindings for reserved-memory nodes
Marek Szyprowski (10):
drivers: of: add initialization code for static reserved memory
drivers: of: add initialization code for dynamic reserved memory
drivers: of: add support for custom reserved memory drivers
drivers: of: add automated assignment of reserved regions to client
devices
drivers: of: initialize and assign reserved memory to newly created
devices
drivers: dma-coherent: add initialization from device tree
drivers: dma-contiguous: add initialization from device tree
arm: add support for reserved memory defined by device tree
arm64: add support for reserved memory defined by device tree
powerpc: add support for reserved memory defined by device tree
.../bindings/reserved-memory/reserved-memory.txt | 138 ++++++++++
arch/arm/Kconfig | 1 +
arch/arm/mm/init.c | 2 +
arch/arm64/Kconfig | 1 +
arch/arm64/mm/init.c | 1 +
arch/powerpc/Kconfig | 1 +
arch/powerpc/kernel/prom.c | 3 +
drivers/base/dma-coherent.c | 41 +++
drivers/base/dma-contiguous.c | 130 +++++++--
drivers/of/Kconfig | 6 +
drivers/of/Makefile | 1 +
drivers/of/fdt.c | 134 +++++++++
drivers/of/of_reserved_mem.c | 291 ++++++++++++++++++++
drivers/of/platform.c | 7 +
include/asm-generic/vmlinux.lds.h | 11 +
include/linux/of_fdt.h | 3 +
include/linux/of_reserved_mem.h | 61 ++++
17 files changed, 810 insertions(+), 22 deletions(-)
create mode 100644 Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
create mode 100644 drivers/of/of_reserved_mem.c
create mode 100644 include/linux/of_reserved_mem.h
--
1.7.9.5
^ permalink raw reply
* Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO
From: Kishon Vijay Abraham I @ 2014-02-21 12:25 UTC (permalink / raw)
To: Roger Quadros, Heikki Krogerus, Felipe Balbi
Cc: devicetree, george.cherian, linux-doc, linux-usb, linux-kernel,
linux-omap, linux-arm-kernel
In-Reply-To: <5304A58C.2030306@ti.com>
Hi Roger,
On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
> Hi,
>
> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>> For the controller drivers the PHYs are just a resource like any
>>>>> other. The controller drivers can't have any responsibility of
>>>>> them. They should not care if PHY drivers are available for them or
>>>>> not, or even if the PHY framework is available or not.
>>>>
>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>> resource; which might be fine, but we need to make sure we don't
>>>> continue probing when a PHY is missing in a platform that certainly
>>>> needs a PHY.
>>>
>>> Yes, true. In my head I was comparing the PHY only to resources like
>>> gpios, clocks, dma channels, etc. that are often optional to the
>>> drivers.
>>>
>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>
>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>> case.
>>>>>>
>>>>>> that's different from what I heard.
>>>>>
>>>>> I don't know where you got that impression, but it's not true. The
>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>> the manufacturers using it can select what they want. So we have
>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>
>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>> difficult task.
>>>
>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>
>>> <snip>
>>>
>>>>> This is really good to get. We have some projects where we are dealing
>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>> should be able to disable the PHY libraries/frameworks.
>>>>
>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>> something like that.
>>>>
>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>> as an indication ?
>>>
>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>> layer?
>>
>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>
>>>> I mean, I need to know that a particular platform depends on a PHY
>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>> isn't needed ;-)
>>>
>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>> to tell us that. If the PHY driver that the platform depends is not
>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>> returning -EPROBE_DEFER.
>>
>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>> not. Consider when the phy_provider_register fails, there is no way to know if
>> PHY driver is available or not. There are a few cases where PHY layer returns
>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>> available and failed or not available at all. It would be best for us to leave
>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>
>
> Just to summarize this thread on what we need
Thanks for summarizing.
>
> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
> It should be as generic as possible.
>
> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
>
> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
>
> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
> e.g. init error.
>
> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
>
> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.
The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
checks for PHY in the glue layer.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH tty-next v2 4/4] Documentation: devicetree: add bindings documentation for bcm63xx-uart
From: Mark Rutland @ 2014-02-21 12:08 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-serial@vger.kernel.org, devicetree@vger.kernel.org,
mbizon@freebox.fr, jogo@openwrt.org, gregkh@linuxfoundation.org,
gregory.0xf0@gmail.com
In-Reply-To: <1392920154-3642-5-git-send-email-f.fainelli@gmail.com>
On Thu, Feb 20, 2014 at 06:15:54PM +0000, Florian Fainelli wrote:
> Add the Device Tree binding documentation for the non-standard BCM63xx
> UART hardware block found in the BCM63xx DSL SoCs.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
This looks fine to me.
Acked-by: Mark Rutland <mark.rutland@arm.com>
> ---
> Changes in v2:
> - update the compatible property to be "brcm,bcm6345-uart" as suggested
> - reword the clocks and clock-names properties based on feedback from Mark
>
> .../devicetree/bindings/serial/bcm63xx-uart.txt | 27 ++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
>
> diff --git a/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
> new file mode 100644
> index 0000000..f288232
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
> @@ -0,0 +1,27 @@
> +Broadcom BCM63xx UART: Non standard UART used in the Broadcom BCM63xx DSL SoCs
> +
> +Required properties:
> +- compatible: must be "brcm,bcm6345-uart"
> +- reg: offset and length of the register set for the device
> +- interrupts: device interrupt
> +- clocks: a list of phandles and clock-specifiers, one for each entry
> + in clock-names
> +- clock-names: should contain "periph" for the functional clock
> +
> +Example:
> +
> +serial0: uart@600 {
> + compatible = "brcm,bcm6345-uart";
> + reg = <0x600 0x1b>;
> + interrupts = <GIC_SPI 32 0>;
> + clocks = <&periph_clk>;
> + clock-names = "periph";
> +};
> +
> +Note: each UART port must have an alias correctly numbered in the "aliases"
> +node, e.g:
> +
> +aliases {
> + uart0 = &serial0;
> + uart1 = &serial1;
> +};
> --
> 1.8.3.2
>
>
^ permalink raw reply
* Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
From: Mark Rutland @ 2014-02-21 12:04 UTC (permalink / raw)
To: Felipe Balbi, Subbaraya Sundeep Bhatta
Cc: Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, michals@xilinx.com,
Subbaraya Sundeep Bhatta, devicetree@vger.kernel.org
In-Reply-To: <20140220182257.GF23217@saruman.home>
On Thu, Feb 20, 2014 at 06:23:13PM +0000, Felipe Balbi wrote:
> Hi,
>
> On Wed, Feb 19, 2014 at 03:10:45PM +0530, Subbaraya Sundeep Bhatta wrote:
> > This patch adds xilinx axi usb2 device driver support
> >
> > Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
> > ---
> > .../devicetree/bindings/usb/xilinx_usb.txt | 21 +
> > drivers/usb/gadget/Kconfig | 14 +
> > drivers/usb/gadget/Makefile | 1 +
> > drivers/usb/gadget/xilinx_udc.c | 2045 ++++++++++++++++++++
> > 4 files changed, 2081 insertions(+), 0 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/usb/xilinx_usb.txt
> > create mode 100644 drivers/usb/gadget/xilinx_udc.c
> >
> > diff --git a/Documentation/devicetree/bindings/usb/xilinx_usb.txt b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
> > new file mode 100644
> > index 0000000..acf03ab
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
> > @@ -0,0 +1,21 @@
> > +Xilinx AXI USB2 device controller
> > +
> > +Required properties:
> > +- compatible : Should be "xlnx,axi-usb2-device-4.00.a"
Is "axi-usb2-device" the official device name?
> > +- reg : Physical base address and size of the Axi USB2
> > + device registers map.
> > +- interrupts : Property with a value describing the interrupt
> > + number.
Does the device only have a single interrupt?
> > +- interrupt-parent : Must be core interrupt controller
Is this strictly necessary?
> > +- xlnx,include-dma : Must be 1 or 0 based on whether DMA is included
> > + in IP or not.
Perhaps xlnx,has-builtin-dma would better describe this?
> > +
> > +Example:
> > + axi-usb2-device@42e00000 {
> > + compatible = "xlnx,axi-usb2-device-4.00.a";
> > + interrupt-parent = <0x1>;
> > + interrupts = <0x0 0x39 0x1>;
> > + reg = <0x42e00000 0x10000>;
> > + xlnx,include-dma = <0x1>;
> > + };
> > +
>
> you need to Cc devicetree@vger.kernel.org for this.
Cheers Filipe; thanks for adding us to Cc :)
[...]
> > + /* Map the registers */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + udc->base_address = devm_ioremap_nocache(&pdev->dev, res->start,
> > + resource_size(res));
>
> use devm_ioremap_resource() instead.
Also, res might be NULL. You should check that before dereferencing it.
Cheers,
Mark.
^ permalink raw reply
* Re: [PATCH 4/7] ECHI Platform: Merge ppc-of EHCI driver into the ehci-platform driver
From: Mark Rutland @ 2014-02-21 11:48 UTC (permalink / raw)
To: Alistair Popple
Cc: devicetree@vger.kernel.org, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
Tony Prisk, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1392964293-13687-5-git-send-email-alistair@popple.id.au>
[Adding Tony Prisk to Cc]
On Fri, Feb 21, 2014 at 06:31:30AM +0000, Alistair Popple wrote:
> Currently the ppc-of driver uses the compatibility string
> "usb-ehci". This means platforms that use device-tree and implement an
> EHCI compatible interface have to either use the ppc-of driver or add
> a compatible line to the ehci-platform driver. It would be more
> appropriate for the platform driver to be compatible with "usb-ehci"
> as non-powerpc platforms are also beginning to utilise device-tree.
>
> This patch merges the device tree property parsing from ehci-ppc-of
> into the platform driver and adds a "usb-ehci" compatibility
> string. The existing ehci-ppc-of driver is removed and the 440EPX
> specific quirks are added to the ehci-platform driver.
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> ---
> drivers/usb/host/Kconfig | 7 +-
> drivers/usb/host/ehci-hcd.c | 5 -
> drivers/usb/host/ehci-platform.c | 87 +++++++++++++-
> drivers/usb/host/ehci-ppc-of.c | 238 --------------------------------------
> 4 files changed, 89 insertions(+), 248 deletions(-)
> delete mode 100644 drivers/usb/host/ehci-ppc-of.c
[...]
> + /* Device tree properties if available will override platform data. */
> + dn = hcd_to_bus(hcd)->controller->of_node;
> + if (dn) {
> + if (of_get_property(dn, "big-endian", NULL)) {
> + ehci->big_endian_mmio = 1;
> + ehci->big_endian_desc = 1;
> + }
> + if (of_get_property(dn, "big-endian-regs", NULL))
> + ehci->big_endian_mmio = 1;
> + if (of_get_property(dn, "big-endian-desc", NULL))
> + ehci->big_endian_desc = 1;
> + }
Please use of_property_read_bool for these.
This driver already handles "via,vt8500-ehci" and "wm,prizm-ehci" which
aren't documented to handle these properties, but now gain support for
them. It might be worth unifying the binding documents if there's
nothing special about those two host controllers.
We seem to have two binding documents for "via,vt8500-ehci", so some
cleanup is definitely in order.
Tony, you seem to have written both documents judging by 95e9fd10f06c
and 8ad551d150e3. Do you have any issue with merging both of these into
a common usb-ehci document?
> +
> + np = of_find_compatible_node(NULL, NULL, "ibm,usb-ohci-440epx");
> + if (np != NULL) {
> + /* claim we really affected by usb23 erratum */
> + if (!of_address_to_resource(np, 0, &res))
> + ehci->ohci_hcctrl_reg =
> + devm_ioremap(&pdev->dev,
> + res.start + OHCI_HCCTRL_OFFSET,
> + OHCI_HCCTRL_LEN);
> + else
> + ehci_dbg(ehci, "%s: no ohci offset in fdt\n", __FILE__);
> + if (!ehci->ohci_hcctrl_reg) {
> + ehci_dbg(ehci, "%s: ioremap for ohci hcctrl failed\n",
> + __FILE__);
> + } else {
> + ehci->has_amcc_usb23 = 1;
> + }
> + }
As this is being dropped into a generic driver, it would be nice to have
a comment explaining why we need to do this -- not everyone using this
will know the powerpc history. It certainly seems odd to look for
another node in the tree that this driver isn't necessarily handling,
and that should probably be explained.
As this bit of fixup is only needed for powerpc, it would be nice to not
have to do it elsewhere. Perhaps these could be factored out into a
ppc_platform_reset function that could be empty stub for other
architectures.
> +
> + if (of_device_is_compatible(dn, "ibm,usb-ehci-440epx")) {
> + retval = ppc44x_enable_bmt(dn);
> + ehci_dbg(ehci, "Break Memory Transfer (BMT) is %senabled!\n",
> + retval ? "NOT " : "");
> + }
> +
Likewise.
[...]
> + /* use request_mem_region to test if the ohci driver is loaded. if so
> + * ensure the ohci core is operational.
> + */
Nit: comment code style (should have a newline after the /* according to
Documentation/CodingStyle).
> + if (ehci->has_amcc_usb23) {
> + np = of_find_compatible_node(NULL, NULL, "ibm,usb-ohci-440epx");
> + if (np != NULL) {
> + if (!of_address_to_resource(np, 0, &res))
> + if (!request_mem_region(res.start,
> + 0x4, hcd_name))
> + set_ohci_hcfs(ehci, 1);
> + else
> + release_mem_region(res.start, 0x4);
> + else
> + ehci_dbg(ehci, "%s: no ohci offset in fdt\n",
> + __FILE__);
> + of_node_put(np);
> + }
> + }
As with the earlier comment, this looks a bit odd. A comment explaining
why we're looking for another node would help. Also, we should only need
this for powerpc.
Cheers,
Mark.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* [PATCH 4/4] ASoC: simple-card: add DT documentation for multi-DAI links
From: Jean-Francois Moine @ 2014-02-21 11:43 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, Kuninori Morimoto, linux-kernel, Xiubo Li, devicetree
In-Reply-To: <cover.1392995566.git.moinejf@free.fr>
There may be many couples of CPU/CODEC DAI links.
The example 2 is extracted from the Cubox DT.
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
.../devicetree/bindings/sound/simple-card.txt | 34 +++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index 0527358..60306bf 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -24,6 +24,9 @@ Required subnodes:
- simple-audio-card,cpu : CPU sub-node
- simple-audio-card,codec : CODEC sub-node
+ There may be one or many couples (simple-audio-card,cpu, simple-audio-card,codec)
+ (see example 2).
+
Required CPU/CODEC subnodes properties:
- sound-dai : phandle and port of CPU/CODEC
@@ -41,7 +44,7 @@ Optional CPU/CODEC subnodes properties:
clock node (= common clock), or "system-clock-frequency"
(if system doens't support common clock)
-Example:
+Example 1:
sound {
compatible = "simple-audio-card";
@@ -83,3 +86,32 @@ sh_fsi2: sh_fsi2@ec230000 {
interrupt-parent = <&gic>;
interrupts = <0 146 0x4>;
};
+
+Example 2:
+
+sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,name = "Cubox Audio";
+
+ simple-audio-card,cpu@0 { /* I2S - HDMI */
+ sound-dai = <&audio1 0>;
+ format = "i2s";
+ };
+ simple-audio-card,codec@0 {
+ sound-dai = <&tda998x 0>;
+ };
+
+ simple-audio-card,cpu@1 { /* S/PDIF - HDMI */
+ sound-dai = <&audio1 1>;
+ };
+ simple-audio-card,codec@1 {
+ sound-dai = <&tda998x 1>;
+ };
+
+ simple-audio-card,cpu@2 { /* S/PDIF - S/PDIF */
+ sound-dai = <&audio1 1>;
+ };
+ simple-audio-card,codec@2 {
+ sound-dai = <&spdif_codec>;
+ };
+};
--
1.9.0
^ permalink raw reply related
* RE: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
From: Subbaraya Sundeep Bhatta @ 2014-02-21 11:27 UTC (permalink / raw)
To: balbi@ti.com
Cc: Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, Michal Simek,
devicetree@vger.kernel.org
In-Reply-To: <20140220182257.GF23217@saruman.home>
Hi,
> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Thursday, February 20, 2014 11:53 PM
> To: Subbaraya Sundeep Bhatta
> Cc: Felipe Balbi; Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; Michal Simek; Subbaraya Sundeep Bhatta;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
>
> Hi,
>
> On Wed, Feb 19, 2014 at 03:10:45PM +0530, Subbaraya Sundeep Bhatta wrote:
> > This patch adds xilinx axi usb2 device driver support
> >
> > Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
> > ---
> > .../devicetree/bindings/usb/xilinx_usb.txt | 21 +
> > drivers/usb/gadget/Kconfig | 14 +
> > drivers/usb/gadget/Makefile | 1 +
> > drivers/usb/gadget/xilinx_udc.c | 2045 ++++++++++++++++++++
> > 4 files changed, 2081 insertions(+), 0 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/usb/xilinx_usb.txt
> > create mode 100644 drivers/usb/gadget/xilinx_udc.c
> >
> > diff --git a/Documentation/devicetree/bindings/usb/xilinx_usb.txt
> b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
> > new file mode 100644
> > index 0000000..acf03ab
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
> > @@ -0,0 +1,21 @@
> > +Xilinx AXI USB2 device controller
> > +
> > +Required properties:
> > +- compatible : Should be "xlnx,axi-usb2-device-4.00.a"
> > +- reg : Physical base address and size of the Axi USB2
> > + device registers map.
> > +- interrupts : Property with a value describing the interrupt
> > + number.
> > +- interrupt-parent : Must be core interrupt controller
> > +- xlnx,include-dma : Must be 1 or 0 based on whether DMA is included
> > + in IP or not.
> > +
> > +Example:
> > + axi-usb2-device@42e00000 {
> > + compatible = "xlnx,axi-usb2-device-4.00.a";
> > + interrupt-parent = <0x1>;
> > + interrupts = <0x0 0x39 0x1>;
> > + reg = <0x42e00000 0x10000>;
> > + xlnx,include-dma = <0x1>;
> > + };
> > +
>
> you need to Cc devicetree@vger.kernel.org for this.
Ok.
>
> > diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> > index 8154165..0b284bf 100644
> > --- a/drivers/usb/gadget/Kconfig
> > +++ b/drivers/usb/gadget/Kconfig
> > @@ -466,6 +466,20 @@ config USB_EG20T
> > ML7213/ML7831 is companion chip for Intel Atom E6xx series.
> > ML7213/ML7831 is completely compatible for Intel EG20T PCH.
> >
> > +config USB_GADGET_XILINX
> > + tristate "Xilinx USB Driver"
> > + depends on MICROBLAZE || ARCH_ZYNQ
>
> This has the tendency to grow and I really don't like seeing a bunch of
> arch-specific dependencies. At a minimum add COMPILE_TEST so I can build
> test on my setup and make sure it builds fine on other architectures.
>
> > + help
> > + USB peripheral controller driver for Xilinx AXI USB2 device.
> > + Xilinx AXI USB2 device is a soft IP which supports both full
> > + and high speed USB 2.0 data transfers. It has seven configurable
> > + endpoints(bulk or interrupt or isochronous), as well as
> > + endpoint zero(for control transfers).
> > +
> > + Say "y" to link the driver statically, or "m" to build a
> > + dynamically linked module called "xilinx_udc" and force all
> > + gadget drivers to also be dynamically linked.
> > +
> > #
> > # LAST -- dummy/emulated controller
> > #
> > diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> > index 5f150bc..3a55564 100644
> > --- a/drivers/usb/gadget/Makefile
> > +++ b/drivers/usb/gadget/Makefile
> > @@ -36,6 +36,7 @@ obj-$(CONFIG_USB_FUSB300) += fusb300_udc.o
> > obj-$(CONFIG_USB_FOTG210_UDC) += fotg210-udc.o
> > obj-$(CONFIG_USB_MV_U3D) += mv_u3d_core.o
> > obj-$(CONFIG_USB_GR_UDC) += gr_udc.o
> > +obj-$(CONFIG_USB_GADGET_XILINX) += xilinx_udc.o
>
> let's start normalizing the names here (I'll patch the others later) and
> call this udc-xilinx.o
>
Ok
> > diff --git a/drivers/usb/gadget/xilinx_udc.c b/drivers/usb/gadget/xilinx_udc.c
> > new file mode 100644
> > index 0000000..3ee8359
> > --- /dev/null
> > +++ b/drivers/usb/gadget/xilinx_udc.c
> > @@ -0,0 +1,2045 @@
> > +/*
> > + * Xilinx USB peripheral controller driver
> > + *
> > + * Copyright (C) 2004 by Thomas Rathbone
> > + * Copyright (C) 2005 by HP Labs
> > + * Copyright (C) 2005 by David Brownell
>
> heh! :-) Brownell was really everywhere, good to still see code from him
> ;-)
>
> > + * Copyright (C) 2010 - 2014 Xilinx, Inc.
> > + *
> > + * Some parts of this driver code is based on the driver for at91-series
> > + * USB peripheral controller (at91_udc.c).
> > + *
> > + * This program is free software; you can redistribute it
> > + * and/or modify it under the terms of the GNU General Public
> > + * License as published by the Free Software Foundation;
> > + * either version 2 of the License, or (at your option) any
> > + * later version.
>
> are you sure you want to allow people ot use GPL v3 on this driver ?
>
> > +#include <linux/interrupt.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/prefetch.h>
> > +#include <linux/usb/ch9.h>
> > +#include <linux/usb/gadget.h>
> > +#include <linux/io.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/dma-mapping.h>
> > +#include "gadget_chips.h"
> > +
> > +/* Register offsets for the USB device.*/
> > +#define XUSB_EP0_CONFIG_OFFSET 0x0000 /* EP0 Config Reg
> Offset */
> > +#define XUSB_SETUP_PKT_ADDR_OFFSET 0x0080 /* Setup Packet
> Address */
> > +#define XUSB_ADDRESS_OFFSET 0x0100 /* Address Register */
> > +#define XUSB_CONTROL_OFFSET 0x0104 /* Control Register */
> > +#define XUSB_STATUS_OFFSET 0x0108 /* Status Register */
> > +#define XUSB_FRAMENUM_OFFSET 0x010C /* Frame Number
> Register */
> > +#define XUSB_IER_OFFSET 0x0110 /* Interrupt Enable
> Register */
> > +#define XUSB_BUFFREADY_OFFSET 0x0114 /* Buffer Ready
> Register */
> > +#define XUSB_TESTMODE_OFFSET 0x0118 /* Test Mode Register
> */
> > +#define XUSB_DMA_RESET_OFFSET 0x0200 /* DMA Soft Reset
> Register */
> > +#define XUSB_DMA_CONTROL_OFFSET 0x0204 /* DMA
> Control Register */
> > +#define XUSB_DMA_DSAR_ADDR_OFFSET 0x0208 /* DMA source
> Address Reg */
> > +#define XUSB_DMA_DDAR_ADDR_OFFSET 0x020C /* DMA destination
> Addr Reg */
> > +#define XUSB_DMA_LENGTH_OFFSET 0x0210 /* DMA Length
> Register */
> > +#define XUSB_DMA_STATUS_OFFSET 0x0214 /* DMA Status Register
> */
> > +
> > +/* Endpoint Configuration Space offsets */
> > +#define XUSB_EP_CFGSTATUS_OFFSET 0x00 /* Endpoint Config
> Status */
> > +#define XUSB_EP_BUF0COUNT_OFFSET 0x08 /* Buffer 0 Count */
> > +#define XUSB_EP_BUF1COUNT_OFFSET 0x0C /* Buffer 1 Count */
> > +
> > +#define XUSB_CONTROL_USB_READY_MASK 0x80000000 /* USB ready
> Mask */
> > +
> > +/* Interrupt register related masks.*/
> > +#define XUSB_STATUS_GLOBAL_INTR_MASK 0x80000000 /* Global Intr
> Enable */
> > +#define XUSB_STATUS_RESET_MASK 0x00800000 /* USB Reset
> Mask */
> > +#define XUSB_STATUS_SUSPEND_MASK 0x00400000 /* USB Suspend
> Mask */
> > +#define XUSB_STATUS_DISCONNECT_MASK 0x00200000 /* USB Disconnect
> Mask */
> > +#define XUSB_STATUS_FIFO_BUFF_RDY_MASK 0x00100000 /* FIFO
> Buff Ready Mask */
> > +#define XUSB_STATUS_FIFO_BUFF_FREE_MASK 0x00080000 /* FIFO
> Buff Free Mask */
> > +#define XUSB_STATUS_SETUP_PACKET_MASK 0x00040000 /* Setup packet
> received */
> > +#define XUSB_STATUS_EP1_BUFF2_COMP_MASK 0x00000200 /* EP 1
> Buff 2 Processed */
> > +#define XUSB_STATUS_EP1_BUFF1_COMP_MASK 0x00000002 /* EP 1
> Buff 1 Processed */
> > +#define XUSB_STATUS_EP0_BUFF2_COMP_MASK 0x00000100 /* EP 0
> Buff 2 Processed */
> > +#define XUSB_STATUS_EP0_BUFF1_COMP_MASK 0x00000001 /* EP 0
> Buff 1 Processed */
> > +#define XUSB_STATUS_HIGH_SPEED_MASK 0x00010000 /* USB Speed
> Mask */
> > +/* Suspend,Reset and Disconnect Mask */
> > +#define XUSB_STATUS_INTR_EVENT_MASK 0x00E00000
> > +/* Buffers completion Mask */
> > +#define XUSB_STATUS_INTR_BUFF_COMP_ALL_MASK 0x0000FEFF
> > +/* Mask for buffer 0 and buffer 1 completion for all Endpoints */
> > +#define XUSB_STATUS_INTR_BUFF_COMP_SHIFT_MASK 0x00000101
> > +#define XUSB_STATUS_EP_BUFF2_SHIFT 8 /* EP buffer offset */
> > +
> > +/* Endpoint Configuration Status Register */
> > +#define XUSB_EP_CFG_VALID_MASK 0x80000000 /* Endpoint Valid
> bit */
> > +#define XUSB_EP_CFG_STALL_MASK 0x40000000 /* Endpoint Stall
> bit */
> > +#define XUSB_EP_CFG_DATA_TOGGLE_MASK 0x08000000 /* Endpoint Data
> toggle */
> > +
> > +/* USB device specific global configuration constants.*/
> > +#define XUSB_MAX_ENDPOINTS 8 /* Maximum End
> Points */
> > +#define XUSB_EP_NUMBER_ZERO 0 /* End point Zero */
> > +
> > +/* Test Modes (Set Feature).*/
> > +#define TEST_J 1 /* Chirp J Test */
> > +#define TEST_K 2 /* Chirp K Test */
> > +#define TEST_SE0_NAK 3 /* Chirp SE0 Test */
> > +#define TEST_PKT 4 /* Packet Test */
> > +
> > +#define CONFIGURATION_ONE 0x01 /* USB device
> configuration*/
> > +#define STANDARD_OUT_DEVICE 0x00 /* Out device */
> > +#define STANDARD_OUT_ENDPOINT 0x02 /* Standard Out end
> point */
>
> DO NOT REDEFINE THESE, use the ones from <linux/usb/ch9.h>
Ok
>
> > +
> > +#define XUSB_DMA_READ_FROM_DPRAM 0x80000000/**< DPRAM is the
> source
> > + address for DMA
> transfer
> > + */
> > +#define XUSB_DMA_DMASR_BUSY 0x80000000 /**< DMA busy*/
> > +#define XUSB_DMA_DMASR_ERROR 0x40000000 /**< DMA Error */
> > +
> > +/*
> > + * When this bit is set, the DMA buffer ready bit is set by hardware upon
> > + * DMA transfer completion.
> > + */
> > +#define XUSB_DMA_BRR_CTRL 0x40000000 /**< DMA
> bufready ctrl bit */
> > +
> > +/* Phase States */
> > +#define SETUP_PHASE 0x0000 /* Setup Phase */
> > +#define DATA_PHASE 0x0001 /* Data Phase */
> > +#define STATUS_PHASE 0x0002 /* Status Phase */
> > +
> > +#define EP_TRANSMIT 0 /* EP is IN endpoint */
> > +#define EP0_MAX_PACKET 64 /* Endpoint 0 maximum packet
> length */
> > +
> > +/**
> > + * struct xusb_request - Xilinx USB device request structure
> > + * @usb_req: Linux usb request structure
> > + * @queue: usb device request queue
> > + */
> > +struct xusb_request {
> > + struct usb_request usb_req;
> > + struct list_head queue;
> > +};
> > +
> > +/**
> > + * struct xusb_ep - USB end point structure.
> > + * @ep_usb: usb endpoint instance
> > + * @queue: endpoint message queue
> > + * @udc: xilinx usb peripheral driver instance pointer
> > + * @desc: pointer to the usb endpoint descriptor
> > + * @data: pointer to the xusb_request structure
> > + * @rambase: the endpoint buffer address
> > + * @endpointoffset: the endpoint register offset value
> > + * @epnumber: endpoint number
> > + * @maxpacket: maximum packet size the endpoint can store
> > + * @buffer0count: the size of the packet recieved in the first buffer
> > + * @buffer0ready: the busy state of first buffer
> > + * @buffer1count: the size of the packet received in the second buffer
> > + * @buffer1ready: the busy state of second buffer
> > + * @eptype: endpoint transfer type (BULK, INTERRUPT)
> > + * @curbufnum: current buffer of endpoint that will be processed next
> > + * @is_in: endpoint direction (IN or OUT)
> > + * @stopped: endpoint active status
> > + * @is_iso: endpoint type(isochronous or non isochronous)
> > + * @name: name of the endpoint
> > + */
> > +struct xusb_ep {
> > + struct usb_ep ep_usb;
> > + struct list_head queue;
> > + struct xusb_udc *udc;
> > + const struct usb_endpoint_descriptor *desc;
> > + struct xusb_request *data;
> > + u32 rambase;
> > + u32 endpointoffset;
> > + u16 epnumber;
> > + u16 maxpacket;
> > + u16 buffer0count;
> > + u16 buffer1count;
> > + u8 buffer0ready;
> > + u8 buffer1ready;
> > + u8 eptype;
> > + u8 curbufnum;
> > + u8 is_in;
> > + u8 stopped;
> > + u8 is_iso;
> > + char name[4];
> > +};
> > +
> > +/**
> > + * struct xusb_udc - USB peripheral driver structure
> > + * @gadget: USB gadget driver instance
> > + * @ep: an array of endpoint structures
> > + * @driver: pointer to the usb gadget driver instance
> > + * @read_fn: function pointer to read device registers
> > + * @write_fn: function pointer to write to device registers
> > + * @base_address: the usb device base address
> > + * @lock: instance of spinlock
> > + * @dma_enabled: flag indicating whether the dma is included in the system
> > + * @status: status flag indicating the device cofiguration
> > + */
> > +struct xusb_udc {
> > + struct usb_gadget gadget;
> > + struct xusb_ep ep[8];
> > + struct usb_gadget_driver *driver;
> > + unsigned int (*read_fn)(void __iomem *);
> > + void (*write_fn)(u32, void __iomem *);
> > + void __iomem *base_address;
> > + spinlock_t lock;
> > + bool dma_enabled;
> > + u8 status;
> > +};
> > +
> > +/**
> > + * struct cmdbuf - Standard USB Command Buffer Structure defined
> > + * @setup: usb_ctrlrequest structure for control requests
> > + * @contreadcount: read data bytes count
> > + * @contwritecount: write data bytes count
> > + * @setupseqtx: tx status
> > + * @setupseqrx: rx status
> > + * @contreadptr: pointer to endpoint0 read data
> > + * @contwriteptr: pointer to endpoint0 write data
> > + * @contreaddatabuffer: read data buffer for endpoint0
> > + */
> > +struct cmdbuf {
> > + struct usb_ctrlrequest setup;
> > + u32 contreadcount;
> > + u32 contwritecount;
> > + u32 setupseqtx;
> > + u32 setupseqrx;
> > + u8 *contreadptr;
> > + u8 *contwriteptr;
> > + u8 contreaddatabuffer[64];
> > +};
> > +
> > +static struct cmdbuf ch9_cmdbuf;
>
> NAK, you should support multiple instances of this device in the same
> SoC.
Ok.
>
> > +/*
> > + * Endpoint buffer start addresses in the core
> > + */
>
> fits in one line.
Ok
>
> > +static u32 rambase[8] = { 0x22, 0x1000, 0x1100, 0x1200, 0x1300, 0x1400,
> 0x1500,
> > + 0x1600 };
> > +
> > +static const char driver_name[] = "xilinx-udc";
> > +static const char ep0name[] = "ep0";
> > +
> > +/* Control endpoint configuration.*/
> > +static struct usb_endpoint_descriptor config_bulk_out_desc = {
> > + .bLength = USB_DT_ENDPOINT_SIZE,
> > + .bDescriptorType = USB_DT_ENDPOINT,
> > + .bEndpointAddress = USB_DIR_OUT,
> > + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> > + .wMaxPacketSize = __constant_cpu_to_le16(0x40),
> > +};
> > +
> > +/**
> > + * to_udc - Return the udc instance pointer
> > + * @g: pointer to the usb gadget driver instance
> > + *
> > + * Return: xusb_udc pointer
> > + */
> > +static inline struct xusb_udc *to_udc(struct usb_gadget *g)
> > +{
> > +
>
> remove this whiteline here. Also, this could very well be a macro
> instead of a function. No strong feelings though.
Ok
>
> > + return container_of(g, struct xusb_udc, gadget);
> > +}
> > +
> > +/**
> > + * xudc_write32 - little endian write to device registers
> > + * @val: data to be written
> > + * @addr: addr of device register
> > + */
> > +static void xudc_write32(u32 val, void __iomem *addr)
>
> usually, people tend to pass addr, offset and value. Then you do the
> quick little math internally:
>
> iowrite32(val, addr + offset);
>
> no strong feelings either.
>
> > +static int xudc_eptxrx(struct xusb_ep *ep, u8 *bufferptr, u32 bufferlen,
> > + u8 direction)
> > +{
> > + u32 *eprambase;
> > + u32 bytestosend;
> > + u8 *temprambase;
> > + unsigned long timeout;
> > + u32 srcaddr = 0;
> > + u32 dstaddr = 0;
> > + int rc = 0;
> > + struct xusb_udc *udc = ep->udc;
> > +
> > + bytestosend = bufferlen;
> > +
> > + /* Put the transmit buffer into the correct ping-pong buffer.*/
> > + if (!ep->curbufnum && !ep->buffer0ready) {
> > + /* Get the Buffer address and copy the transmit data.*/
> > + eprambase = (u32 __force *)(ep->udc->base_address +
> > + ep->rambase);
> > +
> > + if (ep->udc->dma_enabled) {
> > + if (direction == EP_TRANSMIT) {
> > + srcaddr = dma_map_single(
> > + ep->udc->gadget.dev.parent,
> > + bufferptr, bufferlen, DMA_TO_DEVICE);
> > + dstaddr = virt_to_phys(eprambase);
> > + udc->write_fn(bufferlen,
> > + ep->udc->base_address +
> > + ep->endpointoffset +
> > +
> XUSB_EP_BUF0COUNT_OFFSET);
> > + udc->write_fn(XUSB_DMA_BRR_CTRL |
> > + (1 << ep->epnumber),
> > + ep->udc->base_address +
> > +
> XUSB_DMA_CONTROL_OFFSET);
> > + } else {
> > + srcaddr = virt_to_phys(eprambase);
> > + dstaddr = dma_map_single(
> > + ep->udc->gadget.dev.parent,
> > + bufferptr, bufferlen,
> DMA_FROM_DEVICE);
> > + udc->write_fn(XUSB_DMA_BRR_CTRL |
> > + XUSB_DMA_READ_FROM_DPRAM |
> > + (1 << ep->epnumber),
> > + ep->udc->base_address +
> > + XUSB_DMA_CONTROL_OFFSET);
> > + }
> > + /*
> > + * Set the addresses in the DMA source and destination
> > + * registers and then set the length into the DMA length
> > + * register.
> > + */
> > + udc->write_fn(srcaddr, ep->udc->base_address +
> > + XUSB_DMA_DSAR_ADDR_OFFSET);
> > + udc->write_fn(dstaddr, ep->udc->base_address +
> > + XUSB_DMA_DDAR_ADDR_OFFSET);
> > + udc->write_fn(bufferlen, ep->udc->base_address +
> > + XUSB_DMA_LENGTH_OFFSET);
> > + } else {
> > +
> > + while (bytestosend > 3) {
> > + if (direction == EP_TRANSMIT)
> > + *eprambase++ = *(u32 *)bufferptr;
> > + else
> > + *(u32 *)bufferptr = *eprambase++;
> > + bufferptr += 4;
> > + bytestosend -= 4;
> > + }
> > + temprambase = (u8 *)eprambase;
> > + while (bytestosend--) {
> > + if (direction == EP_TRANSMIT)
> > + *temprambase++ = *bufferptr++;
> > + else
> > + *bufferptr++ = *temprambase++;
> > + }
> > + /*
> > + * Set the Buffer count register with transmit length
> > + * and enable the buffer for transmission.
> > + */
> > + if (direction == EP_TRANSMIT)
> > + udc->write_fn(bufferlen,
> > + ep->udc->base_address +
> > + ep->endpointoffset +
> > + XUSB_EP_BUF0COUNT_OFFSET);
> > + udc->write_fn(1 << ep->epnumber,
> > + ep->udc->base_address +
> > + XUSB_BUFFREADY_OFFSET);
> > + }
> > + ep->buffer0ready = 1;
> > + ep->curbufnum = 1;
> > +
> > + } else
> > + if ((ep->curbufnum == 1) && (!ep->buffer1ready)) {
> > +
> > + /* Get the Buffer address and copy the transmit data.*/
> > + eprambase = (u32 __force *)(ep->udc->base_address +
> > + ep->rambase + ep-
> >ep_usb.maxpacket);
> > + if (ep->udc->dma_enabled) {
> > + if (direction == EP_TRANSMIT) {
> > + srcaddr = dma_map_single(
> > + ep->udc->gadget.dev.parent,
> > + bufferptr, bufferlen,
> > + DMA_TO_DEVICE);
> > + dstaddr = virt_to_phys(eprambase);
> > + udc->write_fn(bufferlen,
> > + ep->udc->base_address +
> > + ep->endpointoffset +
> > +
> XUSB_EP_BUF1COUNT_OFFSET);
> > + udc->write_fn(XUSB_DMA_BRR_CTRL
> |
> > + (1 << (ep->epnumber +
> > +
> XUSB_STATUS_EP_BUFF2_SHIFT)),
> > + ep->udc->base_address +
> > +
> XUSB_DMA_CONTROL_OFFSET);
> > + } else {
> > + srcaddr = virt_to_phys(eprambase);
> > + dstaddr = dma_map_single(
> > + ep->udc->gadget.dev.parent,
> > + bufferptr, bufferlen,
> > + DMA_FROM_DEVICE);
> > + udc->write_fn(XUSB_DMA_BRR_CTRL
> |
> > +
> XUSB_DMA_READ_FROM_DPRAM |
> > + (1 << (ep->epnumber +
> > +
> XUSB_STATUS_EP_BUFF2_SHIFT)),
> > + ep->udc->base_address +
> > +
> XUSB_DMA_CONTROL_OFFSET);
> > + }
> > + /*
> > + * Set the addresses in the DMA source and
> > + * destination registers and then set the length
> > + * into the DMA length register.
> > + */
> > + udc->write_fn(srcaddr,
> > + ep->udc->base_address +
> > +
> XUSB_DMA_DSAR_ADDR_OFFSET);
> > + udc->write_fn(dstaddr,
> > + ep->udc->base_address +
> > +
> XUSB_DMA_DDAR_ADDR_OFFSET);
> > + udc->write_fn(bufferlen,
> > + ep->udc->base_address +
> > + XUSB_DMA_LENGTH_OFFSET);
> > + } else {
> > + while (bytestosend > 3) {
> > + if (direction == EP_TRANSMIT)
> > + *eprambase++ =
> > + *(u32 *)bufferptr;
> > + else
> > + *(u32 *)bufferptr =
> > + *eprambase++;
> > + bufferptr += 4;
> > + bytestosend -= 4;
> > + }
> > + temprambase = (u8 *)eprambase;
> > + while (bytestosend--) {
> > + if (direction == EP_TRANSMIT)
> > + *temprambase++ =
> *bufferptr++;
> > + else
> > + *bufferptr++ =
> *temprambase++;
> > + }
> > + /*
> > + * Set the Buffer count register with transmit
> > + * length and enable the buffer for
> > + * transmission.
> > + */
> > + if (direction == EP_TRANSMIT)
> > + udc->write_fn(bufferlen,
> > + ep->udc->base_address +
> > + ep->endpointoffset +
> > +
> XUSB_EP_BUF1COUNT_OFFSET);
> > + udc->write_fn(1 << (ep->epnumber +
> > +
> XUSB_STATUS_EP_BUFF2_SHIFT),
> > + ep->udc->base_address +
> > + XUSB_BUFFREADY_OFFSET);
> > + }
> > + ep->buffer1ready = 1;
> > + ep->curbufnum = 0;
> > + } else {
> > + /*
> > + * None of the ping-pong buffer is free. Return a
> > + * failure.
> > + */
> > + return 1;
> > + }
> > +
> > + if (ep->udc->dma_enabled) {
> > + /*
> > + * Wait till DMA transaction is complete and
> > + * check whether the DMA transaction was
> > + * successful.
> > + */
> > + while ((udc->read_fn(ep->udc->base_address +
> > + XUSB_DMA_STATUS_OFFSET) &
> > + XUSB_DMA_DMASR_BUSY) ==
> XUSB_DMA_DMASR_BUSY) {
> > + timeout = jiffies + 10000;
> > +
> > + if (time_after(jiffies, timeout)) {
> > + rc = -ETIMEDOUT;
> > + goto clean;
> > + }
> > +
> > + }
> > + if ((udc->read_fn(ep->udc->base_address +
> > + XUSB_DMA_STATUS_OFFSET) &
> > + XUSB_DMA_DMASR_ERROR) ==
> XUSB_DMA_DMASR_ERROR)
> > + dev_dbg(&ep->udc->gadget.dev, "DMA Error\n");
> > +clean:
> > + if (direction == EP_TRANSMIT) {
> > + dma_unmap_single(ep->udc->gadget.dev.parent,
> > + srcaddr, bufferlen, DMA_TO_DEVICE);
> > + } else {
> > + dma_unmap_single(ep->udc->gadget.dev.parent,
> > + dstaddr, bufferlen, DMA_FROM_DEVICE);
> > + }
> > + }
> > + return rc;
> > +}
>
> what a big function. Looks like you could split it into smaller
> functions to aid redability.
Ok.
>
> > +static int xudc_ep_enable(struct usb_ep *_ep,
> > + const struct usb_endpoint_descriptor *desc)
> > +{
> > + struct xusb_ep *ep = container_of(_ep, struct xusb_ep, ep_usb);
> > + struct xusb_udc *dev = ep->udc;
> > + u32 tmp;
> > + u8 eptype = 0;
> > + unsigned long flags;
> > + u32 epcfg;
> > + struct xusb_udc *udc = ep->udc;
> > +
> > + /*
> > + * The check for _ep->name == ep0name is not done as this enable i
> used
> > + * for enabling ep0 also. In other gadget drivers, this ep name is not
> > + * used.
> > + */
> > + if (!_ep || !desc || ep->desc ||
> > + desc->bDescriptorType != USB_DT_ENDPOINT) {
> > + dev_dbg(&ep->udc->gadget.dev, "first check fails\n");
>
> you need a more descriptive message here.
Ok.
>
> > + return -EINVAL;
> > + }
> > +
> > + if (!dev->driver || dev->gadget.speed == USB_SPEED_UNKNOWN) {
> > + dev_dbg(&ep->udc->gadget.dev, "bogus device state\n");
> > + return -ESHUTDOWN;
> > + }
> > +
> > +
> > + ep->is_in = ((desc->bEndpointAddress & USB_DIR_IN) != 0);
> > + /*
> > + * Bit 3...0: endpoint number
> > + */
>
> fits in one line, no need for multiline comment.
Ok.
>
> > + ep->epnumber = (desc->bEndpointAddress & 0x0f);
> > + ep->stopped = 0;
> > + ep->desc = desc;
> > + ep->ep_usb.desc = desc;
> > + tmp = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> > + spin_lock_irqsave(&ep->udc->lock, flags);
> > + ep->ep_usb.maxpacket = le16_to_cpu(desc->wMaxPacketSize);
> > +
> > + switch (tmp) {
> > + case USB_ENDPOINT_XFER_CONTROL:
> > + dev_dbg(&ep->udc->gadget.dev, "only one control
> endpoint\n");
> > + /* NON- ISO */
> > + eptype = 0;
> > + spin_unlock_irqrestore(&ep->udc->lock, flags);
> > + return -EINVAL;
> > + case USB_ENDPOINT_XFER_INT:
> > + /* NON- ISO */
> > + eptype = 0;
> > + if (ep->ep_usb.maxpacket > 64)
> > + goto bogus_max;
> > + break;
> > + case USB_ENDPOINT_XFER_BULK:
> > + /* NON- ISO */
> > + eptype = 0;
> > + switch (ep->ep_usb.maxpacket) {
> > + case 8:
> > + case 16:
> > + case 32:
> > + case 64:
> > + case 512:
> > + goto ok;
>
> bogus indentation.
Ok
>
> > + }
> > +bogus_max:
> > + dev_dbg(&ep->udc->gadget.dev, "bogus maxpacket %d\n",
> > + ep->ep_usb.maxpacket);
> > + spin_unlock_irqrestore(&ep->udc->lock, flags);
> > + return -EINVAL;
> > + case USB_ENDPOINT_XFER_ISOC:
> > + /* ISO */
> > + eptype = 1;
> > + ep->is_iso = 1;
> > + break;
> > + }
> > +
> > +ok: ep->eptype = eptype;
>
> the label sits in a line by itself
Ok. Will change.
>
> ok:
> ep->eptype = eptype;
>
> ...
>
> > +static int xudc_ep_disable(struct usb_ep *_ep)
> > +{
> > + struct xusb_ep *ep = container_of(_ep, struct xusb_ep, ep_usb);
> > + unsigned long flags;
> > + u32 epcfg;
> > + struct xusb_udc *udc = ep->udc;
> > +
> > + if (ep == &ep->udc->ep[XUSB_EP_NUMBER_ZERO]) {
> > + dev_dbg(&ep->udc->gadget.dev, "Ep0 disable called\n");
> > + return -EINVAL;
> > + }
> > + spin_lock_irqsave(&ep->udc->lock, flags);
> > +
> > + xudc_nuke(ep, -ESHUTDOWN);
> > +
> > + /* Restore the endpoint's pristine config */
> > + ep->desc = NULL;
> > + ep->ep_usb.desc = NULL;
> > +
> > + ep->stopped = 1;
> > +
> > + dev_dbg(&ep->udc->gadget.dev, "USB Ep %d disable\n ", ep-
> >epnumber);
> > +
> > + /* Disable the endpoint.*/
> > + epcfg = udc->read_fn(ep->udc->base_address + ep->endpointoffset);
> > + epcfg &= ~XUSB_EP_CFG_VALID_MASK;
> > + udc->write_fn(epcfg, ep->udc->base_address + ep->endpointoffset);
> > + spin_unlock_irqrestore(&ep->udc->lock, flags);
> > + return 0;
> > +}
> > +
> > +/**
> > + * xudc_ep_alloc_request - Initializes the request queue.
> > + * @_ep: pointer to the usb device endpoint structure.
> > + * @gfp_flags: Flags related to the request call.
> > + *
> > + * Return: pointer to request structure on success and a NULL on failure.
> > + */
> > +static struct usb_request *xudc_ep_alloc_request(struct usb_ep *_ep,
> > + gfp_t gfp_flags)
> > +{
> > + struct xusb_request *req;
> > +
> > + req = kmalloc(sizeof(*req), gfp_flags);
>
> use kzalloc...
>
> > + if (!req)
> > + return NULL;
> > +
> > + memset(req, 0, sizeof(*req));
>
> ... and drop this list head.
>
> > + INIT_LIST_HEAD(&req->queue);
>
> remove this INIT_LIST_HEAD();
Ok.
>
> also, before returning, I suppose you probably want to link the request
> to the endpoint somehow. Usually people save the endpoint pointer inside
> the private request structure (iow: req->ep = ep;)
Ok.
>
> > + return &req->usb_req;
> > +}
> > +
> > +/**
> > + * xudc_free_request - Releases the request from queue.
> > + * @_ep: pointer to the usb device endpoint structure.
> > + * @_req: pointer to the usb request structure.
> > + */
> > +static void xudc_free_request(struct usb_ep *_ep, struct usb_request *_req)
> > +{
> > + struct xusb_ep *ep = container_of(_ep, struct xusb_ep, ep_usb);
> > + struct xusb_request *req;
> > +
> > + req = container_of(_req, struct xusb_request, usb_req);
>
> define helper macros for these two container_of(). It helps with
> readability.
Ok.
>
> > + if (!list_empty(&req->queue))
> > + dev_warn(&ep->udc->gadget.dev, "Error: No memory to free");
>
> what did you want to do here ? Perhaps:
>
> if (list_empty(&req->queue)) {
> dev_warn(&ep->udc->gadget.dev, "Error: no request to free");
> return;
> }
>
> ???
>
> > + kfree(req);
> > +}
> > +
> > +/**
> > + * xudc_ep_queue - Adds the request to the queue.
> > + * @_ep: pointer to the usb device endpoint structure.
> > + * @_req: pointer to the usb request structure.
> > + * @gfp_flags: Flags related to the request call.
> > + *
> > + * Return: 0 for success and error value on failure
> > + */
> > +static int xudc_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
> > + gfp_t gfp_flags)
> > +{
> > + struct xusb_request *req;
> > + struct xusb_ep *ep;
> > + struct xusb_udc *dev;
> > + unsigned long flags;
> > + u32 length, count;
> > + u8 *corebuf;
> > + struct xusb_udc *udc;
> > +
> > + req = container_of(_req, struct xusb_request, usb_req);
> > + ep = container_of(_ep, struct xusb_ep, ep_usb);
> > + udc = ep->udc;
> > +
> > + if (!_req || !_req->complete || !_req->buf ||
> > + !list_empty(&req->queue)) {
> > + dev_dbg(&ep->udc->gadget.dev, "invalid request\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (!_ep || (!ep->desc && ep->ep_usb.name != ep0name)) {
> > + dev_dbg(&ep->udc->gadget.dev, "invalid ep\n");
> > + return -EINVAL;
> > + }
> > +
> > + dev = ep->udc;
> > + if (!dev || !dev->driver || dev->gadget.speed ==
> USB_SPEED_UNKNOWN) {
> > + dev_dbg(&ep->udc->gadget.dev, "%s, bogus device state %p\n",
> > + __func__, dev->driver);
> > + return -ESHUTDOWN;
> > + }
> > +
> > + spin_lock_irqsave(&dev->lock, flags);
> > +
> > + _req->status = -EINPROGRESS;
> > + _req->actual = 0;
> > +
> > + /* Try to kickstart any empty and idle queue */
> > + if (list_empty(&ep->queue)) {
> > + if (!ep->epnumber) {
> > + ep->data = req;
> > + if (ch9_cmdbuf.setup.bRequestType & USB_DIR_IN) {
> > + ch9_cmdbuf.contwriteptr = req->usb_req.buf +
> > + req->usb_req.actual;
> > + prefetch(ch9_cmdbuf.contwriteptr);
> > + length = req->usb_req.length -
> > + req->usb_req.actual;
> > + corebuf = (void __force *) ((ep->rambase << 2)
> +
> > + ep->udc->base_address);
> > + ch9_cmdbuf.contwritecount = length;
> > + length = count = min_t(u32, length,
> > + EP0_MAX_PACKET);
> > + while (length--)
> > + *corebuf++ =
> *ch9_cmdbuf.contwriteptr++;
> > + udc->write_fn(count, ep->udc->base_address +
> > + XUSB_EP_BUF0COUNT_OFFSET);
> > + udc->write_fn(1, ep->udc->base_address +
> > + XUSB_BUFFREADY_OFFSET);
> > + ch9_cmdbuf.contwritecount -= count;
> > + } else {
> > + if (ch9_cmdbuf.setup.wLength) {
> > + ch9_cmdbuf.contreadptr =
> > + req->usb_req.buf +
> > + req->usb_req.actual;
> > + udc->write_fn(req->usb_req.length ,
> > + ep->udc->base_address +
> > +
> XUSB_EP_BUF0COUNT_OFFSET);
> > + udc->write_fn(1, ep->udc-
> >base_address
> > + + XUSB_BUFFREADY_OFFSET);
> > + } else {
> > + xudc_wrstatus(udc);
> > + req = NULL;
> > + }
> > + }
> > + } else {
> > +
> > + if (ep->is_in) {
> > + dev_dbg(&ep->udc->gadget.dev,
> > + "xudc_write_fifo called from
> queue\n");
> > + if (xudc_write_fifo(ep, req) == 1)
> > + req = NULL;
> > + } else {
> > + dev_dbg(&ep->udc->gadget.dev,
> > + "xudc_read_fifo called from queue\n");
> > + if (xudc_read_fifo(ep, req) == 1)
> > + req = NULL;
> > + }
> > + }
> > + }
> > +
> > + if (req != NULL)
> > + list_add_tail(&req->queue, &ep->queue);
> > + spin_unlock_irqrestore(&dev->lock, flags);
> > + return 0;
> > +}
> > +
> > +/**
> > + * xudc_ep_dequeue - Removes the request from the queue.
> > + * @_ep: pointer to the usb device endpoint structure.
> > + * @_req: pointer to the usb request structure.
> > + *
> > + * Return: 0 for success and error value on failure
> > + */
> > +static int xudc_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> > +{
> > + struct xusb_ep *ep;
> > + struct xusb_request *req;
> > + unsigned long flags;
> > +
> > + ep = container_of(_ep, struct xusb_ep, ep_usb);
> > +
> > + if (!_ep || ep->ep_usb.name == ep0name)
> > + return -EINVAL;
> > +
> > + spin_lock_irqsave(&ep->udc->lock, flags);
> > + /* Make sure it's actually queued on this endpoint */
> > + list_for_each_entry(req, &ep->queue, queue) {
> > + if (&req->usb_req == _req)
> > + break;
> > + }
> > + if (&req->usb_req != _req) {
> > + spin_unlock_irqrestore(&ep->udc->lock, flags);
> > + return -EINVAL;
> > + }
> > +
> > + xudc_done(ep, req, -ECONNRESET);
> > + spin_unlock_irqrestore(&ep->udc->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static struct usb_ep_ops xusb_ep_ops = {
> > + .enable = xudc_ep_enable,
> > + .disable = xudc_ep_disable,
> > +
> > + .alloc_request = xudc_ep_alloc_request,
> > + .free_request = xudc_free_request,
> > +
> > + .queue = xudc_ep_queue,
> > + .dequeue = xudc_ep_dequeue,
> > + .set_halt = xudc_ep_set_halt,
> > +};
> > +
> > +/**
> > + * xudc_get_frame - Reads the current usb frame number.
> > + * @gadget: pointer to the usb gadget structure.
> > + *
> > + * Return: current frame number for success and error value on failure.
> > + */
> > +static int xudc_get_frame(struct usb_gadget *gadget)
> > +{
> > +
> > + struct xusb_udc *udc = to_udc(gadget);
> > + unsigned long flags;
> > + int retval;
> > +
> > + if (!gadget)
> > + return -ENODEV;
> > +
> > + local_irq_save(flags);
> > + retval = udc->read_fn(udc->base_address +
> XUSB_FRAMENUM_OFFSET);
> > + local_irq_restore(flags);
> > + return retval;
> > +}
> > +
> > +/**
> > + * xudc_reinit - Restores inital software state.
> > + * @udc: pointer to the usb device controller structure.
> > + */
> > +static void xudc_reinit(struct xusb_udc *udc)
> > +{
> > + u32 ep_number;
> > + char name[4];
> > +
> > + INIT_LIST_HEAD(&udc->gadget.ep_list);
> > + INIT_LIST_HEAD(&udc->gadget.ep0->ep_list);
> > +
> > + for (ep_number = 0; ep_number < XUSB_MAX_ENDPOINTS;
> ep_number++) {
> > + struct xusb_ep *ep = &udc->ep[ep_number];
> > +
> > + if (ep_number) {
> > + list_add_tail(&ep->ep_usb.ep_list,
> > + &udc->gadget.ep_list);
> > + ep->ep_usb.maxpacket = (unsigned short)~0;
> > + sprintf(name, "ep%d", ep_number);
> > + strcpy(ep->name, name);
> > + ep->ep_usb.name = ep->name;
> > + } else {
> > + ep->ep_usb.name = ep0name;
> > + ep->ep_usb.maxpacket = 0x40;
> > + }
> > +
> > + ep->ep_usb.ops = &xusb_ep_ops;
> > + ep->udc = udc;
> > + ep->epnumber = ep_number;
> > + ep->desc = NULL;
> > + ep->stopped = 0;
> > + /*
> > + * The configuration register address offset between
> > + * each endpoint is 0x10.
> > + */
> > + ep->endpointoffset = XUSB_EP0_CONFIG_OFFSET +
> > + (ep_number * 0x10);
> > + ep->is_in = 0;
> > + ep->is_iso = 0;
> > + ep->maxpacket = 0;
> > + xudc_epconfig(ep, udc);
> > + udc->status = 0;
> > +
> > + /* Initialize one queue per endpoint */
> > + INIT_LIST_HEAD(&ep->queue);
> > + }
> > +}
> > +
> > +/**
> > + * xudc_stop_activity - Stops any further activity on the device.
> > + * @udc: pointer to the usb device controller structure.
> > + */
> > +static void xudc_stop_activity(struct xusb_udc *udc)
> > +{
> > + struct usb_gadget_driver *driver = udc->driver;
> > + int i;
> > +
> > + if (udc->gadget.speed == USB_SPEED_UNKNOWN)
> > + driver = NULL;
> > + udc->gadget.speed = USB_SPEED_HIGH;
> > +
> > + for (i = 0; i < XUSB_MAX_ENDPOINTS; i++) {
> > + struct xusb_ep *ep = &udc->ep[i];
> > +
> > + ep->stopped = 1;
> > + xudc_nuke(ep, -ESHUTDOWN);
> > + }
> > + if (driver) {
> > + spin_unlock(&udc->lock);
> > + driver->disconnect(&udc->gadget);
> > + spin_lock(&udc->lock);
> > + }
>
> you shouldn't be calling disconnect here, udc-core is doing that for
> you.
Ok.
>
> > +
> > + xudc_reinit(udc);
> > +}
> > +
> > +/**
> > + * xudc_start - Starts the device.
> > + * @gadget: pointer to the usb gadget structure
> > + * @driver: pointer to gadget driver structure
> > + *
> > + * Return: zero always
> > + */
> > +static int xudc_start(struct usb_gadget *gadget,
> > + struct usb_gadget_driver *driver)
> > +{
> > + struct xusb_udc *udc = to_udc(gadget);
> > + const struct usb_endpoint_descriptor *d = &config_bulk_out_desc;
> > +
> > + driver->driver.bus = NULL;
> > + /* hook up the driver */
> > + udc->driver = driver;
> > + udc->gadget.dev.driver = &driver->driver;
> > + udc->gadget.speed = driver->max_speed;
> > +
> > + /* Enable the USB device.*/
> > + xudc_ep_enable(&udc->ep[XUSB_EP_NUMBER_ZERO].ep_usb, d);
> > + udc->write_fn(0, (udc->base_address + XUSB_ADDRESS_OFFSET));
> > + udc->write_fn(XUSB_CONTROL_USB_READY_MASK,
> > + udc->base_address + XUSB_CONTROL_OFFSET);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xudc_stop - stops the device.
> > + * @gadget: pointer to the usb gadget structure
> > + * @driver: pointer to usb gadget driver structure
> > + *
> > + * Return: zero always
> > + */
> > +static int xudc_stop(struct usb_gadget *gadget,
> > + struct usb_gadget_driver *driver)
> > +{
> > + struct xusb_udc *udc = to_udc(gadget);
> > + unsigned long flags;
> > + u32 crtlreg;
> > +
> > + /* Disable USB device.*/
> > + crtlreg = udc->read_fn(udc->base_address + XUSB_CONTROL_OFFSET);
> > + crtlreg &= ~XUSB_CONTROL_USB_READY_MASK;
> > + udc->write_fn(crtlreg, udc->base_address + XUSB_CONTROL_OFFSET);
> > + spin_lock_irqsave(&udc->lock, flags);
> > + udc->gadget.speed = USB_SPEED_UNKNOWN;
> > + xudc_stop_activity(udc);
> > + spin_unlock_irqrestore(&udc->lock, flags);
> > +
> > + udc->gadget.dev.driver = NULL;
> > + udc->driver = NULL;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct usb_gadget_ops xusb_udc_ops = {
> > + .get_frame = xudc_get_frame,
> > + .udc_start = xudc_start,
> > + .udc_stop = xudc_stop,
> > +};
> > +
> > +/**
> > + * xudc_startup_handler - The usb device controller interrupt handler.
> > + * @callbackref: pointer to the reference value being passed.
> > + * @intrstatus: The mask value containing the interrupt sources.
> > + *
> > + * This handler handles the RESET, SUSPEND and DISCONNECT interrupts.
> > + */
> > +static void xudc_startup_handler(void *callbackref, u32 intrstatus)
>
> why void ? why isn't this returning irqreturn_t ?
>
> > +{
> > + struct xusb_udc *udc;
> > + u32 intrreg;
> > +
> > + udc = (struct xusb_udc *) callbackref;
>
> you don't need the cast here.
>
> > + if (intrstatus & XUSB_STATUS_RESET_MASK) {
> > + dev_dbg(&udc->gadget.dev, "Reset\n");
> > + if (intrstatus & XUSB_STATUS_HIGH_SPEED_MASK)
> > + udc->gadget.speed = USB_SPEED_HIGH;
> > + else
> > + udc->gadget.speed = USB_SPEED_FULL;
> > +
> > + if (udc->status == 1) {
> > + udc->status = 0;
> > + /* Set device address to 0.*/
> > + udc->write_fn(0, udc->base_address +
> > + XUSB_ADDRESS_OFFSET);
> > + }
> > + /* Disable the Reset interrupt.*/
> > + intrreg = udc->read_fn(udc->base_address +
> > + XUSB_IER_OFFSET);
> > +
> > + intrreg &= ~XUSB_STATUS_RESET_MASK;
> > + udc->write_fn(intrreg, udc->base_address + XUSB_IER_OFFSET);
> > + /* Enable thesuspend and disconnect.*/
> > + intrreg =
> > + udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> > + intrreg |=
> > + (XUSB_STATUS_SUSPEND_MASK |
> > + XUSB_STATUS_DISCONNECT_MASK);
> > + udc->write_fn(intrreg, udc->base_address + XUSB_IER_OFFSET);
> > + }
> > +
> > + if (intrstatus & XUSB_STATUS_DISCONNECT_MASK) {
> > +
> > + /* Disable the Disconnect interrupt.*/
> > + intrreg =
> > + udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> > + intrreg &= ~XUSB_STATUS_DISCONNECT_MASK;
> > + udc->write_fn(intrreg, udc->base_address + XUSB_IER_OFFSET);
> > + dev_dbg(&udc->gadget.dev, "Disconnect\n");
> > + if (udc->status == 1) {
> > + udc->status = 0;
> > + /* Set device address to 0.*/
> > + udc->write_fn(0, udc->base_address +
> > + XUSB_ADDRESS_OFFSET);
> > + /* Enable the USB device.*/
> > + udc->write_fn(XUSB_CONTROL_USB_READY_MASK,
> > + udc->base_address +
> XUSB_CONTROL_OFFSET);
> > + }
> > +
> > + /* Enable the suspend and reset interrupts.*/
> > + intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET)
> |
> > + XUSB_STATUS_SUSPEND_MASK |
> > + XUSB_STATUS_RESET_MASK;
> > + udc->write_fn(intrreg, udc->base_address + XUSB_IER_OFFSET);
> > + xudc_stop_activity(udc);
> > + }
> > +
> > + if (intrstatus & XUSB_STATUS_SUSPEND_MASK) {
> > + dev_dbg(&udc->gadget.dev, "Suspend\n");
> > + /* Disable the Suspend interrupt.*/
> > + intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET)
> &
> > + ~XUSB_STATUS_SUSPEND_MASK;
> > + udc->write_fn(intrreg, udc->base_address + XUSB_IER_OFFSET);
> > + /* Enable the Disconnect and reset interrupts. */
> > + intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET)
> |
> > + XUSB_STATUS_DISCONNECT_MASK |
> > + XUSB_STATUS_RESET_MASK;
> > + udc->write_fn(intrreg, udc->base_address + XUSB_IER_OFFSET);
> > + }
> > +}
> > +
> > +/**
> > + * xudc_set_clear_feature - Executes the set feature and clear feature
> commands.
> > + * @udc: pointer to the usb device controller structure.
> > + * @flag: Value deciding the set or clear action.
> > + *
> > + * Processes the SET_FEATURE and CLEAR_FEATURE commands.
> > + */
> > +static void xudc_set_clear_feature(struct xusb_udc *udc, int flag)
> > +{
> > + u8 endpoint;
> > + u8 outinbit;
> > + u32 epcfgreg;
> > +
> > + switch (ch9_cmdbuf.setup.bRequestType) {
> > + case STANDARD_OUT_DEVICE:
>
> use the macros from <linux/usb/ch9.h>
Ok.
>
> > + switch (ch9_cmdbuf.setup.wValue) {
> > + case USB_DEVICE_TEST_MODE:
> > + /*
> > + * The Test Mode will be executed
> > + * after the status phase.
> > + */
> > + break;
> > +
> > + default:
> > + epcfgreg = udc->read_fn(udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].endpointoffset);
> > + epcfgreg |= XUSB_EP_CFG_STALL_MASK;
> > + udc->write_fn(epcfgreg, udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].endpointoffset);
> > + break;
> > + }
> > + break;
> > +
> > + case STANDARD_OUT_ENDPOINT:
>
> use the macros from <linux/usb/ch9.h>
Ok.
>
> > + if (!ch9_cmdbuf.setup.wValue) {
> > + endpoint = ch9_cmdbuf.setup.wIndex & 0xf;
> > + outinbit = ch9_cmdbuf.setup.wIndex & 0x80;
> > + outinbit = outinbit >> 7;
> > +
> > + /* Make sure direction matches.*/
> > + if (outinbit != udc->ep[endpoint].is_in) {
> > + epcfgreg = udc->read_fn(udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].
> > + endpointoffset);
> > + epcfgreg |= XUSB_EP_CFG_STALL_MASK;
> > +
> > + udc->write_fn(epcfgreg, udc->base_address +
> > + udc->ep[XUSB_EP_NUMBER_ZERO].
> > + endpointoffset);
> > + return;
> > + }
> > +
> > + if (!endpoint) {
> > + /* Clear the stall.*/
> > + epcfgreg = udc->read_fn(udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].
> > + endpointoffset);
> > +
> > + epcfgreg &= ~XUSB_EP_CFG_STALL_MASK;
> > +
> > + udc->write_fn(epcfgreg, udc->base_address +
> > + udc->ep[XUSB_EP_NUMBER_ZERO].
> > + endpointoffset);
> > + break;
> > + } else {
> > + if (flag == 1) {
> > + epcfgreg =
> > + udc->read_fn(udc-
> >base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].
> > + endpointoffset);
> > + epcfgreg |=
> XUSB_EP_CFG_STALL_MASK;
> > +
> > + udc->write_fn(epcfgreg,
> > + udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].
> > + endpointoffset);
> > + } else {
> > + /* Unstall the endpoint.*/
> > + epcfgreg =
> > + udc->read_fn(udc-
> >base_address +
> > + udc->ep[endpoint].
> > + endpointoffset);
> > + epcfgreg &=
> > + ~(XUSB_EP_CFG_STALL_MASK
> |
> > +
> XUSB_EP_CFG_DATA_TOGGLE_MASK);
> > + udc->write_fn(epcfgreg,
> > + udc->base_address +
> > + udc->ep[endpoint].
> > + endpointoffset);
> > + }
> > + }
> > + }
> > + break;
> > +
> > + default:
> > + epcfgreg = udc->read_fn(udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].endpointoffset);
> > + epcfgreg |= XUSB_EP_CFG_STALL_MASK;
> > + udc->write_fn(epcfgreg, udc->base_address +
> > + udc->ep[XUSB_EP_NUMBER_ZERO].endpointoffset);
> > + return;
> > + }
> > +
> > + /* Cause and valid status phase to be issued.*/
> > + xudc_wrstatus(udc);
> > +
> > + return;
> > +}
> > +
> > +/**
> > + * xudc_execute_cmd - Processes the USB specification chapter 9 commands.
> > + * @udc: pointer to the usb device controller structure.
> > + *
> > + * Return: 0 for success and the same reuqest command if it is not handled.
> > + */
> > +static int xudc_execute_cmd(struct xusb_udc *udc)
> > +{
> > +
> > + if ((ch9_cmdbuf.setup.bRequestType & USB_TYPE_MASK) ==
> > + USB_TYPE_STANDARD) {
> > + /* Process the chapter 9 command.*/
> > + switch (ch9_cmdbuf.setup.bRequest) {
> > +
> > + case USB_REQ_CLEAR_FEATURE:
> > + xudc_set_clear_feature(udc, 0);
> > + break;
> > +
> > + case USB_REQ_SET_FEATURE:
> > + xudc_set_clear_feature(udc, 1);
> > + break;
> > +
> > + case USB_REQ_SET_ADDRESS:
> > + xudc_wrstatus(udc);
> > + break;
> > +
> > + case USB_REQ_SET_CONFIGURATION:
> > + udc->status = 1;
> > + return ch9_cmdbuf.setup.bRequest;
> > +
> > + default:
> > + /*
> > + * Return the same request to application for
> > + * handling.
> > + */
> > + return ch9_cmdbuf.setup.bRequest;
> > + }
> > +
> > + } else {
> > + if ((ch9_cmdbuf.setup.bRequestType & USB_TYPE_MASK) ==
> > + USB_TYPE_CLASS)
> > + return ch9_cmdbuf.setup.bRequest;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * xudc_handle_setup - Processes the setup packet.
> > + * @udc: pointer to the usb device controller structure.
> > + * @ctrl: pointer to the usb control endpoint structure.
> > + *
> > + * Return: 0 for success and request to be handled by application if
> > + * is not handled by the driver.
> > + */
> > +static int xudc_handle_setup(struct xusb_udc *udc, struct usb_ctrlrequest
> *ctrl)
> > +{
> > + u32 *ep0rambase;
> > +
> > + /* Load up the chapter 9 command buffer.*/
> > + ep0rambase = (u32 __force *) (udc->base_address +
> > + XUSB_SETUP_PKT_ADDR_OFFSET);
> > + memcpy((void *)&ch9_cmdbuf.setup, (void *)ep0rambase, 8);
> > +
> > + ctrl->bRequestType = ch9_cmdbuf.setup.bRequestType;
> > + ctrl->bRequest = ch9_cmdbuf.setup.bRequest;
> > + ctrl->wValue = ch9_cmdbuf.setup.wValue;
> > + ctrl->wIndex = ch9_cmdbuf.setup.wIndex;
> > + ctrl->wLength = ch9_cmdbuf.setup.wLength;
> > +
> > + ch9_cmdbuf.setup.wValue = cpu_to_le16(ch9_cmdbuf.setup.wValue);
> > + ch9_cmdbuf.setup.wIndex = cpu_to_le16(ch9_cmdbuf.setup.wIndex);
> > + ch9_cmdbuf.setup.wLength = cpu_to_le16(ch9_cmdbuf.setup.wLength);
> > +
> > + /* Restore ReadPtr to data buffer.*/
> > + ch9_cmdbuf.contreadptr = &ch9_cmdbuf.contreaddatabuffer[0];
> > + ch9_cmdbuf.contreadcount = 0;
> > +
> > + if (ch9_cmdbuf.setup.bRequestType & USB_DIR_IN) {
> > + /* Execute the get command.*/
> > + ch9_cmdbuf.setupseqrx = STATUS_PHASE;
> > + ch9_cmdbuf.setupseqtx = DATA_PHASE;
> > + return xudc_execute_cmd(udc);
> > + } else {
> > + /* Execute the put command.*/
> > + ch9_cmdbuf.setupseqrx = DATA_PHASE;
> > + ch9_cmdbuf.setupseqtx = STATUS_PHASE;
> > + return xudc_execute_cmd(udc);
> > + }
> > + /* Control should never come here.*/
> > + return 0;
> > +}
> > +
> > +/**
> > + * xudc_ep0_out - Processes the endpoint 0 OUT token.
> > + * @udc: pointer to the usb device controller structure.
> > + */
> > +static void xudc_ep0_out(struct xusb_udc *udc)
> > +{
> > + struct xusb_ep *ep;
> > + u8 count;
> > + u8 *ep0rambase;
> > + u16 index;
> > +
> > + ep = &udc->ep[0];
> > + switch (ch9_cmdbuf.setupseqrx) {
> > + case STATUS_PHASE:
>
> what about the setup phase ?
This not USB phases and just internal to driver.
>
> > + /*
> > + * This resets both state machines for the next
> > + * Setup packet.
> > + */
> > + ch9_cmdbuf.setupseqrx = SETUP_PHASE;
> > + ch9_cmdbuf.setupseqtx = SETUP_PHASE;
> > + ep->data->usb_req.actual = ep->data->usb_req.length;
> > + xudc_done(ep, ep->data, 0);
> > + break;
> > +
> > + case DATA_PHASE:
> > + count = udc->read_fn(udc->base_address +
> > + XUSB_EP_BUF0COUNT_OFFSET);
> > + /* Copy the data to be received from the DPRAM. */
> > + ep0rambase =
> > + (u8 __force *) (udc->base_address +
> > + (udc->ep[XUSB_EP_NUMBER_ZERO].rambase
> << 2));
> > +
> > + for (index = 0; index < count; index++)
> > + *ch9_cmdbuf.contreadptr++ = *ep0rambase++;
> > +
> > + ch9_cmdbuf.contreadcount += count;
> > + if (ch9_cmdbuf.setup.wLength == ch9_cmdbuf.contreadcount) {
> > + xudc_wrstatus(udc);
> > + } else {
> > + /* Set the Tx packet size and the Tx enable bit.*/
> > + udc->write_fn(0, udc->base_address +
> > + XUSB_EP_BUF0COUNT_OFFSET);
> > + udc->write_fn(1, udc->base_address +
> > + XUSB_BUFFREADY_OFFSET);
> > + }
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +/**
> > + * xudc_ep0_in - Processes the endpoint 0 IN token.
> > + * @udc: pointer to the usb device controller structure.
> > + */
> > +static void xudc_ep0_in(struct xusb_udc *udc)
> > +{
> > + struct xusb_ep *ep;
> > + u32 epcfgreg;
> > + u16 count;
> > + u16 length;
> > + u8 *ep0rambase;
> > +
> > + ep = &udc->ep[0];
> > + switch (ch9_cmdbuf.setupseqtx) {
> > + case STATUS_PHASE:
> > + if (ch9_cmdbuf.setup.bRequest == USB_REQ_SET_ADDRESS) {
> > + /* Set the address of the device.*/
> > + udc->write_fn(ch9_cmdbuf.setup.wValue,
> > + (udc->base_address +
> > + XUSB_ADDRESS_OFFSET));
> > + break;
> > + } else {
> > + if (ch9_cmdbuf.setup.bRequest ==
> USB_REQ_SET_FEATURE) {
> > + if (ch9_cmdbuf.setup.bRequestType ==
> > + STANDARD_OUT_DEVICE) {
> > + if (ch9_cmdbuf.setup.wValue ==
> > + USB_DEVICE_TEST_MODE)
> > + udc->write_fn(TEST_J,
> > + udc->base_address +
> > +
> XUSB_TESTMODE_OFFSET);
>
> use a switch.
>
> > + }
> > + }
> > + }
> > + ep->data->usb_req.actual = ch9_cmdbuf.setup.wLength;
> > + xudc_done(ep, ep->data, 0);
> > + break;
> > +
> > + case DATA_PHASE:
> > + if (!ch9_cmdbuf.contwritecount) {
> > + /*
> > + * We're done with data transfer, next
> > + * will be zero length OUT with data toggle of
> > + * 1. Setup data_toggle.
> > + */
> > + epcfgreg = udc->read_fn(udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].endpointoffset);
> > + epcfgreg |= XUSB_EP_CFG_DATA_TOGGLE_MASK;
> > + udc->write_fn(epcfgreg, udc->base_address +
> > + udc-
> >ep[XUSB_EP_NUMBER_ZERO].endpointoffset);
> > + count = 0;
> > +
> > + ch9_cmdbuf.setupseqtx = STATUS_PHASE;
> > +
> > + } else {
> > + length = count = min_t(u32,
> ch9_cmdbuf.contwritecount,
> > + EP0_MAX_PACKET);
> > + /* Copy the data to be transmitted into the DPRAM. */
> > + ep0rambase = (u8 __force *) (udc->base_address +
> > + (udc->ep[XUSB_EP_NUMBER_ZERO].rambase
> << 2));
> > + while (length--)
> > + *ep0rambase++ =
> *ch9_cmdbuf.contwriteptr++;
> > +
> > + ch9_cmdbuf.contwritecount -= count;
> > + }
> > + udc->write_fn(count, udc->base_address +
> > + XUSB_EP_BUF0COUNT_OFFSET);
> > + udc->write_fn(1, udc->base_address +
> XUSB_BUFFREADY_OFFSET);
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +/**
> > + * xudc_ctrl_ep_handler - Endpoint 0 interrupt handler.
> > + * @callbackref: pointer to the call back reference passed by the
> > + * main interrupt handler.
> > + * @intrstatus: It's the mask value for the interrupt sources on
> endpoint 0.
> > + *
> > + * Processes the commands received during enumeration phase.
> > + */
> > +static void xudc_ctrl_ep_handler(void *callbackref, u32 intrstatus)
> > +{
> > + struct xusb_udc *udc;
> > + struct usb_ctrlrequest ctrl;
> > + int status;
> > + int epnum;
> > + u32 intrreg;
> > +
> > + udc = (struct xusb_udc *) callbackref;
> > + /* Process the end point zero buffer interrupt.*/
> > + if (intrstatus & XUSB_STATUS_EP0_BUFF1_COMP_MASK) {
> > + if (intrstatus & XUSB_STATUS_SETUP_PACKET_MASK) {
> > + /*
> > + * Enable the Disconnect, suspend and reset
> > + * interrupts.
> > + */
> > + intrreg = udc->read_fn(udc->base_address +
> > + XUSB_IER_OFFSET);
> > + intrreg |= XUSB_STATUS_DISCONNECT_MASK |
> > + XUSB_STATUS_SUSPEND_MASK |
> > + XUSB_STATUS_RESET_MASK;
> > + udc->write_fn(intrreg,
> > + udc->base_address + XUSB_IER_OFFSET);
> > + status = xudc_handle_setup(udc, &ctrl);
> > + if (status || ((ch9_cmdbuf.setup.bRequestType &
> > + USB_TYPE_MASK) ==
> USB_TYPE_CLASS)) {
> > + /*
> > + * Request is to be handled by the gadget
> > + * driver.
> > + */
> > + spin_unlock(&udc->lock);
> > + udc->driver->setup(&udc->gadget, &ctrl);
> > + spin_lock(&udc->lock);
> > + } else {
> > + if (ctrl.bRequest ==
> USB_REQ_CLEAR_FEATURE) {
> > + epnum = ctrl.wIndex & 0xf;
> > + udc->ep[epnum].stopped = 0;
> > + }
> > + if (ctrl.bRequest == USB_REQ_SET_FEATURE) {
> > + epnum = ctrl.wIndex & 0xf;
> > + udc->ep[epnum].stopped = 1;
> > + }
> > + }
> > + } else {
> > + if (intrstatus & XUSB_STATUS_FIFO_BUFF_RDY_MASK)
> > + xudc_ep0_out(udc);
> > + else if (intrstatus &
> > + XUSB_STATUS_FIFO_BUFF_FREE_MASK)
> > + xudc_ep0_in(udc);
> > + }
> > + }
> > +}
> > +
> > +/**
> > + * xudc_nonctrl_ep_handler - Non control endpoint interrupt handler.
> > + * @callbackref: pointer to the call back reference passed by the
> > + * main interrupt handler.
> > + * @epnum: End point number for which the interrupt is to be processed
> > + * @intrstatus: It's the mask value for the interrupt sources on
> endpoint 0.
> > + */
> > +static void xudc_nonctrl_ep_handler(void *callbackref, u8 epnum,
> > + u32 intrstatus)
> > +{
> > +
> > + struct xusb_request *req;
> > + struct xusb_udc *udc;
> > + struct xusb_ep *ep;
> > +
> > + udc = (struct xusb_udc *) callbackref;
> > + ep = &udc->ep[epnum];
> > + /* Process the End point interrupts.*/
> > + if (intrstatus & (XUSB_STATUS_EP0_BUFF1_COMP_MASK << epnum))
> > + ep->buffer0ready = 0;
> > + if (intrstatus & (XUSB_STATUS_EP0_BUFF2_COMP_MASK << epnum))
> > + ep->buffer1ready = 0;
> > +
> > + if (list_empty(&ep->queue))
> > + req = NULL;
> > + else
> > + req = list_entry(ep->queue.next, struct xusb_request, queue);
> > + if (!req)
> > + return;
> > + if (ep->is_in)
> > + xudc_write_fifo(ep, req);
> > + else
> > + xudc_read_fifo(ep, req);
> > +}
> > +
> > +/**
> > + * xudc_irq - The main interrupt handler.
> > + * @irq: The interrupt number.
> > + * @_udc: pointer to the usb device controller structure.
> > + *
> > + * Return: IRQ_HANDLED after the interrupt is handled.
> > + */
> > +static irqreturn_t xudc_irq(int irq, void *_udc)
> > +{
> > + struct xusb_udc *udc = _udc;
> > + u32 intrstatus;
> > + u8 index;
> > + u32 bufintr;
> > +
> > + spin_lock(&(udc->lock));
> > +
> > + /* Read the Interrupt Status Register.*/
> > + intrstatus = udc->read_fn(udc->base_address + XUSB_STATUS_OFFSET);
> > + /* Call the handler for the event interrupt.*/
> > + if (intrstatus & XUSB_STATUS_INTR_EVENT_MASK) {
> > + /*
> > + * Check if there is any action to be done for :
> > + * - USB Reset received {XUSB_STATUS_RESET_MASK}
> > + * - USB Suspend received {XUSB_STATUS_SUSPEND_MASK}
>
> what about resume ? No resume ?
>
> > + * - USB Disconnect received
> {XUSB_STATUS_DISCONNECT_MASK}
> > + */
> > + xudc_startup_handler(udc, intrstatus);
> > + }
> > +
> > + /* Check the buffer completion interrupts */
> > + if (intrstatus & XUSB_STATUS_INTR_BUFF_COMP_ALL_MASK) {
> > + if (intrstatus & XUSB_STATUS_EP0_BUFF1_COMP_MASK)
> > + xudc_ctrl_ep_handler(udc, intrstatus);
> > +
> > + for (index = 1; index < 8; index++) {
> > + bufintr = ((intrstatus &
> > +
> (XUSB_STATUS_EP1_BUFF1_COMP_MASK <<
> > + (index - 1))) ||
> > + (intrstatus &
> > +
> (XUSB_STATUS_EP1_BUFF2_COMP_MASK <<
> > + (index - 1))));
> > +
> > + if (bufintr)
> > + xudc_nonctrl_ep_handler(udc, index,
> > + intrstatus);
> > + }
> > + }
> > + spin_unlock(&(udc->lock));
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +
> > +
> > +/**
> > + * xudc_release - Releases device structure
> > + * @dev: pointer to device structure
> > + */
> > +static void xudc_release(struct device *dev)
> > +{
> > +}
>
> you don't need to define this, udc-core will give you a release method.
Ok.
>
> > +/**
> > + * xudc_probe - The device probe function for driver initialization.
> > + * @pdev: pointer to the platform device structure.
> > + *
> > + * Return: 0 for success and error value on failure
> > + */
> > +static int xudc_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res;
> > + struct xusb_udc *udc;
> > + int irq;
> > + int ret;
> > +
> > + dev_dbg(&pdev->dev, "%s(%p)\n", __func__, pdev);
> > +
> > + udc = devm_kzalloc(&pdev->dev, sizeof(*udc), GFP_KERNEL);
> > + if (!udc)
> > + return -ENOMEM;
> > +
> > + /* Map the registers */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + udc->base_address = devm_ioremap_nocache(&pdev->dev, res->start,
> > + resource_size(res));
>
> use devm_ioremap_resource() instead.
Ok.
>
> > + if (!udc->base_address)
> > + return -ENOMEM;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(&pdev->dev, "unable to get irq\n");
> > + return irq;
> > + }
> > + ret = request_irq(irq, xudc_irq, 0, dev_name(&pdev->dev), udc);
>
> devm_request_irq()
Ok.
>
> > + if (ret < 0) {
> > + dev_dbg(&pdev->dev, "unable to request irq %d", irq);
> > + goto fail0;
> > + }
> > +
> > + udc->dma_enabled = of_property_read_bool(np, "xlnx,include-dma");
> > +
> > + /* Setup gadget structure */
> > + udc->gadget.ops = &xusb_udc_ops;
> > + udc->gadget.max_speed = USB_SPEED_HIGH;
> > + udc->gadget.speed = USB_SPEED_HIGH;
> > + udc->gadget.ep0 = &udc->ep[XUSB_EP_NUMBER_ZERO].ep_usb;
> > + udc->gadget.name = driver_name;
> > +
> > + dev_set_name(&udc->gadget.dev, "xilinx_udc");
> > + udc->gadget.dev.release = xudc_release;
> > + udc->gadget.dev.parent = &pdev->dev;
>
> don't touch the gadget device directly, udc-core handles all of that for
> you.
>
> > +
> > + spin_lock_init(&udc->lock);
> > +
> > + /* Check for IP endianness */
> > + udc->write_fn = xudc_write32_be;
> > + udc->read_fn = xudc_read32_be;
> > + udc->write_fn(TEST_J, udc->base_address + XUSB_TESTMODE_OFFSET);
> > + if ((udc->read_fn(udc->base_address + XUSB_TESTMODE_OFFSET))
> > + != TEST_J) {
> > + udc->write_fn = xudc_write32;
> > + udc->read_fn = xudc_read32;
> > + }
> > + udc->write_fn(0, udc->base_address + XUSB_TESTMODE_OFFSET);
> > +
> > + xudc_reinit(udc);
> > +
> > + /* Set device address to 0.*/
> > + udc->write_fn(0, udc->base_address + XUSB_ADDRESS_OFFSET);
> > +
> > + ret = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
> > + if (ret)
> > + goto fail1;
> > +
> > + /* Enable the interrupts.*/
> > + udc->write_fn(XUSB_STATUS_GLOBAL_INTR_MASK |
> XUSB_STATUS_RESET_MASK |
> > + XUSB_STATUS_DISCONNECT_MASK |
> XUSB_STATUS_SUSPEND_MASK |
> > + XUSB_STATUS_FIFO_BUFF_RDY_MASK |
> > + XUSB_STATUS_FIFO_BUFF_FREE_MASK |
> > + XUSB_STATUS_EP0_BUFF1_COMP_MASK,
> > + udc->base_address + XUSB_IER_OFFSET);
> > +
> > + platform_set_drvdata(pdev, udc);
> > +
> > + dev_info(&pdev->dev, "%s #%d at 0x%08X mapped to 0x%08X\n",
> > + driver_name, 0, (u32)res->start,
> > + (u32 __force)udc->base_address);
>
> make this a dev_vdbg().
Ok.
>
> > +
> > + return 0;
> > +
> > +fail1:
> > + free_irq(irq, udc);
> > +fail0:
> > + dev_dbg(&pdev->dev, "probe failed, %d\n", ret);
>
> this should be dev_err().
Ok.
>
> > + return ret;
> > +}
> > +
> > +/**
> > + * xudc_remove - Releases the resources allocated during the initialization.
> > + * @pdev: pointer to the platform device structure.
> > + *
> > + * Return: 0 for success and error value on failure
> > + */
> > +static int xudc_remove(struct platform_device *pdev)
> > +{
> > + struct xusb_udc *udc = platform_get_drvdata(pdev);
> > +
> > + dev_dbg(&pdev->dev, "remove\n");
> > + usb_del_gadget_udc(&udc->gadget);
> > + if (udc->driver)
> > + return -EBUSY;
> > +
> > + device_unregister(&udc->gadget.dev);
>
> remove this line.
Ok.
>
> Also, you're leaking your IRQ handler, if you switch to
> devm_request_irq() then you don't need to free it, though.
Ok.
>
> From the looks of it, I doubt this was actually tested, you need a lot
> of work on this driver.
Tested on both ARM and Microblaze architectures with Mass storage gadget.
Will send a v2 after addressing all your comments.
>
> cheers
>
> --
> balbi
Thanks,
Sundeep.B.S.
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: [PATCH 2/7] IBM Akebono: Add support for a new PHY interface to the IBM emac driver
From: Mark Rutland @ 2014-02-21 11:18 UTC (permalink / raw)
To: Alistair Popple
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
David S. Miller
In-Reply-To: <1392964293-13687-3-git-send-email-alistair-Y4h6yKqj69EXC2x5gXVKYQ@public.gmane.org>
On Fri, Feb 21, 2014 at 06:31:28AM +0000, Alistair Popple wrote:
> The IBM PPC476GTR SoC that is used on the Akebono board uses a
> different ethernet PHY interface that has wake on lan (WOL) support
> with the IBM emac. This patch adds support to the IBM emac driver for
> this new PHY interface.
>
> At this stage the wake on lan functionality has not been implemented.
>
> Signed-off-by: Alistair Popple <alistair-Y4h6yKqj69EXC2x5gXVKYQ@public.gmane.org>
> Acked-by: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> ---
[...]
> + /* Check for RGMII flags */
> + if (of_get_property(ofdev->dev.of_node, "has-mdio", NULL))
> + dev->flags |= EMAC_RGMII_FLAG_HAS_MDIO;
You can use of_property_read_bool here.
Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 3/3] Documentation: mfd: Add binding document for S2MPA01
From: Sachin Kamat @ 2014-02-21 11:09 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LKML
Cc: Mark Rutland, Sachin Kamat
In-Reply-To: <1392273476-32736-3-git-send-email-sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On 13 February 2014 12:07, Sachin Kamat <sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Added initial binding documentation for S2MPA01 MFD.
>
> Signed-off-by: Sachin Kamat <sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Changes since v2:
> * Re-worded as suggested by Mark Rutland
> ---
> Documentation/devicetree/bindings/mfd/s2mpa01.txt | 90 +++++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/s2mpa01.txt
>
Mark,
Any comments on this patch?
--
With warm regards,
Sachin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/2] i2c: New bus driver for the Qualcomm QUP I2C controller
From: Mark Rutland @ 2014-02-21 11:06 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Pawel Moll, Ian Campbell, Kumar Gala, Rob Landley,
Wolfram Sang, grant.likely@linaro.org, devicetree@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-i2c@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Andy Gross, Stephen Boyd,
Ivan T. Ivanov
In-Reply-To: <1392943090-30556-3-git-send-email-bjorn.andersson@sonymobile.com>
On Fri, Feb 21, 2014 at 12:38:10AM +0000, Bjorn Andersson wrote:
> This bus driver supports the QUP i2c hardware controller in the Qualcomm SOCs.
> The Qualcomm Universal Peripheral Engine (QUP) is a general purpose data path
> engine with input/output FIFOs and an embedded i2c mini-core. The driver
> supports FIFO mode (for low bandwidth applications) and block mode (interrupt
> generated for each block-size data transfer).
>
> Cc: Andy Gross <agross@codeaurora.org>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-qup.c | 772 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 783 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-qup.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index f5ed031..403e196 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -648,6 +648,16 @@ config I2C_PXA_SLAVE
> is necessary for systems where the PXA may be a target on the
> I2C bus.
[...]
> +static const struct of_device_id qup_i2c_dt_match[] = {
> + { .compatible = "qcom,i2c-qup-v1.1.1" },
> + { .compatible = "qcom,i2c-qup-v2.1.1" },
> + { .compatible = "qcom,i2c-qup-v2.2.1" },
The all seem to be handled the same.
Are they all compatible with the "qcom,i2c-qup-v1.1.1" programming
model, such that it could be used as a fallback in the compatible list
(and the driver would only need to look for it for now)?
Cheers,
Mark.
^ permalink raw reply
* Re: [PATCH v3 1/2] i2c: qup: Add device tree bindings information
From: Mark Rutland @ 2014-02-21 11:01 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Pawel Moll, Ian Campbell, Kumar Gala, Rob Landley,
Wolfram Sang, grant.likely@linaro.org, devicetree@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-i2c@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Ivan T. Ivanov
In-Reply-To: <1392943090-30556-2-git-send-email-bjorn.andersson@sonymobile.com>
On Fri, Feb 21, 2014 at 12:38:09AM +0000, Bjorn Andersson wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>
> The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and
> provide input and output FIFO's for it. I2C controller can operate
> as master with supported bus speeds of 100Kbps and 400Kbps.
>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> [bjorn: reformulated part of binding description
> added version to compatible
> cleaned up example]
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> .../devicetree/bindings/i2c/qcom,i2c-qup.txt | 44 ++++++++++++++++++++
> 1 file changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
>
> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt b/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
> new file mode 100644
> index 0000000..38d505f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
> @@ -0,0 +1,44 @@
> +Qualcomm Universal Peripheral (QUP) I2C controller
> +
> +Required properties:
> + - compatible: Should be
> + "qcom,i2c-qup-v1.1.1" for 8660, 8960 and 8064.
> + "qcom,i2c-qup-v2.1.1" for 8974 v1.
> + "qcom,i2c-qup-v2.2.1" for 8974 v2 and later.
> + - reg: Should contain QUP register address and length.
> + - interrupts: Should contain I2C interrupt.
> +
> + - clocks: Should contain the core clock and the AHB clock.
> + - clock-names: Should be "core" for the core clock and "iface" for the
> + AHB clock.
Could you please describe clocks in terms of clock-names? It makes the
relationship clearer, and avoids redundancy (which makes extending the
bindnig easier):
- clocks: A list of phandles + clock-specifiers, one for each entry in
clock-names.
- clock-names: should contain:
* "core" for the core clock
* "iface" for the AHB clock
Cheers,
Mark.
> +
> + - #address-cells: Should be <1> Address cells for i2c device address
> + - #size-cells: Should be <0> as i2c addresses have no size component
> +
> +Optional properties:
> + - clock-frequency: Should specify the desired i2c bus clock frequency in Hz,
> + defaults to 100kHz if omitted.
> +
> +Child nodes should conform to i2c bus binding.
> +
> +Example:
> +
> + i2c@f9924000 {
> + compatible = "qcom,i2c-qup-v2.2.1";
> + reg = <0xf9924000 0x1000>;
> + interrupts = <0 96 0>;
> +
> + clocks = <&gcc GCC_BLSP1_QUP2_I2C_APPS_CLK>, <&gcc GCC_BLSP1_AHB_CLK>;
> + clock-names = "core", "iface";
> +
> + clock-frequency = <355000>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dummy@60 {
> + compatible = "dummy";
> + reg = <0x60>;
> + };
> + };
> +
> --
> 1.7.9.5
>
>
^ permalink raw reply
* Re: [PATCH v4 1/7] drivers: of: add initialization code for reserved memory
From: Marek Szyprowski @ 2014-02-21 11:00 UTC (permalink / raw)
To: Grant Likely, linux-kernel, linux-arm-kernel, linaro-mm-sig,
devicetree, linux-doc
Cc: Benjamin Herrenschmidt, Arnd Bergmann, Michal Nazarewicz,
Tomasz Figa, Sascha Hauer, Laura Abbott, Rob Herring,
Olof Johansson, Pawel Moll, Mark Rutland, Stephen Warren,
Ian Campbell, Tomasz Figa, Kumar Gala, Nishanth Peethambaran,
Marc, Josh Cartwright, Catalin Marinas, Will Deacon,
Paul Mackerras
In-Reply-To: <20140220140100.5BD65C4050F@trevor.secretlab.ca>
Hello,
On 2014-02-20 15:01, Grant Likely wrote:
> On Thu, 20 Feb 2014 11:52:41 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > This patch adds device tree support for contiguous and reserved memory
> > regions defined in device tree.
> >
> > Large memory blocks can be reliably reserved only during early boot.
> > This must happen before the whole memory management subsystem is
> > initialized, because we need to ensure that the given contiguous blocks
> > are not yet allocated by kernel. Also it must happen before kernel
> > mappings for the whole low memory are created, to ensure that there will
> > be no mappings (for reserved blocks) or mapping with special properties
> > can be created (for CMA blocks). This all happens before device tree
> > structures are unflattened, so we need to get reserved memory layout
> > directly from fdt.
> >
> > Later, those reserved memory regions are assigned to devices on each
> > device structure initialization.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > [joshc: rework to implement new DT binding, provide mechanism for
> > plugging in new reserved-memory node handlers via
> > RESERVEDMEM_OF_DECLARE]
> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > [mszyprow: added generic memory reservation code, refactored code to
> > put it directly into fdt.c]
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> > drivers/of/Kconfig | 6 +
> > drivers/of/Makefile | 1 +
> > drivers/of/fdt.c | 145 ++++++++++++++++++
> > drivers/of/of_reserved_mem.c | 296 +++++++++++++++++++++++++++++++++++++
> > drivers/of/platform.c | 7 +
> > include/asm-generic/vmlinux.lds.h | 11 ++
> > include/linux/of_reserved_mem.h | 65 ++++++++
> > 7 files changed, 531 insertions(+)
> > create mode 100644 drivers/of/of_reserved_mem.c
> > create mode 100644 include/linux/of_reserved_mem.h
>
> Hi Marek,
>
> There's a lot of moving parts in this patch. Can you split the patch up a bit please. There are parts that I'm not entierly comfortable with yet and it will help reviewing them if they are added separately. For instance, the attaching regions to devices is something that I want to have some discussion about, but the core reserving static ranges I think is pretty much ready to be merged. I can merge the later while still debating the former if they are split.
>
> I would recommend splitting into four:
> 1) reservation of static regions without the support code for referencing them later
> 2) code to also do dynamic allocations of reserved regions - again without any driver references
> 3) add hooks to reference specific regions.
> 4) hooks into drivers/of/platform.c for wiring into the driver model.
>
> Can you also make the binding doc the first patch?
Ok, I will slice the patch into 4 pieces.
(snipped)
> > +/**
> > + * res_mem_alloc_size() - allocate reserved memory described by 'size', 'align'
> > + * and 'alloc-ranges' properties
> > + */
> > +static int __init
> > +__reserved_mem_alloc_size(unsigned long node, const char *uname,
> > + phys_addr_t *res_base, phys_addr_t *res_size)
> > +{
> > + int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> > + phys_addr_t start = 0, end = 0;
> > + phys_addr_t base = 0, align = 0, size;
> > + unsigned long len;
> > + __be32 *prop;
> > + int nomap;
> > + int ret;
> > +
> > + prop = of_get_flat_dt_prop(node, "size", &len);
> > + if (!prop)
> > + return -EINVAL;
> > +
> > + if (len != dt_root_size_cells * sizeof(__be32)) {
> > + pr_err("Reserved memory: invalid size property in '%s' node.\n",
> > + uname);
> > + return -EINVAL;
> > + }
> > + size = dt_mem_next_cell(dt_root_size_cells, &prop);
> > +
> > + nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> > +
> > + prop = of_get_flat_dt_prop(node, "align", &len);
> > + if (prop) {
> > + if (len != dt_root_addr_cells * sizeof(__be32)) {
> > + pr_err("Reserved memory: invalid align property in '%s' node.\n",
> > + uname);
> > + return -EINVAL;
> > + }
> > + align = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > + }
> > +
> > + prop = of_get_flat_dt_prop(node, "alloc-ranges", &len);
> > + if (prop) {
> > +
> > + if (len % t_len != 0) {
> > + pr_err("Reserved memory: invalid alloc-ranges property in '%s', skipping node.\n",
> > + uname);
> > + return -EINVAL;
> > + }
> > +
> > + base = 0;
> > +
> > + while (len > 0) {
> > + start = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > + end = start + dt_mem_next_cell(dt_root_size_cells,
> > + &prop);
> > +
> > + ret = early_init_dt_alloc_reserved_memory_arch(size,
> > + align, start, end, nomap, &base);
> > + if (ret == 0) {
> > + pr_debug("Reserved memory: allocated memory for '%s' node: base %pa, size %ld MiB\n",
> > + uname, &base,
> > + (unsigned long)size / SZ_1M);
> > + break;
> > + }
> > + len -= t_len;
> > + }
> > +
> > + } else {
> > + ret = early_init_dt_alloc_reserved_memory_arch(size, align,
> > + 0, 0, nomap, &base);
> > + if (ret == 0)
> > + pr_debug("Reserved memory: allocated memory for '%s' node: base %pa, size %ld MiB\n",
> > + uname, &base, (unsigned long)size / SZ_1M);
> > + }
> > +
> > + if (base == 0) {
> > + pr_info("Reserved memory: failed to allocate memory for node '%s'\n",
> > + uname);
> > + return -ENOMEM;
> > + }
>
> <off topic> Wow, the flattree parsing code has to be really verbose. We really need better
> flat tree parsing functions and helpers.
Yes, parsing fdt is a real pain, but please don't ask me to implement
all the
helpers to make it easier together with this patch. I (and probably other
developers) would really like to get this piece merged asap.
> > +
> > + *res_base = base;
> > + *res_size = size;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id __rmem_of_table_sentinel
> > + __used __section(__reservedmem_of_table_end);
> > +
> > +/**
> > + * res_mem_init_node() - call region specific reserved memory init code
> > + */
> > +static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> > +{
> > + extern const struct of_device_id __reservedmem_of_table[];
> > + const struct of_device_id *i;
> > +
> > + for (i = __reservedmem_of_table; i < &__rmem_of_table_sentinel; i++) {
> > + reservedmem_of_init_fn initfn = i->data;
> > + const char *compat = i->compatible;
> > +
> > + if (!of_flat_dt_is_compatible(rmem->fdt_node, compat))
> > + continue;
>
> What if two entries both match the compatible list? Ideally score would
> be taken into account. (I won't block on this issue, it can be a future
> enhancement)
If two entries have same compatible value they will be probed in the order
of presence in the kernel binary. The return value is checked and the next
one is being tried if init fails for the given function. The provided code
already makes use of this feature. Both DMA coherent and CMA use
"shared-dma-pool" compatible. DMA coherent init fails if 'reusable'
property has been found. On the other hand, CMA fails initialization if
'reusable' property is missing. Frankly I would like to change standard
DMA coherent compatible value to 'dma-pool' and keep 'shared-dma-pool'
only for CMA, but I've implemented it the way it has been described in
your binding documentation.
(snipped)
Thanks for your comments, I will send updated patches asap.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH RFC v1 3/3] clk: Add handling of clk parent and rate assigned from DT
From: Sylwester Nawrocki @ 2014-02-21 10:38 UTC (permalink / raw)
To: Grant Likely
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
mturquette-QSEj5FYQhm4dnm+yROfE0A, linux-lFZ/pmaqli7XmaaqVzeoHQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
galak-sgV2jX0FEOL9JmXXK+q4OQ,
kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ,
sw0312.kim-Sze3O3UU22JBDgjK7y7TUQ,
m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ,
t.figa-Sze3O3UU22JBDgjK7y7TUQ
In-Reply-To: <20140220140928.27132C4050F-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
On 20/02/14 15:09, Grant Likely wrote:
[...]
>> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > index 7c52c29..d618498 100644
>> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > @@ -115,3 +115,27 @@ clock signal, and a UART.
>> > ("pll" and "pll-switched").
>> > * The UART has its baud clock connected the external oscillator and its
>> > register clock connected to the PLL clock (the "pll-switched" signal)
>> > +
>> > +==Static initial configuration of clock parent and clock frequency==
>> > +
>> > +Some platforms require static configuration of (parts of) the clock controller
>> > +often determined by the board design. Such a configuration can be specified in
>> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
>> > +properties. The former should contain phandle and clock specifier of the parent
>> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
>> > +should be listed in the clock-names property and a phandle and a clock specifier
>> > +pair corresponding to it should be present in the clocks property.
>> > +
>> > + uart@a000 {
>> > + compatible = "fsl,imx-uart";
>> > + reg = <0xa000 0x1000>;
>> > + ...
>> > + clocks = <&clkcon 0>, <&clkcon 3>;
>> > + clock-names = "baud", "mux";
>> > +
>> > + mux-clk-parent = <&pll 1>;
>> > + baud-clk-rate = <460800>;
>
> This mixes patterns for references to clocks. Plus it requires composing
> property names which is a little painful. I'd rather see a list of
> tuples to match the existing pattern already in use
>
> clocks = <&clkcon 0>, <&clkcon 3>;
> clock-names = "baud", "mux";
> clock-parents = <0> <&pll 1>;
> clock-rates = <0> <460800>;
Thank you for the review. This looks much better to me. My bad, I wasn't
aware 0 can be used to denote an empty phandle like this. ePAPR seems not
to be specifying exact meaning of the 'phandle' property values, except
they be unique. Anyway, it seems to be clearly documented within the
__of_parse_phandle_with_args() function.
I'll try this modified binding instead, presumably it would be useful to
have a variant of __of_parse_phandle_with_args() function which would
accept a context data containing result of previous call within an iteration,
similarly as of_property_next_string() is written. So we don't iterate
from beginning of the list with each __of_parse_phandle_with_args() call.
But it's an optimization issue that could be considered separately I guess.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/3] mfd: fsl imx25 Touchscreen ADC driver
From: Markus Pargmann @ 2014-02-21 10:18 UTC (permalink / raw)
To: Fabio Estevam
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Dmitry Torokhov, Samuel Ortiz,
Lee Jones, Jonathan Cameron,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Sascha Hauer
In-Reply-To: <CAOMZO5BDyyvXs9CxJQOkuEpxZ8fqamCPB67sNhJxES+XU2XkXQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4217 bytes --]
Hi Fabio,
On Thu, Feb 20, 2014 at 02:17:33PM -0300, Fabio Estevam wrote:
> Hi Markus,
>
> On Thu, Feb 20, 2014 at 1:21 PM, Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > This is the core driver for imx25 touchscreen/adc driver. The module
> > has one shared ADC and two different conversion queues which use the
> > ADC. The two queues are identical. Both can be used for general purpose
> > ADC but one is meant to be used for touchscreens.
> >
> > This driver is the core which manages the central components and
> > registers of the TSC/ADC unit. It manages the IRQs and forwards them to
> > the correct components.
> >
> > Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> That's great :-) Nice work!
>
> > ---
> > .../devicetree/bindings/mfd/fsl-imx25-tsadc.txt | 46 ++++
> > drivers/mfd/Kconfig | 9 +
> > drivers/mfd/Makefile | 2 +
> > drivers/mfd/fsl-imx25-tsadc.c | 234 +++++++++++++++++++++
> > include/linux/mfd/imx25-tsadc.h | 138 ++++++++++++
> > 5 files changed, 429 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> > create mode 100644 drivers/mfd/fsl-imx25-tsadc.c
> > create mode 100644 include/linux/mfd/imx25-tsadc.h
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt b/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> > new file mode 100644
> > index 0000000..a857af0e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> > @@ -0,0 +1,46 @@
> > +Freescale mx25 ADC/TSC multifunction device
> > +
> > +This device combines two general purpose conversion queues one used for general
> > +ADC and the other used for touchscreens.
> > +
> > +Required properties:
> > + - compatible: Should be "fsl,imx25-tsadc".
> > + - reg: Memory range of the device.
> > + - interrupts: Interrupt for this device as described in
> > + interrupts/interrupts.txt
> > + - clocks: An 'ipg' clock defined as described in clocks/clock.txt
> > + - interrupt-controller: This device is an interrupt controller. It controls
> > + the interrupts of both conversion queues.
> > + - #interrupt-cells: Should be '<1>'.
> > + - #address-cells: Should be '<1>'.
> > + - #size-cells: Should be '<1>'.
> > + - ranges
> > +
> > +This device includes two conversion queues which can be added as subnodes.
> > +The first queue is for the touchscreen, the second for general purpose ADC.
> > +
> > +Example:
> > + tscadc: tscadc@50030000 {
> > + compatible = "fsl,imx25-tsadc";
> > + reg = <0x50030000 0xc>;
> > + interrupts = <46>;
> > + clocks = <&clks 119>;
> > + clock-names = "ipg";
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > +
> > + tsc: tcq@50030400 {
> > + compatible = "fsl,imx25-tcq";
> > + reg = <0x50030400 0x60>;
> > + ...
> > + };
> > +
> > + adc: gcq@50030800 {
> > + compatible = "fsl,imx25-gcq";
> > + reg = <0x50030800 0x60>;
> > + ...
> > + };
> > + };
>
> The meaning of 'tcq' and 'gcq' acronyms are not obvious. Could they be
> written more explicitily?
>
> Also, what does the '...' mean?
I forgot to answer this one. The bindings for tcq and gcq are described
in the other files, so I didn't want to show any details that are not
relevant for this driver.
Regards,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] iio: adc: fsl,imx25-gcq driver
From: Markus Pargmann @ 2014-02-21 10:12 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: devicetree, linux-input, linux-iio, Dmitry Torokhov, Samuel Ortiz,
Lee Jones, Jonathan Cameron, linux-arm-kernel, kernel
In-Reply-To: <53063F72.9070006@metafoo.de>
[-- Attachment #1: Type: text/plain, Size: 8952 bytes --]
Hi,
On Thu, Feb 20, 2014 at 06:46:26PM +0100, Lars-Peter Clausen wrote:
> On 02/20/2014 05:21 PM, Markus Pargmann wrote:
> >This is a conversion queue driver for the mx25 SoC. It uses the central
> >ADC which is used by two seperate independent queues. This driver
> >prepares different conversion configurations for each possible input.
> >For a conversion it creates a conversionqueue of one item with the
> >correct configuration for the chosen channel. It then executes the queue
> >once and disables the conversion queue afterwards.
> >
> >The reference voltages are configurable through devicetree subnodes,
> >depending on the connections of the ADC inputs.
> >
> >Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>
> Looks good mostly. Just a couple of minor code-style issues. Try to
> run checkpatch, sparse and friends on your driver before submission.
> They catch most of the stuff that needs to be fixed in this driver.
I used checkpatch and will have a look at sparse.
>
> [..]
> >diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >index 2209f28..a421445 100644
> >--- a/drivers/iio/adc/Kconfig
> >+++ b/drivers/iio/adc/Kconfig
> >@@ -113,6 +113,13 @@ config EXYNOS_ADC
> > of SoCs for drivers such as the touchscreen and hwmon to use to share
> > this resource.
> >
> >+config MX25_ADC
> >+ tristate "Freescale MX25 ADC driver"
> >+ depends on MFD_MX25_TSADC
> >+ help
> >+ Generic Conversion Queue driver used for general purpose ADC in the
> >+ MX25. This driver supports single measurements using the MX25 ADC.
> >+
>
> alphabetical order
I changed the config option name to "FSL_MX25_ADC" to have it in
alphabetical order for the user visible menuconfig and the config
symbols.
>
> > config LP8788_ADC
> > bool "LP8788 ADC driver"
> > depends on MFD_LP8788
> >diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >index ba9a10a..63daa2c 100644
> >--- a/drivers/iio/adc/Makefile
> >+++ b/drivers/iio/adc/Makefile
> >@@ -22,3 +22,4 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> > obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> > obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
> > obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> >+obj-$(CONFIG_MX25_ADC) += fsl-imx25-gcq.o
>
> Here too
Fixed.
>
> [...]
>
> >+const struct iio_chan_spec mx25_gcq_channels[MX25_NUM_CFGS] = {
>
> static
Fixed.
>
> >+ MX25_IIO_CHAN(0, "xp"),
> >+ MX25_IIO_CHAN(1, "yp"),
> >+ MX25_IIO_CHAN(2, "xn"),
> >+ MX25_IIO_CHAN(3, "yn"),
> >+ MX25_IIO_CHAN(4, "wiper"),
> >+ MX25_IIO_CHAN(5, "inaux0"),
> >+ MX25_IIO_CHAN(6, "inaux1"),
> >+ MX25_IIO_CHAN(7, "inaux2"),
> >+};
> >+
>
> >+struct iio_info mx25_gcq_iio_info = {
>
> static const
Fixed.
>
> >+ .read_raw = mx25_gcq_read_raw,
> >+};
> >+
> >+static struct regmap_config mx25_gcq_regconfig = {
>
> const
Fixed.
>
> >+ .fast_io = true,
>
> You don't need to set fast_io since this is already done at the
> regmap_bus level.
Okay, I removed it.
>
> >+ .max_register = 0x5c,
> >+ .reg_bits = 32,
> >+ .val_bits = 32,
> >+ .reg_stride = 4,
> >+};
> >+
> >+static void mx25_gcq_iio_dev_setup(struct platform_device *pdev,
> >+ struct iio_dev *idev)
> >+{
> >+ idev->dev.parent = &pdev->dev;
> >+ idev->channels = mx25_gcq_channels;
> >+ idev->num_channels = ARRAY_SIZE(mx25_gcq_channels);
> >+ idev->info = &mx25_gcq_iio_info;
>
> Any reason why this needs to be in a separate function and can't be
> inlined in probe()?
No reason for that. I moved it back to probe().
>
> >+}
> >+
> >+static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> >+ struct mx25_gcq_priv *priv)
> >+{
> >+ struct device_node *np = pdev->dev.of_node;
> >+ struct device_node *child;
> >+ struct device *dev = &pdev->dev;
> >+ int ret;
> >+ int i;
> >+
> >+ /* Setup all configurations registers with a default conversion
> >+ * configuration for each input */
> >+ for (i = 0; i != MX25_NUM_CFGS; ++i)
> >+ regmap_write(priv->regs, MX25_ADCQ_CFG(i),
> >+ MX25_ADCQ_CFG_YPLL_OFF |
> >+ MX25_ADCQ_CFG_XNUR_OFF |
> >+ MX25_ADCQ_CFG_XPUL_OFF |
> >+ MX25_ADCQ_CFG_REFP_INT |
> >+ (i << 4) |
> >+ MX25_ADCQ_CFG_REFN_NGND2);
> >+
> >+ for_each_child_of_node(np, child) {
> >+ u32 reg;
> >+ u32 refn;
> >+ u32 refp;
> >+
> >+ ret = of_property_read_u32(child, "reg", ®);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to get reg property\n");
> >+ return ret;
> >+ }
> >+ if (reg > MX25_NUM_CFGS) {
> >+ dev_err(dev, "reg value is greater than the number of available configuration registers\n");
> >+ return -EINVAL;
> >+ }
> >+
> >+ ret = of_property_read_u32(child, "fsl,adc-refn", &refn);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to get fsl,adc-refn property\n");
> >+ return ret;
> >+ }
> >+
> >+ ret = of_property_read_u32(child, "fsl,adc-refp", &refp);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to get fsl,adc-refp property\n");
> >+ return ret;
> >+ }
>
> Range check for refp and refn?
Range check added for both.
>
> >+
> >+ regmap_update_bits(priv->regs, MX25_ADCQ_CFG(reg),
> >+ MX25_ADCQ_CFG_REFP_MASK |
> >+ MX25_ADCQ_CFG_REFN_MASK,
> >+ (refp << 7) | (refn << 2));
> >+ }
> >+ regmap_update_bits(priv->regs, MX25_ADCQ_CR,
> >+ MX25_ADCQ_CR_FRST | MX25_ADCQ_CR_QRST,
> >+ MX25_ADCQ_CR_FRST | MX25_ADCQ_CR_QRST);
> >+
> >+ regmap_write(priv->regs, MX25_ADCQ_CR,
> >+ MX25_ADCQ_CR_PDMSK |
> >+ MX25_ADCQ_CR_QSM_FQS);
> >+
> >+ return 0;
> >+}
> >+
> >+static int mx25_gcq_probe(struct platform_device *pdev)
> >+{
> >+ struct iio_dev *idev;
> >+ struct mx25_gcq_priv *priv;
> >+ struct resource *res;
> >+ struct device *dev = &pdev->dev;
> >+ int ret;
> >+ int irq;
> >+ void __iomem *mem;
> >+
> >+ idev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> >+ if (!idev)
> >+ return -ENOMEM;
> >+
> >+ priv = iio_priv(idev);
> >+
> >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+ mem = devm_ioremap_resource(dev, res);
> >+ if (!mem) {
> >+ dev_err(dev, "Failed to get iomem");
>
> devm_ioremap_resource already prints a error message for you
Okay, I removed dev_err.
>
> >+ return -ENOMEM;
> >+ }
> >+
> >+ priv->regs = devm_regmap_init_mmio(dev, mem, &mx25_gcq_regconfig);
> >+ if (IS_ERR(priv->regs)) {
> >+ dev_err(dev, "Failed to initialize regmap\n");
> >+ return PTR_ERR(priv->regs);
> >+ }
> >+
> >+ irq = platform_get_irq(pdev, 0);
> >+ if (irq < 0) {
>
> 0 is not a valid IRQ number either
Fixed.
>
> >+ dev_err(dev, "Failed to get IRQ\n");
> >+ return irq;
> >+ }
> >+
> >+ ret = devm_request_irq(dev, irq, mx25_gcq_irq, IRQF_ONESHOT, pdev->name,
> >+ priv);
>
> IRQF_ONESHOT does not make much sense for non-threaded IRQs. Also it
> is probably safer to use the non managed variant of request_irq
> here.
Right, I removed the IRQF_ONESHOT flag. Why is it safer to use the non
managed request_irq?
>
> >+ if (ret) {
> >+ dev_err(dev, "Failed requesting IRQ\n");
>
> It usually makes sense to include the error number in the message.
> Same for the other error messages in probe.
The error values are returned from the probe function, so the error
number should be displayed by really_probe() in drivers/base/dd.c .
>
> >+ return ret;
> >+ }
> >+
> >+ ret = mx25_gcq_setup_cfgs(pdev, priv);
> >+ if (ret)
> >+ return ret;
> >+
> >+ mx25_gcq_iio_dev_setup(pdev, idev);
> >+
> >+ ret = iio_device_register(idev);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to register iio device\n");
> >+ return ret;
> >+ }
> >+
> >+ init_completion(&priv->completed);
>
> Should be done before the interrupt handler is registered. You
> reference it in there.
>
> >+
> >+ priv->clk = mx25_tsadc_get_ipg(pdev->dev.parent);
> >+ ret = clk_prepare_enable(priv->clk);
>
> The clock should probably also be enabled before the interrupt
> handler and the IIO device are registered.
Fixed. The last two items are request_irq() and iio_device_register().
>
> >+ if (ret) {
> >+ dev_err(dev, "Failed to enable clock\n");
> >+ return ret;
> >+ }
> >+
> >+ platform_set_drvdata(pdev, priv);
> >+
> >+ return 0;
> >+}
> >+
> >+static int mx25_gcq_remove(struct platform_device *pdev)
> >+{
> >+ struct mx25_gcq_priv *priv = platform_get_drvdata(pdev);
> >+
>
> iio_device_unregister().
Fixed.
>
> >+ clk_disable_unprepare(priv->clk);
> >+
> >+ return 0;
> >+}
>
>
Thanks,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v10 01/15] usb: doc: phy-mxs: Add more compatible strings
From: Marc Kleine-Budde @ 2014-02-21 9:46 UTC (permalink / raw)
To: Peter Chen, Mark Rutland
Cc: balbi@ti.com, shawn.guo@linaro.org, robh+dt@kernel.org,
grant.likely@linaro.org, Pawel Moll,
alexander.shishkin@linux.intel.com, linux-usb@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, festevam@gmail.com,
marex@denx.de, kernel@pengutronix.de, m.grzeschik@pengutronix.de,
Frank.Li@freescale.com, gregkh@linuxfoundation.org,
devicetree@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <5e4d795b507a4ea7b9faa69dd05148c4@BL2PR03MB226.namprd03.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]
On 02/21/2014 10:40 AM, Peter Chen wrote:
>
>>>
>>> Required properties:
>>> -- compatible: Should be "fsl,imx23-usbphy"
>>> +- compatible: "fsl,imx23-usbphy" for imx23 and imx28, "fsl,imx6q-
>> usbphy"
>>> + for imx6dq and imx6dl, "fsl,imx6sl-usbphy" for imx6sl
>>
>> Minor nit, but could we restructure this as something like the following,
>> with each string on a new line:
>>
>> - compatible: should contain:
>> * "fsl,imx23-usbphy" for imx23 and imx28
>> * "fsl,imx6q-usbphy" for imx6dq and imx6dl
>> * "fsl,imx6sl-usbphy" for imx6sl
>>
>> It makes it a bit easier to read.
>
> Thanks, will change like above.
>
>>
>> I see the existing "fsl,imx23-usbphy" is used as a fallback for
>> "fsl,imx28-usbphy", "fsl,imx6q-usbphy", and "fsl,imx6sl-usbphy" in
>> existing DTs.
>>
>> Is this expected going forward? It might be worth mentioning.
>>
>
> These SoCs used the same FSL imx PHY, but different versions.
> imx23/imx28 are the first version, more improvements are at
> later SoCs (like imx6x) version. Keep "fsl,imx23-usbphy" at
> imx6 dts will be user know it is from imx23's. If you think
> it does not need, I can delete "fsl,imx23-usbphy" from imx6 dts.
You should go after compatibility here. List (all) phys that are
comaptible, start with most specific end with most generic.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply
* RE: [PATCH v10 01/15] usb: doc: phy-mxs: Add more compatible strings
From: Peter Chen @ 2014-02-21 9:40 UTC (permalink / raw)
To: Mark Rutland
Cc: balbi@ti.com, shawn.guo@linaro.org, robh+dt@kernel.org,
grant.likely@linaro.org, Pawel Moll,
alexander.shishkin@linux.intel.com, linux-usb@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, festevam@gmail.com,
marex@denx.de, kernel@pengutronix.de, m.grzeschik@pengutronix.de,
Frank.Li@freescale.com, gregkh@linuxfoundation.org,
devicetree@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <20140221091345.GA7541@e106331-lin.cambridge.arm.com>
> >
> > Required properties:
> > -- compatible: Should be "fsl,imx23-usbphy"
> > +- compatible: "fsl,imx23-usbphy" for imx23 and imx28, "fsl,imx6q-
> usbphy"
> > + for imx6dq and imx6dl, "fsl,imx6sl-usbphy" for imx6sl
>
> Minor nit, but could we restructure this as something like the following,
> with each string on a new line:
>
> - compatible: should contain:
> * "fsl,imx23-usbphy" for imx23 and imx28
> * "fsl,imx6q-usbphy" for imx6dq and imx6dl
> * "fsl,imx6sl-usbphy" for imx6sl
>
> It makes it a bit easier to read.
Thanks, will change like above.
>
> I see the existing "fsl,imx23-usbphy" is used as a fallback for
> "fsl,imx28-usbphy", "fsl,imx6q-usbphy", and "fsl,imx6sl-usbphy" in
> existing DTs.
>
> Is this expected going forward? It might be worth mentioning.
>
These SoCs used the same FSL imx PHY, but different versions.
imx23/imx28 are the first version, more improvements are at
later SoCs (like imx6x) version. Keep "fsl,imx23-usbphy" at
imx6 dts will be user know it is from imx23's. If you think
it does not need, I can delete "fsl,imx23-usbphy" from imx6 dts.
Peter
^ permalink raw reply
* Re: [Patch v6 2/2] dmaengine: add Qualcomm BAM dma driver
From: Mark Rutland @ 2014-02-21 9:33 UTC (permalink / raw)
To: Andy Gross
Cc: devicetree@vger.kernel.org, Vinod Koul,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
dmaengine@vger.kernel.org, Dan Williams,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1392964986-5207-3-git-send-email-agross@codeaurora.org>
On Fri, Feb 21, 2014 at 06:43:05AM +0000, Andy Gross wrote:
> Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller
> found in the MSM 8x74 platforms.
>
> Each BAM DMA device is associated with a specific on-chip peripheral. Each
> channel provides a uni-directional data transfer engine that is capable of
> transferring data between the peripheral and system memory (System mode), or
> between two peripherals (BAM2BAM).
>
> The initial release of this driver only supports slave transfers between
> peripherals and system memory.
>
> Signed-off-by: Andy Gross <agross@codeaurora.org>
> ---
> drivers/dma/Kconfig | 9 +
> drivers/dma/Makefile | 1 +
> drivers/dma/qcom_bam_dma.c | 1107 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1117 insertions(+)
> create mode 100644 drivers/dma/qcom_bam_dma.c
[...]
> + bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
The binding document should describe the "bam_clk" string in the
clock-names description.
[...]
> + ret = of_property_read_u32(pdev->dev.of_node, "qcom,ee", &bdev->ee);
> + if (ret) {
> + dev_err(bdev->dev, "EE unspecified\n");
It would be nice to expand EE to what it stands for, at least in the
error but possibly the binding too.
> +static const struct of_device_id bam_of_match[] = {
> + { .compatible = "qcom,bam-v1.4.0", },
> + { .compatible = "qcom,bam-v1.4.1", },
What are the differences between the two?
Is v1.4.1 an extension of v1.4.0?
Could you have the following in DTs:
compatible = "qcom,bam-v1.4.1", "qcom,bam-v1.4.0";
Then you'd only need the v1.4.0 string in the driver for now.
Thanks,
Mark.
^ permalink raw reply
* Re: [Patch v6 1/2] dmaengine: qcom_bam_dma: Add device tree binding
From: Mark Rutland @ 2014-02-21 9:26 UTC (permalink / raw)
To: Andy Gross
Cc: Vinod Koul, Dan Williams, dmaengine@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1392964986-5207-2-git-send-email-agross@codeaurora.org>
On Fri, Feb 21, 2014 at 06:43:04AM +0000, Andy Gross wrote:
> Add device tree binding support for the QCOM BAM DMA driver.
>
> Acked-by: Kumar Gala <galak@codeaurora.org>
> Signed-off-by: Andy Gross <agross@codeaurora.org>
> ---
> .../devicetree/bindings/dma/qcom_bam_dma.txt | 48 ++++++++++++++++++++
> 1 file changed, 48 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
>
> diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> new file mode 100644
> index 0000000..86344f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> @@ -0,0 +1,48 @@
> +QCOM BAM DMA controller
> +
> +Required properties:
> +- compatible: Must be "qcom,bam-v1.4.0" for MSM8974 V1
> + Must be "qcom,bam-v1.4.1" for MSM8974 V2
This looks a bit odd. How about:
- compatible: must contain:
* "qcom,bam-v1.4.0" for MSM8974 V1
* "qcom,bam-v1.4.1" for MSM8974 V2
> +- reg: Address range for DMA registers
> +- interrupts: single interrupt for this controller
This device only has a single interrupt? Or there's only one we care
about at the moment?
> +- #dma-cells: must be <1>
> +- clocks: required clock
> +- clock-names: name of clock
Either describe the _exact_ name this binding expects for any clocks
input, or get rid of clock-names. I would prefer the former.
> +- qcom,ee : indicates the active Execution Environment identifier (0-7)
> +
> +Example:
> +
> + uart-bam: dma@f9984000 = {
> + compatible = "qcom,bam-v1.4.1";
> + reg = <0xf9984000 0x15000>;
> + interrupts = <0 94 0>;
> + clocks = <&gcc GCC_BAM_DMA_AHB_CLK>;
> + clock-names = "bam_clk";
> + #dma-cells = <1>;
> + qcom,ee = <0>;
> + };
> +
> +Client:
> +Required properties:
> +- dmas: List of dma channel requests
> +- dma-names: Names of aforementioned requested channels
Do we really need to describe the client binding? Do we not have a
generic DMA binding doc we can refer to?
> +
> +Clients must use the format described in the dma.txt file, using a two cell
> +specifier for each channel.
> +
> +The three cells in order are:
> + 1. A phandle pointing to the DMA controller
> + 2. The channel number
s/three/two/
I think this can go if we refer to a generic document for the client
binding and state in the #dma-cells description that the single dma cell
is the channel number.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v10 10/15] usb: phy-mxs: Add implementation of set_wakeup
From: Mark Rutland @ 2014-02-21 9:21 UTC (permalink / raw)
To: Peter Chen
Cc: balbi@ti.com, shawn.guo@linaro.org, robh+dt@kernel.org,
grant.likely@linaro.org, Pawel Moll,
alexander.shishkin@linux.intel.com, linux-usb@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, festevam@gmail.com,
marex@denx.de, kernel@pengutronix.de, m.grzeschik@pengutronix.de,
frank.li@freescale.com, gregkh@linuxfoundation.org,
devicetree@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <1392873284-9386-11-git-send-email-peter.chen@freescale.com>
On Thu, Feb 20, 2014 at 05:14:39AM +0000, Peter Chen wrote:
> When we need the PHY can be waken up by external signals,
> we can call this API. Besides, we call mxs_phy_disconnect_line
> at this API to close the connection between USB PHY and
> controller, after that, the line state from controller is SE0.
> Once the PHY is out of power, without calling mxs_phy_disconnect_line,
> there are unknown wakeups due to dp/dm floating at device mode.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
> drivers/usb/phy/phy-mxs-usb.c | 116 +++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 116 insertions(+), 0 deletions(-)
[...]
> +static void mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool on)
> +{
> + bool vbus_is_on = false;
> +
> + /* If the SoCs don't need to disconnect line without vbus, quit */
> + if (!(mxs_phy->data->flags & MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS))
> + return;
> +
> + /* If the SoCs don't have anatop, quit */
> + if (!mxs_phy->regmap_anatop)
> + return;
So it looks like fsl,anatop is truly optional.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH 1/3] mfd: fsl imx25 Touchscreen ADC driver
From: Markus Pargmann @ 2014-02-21 9:18 UTC (permalink / raw)
To: Fabio Estevam
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Dmitry Torokhov, Samuel Ortiz,
Lee Jones, Jonathan Cameron,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Sascha Hauer
In-Reply-To: <CAOMZO5BDyyvXs9CxJQOkuEpxZ8fqamCPB67sNhJxES+XU2XkXQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 8010 bytes --]
Hi Fabio,
On Thu, Feb 20, 2014 at 02:17:33PM -0300, Fabio Estevam wrote:
> Hi Markus,
>
> On Thu, Feb 20, 2014 at 1:21 PM, Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > This is the core driver for imx25 touchscreen/adc driver. The module
> > has one shared ADC and two different conversion queues which use the
> > ADC. The two queues are identical. Both can be used for general purpose
> > ADC but one is meant to be used for touchscreens.
> >
> > This driver is the core which manages the central components and
> > registers of the TSC/ADC unit. It manages the IRQs and forwards them to
> > the correct components.
> >
> > Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> That's great :-) Nice work!
Thanks :)
> > ---
> > .../devicetree/bindings/mfd/fsl-imx25-tsadc.txt | 46 ++++
> > drivers/mfd/Kconfig | 9 +
> > drivers/mfd/Makefile | 2 +
> > drivers/mfd/fsl-imx25-tsadc.c | 234 +++++++++++++++++++++
> > include/linux/mfd/imx25-tsadc.h | 138 ++++++++++++
> > 5 files changed, 429 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> > create mode 100644 drivers/mfd/fsl-imx25-tsadc.c
> > create mode 100644 include/linux/mfd/imx25-tsadc.h
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt b/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> > new file mode 100644
> > index 0000000..a857af0e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> > @@ -0,0 +1,46 @@
> > +Freescale mx25 ADC/TSC multifunction device
> > +
> > +This device combines two general purpose conversion queues one used for general
> > +ADC and the other used for touchscreens.
> > +
> > +Required properties:
> > + - compatible: Should be "fsl,imx25-tsadc".
> > + - reg: Memory range of the device.
> > + - interrupts: Interrupt for this device as described in
> > + interrupts/interrupts.txt
> > + - clocks: An 'ipg' clock defined as described in clocks/clock.txt
> > + - interrupt-controller: This device is an interrupt controller. It controls
> > + the interrupts of both conversion queues.
> > + - #interrupt-cells: Should be '<1>'.
> > + - #address-cells: Should be '<1>'.
> > + - #size-cells: Should be '<1>'.
> > + - ranges
> > +
> > +This device includes two conversion queues which can be added as subnodes.
> > +The first queue is for the touchscreen, the second for general purpose ADC.
> > +
> > +Example:
> > + tscadc: tscadc@50030000 {
> > + compatible = "fsl,imx25-tsadc";
> > + reg = <0x50030000 0xc>;
> > + interrupts = <46>;
> > + clocks = <&clks 119>;
> > + clock-names = "ipg";
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > +
> > + tsc: tcq@50030400 {
> > + compatible = "fsl,imx25-tcq";
> > + reg = <0x50030400 0x60>;
> > + ...
> > + };
> > +
> > + adc: gcq@50030800 {
> > + compatible = "fsl,imx25-gcq";
> > + reg = <0x50030800 0x60>;
> > + ...
> > + };
> > + };
>
> The meaning of 'tcq' and 'gcq' acronyms are not obvious. Could they be
> written more explicitily?
I assume you mean the node names? Perhaps something like 'adcqueue'?
>
> Also, what does the '...' mean?
>
> > +static int mx25_tsadc_domain_map(struct irq_domain *d, unsigned int irq,
> > + irq_hw_number_t hwirq)
> > +{
> > + struct mx25_tsadc_priv *priv = d->host_data;
> > +
> > + irq_set_chip_data(irq, priv);
> > + irq_set_chip_and_handler(irq, &mx25_tsadc_irq_chip,
> > + handle_level_irq);
> > +
> > +#ifdef CONFIG_ARM
> > + set_irq_flags(irq, IRQF_VALID);
> > +#else
> > + irq_set_noprobe(irq);
> > +#endif
>
> Do we really need these ifdef's? Can't we just assume that CONFIG_ARM
> is always selected for this driver?
Yes right, this is not necessary here.
>
> > +static int mx25_tsadc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node;
> > + struct mx25_tsadc_priv *priv;
> > + struct resource *iores;
> > + int ret;
> > + void __iomem *iomem;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + iomem = devm_ioremap_resource(dev, iores);
> > + if (IS_ERR(iomem)) {
> > + dev_err(dev, "Failed to remap iomem\n");
> > + return PTR_ERR(iomem);
> > + }
> > +
> > + priv->regs = regmap_init_mmio(dev, iomem, &mx25_tsadc);
> > + if (IS_ERR(priv->regs)) {
> > + dev_err(dev, "Failed to initialize regmap\n");
> > + return PTR_ERR(priv->regs);
> > + }
> > +
> > + priv->clk = devm_clk_get(dev, "ipg");
> > + if (IS_ERR(priv->clk)) {
> > + dev_err(dev, "Failed to get ipg clock\n");
> > + return PTR_ERR(priv->clk);
> > + }
> > +
> > + /* Enable clock and reset the component */
> > + regmap_update_bits(priv->regs, MX25_TSC_TGCR, MX25_TGCR_CLK_EN,
> > + MX25_TGCR_CLK_EN);
> > + regmap_update_bits(priv->regs, MX25_TSC_TGCR, MX25_TGCR_TSC_RST,
> > + MX25_TGCR_TSC_RST);
> > +
> > + /* Setup powersaving mode, but enable internal reference voltage */
> > + regmap_update_bits(priv->regs, MX25_TSC_TGCR, MX25_TGCR_POWERMODE_MASK,
> > + MX25_TGCR_POWERMODE_SAVE);
> > + regmap_update_bits(priv->regs, MX25_TSC_TGCR, MX25_TGCR_INTREFEN,
> > + MX25_TGCR_INTREFEN);
> > +
> > + ret = mx25_tsadc_setup_irq(pdev, priv);
> > + if (ret) {
> > + dev_err(dev, "Failed to setup irqs\n");
> > + return ret;
> > + }
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + of_platform_populate(np, NULL, NULL, dev);
> > +
> > + dev_info(dev, "i.MX25/25 Touchscreen and ADC core driver loaded\n");
>
> You could remove the double '25/25'.
Yes, fixed.
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id mx25_tsadc_ids[] = {
> > + { .compatible = "fsl,imx25-tsadc", },
> > + { /* Sentinel */ }
> > +};
> > +
> > +static struct platform_driver mx25_tsadc_driver = {
> > + .driver = {
> > + .name = "mx25-tsadc",
> > + .owner = THIS_MODULE,
> > + .of_match_table = mx25_tsadc_ids,
> > + },
> > + .probe = mx25_tsadc_probe,
> > +};
> > +module_platform_driver(mx25_tsadc_driver);
> > +
> > +MODULE_DESCRIPTION("MFD for ADC/TSC for Freescale mx25");
> > +MODULE_AUTHOR("Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
> > +MODULE_LICENSE("GPL v2");
>
> MODULE_ALIAS() as well?
I am not sure if this is necessary, but I added
MODULE_ALIAS("platform:mx25-tsadc")
Thanks,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v10 04/15] usb: doc: phy-mxs: update binding for adding anatop phandle
From: Mark Rutland @ 2014-02-21 9:14 UTC (permalink / raw)
To: Peter Chen
Cc: balbi-l0cyMroinI0@public.gmane.org,
shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, Pawel Moll,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
marex-ynQEQJNshbs@public.gmane.org,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org,
m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org,
frank.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1392873284-9386-5-git-send-email-peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
On Thu, Feb 20, 2014 at 05:14:33AM +0000, Peter Chen wrote:
> Add anatop phandle which is used to access anatop registers to
> control PHY's power and other USB operations.
>
> Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
> Documentation/devicetree/bindings/usb/mxs-phy.txt | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/mxs-phy.txt b/Documentation/devicetree/bindings/usb/mxs-phy.txt
> index b43d4c9e..fc6659d 100644
> --- a/Documentation/devicetree/bindings/usb/mxs-phy.txt
> +++ b/Documentation/devicetree/bindings/usb/mxs-phy.txt
> @@ -5,10 +5,12 @@ Required properties:
> for imx6dq and imx6dl, "fsl,imx6sl-usbphy" for imx6sl
> - reg: Should contain registers location and length
> - interrupts: Should contain phy interrupt
> +- fsl,anatop: phandle for anatop register, it is only for imx6 SoC series
This is now required?
What happens to those existing DTBs that claim compatibility with
"fsl,imx6q-usbphy" but don't have an fsl,anatop property?
Thanks,
Mark.
>
> Example:
> usbphy1: usbphy@020c9000 {
> compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
> reg = <0x020c9000 0x1000>;
> interrupts = <0 44 0x04>;
> + fsl,anatop = <&anatop>;
> };
> --
> 1.7.8
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox