* Re: [PATCH v9 3/4] dtc: Plugin and fixup support
From: Pantelis Antoniou @ 2016-11-25 10:55 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161125041124.GB12287-K0bRW+63XPQe6aEkudXLsA@public.gmane.org>
Hi David,
> On Nov 25, 2016, at 06:11 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Thu, Nov 24, 2016 at 02:31:32PM +0200, Pantelis Antoniou wrote:
>> This patch enable the generation of symbols & local fixup information
>> for trees compiled with the -@ (--symbols) option.
>>
>> Using this patch labels in the tree and their users emit information
>> in __symbols__ and __local_fixups__ nodes.
>>
>> The __fixups__ node make possible the dynamic resolution of phandle
>> references which are present in the plugin tree but lie in the
>> tree that are applying the overlay against.
>>
>> While there is a new magic number for dynamic device tree/overlays blobs
>> it is by default disabled. This is in order to give time for DT blob
>> methods to be updated.
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> ---
>> Documentation/manual.txt | 25 +++++-
>> checks.c | 8 +-
>> dtc-lexer.l | 5 ++
>> dtc-parser.y | 49 +++++++++--
>> dtc.c | 39 +++++++-
>> dtc.h | 20 ++++-
>> fdtdump.c | 2 +-
>> flattree.c | 17 ++--
>> fstree.c | 2 +-
>> libfdt/fdt.c | 2 +-
>> libfdt/fdt.h | 3 +-
>> livetree.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++-
>> tests/mangle-layout.c | 7 +-
>> treesource.c | 7 +-
>> 14 files changed, 380 insertions(+), 31 deletions(-)
>>
>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>> index 398de32..65fbf09 100644
>> --- a/Documentation/manual.txt
>> +++ b/Documentation/manual.txt
>> @@ -119,6 +119,24 @@ Options:
>> Make space for <number> reserve map entries
>> Relevant for dtb and asm output only.
>>
>> + -@
>> + Generates a __symbols__ node at the root node of the resulting blob
>> + for any node labels used, and for any local references using phandles
>> + it also generates a __local_fixups__ node that tracks them.
>> +
>> + When using the /plugin/ tag all unresolved label references to
>> + be tracked in the __fixups__ node, making dynamic resolution possible.
>> +
>> + -A
>> + Generate automatically aliases for all node labels. This is similar to
>> + the -@ option (the __symbols__ node contain identical information) but
>> + the semantics are slightly different since no phandles are automatically
>> + generated for labeled nodes.
>> +
>> + -M
>> + Generate blobs with the new FDT magic number. By default blobs with the
>> + standard FDT magic number are generated.
>
> First, this description is incomplete since -M *only* affects the
> magic number for /plugin/ input, not in other cases. Second, the
> default is the wrong way around. If we make old-style the default,
> then new-style will never be used, which defeats the purpose.
>
Then we’ll break user-space that has this assumption (i.e. that the magic is the same).
I can certainly do it the other way around.
>> +
>> -S <bytes>
>> Ensure the blob at least <bytes> long, adding additional
>> space if needed.
>> @@ -146,13 +164,18 @@ Additionally, dtc performs various sanity checks on the tree.
>> Here is a very rough overview of the layout of a DTS source file:
>>
>>
>> - sourcefile: list_of_memreserve devicetree
>> + sourcefile: versioninfo plugindecl list_of_memreserve devicetree
>>
>> memreserve: label 'memreserve' ADDR ADDR ';'
>> | label 'memreserve' ADDR '-' ADDR ';'
>>
>> devicetree: '/' nodedef
>>
>> + versioninfo: '/' 'dts-v1' '/' ';'
>> +
>> + plugindecl: '/' 'plugin' '/' ';'
>> + | /* empty */
>> +
>> nodedef: '{' list_of_property list_of_subnode '}' ';'
>>
>> property: label PROPNAME '=' propdata ';'
>> diff --git a/checks.c b/checks.c
>> index 609975a..bc03d42 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -486,8 +486,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
>>
>> refnode = get_node_by_ref(dt, m->ref);
>> if (! refnode) {
>> - FAIL(c, "Reference to non-existent node or label \"%s\"\n",
>> - m->ref);
>> + if (!(bi->versionflags & VF_PLUGIN))
>> + FAIL(c, "Reference to non-existent node or "
>> + "label \"%s\"\n", m->ref);
>> + else /* mark the entry as unresolved */
>> + *((cell_t *)(prop->val.val + m->offset)) =
>> + cpu_to_fdt32(0xffffffff);
>> continue;
>> }
>>
>> diff --git a/dtc-lexer.l b/dtc-lexer.l
>> index 790fbf6..40bbc87 100644
>> --- a/dtc-lexer.l
>> +++ b/dtc-lexer.l
>> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
>> return DT_V1;
>> }
>>
>> +<*>"/plugin/" {
>> + DPRINT("Keyword: /plugin/\n");
>> + return DT_PLUGIN;
>> + }
>> +
>> <*>"/memreserve/" {
>> DPRINT("Keyword: /memreserve/\n");
>> BEGIN_DEFAULT();
>> diff --git a/dtc-parser.y b/dtc-parser.y
>> index 14aaf2e..4afc592 100644
>> --- a/dtc-parser.y
>> +++ b/dtc-parser.y
>> @@ -19,6 +19,7 @@
>> */
>> %{
>> #include <stdio.h>
>> +#include <inttypes.h>
>>
>> #include "dtc.h"
>> #include "srcpos.h"
>> @@ -33,6 +34,9 @@ extern void yyerror(char const *s);
>>
>> extern struct boot_info *the_boot_info;
>> extern bool treesource_error;
>> +
>> +/* temporary while the tree is not built */
>> +static unsigned int the_versionflags;
>
> Hrm. Using a global during parsing is pretty dangerous - it makes
> assumptions about the order in which bison will execute semantic
> actions that may not always be correct.
>
> It'll probably work in practice, so I *might* be convinced it's
> adequate for a first cut. I'm a bit reluctant though, since I suspect
> once merged it will become entrenched.
>
We use bison, globals are the way of life. It’s not going to be used
anywhere else, it’s static in the parser file.
We could allocate the boot_info earlier (when the v1tag is detected) but
that would be quite a big change for something as trivial.
> The correct way to handle this, IMO, is not to ever attempt to apply
> overlays during the parse. Basically, we'd change the overall
> structure so that the output from the parser is not a single tree, but
> a list of overlays / fragments. Then, once the parse is complete, so
> versioninfo (which could now become a member of bootinfo) is well
> defined, we either apply the fragments in place (base tree) or encode
> them into the overlay structure (plugin mode).
>
> See https://github.com/dgibson/dtc/tree/overlay for some incomplete
> work I did in this direction.
>
This tree is pretty stale; last commit was back in march.
I thing there’s a wrong assumption here. The purpose of this patch is not
to apply overlays during compile time, is to generate a blob that can be
applied at runtime by another piece of software.
> In addition to not making unsafe assumptions about parser operation, I
> think this will also allow for better error reporting. It also moves
> more code into plain-old-C instead of potentially hard to follow bison
> code fragments.
>
We don’t try to apply overlays during parse, and especially in the parse code.
The global is used only for the syntactic sugar method of turning a &ref { };
node into an overlay.
>> %}
>>
>> %union {
>> @@ -52,9 +56,11 @@ extern bool treesource_error;
>> struct node *nodelist;
>> struct reserve_info *re;
>> uint64_t integer;
>> + unsigned int flags;
>> }
>>
>> %token DT_V1
>> +%token DT_PLUGIN
>> %token DT_MEMRESERVE
>> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
>> %token DT_BITS
>> @@ -71,6 +77,8 @@ extern bool treesource_error;
>>
>> %type <data> propdata
>> %type <data> propdataprefix
>> +%type <flags> versioninfo
>> +%type <flags> plugindecl
>> %type <re> memreserve
>> %type <re> memreserves
>> %type <array> arrayprefix
>> @@ -101,16 +109,36 @@ extern bool treesource_error;
>> %%
>>
>> sourcefile:
>> - v1tag memreserves devicetree
>> + versioninfo plugindecl memreserves devicetree
>> + {
>> + the_boot_info = build_boot_info($1 | $2, $3, $4,
>> + guess_boot_cpuid($4));
>> + }
>> + ;
>> +
>> +versioninfo:
>> + v1tag
>> {
>> - the_boot_info = build_boot_info($2, $3,
>> - guess_boot_cpuid($3));
>> + the_versionflags |= VF_DT_V1;
>> + $$ = the_versionflags;
>> }
>> ;
>>
>> v1tag:
>> DT_V1 ';'
>> + | DT_V1
>> | DT_V1 ';' v1tag
>> +
>> +plugindecl:
>> + DT_PLUGIN ';'
>> + {
>> + the_versionflags |= VF_PLUGIN;
>> + $$ = VF_PLUGIN;
>> + }
>> + | /* empty */
>> + {
>> + $$ = 0;
>> + }
>> ;
>>
>> memreserves:
>> @@ -161,10 +189,14 @@ devicetree:
>> {
>> struct node *target = get_node_by_ref($1, $2);
>>
>> - if (target)
>> + if (target) {
>> merge_nodes(target, $3);
>> - else
>> - ERROR(&@2, "Label or path %s not found", $2);
>> + } else {
>> + if (the_versionflags & VF_PLUGIN)
>> + add_orphan_node($1, $3, $2);
>> + else
>> + ERROR(&@2, "Label or path %s not found", $2);
>> + }
>> $$ = $1;
>> }
>> | devicetree DT_DEL_NODE DT_REF ';'
>> @@ -179,6 +211,11 @@ devicetree:
>>
>> $$ = $1;
>> }
>> + | /* empty */
>> + {
>> + /* build empty node */
>> + $$ = name_node(build_node(NULL, NULL), "");
>> + }
>> ;
>>
>> nodedef:
>> diff --git a/dtc.c b/dtc.c
>> index 9dcf640..a654267 100644
>> --- a/dtc.c
>> +++ b/dtc.c
>> @@ -32,6 +32,9 @@ int minsize; /* Minimum blob size */
>> int padsize; /* Additional padding to blob */
>> int alignsize; /* Additional padding to blob accroding to the alignsize */
>> int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
>> +int symbol_fixup_support; /* enable symbols & fixup support */
>> +int auto_label_aliases; /* auto generate labels -> aliases */
>> +int new_magic; /* use new FDT magic values for objects */
>>
>> static int is_power_of_2(int x)
>> {
>> @@ -59,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>> #define FDT_VERSION(version) _FDT_VERSION(version)
>> #define _FDT_VERSION(version) #version
>> static const char usage_synopsis[] = "dtc [options] <input file>";
>> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
>> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMhv";
>> static struct option const usage_long_opts[] = {
>> {"quiet", no_argument, NULL, 'q'},
>> {"in-format", a_argument, NULL, 'I'},
>> @@ -78,6 +81,9 @@ static struct option const usage_long_opts[] = {
>> {"phandle", a_argument, NULL, 'H'},
>> {"warning", a_argument, NULL, 'W'},
>> {"error", a_argument, NULL, 'E'},
>> + {"symbols", no_argument, NULL, '@'},
>> + {"auto-alias", no_argument, NULL, 'A'},
>> + {"new-magic", no_argument, NULL, 'M'},
>> {"help", no_argument, NULL, 'h'},
>> {"version", no_argument, NULL, 'v'},
>> {NULL, no_argument, NULL, 0x0},
>> @@ -109,6 +115,9 @@ static const char * const usage_opts_help[] = {
>> "\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
>> "\n\tEnable/disable warnings (prefix with \"no-\")",
>> "\n\tEnable/disable errors (prefix with \"no-\")",
>> + "\n\tEnable symbols/fixup support",
>> + "\n\tEnable auto-alias of labels",
>> + "\n\tUse new blog magic value",
>> "\n\tPrint this help and exit",
>> "\n\tPrint version and exit",
>> NULL,
>> @@ -153,7 +162,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
>> fclose(f);
>>
>> magic = fdt32_to_cpu(magic);
>> - if (magic == FDT_MAGIC)
>> + if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
>> return "dtb";
>>
>> return guess_type_by_name(fname, fallback);
>> @@ -172,6 +181,7 @@ int main(int argc, char *argv[])
>> FILE *outf = NULL;
>> int outversion = DEFAULT_FDT_VERSION;
>> long long cmdline_boot_cpuid = -1;
>> + fdt32_t out_magic = FDT_MAGIC;
>>
>> quiet = 0;
>> reservenum = 0;
>> @@ -249,6 +259,16 @@ int main(int argc, char *argv[])
>> parse_checks_option(false, true, optarg);
>> break;
>>
>> + case '@':
>> + symbol_fixup_support = 1;
>> + break;
>> + case 'A':
>> + auto_label_aliases = 1;
>> + break;
>> + case 'M':
>> + new_magic = 1;
>> + break;
>> +
>> case 'h':
>> usage(NULL);
>> default:
>> @@ -306,6 +326,14 @@ int main(int argc, char *argv[])
>> fill_fullpaths(bi->dt, "");
>> process_checks(force, bi);
>>
>> + if (auto_label_aliases)
>> + generate_label_tree(bi->dt, "aliases", false);
>> +
>> + if (symbol_fixup_support) {
>> + generate_label_tree(bi->dt, "__symbols__", true);
>> + generate_fixups_tree(bi->dt);
>> + }
>> +
>> if (sort)
>> sort_tree(bi);
>>
>> @@ -318,12 +346,15 @@ int main(int argc, char *argv[])
>> outname, strerror(errno));
>> }
>>
>> + if (new_magic && (bi->versionflags & VF_PLUGIN))
>> + out_magic = FDT_MAGIC_DTBO;
>> +
>> if (streq(outform, "dts")) {
>> dt_to_source(outf, bi);
>> } else if (streq(outform, "dtb")) {
>> - dt_to_blob(outf, bi, outversion);
>> + dt_to_blob(outf, bi, out_magic, outversion);
>> } else if (streq(outform, "asm")) {
>> - dt_to_asm(outf, bi, outversion);
>> + dt_to_asm(outf, bi, out_magic, outversion);
>> } else if (streq(outform, "null")) {
>> /* do nothing */
>> } else {
>> diff --git a/dtc.h b/dtc.h
>> index 32009bc..889b8f8 100644
>> --- a/dtc.h
>> +++ b/dtc.h
>> @@ -55,6 +55,9 @@ extern int minsize; /* Minimum blob size */
>> extern int padsize; /* Additional padding to blob */
>> extern int alignsize; /* Additional padding to blob accroding to the alignsize */
>> extern int phandle_format; /* Use linux,phandle or phandle properties */
>> +extern int symbol_fixup_support;/* enable symbols & fixup support */
>> +extern int auto_label_aliases; /* auto generate labels -> aliases */
>> +extern int new_magic; /* use new FDT magic values for objects */
>>
>> #define PHANDLE_LEGACY 0x1
>> #define PHANDLE_EPAPR 0x2
>> @@ -195,6 +198,7 @@ struct node *build_node_delete(void);
>> struct node *name_node(struct node *node, char *name);
>> struct node *chain_node(struct node *first, struct node *list);
>> struct node *merge_nodes(struct node *old_node, struct node *new_node);
>> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
>>
>> void add_property(struct node *node, struct property *prop);
>> void delete_property_by_name(struct node *node, char *name);
>> @@ -202,6 +206,8 @@ void delete_property(struct property *prop);
>> void add_child(struct node *parent, struct node *child);
>> void delete_node_by_name(struct node *parent, char *name);
>> void delete_node(struct node *node);
>> +void append_to_property(struct node *node,
>> + char *name, const void *data, int len);
>>
>> const char *get_unitname(struct node *node);
>> struct property *get_property(struct node *node, const char *propname);
>> @@ -237,14 +243,22 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>>
>>
>> struct boot_info {
>> + unsigned int versionflags;
>> struct reserve_info *reservelist;
>> struct node *dt; /* the device tree */
>> uint32_t boot_cpuid_phys;
>> };
>>
>> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
>> +/* version flags definitions */
>> +#define VF_DT_V1 0x0001 /* /dts-v1/ */
>> +#define VF_PLUGIN 0x0002 /* /plugin/ */
>> +
>> +struct boot_info *build_boot_info(unsigned int versionflags,
>> + struct reserve_info *reservelist,
>> struct node *tree, uint32_t boot_cpuid_phys);
>> void sort_tree(struct boot_info *bi);
>> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
>> +void generate_fixups_tree(struct node *dt);
>>
>> /* Checks */
>>
>> @@ -253,8 +267,8 @@ void process_checks(bool force, struct boot_info *bi);
>>
>> /* Flattened trees */
>>
>> -void dt_to_blob(FILE *f, struct boot_info *bi, int version);
>> -void dt_to_asm(FILE *f, struct boot_info *bi, int version);
>> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>>
>> struct boot_info *dt_from_blob(const char *fname);
>>
>> diff --git a/fdtdump.c b/fdtdump.c
>> index a9a2484..dd63ac2 100644
>> --- a/fdtdump.c
>> +++ b/fdtdump.c
>> @@ -201,7 +201,7 @@ int main(int argc, char *argv[])
>> p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
>> if (!p)
>> break;
>> - if (fdt_magic(p) == FDT_MAGIC) {
>> + if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
>> /* try and validate the main struct */
>> off_t this_len = endp - p;
>> fdt32_t max_version = 17;
>> diff --git a/flattree.c b/flattree.c
>> index a9d9520..57d76cf 100644
>> --- a/flattree.c
>> +++ b/flattree.c
>> @@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
>> }
>>
>> static void make_fdt_header(struct fdt_header *fdt,
>> + fdt32_t magic,
>> struct version_info *vi,
>> int reservesize, int dtsize, int strsize,
>> int boot_cpuid_phys)
>> @@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>>
>> memset(fdt, 0xff, sizeof(*fdt));
>>
>> - fdt->magic = cpu_to_fdt32(FDT_MAGIC);
>> + fdt->magic = cpu_to_fdt32(magic);
>> fdt->version = cpu_to_fdt32(vi->version);
>> fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
>>
>> @@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>> fdt->size_dt_struct = cpu_to_fdt32(dtsize);
>> }
>>
>> -void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>> {
>> struct version_info *vi = NULL;
>> int i;
>> @@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>> reservebuf = flatten_reserve_list(bi->reservelist, vi);
>>
>> /* Make header */
>> - make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
>> + make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
>> bi->boot_cpuid_phys);
>>
>> /*
>> @@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
>> }
>> }
>>
>> -void dt_to_asm(FILE *f, struct boot_info *bi, int version)
>> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>> {
>> struct version_info *vi = NULL;
>> int i;
>> @@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
>> struct node *tree;
>> uint32_t val;
>> int flags = 0;
>> + unsigned int versionflags = VF_DT_V1;
>>
>> f = srcfile_relative_open(fname, NULL);
>>
>> @@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
>> }
>>
>> magic = fdt32_to_cpu(magic);
>> - if (magic != FDT_MAGIC)
>> + if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
>> die("Blob has incorrect magic number\n");
>>
>> + if (magic == FDT_MAGIC_DTBO)
>> + versionflags |= VF_PLUGIN;
>> +
>> rc = fread(&totalsize, sizeof(totalsize), 1, f);
>> if (ferror(f))
>> die("Error reading DT blob size: %s\n", strerror(errno));
>> @@ -942,5 +947,5 @@ struct boot_info *dt_from_blob(const char *fname)
>>
>> fclose(f);
>>
>> - return build_boot_info(reservelist, tree, boot_cpuid_phys);
>> + return build_boot_info(versionflags, reservelist, tree, boot_cpuid_phys);
>> }
>> diff --git a/fstree.c b/fstree.c
>> index 6d1beec..54f520b 100644
>> --- a/fstree.c
>> +++ b/fstree.c
>> @@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
>> tree = read_fstree(dirname);
>> tree = name_node(tree, "");
>>
>> - return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
>> + return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
>> }
>>
>> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
>> index 22286a1..28d422c 100644
>> --- a/libfdt/fdt.c
>> +++ b/libfdt/fdt.c
>> @@ -57,7 +57,7 @@
>>
>> int fdt_check_header(const void *fdt)
>> {
>> - if (fdt_magic(fdt) == FDT_MAGIC) {
>> + if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
>> /* Complete tree */
>> if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>> return -FDT_ERR_BADVERSION;
>> diff --git a/libfdt/fdt.h b/libfdt/fdt.h
>> index 526aedb..493cd55 100644
>> --- a/libfdt/fdt.h
>> +++ b/libfdt/fdt.h
>> @@ -55,7 +55,7 @@
>> #ifndef __ASSEMBLY__
>>
>> struct fdt_header {
>> - fdt32_t magic; /* magic word FDT_MAGIC */
>> + fdt32_t magic; /* magic word FDT_MAGIC[|_DTBO] */
>> fdt32_t totalsize; /* total size of DT block */
>> fdt32_t off_dt_struct; /* offset to structure */
>> fdt32_t off_dt_strings; /* offset to strings */
>> @@ -93,6 +93,7 @@ struct fdt_property {
>> #endif /* !__ASSEMBLY */
>>
>> #define FDT_MAGIC 0xd00dfeed /* 4: version, 4: total size */
>> +#define FDT_MAGIC_DTBO 0xd00dfdb0 /* DTBO magic */
>> #define FDT_TAGSIZE sizeof(fdt32_t)
>>
>> #define FDT_BEGIN_NODE 0x1 /* Start node: full name */
>> diff --git a/livetree.c b/livetree.c
>> index 3dc7559..1a2f4b1 100644
>> --- a/livetree.c
>> +++ b/livetree.c
>> @@ -216,6 +216,31 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>> return old_node;
>> }
>>
>> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
>> +{
>> + static unsigned int next_orphan_fragment = 0;
>> + struct node *node = xmalloc(sizeof(*node));
>
> You shouldn't use a bare malloc() for a node. Use build_node() instead.
>
OK.
>> + struct property *p;
>> + struct data d = empty_data;
>> + char *name;
>> +
>> + memset(node, 0, sizeof(*node));
>> +
>> + d = data_add_marker(d, REF_PHANDLE, ref);
>> + d = data_append_integer(d, 0xffffffff, 32);
>> +
>> + p = build_property("target", d);
>> + add_property(node, p);
>> +
>> + xasprintf(&name, "fragment@%u",
>> + next_orphan_fragment++);
>> + name_node(node, name);
>> + name_node(new_node, "__overlay__");
>> +
>> + add_child(dt, node);
>> + add_child(node, new_node);
>> +}
>> +
>> struct node *chain_node(struct node *first, struct node *list)
>> {
>> assert(first->next_sibling == NULL);
>> @@ -296,6 +321,23 @@ void delete_node(struct node *node)
>> delete_labels(&node->labels);
>> }
>>
>> +void append_to_property(struct node *node,
>> + char *name, const void *data, int len)
>> +{
>> + struct data d;
>> + struct property *p;
>> +
>> + p = get_property(node, name);
>> + if (p) {
>> + d = data_append_data(p->val, data, len);
>> + p->val = d;
>> + } else {
>> + d = data_append_data(empty_data, data, len);
>> + p = build_property(name, d);
>> + add_property(node, p);
>> + }
>> +}
>> +
>> struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>> {
>> struct reserve_info *new = xmalloc(sizeof(*new));
>> @@ -335,12 +377,14 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>> return list;
>> }
>>
>> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
>> +struct boot_info *build_boot_info(unsigned int versionflags,
>> + struct reserve_info *reservelist,
>> struct node *tree, uint32_t boot_cpuid_phys)
>> {
>> struct boot_info *bi;
>>
>> bi = xmalloc(sizeof(*bi));
>> + bi->versionflags = versionflags;
>> bi->reservelist = reservelist;
>> bi->dt = tree;
>> bi->boot_cpuid_phys = boot_cpuid_phys;
>> @@ -709,3 +753,182 @@ void sort_tree(struct boot_info *bi)
>> sort_reserve_entries(bi);
>> sort_node(bi->dt);
>> }
>> +
>> +/* utility helper to avoid code duplication */
>> +static struct node *build_and_name_child_node(struct node *parent, char *name)
>> +{
>> + struct node *node;
>> +
>> + node = build_node(NULL, NULL);
>> + name_node(node, xstrdup(name));
>> + add_child(parent, node);
>> +
>> + return node;
>> +}
>> +
>> +static void generate_label_tree_internal(struct node *dt, struct node *node,
>> + struct node *an, bool allocph)
>> +{
>> + struct node *c;
>> + struct property *p;
>> + struct label *l;
>> +
>> + /* if if there are labels */
>> + if (node->labels) {
>> + /* now add the label in the node */
>> + for_each_label(node->labels, l) {
>> + /* check whether the label already exists */
>> + p = get_property(an, l->label);
>> + if (p) {
>> + fprintf(stderr, "WARNING: label %s already"
>> + " exists in /%s", l->label,
>> + an->name);
>> + continue;
>> + }
>> +
>> + /* insert it */
>> + p = build_property(l->label,
>> + data_copy_escape_string(node->fullpath,
>> + strlen(node->fullpath)));
>
> Um.. what? The node path should absolutely not be an escape string.
> It shouldn't contain escapes to begin with, and if it does, we
> absolutely shouldn't be turning them into special characters here.
>
OK, trivial enough to do. I tried to play it safe here but this is an overkill.
>> + add_property(an, p);
>> + }
>> +
>> + /* force allocation of a phandle for this node */
>> + if (allocph)
>> + (void)get_node_phandle(dt, node);
>> + }
>> +
>> + for_each_child(node, c)
>> + generate_label_tree_internal(dt, c, an, allocph);
>> +}
>> +
>> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
>> +{
>> + struct node *an;
>> +
>> + for_each_child(dt, an)
>> + if (streq(gen_node_name, an->name))
>> + break;
>> +
>> + if (!an)
>> + an = build_and_name_child_node(dt, gen_node_name);
>> + if (!an)
>> + die("Could not build label node /%s\n", gen_node_name);
>> +
>> + generate_label_tree_internal(dt, dt, an, allocph);
>> +}
>> +
>> +static char *fixups_name = "__fixups__";
>> +static char *local_fixups_name = "__local_fixups__";
>
> I'd actually prefer #defines for these, and all-caps names, so it's
> more obvious when they're used that these are global constants.
>
OK.
>> +
>> +static void add_fixup_entry(struct node *dt, struct node *node,
>> + struct property *prop, struct marker *m)
>> +{
>> + struct node *fn; /* fixup node */
>> + char *entry;
>> +
>> + /* m->ref can only be a REF_PHANDLE, but check anyway */
>> + assert(m->type == REF_PHANDLE);
>> +
>> + /* fn is the node we're putting entries in */
>> + fn = get_subnode(dt, fixups_name);
>> + assert(fn != NULL);
>> +
>> + /* there shouldn't be any ':' in the arguments */
>> + if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
>> + die("arguments should not contain ':'\n");
>> +
>> + xasprintf(&entry, "%s:%s:%u",
>> + node->fullpath, prop->name, m->offset);
>> + append_to_property(fn, m->ref, entry, strlen(entry) + 1);
>> +}
>> +
>> +static void add_local_fixup_entry(struct node *dt, struct node *node,
>> + struct property *prop, struct marker *m,
>> + struct node *refnode)
>> +{
>> + struct node *lfn, *wn, *nwn; /* local fixup node, walk node, new */
>> + uint32_t value_32;
>> + char *s, *e, *comp;
>> + int len;
>> +
>> + /* fn is the node we're putting entries in */
>> + lfn = get_subnode(dt, local_fixups_name);
>> + assert(lfn != NULL);
>> +
>> + /* walk the path components creating nodes if they don't exist */
>> + comp = xmalloc(strlen(node->fullpath) + 1);
>> + /* start skipping the first / */
>> + s = node->fullpath + 1;
>> + wn = lfn;
>> + while (*s) {
>> + /* retrieve path component */
>> + e = strchr(s, '/');
>> + if (e == NULL)
>> + e = s + strlen(s);
>> + len = e - s;
>> + memcpy(comp, s, len);
>> + comp[len] = '\0';
>> +
>> + /* if no node exists, create it */
>> + nwn = get_subnode(wn, comp);
>> + if (!nwn)
>> + nwn = build_and_name_child_node(wn, comp);
>> + wn = nwn;
>> +
>> + /* last path component */
>> + if (!*e)
>> + break;
>> +
>> + /* next path component */
>> + s = e + 1;
>> + }
>> + free(comp);
>> +
>> + value_32 = cpu_to_fdt32(m->offset);
>> + append_to_property(wn, prop->name, &value_32, sizeof(value_32));
>> +}
>> +
>> +static void generate_fixups_tree_internal(struct node *dt, struct node *node)
>> +{
>> + struct node *c;
>> + struct property *prop;
>> + struct marker *m;
>> + struct node *refnode;
>> +
>> + for_each_property(node, prop) {
>> + m = prop->val.markers;
>> + for_each_marker_of_type(m, REF_PHANDLE) {
>> + refnode = get_node_by_ref(dt, m->ref);
>> + if (!refnode)
>> + add_fixup_entry(dt, node, prop, m);
>> + else
>> + add_local_fixup_entry(dt, node, prop, m,
>> + refnode);
>> + }
>> + }
>> +
>> + for_each_child(node, c)
>> + generate_fixups_tree_internal(dt, c);
>> +}
>> +
>> +void generate_fixups_tree(struct node *dt)
>> +{
>> + struct node *an;
>> +
>> + for_each_child(dt, an)
>> + if (streq(fixups_name, an->name))
>> + break;
>> +
>> + if (!an)
>> + build_and_name_child_node(dt, fixups_name);
>> +
>> + for_each_child(dt, an)
>> + if (streq(local_fixups_name, an->name))
>> + break;
>> +
>> + if (!an)
>> + build_and_name_child_node(dt, local_fixups_name);
>> +
>> + generate_fixups_tree_internal(dt, dt);
>> +}
>> diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
>> index a76e51e..d29ebc6 100644
>> --- a/tests/mangle-layout.c
>> +++ b/tests/mangle-layout.c
>> @@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
>> buf->size = newsize;
>> }
>>
>> -static void new_header(struct bufstate *buf, int version, const void *fdt)
>> +static void new_header(struct bufstate *buf, fdt32_t magic, int version,
>> + const void *fdt)
>> {
>> int hdrsize;
>>
>> @@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
>> expand_buf(buf, hdrsize);
>> memset(buf->buf, 0, hdrsize);
>>
>> - fdt_set_magic(buf->buf, FDT_MAGIC);
>> + fdt_set_magic(buf->buf, magic);
>> fdt_set_version(buf->buf, version);
>> fdt_set_last_comp_version(buf->buf, 16);
>> fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
>> @@ -145,7 +146,7 @@ int main(int argc, char *argv[])
>> if (fdt_version(fdt) < 17)
>> CONFIG("Input tree must be v17");
>>
>> - new_header(&buf, version, fdt);
>> + new_header(&buf, FDT_MAGIC, version, fdt);
>>
>> while (*blockorder) {
>> add_block(&buf, version, *blockorder, fdt);
>> diff --git a/treesource.c b/treesource.c
>> index a55d1d1..75e920d 100644
>> --- a/treesource.c
>> +++ b/treesource.c
>> @@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
>> {
>> struct reserve_info *re;
>>
>> - fprintf(f, "/dts-v1/;\n\n");
>> + fprintf(f, "/dts-v1/");
>> +
>> + if (bi->versionflags & VF_PLUGIN)
>> + fprintf(f, " /plugin/");
>> +
>> + fprintf(f, ";\n\n");
>
> I'm not sure this really makes sense. The /plugin/ tag triggers the
> fixup construction and encoding of overlay fragments. But in an
> incoming dtb, that processing has already been done once, doing it
> again would not make sense. So I think the output should not have the
> /plugin/ tag, even if the input did.
>
> Unless, of course, you parsed the input into an explicit list of
> overlays, then output it here again as a list of overlays, not the
> encoded fragments. i.e. if this was the original tree:
>
> /dts-v1/ /plugin/;
>
> &somwhere {
> name = "value";
> };
>
> then legitimately the output of -I dts -O dts could be either:
>
> /dts-v1/ /plugin/;
>
> &somwhere {
> name = "value";
> };
>
> OR
>
> /dts-v1/;
>
> / {
> fragment@0 {
> target = <0xffffffff>;
> __overlay__{
> name = "value";
> };
> };
> __fixups__ {
> somewhere = "/fragment@0:target:0";
> };
> };
>
> But it doesn't make sense to combine the two: the second structure
> with the "/plugin/" tag.
>
> Another advantage of moving the overlay application out of the parser
> is you can sanely produce either output, both of which could be useful
> in the right circumstances. You can potentially even produce the
> first output (or something close) from dtb input, with correct parsing
> of the overlay structure on dtb input.
>
Yes, this makes sense. For now I’ll remove the plugin tag, but if the blob
was compiled with -@ you could conceivably reverse back to a form that contains
labels and phandle references correctly.
But this is quite complicated to undertake in this patch series.
To re-iterate though there is no overlay application here :)
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
Regards
— Pantelis
--
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: [RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings
From: Philipp Zabel @ 2016-11-25 10:54 UTC (permalink / raw)
To: zhangfei
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann, Stephen Boyd
In-Reply-To: <e71cfe28-cd31-828f-8c7f-b4faeed5d27f-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Am Freitag, den 25.11.2016, 18:42 +0800 schrieb zhangfei:
>
> On 2016年11月25日 18:25, Philipp Zabel wrote:
> > Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
> >> On 2016年11月24日 17:50, Philipp Zabel wrote:
> >>> Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
> >>>> On 2016年11月24日 17:26, Philipp Zabel wrote:
> >>>>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >>>>>> Add DT bindings documentation for hi3660 SoC reset controller.
> >>>>>>
> >>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>>> ---
> >>>>>> .../bindings/reset/hisilicon,hi3660-reset.txt | 51 ++++++++++++++++++++++
> >>>>>> 1 file changed, 51 insertions(+)
> >>>>>> create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..250daf2
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>> @@ -0,0 +1,51 @@
> >>>>>> +Hisilicon System Reset Controller
> >>>>>> +======================================
> >>>>>> +
> >>>>>> +Please also refer to reset.txt in this directory for common reset
> >>>>>> +controller binding usage.
> >>>>>> +
> >>>>>> +The reset controller registers are part of the system-ctl block on
> >>>>>> +hi3660 SoC.
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +- compatible: should be
> >>>>>> + "hisilicon,hi3660-reset"
> >>>>>> +- #reset-cells: 1, see below
> >>>>>> +- hisi,rst-syscon: phandle of the reset's syscon.
> >>>>>> +- hisi,reset-bits: Contains the reset control register information
> >>>>>> + Should contain 2 cells for each reset exposed to
> >>>>>> + consumers, defined as:
> >>>>>> + Cell #1 : offset from the syscon register base
> >>>>>> + Cell #2 : bits position of the control register
> >>>>>> +
> >>>>>> +Example:
> >>>>>> + iomcu: iomcu@ffd7e000 {
> >>>>>> + compatible = "hisilicon,hi3660-iomcu", "syscon";
> >>>>>> + reg = <0x0 0xffd7e000 0x0 0x1000>;
> >>>>>> + };
> >>>>>> +
> >>>>>> + iomcu_rst: iomcu_rst_controller {
> >>>>> This should be
> >>>>> iomcu_rst: reset-controller {
> >>>>>
> >>>>>> + compatible = "hisilicon,hi3660-reset";
> >>>>>> + #reset-cells = <1>;
> >>>>>> + hisi,rst-syscon = <&iomcu>;
> >>>>>> + hisi,reset-bits = <0x20 0x8 /* 0: i2c0 */
> >>>>>> + 0x20 0x10 /* 1: i2c1 */
> >>>>>> + 0x20 0x20 /* 2: i2c2 */
> >>>>>> + 0x20 0x8000000>; /* 3: i2c6 */
> >>>>>> + };
> >>>>> The reset lines are controlled through iomcu bits, is there a reason not
> >>>>> to put the iomcu_rst node inside the iomcu node? That way the
> >>>>> hisi,rst-syscon property could be removed and the syscon could be
> >>>>> retrieved via the reset-controller parent node.
> >>>> iomcu is common registers, controls clock and reset, etc.
> >>>> So we use syscon, without mapping the registers everywhere.
> >>>> It is common case in hisilicon, same in hi6220.
> >>>>
> >>>> Also the #clock-cells and #reset-cells can not be put in the same node,
> >>>> if they are both using probe, since reset_probe will not be called.
> >>>>
> >>>> So we use hisi,rst-syscon as a general solution.
> >>> What I meant is this:
> >>>
> >>> iomcu: iomcu@ffd7e000 {
> >>> compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
> >>> reg = <0x0 0xffd7e000 0x0 0x1000>;
> >> #clock-cells = <1>;
> >>
> >> In my test, if there add #clock-cells = <1>, reset_probe will not be
> >> called any more.
> >> Since clk_probe is called first.
> >> No matter iomcu_rst is child node or not.
> > I don't understand this, does the clock driver bind to the iomcu node
> > using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?
>
> This method:CLK_OF_DECLARE_DRIVER is not prefered in clock,
> and we have to use probe instead, to make all driver build as modules as
> possible.
>
> For example hi3660.
> static struct platform_driver hi3660_clk_driver = {
> .probe = hi3660_clk_probe,
> .driver = {
> .name = "hi3660-clk",
> .of_match_table = hi3660_clk_match_table,
> },
> };
hi3660_clk_match_table contains the "hisilicon,hi3660-iomcu" compatible?
If so, you could call
of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
from hi3660_clk_probe instead of using "simple-mfd" to probe the iomcu
node's children.
regards
Philipp
--
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 01/10] power: supply: axp20x_usb_power: use of_device_id data field instead of device_is_compatible
From: kbuild test robot @ 2016-11-25 10:52 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, sre-DgEjT+Ai2ygdnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
wens-jdAy2FN1RRM, linux-I+IVW8TIWO2tmTQ+vhA3Yw,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
lee.jones-QSEj5FYQhm4dnm+yROfE0A, Quentin Schulz,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <20161125090921.23138-2-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
Hi Quentin,
[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.9-rc6 next-20161124]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Quentin-Schulz/add-support-for-VBUS-max-current-and-min-voltage-limits-AXP20X-and-AXP22X-PMICs/20161125-172224
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-randconfig-s5-11251757 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/power/supply/axp20x_usb_power.c: In function 'axp20x_usb_power_probe':
>> drivers/power/supply/axp20x_usb_power.c:233:21: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
power->axp20x_id = (int)of_id->data;
^
vim +233 drivers/power/supply/axp20x_usb_power.c
217 if (!of_device_is_available(pdev->dev.of_node))
218 return -ENODEV;
219
220 of_id = of_match_device(axp20x_usb_power_match, &pdev->dev);
221 if (!of_id)
222 return -ENODEV;
223
224 if (!axp20x) {
225 dev_err(&pdev->dev, "Parent drvdata not set\n");
226 return -EINVAL;
227 }
228
229 power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
230 if (!power)
231 return -ENOMEM;
232
> 233 power->axp20x_id = (int)of_id->data;
234
235 power->np = pdev->dev.of_node;
236 power->regmap = axp20x->regmap;
237
238 if (power->axp20x_id == AXP202_ID) {
239 /* Enable vbus valid checking */
240 ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
241 AXP20X_VBUS_MON_VBUS_VALID,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23302 bytes --]
^ permalink raw reply
* Re: [RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings
From: zhangfei @ 2016-11-25 10:42 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann, Stephen Boyd
In-Reply-To: <1480069523.4058.17.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 2016年11月25日 18:25, Philipp Zabel wrote:
> Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
>> On 2016年11月24日 17:50, Philipp Zabel wrote:
>>> Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
>>>> On 2016年11月24日 17:26, Philipp Zabel wrote:
>>>>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
>>>>>> Add DT bindings documentation for hi3660 SoC reset controller.
>>>>>>
>>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>> ---
>>>>>> .../bindings/reset/hisilicon,hi3660-reset.txt | 51 ++++++++++++++++++++++
>>>>>> 1 file changed, 51 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..250daf2
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>> @@ -0,0 +1,51 @@
>>>>>> +Hisilicon System Reset Controller
>>>>>> +======================================
>>>>>> +
>>>>>> +Please also refer to reset.txt in this directory for common reset
>>>>>> +controller binding usage.
>>>>>> +
>>>>>> +The reset controller registers are part of the system-ctl block on
>>>>>> +hi3660 SoC.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: should be
>>>>>> + "hisilicon,hi3660-reset"
>>>>>> +- #reset-cells: 1, see below
>>>>>> +- hisi,rst-syscon: phandle of the reset's syscon.
>>>>>> +- hisi,reset-bits: Contains the reset control register information
>>>>>> + Should contain 2 cells for each reset exposed to
>>>>>> + consumers, defined as:
>>>>>> + Cell #1 : offset from the syscon register base
>>>>>> + Cell #2 : bits position of the control register
>>>>>> +
>>>>>> +Example:
>>>>>> + iomcu: iomcu@ffd7e000 {
>>>>>> + compatible = "hisilicon,hi3660-iomcu", "syscon";
>>>>>> + reg = <0x0 0xffd7e000 0x0 0x1000>;
>>>>>> + };
>>>>>> +
>>>>>> + iomcu_rst: iomcu_rst_controller {
>>>>> This should be
>>>>> iomcu_rst: reset-controller {
>>>>>
>>>>>> + compatible = "hisilicon,hi3660-reset";
>>>>>> + #reset-cells = <1>;
>>>>>> + hisi,rst-syscon = <&iomcu>;
>>>>>> + hisi,reset-bits = <0x20 0x8 /* 0: i2c0 */
>>>>>> + 0x20 0x10 /* 1: i2c1 */
>>>>>> + 0x20 0x20 /* 2: i2c2 */
>>>>>> + 0x20 0x8000000>; /* 3: i2c6 */
>>>>>> + };
>>>>> The reset lines are controlled through iomcu bits, is there a reason not
>>>>> to put the iomcu_rst node inside the iomcu node? That way the
>>>>> hisi,rst-syscon property could be removed and the syscon could be
>>>>> retrieved via the reset-controller parent node.
>>>> iomcu is common registers, controls clock and reset, etc.
>>>> So we use syscon, without mapping the registers everywhere.
>>>> It is common case in hisilicon, same in hi6220.
>>>>
>>>> Also the #clock-cells and #reset-cells can not be put in the same node,
>>>> if they are both using probe, since reset_probe will not be called.
>>>>
>>>> So we use hisi,rst-syscon as a general solution.
>>> What I meant is this:
>>>
>>> iomcu: iomcu@ffd7e000 {
>>> compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
>>> reg = <0x0 0xffd7e000 0x0 0x1000>;
>> #clock-cells = <1>;
>>
>> In my test, if there add #clock-cells = <1>, reset_probe will not be
>> called any more.
>> Since clk_probe is called first.
>> No matter iomcu_rst is child node or not.
> I don't understand this, does the clock driver bind to the iomcu node
> using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?
This method:CLK_OF_DECLARE_DRIVER is not prefered in clock,
and we have to use probe instead, to make all driver build as modules as
possible.
For example hi3660.
static struct platform_driver hi3660_clk_driver = {
.probe = hi3660_clk_probe,
.driver = {
.name = "hi3660-clk",
.of_match_table = hi3660_clk_match_table,
},
};
static int __init hi3660_clk_init(void)
{
return platform_driver_register(&hi3660_clk_driver);
}
core_initcall(hi3660_clk_init);
And many examples in drivers/clock, just
#grep -rn probe drivers/clk/
drivers/clk/clk-axm5516.c:587: .probe = axmclk_probe,
If the parent node happen to be clock, and set
#clock-cells = <1>;
Then clock_probe/hi3660_clk_probe will be called first.
But reset_probe will not be called any more.
Thanks
>
> My comment was based only on this reset binding documentation and the
> example DT snippet. Could you point me to the clock driver probe code
> and show me a more complete part of the hi3660 device tree?
>
> regards
> Philipp
>
--
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 v6 3/5] ARM: dts: sun8i-h3: add HDMI video nodes
From: Icenowy Zheng @ 2016-11-25 10:32 UTC (permalink / raw)
To: Jean-Francois Moine
Cc: Dave Airlie, Maxime Ripard, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
In-Reply-To: <20161125112213.83420594eb435b6bb1a4d164-GANU6spQydw@public.gmane.org>
25.11.2016, 18:22, "Jean-Francois Moine" <moinejf-GANU6spQydw@public.gmane.org>:
> On Fri, 25 Nov 2016 17:41:51 +0800
> Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org> wrote:
>
>> After removing CLK_PLL_DE's assigned-clock, the kernel passes compilation.
>
> The 'pll-de' and 'de' must have a fixed rate. Otherwise, if you do not
> use the legacy u-boot, I don't know which can be the rate of the DE.
Can't CCU deal with this?
I still kept the CLK_DE's assigned-clock...
>
>> However, it cannot recognize any HDMI screen...
>>
>> (My board is Orange Pi One, and I manually added status="okay"; to &lcd0, &de, &hdmi)
>>
>> [ 16.507802] sun8i-de2 1000000.de-controller: bound 1c0c000.lcd-controller (ops de2_lcd_ops [sun8i_de2_drm])
>> [ 16.675948] sun8i-de2 1000000.de-controller: bound 1ee0000.hdmi (ops de2_hdmi_fini [sun8i_de2_hdmi])
>> [ 16.685120] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> [ 16.695876] [drm] No driver support for vblank timestamp query.
>> [ 16.701862] sun8i-de2 1000000.de-controller: No connectors reported connected with modes
>> [ 16.713061] [drm] Cannot find any crtc or sizes - going 1024x768
>> [ 16.734214] Console: switching to colour frame buffer device 128x48
>> [ 16.751022] sun8i-de2 1000000.de-controller: fb0: frame buffer device
>
> I put a 'pr_warn' message is case the EDID cannot be read.
> Have you this message?
Seems no...
>
> Anyway, there is a problem with the EDID:
> - my Orange Pi 2 (H3) randomly fails to read it. But this succeeds after
> rebooting once or twice.
> - my Banana Pi M2+ (H3) reads it correctly each time.
> - my Banana Pi M3 (A83T) can never read it.
>
> BTW, on first tries, I was forcing a CEA mode thru the kernel command
> line. This was working with the OPi2 and BPiM3, but there was no sound.
> In the last version, I use a EDID in edid_firmware for having sound
> with the BPiM3. This works fine.
> But, forcing a CEA mode is no more possible, so, when the OPi2 cannot
> read the EDID, the system switches to a VGA mode (default 1024x768)
> which is not supported. It seems that this is your case.
>
> So, in brief, if your board cannot read the EDID, put a EDID somewhere
> and its path in /sys/module/drm_kms_helper/parameters/edid_firmware.
> There will be no console, but X11 will work correctly.
The problem seems to be that the CRTC is not properly initialized...
>
> --
> Ken ar c'hentañ | ** Breizh ha Linux atav! **
> Jef | http://moinejf.free.fr/
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings
From: Philipp Zabel @ 2016-11-25 10:27 UTC (permalink / raw)
To: zhangfei
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <01de97d9-e910-d9f3-f081-215a78f7f4d2-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Am Freitag, den 25.11.2016, 11:04 +0800 schrieb zhangfei:
>
> On 2016年11月24日 17:26, Philipp Zabel wrote:
> > Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >> Add DT bindings documentation for hi3660 SoC reset controller.
> >>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >> .../bindings/reset/hisilicon,hi3660-reset.txt | 51 ++++++++++++++++++++++
> >> 1 file changed, 51 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >> new file mode 100644
> >> index 0000000..250daf2
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >> @@ -0,0 +1,51 @@
> >> +Hisilicon System Reset Controller
> >> +======================================
> >> +
> >> +Please also refer to reset.txt in this directory for common reset
> >> +controller binding usage.
> >> +
> >> +The reset controller registers are part of the system-ctl block on
> >> +hi3660 SoC.
> >> +
> >> +Required properties:
> >> +- compatible: should be
> >> + "hisilicon,hi3660-reset"
> >> +- #reset-cells: 1, see below
> >> +- hisi,rst-syscon: phandle of the reset's syscon.
> >> +- hisi,reset-bits: Contains the reset control register information
> >> + Should contain 2 cells for each reset exposed to
> >> + consumers, defined as:
> >> + Cell #1 : offset from the syscon register base
> >> + Cell #2 : bits position of the control register
> >> +
> >> +Example:
> >> + iomcu: iomcu@ffd7e000 {
> >> + compatible = "hisilicon,hi3660-iomcu", "syscon";
> >> + reg = <0x0 0xffd7e000 0x0 0x1000>;
> >> + };
> >> +
> >> + iomcu_rst: iomcu_rst_controller {
> > This should be
> > iomcu_rst: reset-controller {
> By the way, could I keep the original name?
> Since there will be build error if several nodes use the same name.
> like:
> - iomcu_rst: iomcu_rst_controller {
> + iomcu_rst: reset-controller {
>
> - crg_rst: crg_rst_controller {
> + crg_rst: reset-controller {
That should not be a problem if they are moved inside the controlling
node:
iomcu {
iomcu_rst: reset-controller {
};
};
crg {
crg_rst: reset-controller {
};
};
regards
Philipp
--
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: [RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings
From: Philipp Zabel @ 2016-11-25 10:25 UTC (permalink / raw)
To: zhangfei
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <8328a235-faeb-2218-4c49-a3f282599e95-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
>
> On 2016年11月24日 17:50, Philipp Zabel wrote:
> > Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
> >> On 2016年11月24日 17:26, Philipp Zabel wrote:
> >>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >>>> Add DT bindings documentation for hi3660 SoC reset controller.
> >>>>
> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>> ---
> >>>> .../bindings/reset/hisilicon,hi3660-reset.txt | 51 ++++++++++++++++++++++
> >>>> 1 file changed, 51 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>> new file mode 100644
> >>>> index 0000000..250daf2
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>> @@ -0,0 +1,51 @@
> >>>> +Hisilicon System Reset Controller
> >>>> +======================================
> >>>> +
> >>>> +Please also refer to reset.txt in this directory for common reset
> >>>> +controller binding usage.
> >>>> +
> >>>> +The reset controller registers are part of the system-ctl block on
> >>>> +hi3660 SoC.
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: should be
> >>>> + "hisilicon,hi3660-reset"
> >>>> +- #reset-cells: 1, see below
> >>>> +- hisi,rst-syscon: phandle of the reset's syscon.
> >>>> +- hisi,reset-bits: Contains the reset control register information
> >>>> + Should contain 2 cells for each reset exposed to
> >>>> + consumers, defined as:
> >>>> + Cell #1 : offset from the syscon register base
> >>>> + Cell #2 : bits position of the control register
> >>>> +
> >>>> +Example:
> >>>> + iomcu: iomcu@ffd7e000 {
> >>>> + compatible = "hisilicon,hi3660-iomcu", "syscon";
> >>>> + reg = <0x0 0xffd7e000 0x0 0x1000>;
> >>>> + };
> >>>> +
> >>>> + iomcu_rst: iomcu_rst_controller {
> >>> This should be
> >>> iomcu_rst: reset-controller {
> >>>
> >>>> + compatible = "hisilicon,hi3660-reset";
> >>>> + #reset-cells = <1>;
> >>>> + hisi,rst-syscon = <&iomcu>;
> >>>> + hisi,reset-bits = <0x20 0x8 /* 0: i2c0 */
> >>>> + 0x20 0x10 /* 1: i2c1 */
> >>>> + 0x20 0x20 /* 2: i2c2 */
> >>>> + 0x20 0x8000000>; /* 3: i2c6 */
> >>>> + };
> >>> The reset lines are controlled through iomcu bits, is there a reason not
> >>> to put the iomcu_rst node inside the iomcu node? That way the
> >>> hisi,rst-syscon property could be removed and the syscon could be
> >>> retrieved via the reset-controller parent node.
> >> iomcu is common registers, controls clock and reset, etc.
> >> So we use syscon, without mapping the registers everywhere.
> >> It is common case in hisilicon, same in hi6220.
> >>
> >> Also the #clock-cells and #reset-cells can not be put in the same node,
> >> if they are both using probe, since reset_probe will not be called.
> >>
> >> So we use hisi,rst-syscon as a general solution.
> > What I meant is this:
> >
> > iomcu: iomcu@ffd7e000 {
> > compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
> > reg = <0x0 0xffd7e000 0x0 0x1000>;
> #clock-cells = <1>;
>
> In my test, if there add #clock-cells = <1>, reset_probe will not be
> called any more.
> Since clk_probe is called first.
> No matter iomcu_rst is child node or not.
I don't understand this, does the clock driver bind to the iomcu node
using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?
My comment was based only on this reset binding documentation and the
example DT snippet. Could you point me to the clock driver probe code
and show me a more complete part of the hi3660 device tree?
regards
Philipp
--
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 v6 3/5] ARM: dts: sun8i-h3: add HDMI video nodes
From: Jean-Francois Moine @ 2016-11-25 10:22 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Dave Airlie, Maxime Ripard, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
In-Reply-To: <3188681480066911-UANfZzvcKe1uio3avFS2gg@public.gmane.org>
On Fri, 25 Nov 2016 17:41:51 +0800
Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org> wrote:
> After removing CLK_PLL_DE's assigned-clock, the kernel passes compilation.
The 'pll-de' and 'de' must have a fixed rate. Otherwise, if you do not
use the legacy u-boot, I don't know which can be the rate of the DE.
> However, it cannot recognize any HDMI screen...
>
> (My board is Orange Pi One, and I manually added status="okay"; to &lcd0, &de, &hdmi)
>
> [ 16.507802] sun8i-de2 1000000.de-controller: bound 1c0c000.lcd-controller (ops de2_lcd_ops [sun8i_de2_drm])
> [ 16.675948] sun8i-de2 1000000.de-controller: bound 1ee0000.hdmi (ops de2_hdmi_fini [sun8i_de2_hdmi])
> [ 16.685120] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [ 16.695876] [drm] No driver support for vblank timestamp query.
> [ 16.701862] sun8i-de2 1000000.de-controller: No connectors reported connected with modes
> [ 16.713061] [drm] Cannot find any crtc or sizes - going 1024x768
> [ 16.734214] Console: switching to colour frame buffer device 128x48
> [ 16.751022] sun8i-de2 1000000.de-controller: fb0: frame buffer device
I put a 'pr_warn' message is case the EDID cannot be read.
Have you this message?
Anyway, there is a problem with the EDID:
- my Orange Pi 2 (H3) randomly fails to read it. But this succeeds after
rebooting once or twice.
- my Banana Pi M2+ (H3) reads it correctly each time.
- my Banana Pi M3 (A83T) can never read it.
BTW, on first tries, I was forcing a CEA mode thru the kernel command
line. This was working with the OPi2 and BPiM3, but there was no sound.
In the last version, I use a EDID in edid_firmware for having sound
with the BPiM3. This works fine.
But, forcing a CEA mode is no more possible, so, when the OPi2 cannot
read the EDID, the system switches to a VGA mode (default 1024x768)
which is not supported. It seems that this is your case.
So, in brief, if your board cannot read the EDID, put a EDID somewhere
and its path in /sys/module/drm_kms_helper/parameters/edid_firmware.
There will be no console, but X11 will work correctly.
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* [PATCH] devicetree: bindings: Add vendor prefix for Oki
From: Geert Uytterhoeven @ 2016-11-25 10:15 UTC (permalink / raw)
To: Rob Herring, Mark Rutland
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven
Already in use for "oki,ml86v7667".
Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index dc7d5998d0abc632..2868be624c0e3f75 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -198,6 +198,7 @@ nuvoton Nuvoton Technology Corporation
nvidia NVIDIA
nxp NXP Semiconductors
okaya Okaya Electric America, Inc.
+oki Oki Electric Industry Co., Ltd.
olimex OLIMEX Ltd.
onion Onion Corporation
onnn ON Semiconductor Corp.
--
1.9.1
--
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 related
* Re: [PATCH 10/10] ARM: dtsi: sun8i-reference-design-tablet: use AXP223 DTSI
From: Chen-Yu Tsai @ 2016-11-25 10:06 UTC (permalink / raw)
To: Quentin Schulz
Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Chen-Yu Tsai,
Russell King, Maxime Ripard, Lee Jones, open list:THERMAL,
devicetree, linux-kernel, linux-arm-kernel, Thomas Petazzoni
In-Reply-To: <20161125090921.23138-11-quentin.schulz@free-electrons.com>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Previously, the sun8i tablets used everything declared in AXP221 DTSI
> while they have an AXP223 PMIC.
>
> This corrects that so the sun8i tablets can get some features the AXP223
> has (at the moment, ability to have 100mA as maximal current on VBUS
> power supply).
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
^ permalink raw reply
* Re: [PATCH 09/10] ARM: dts: sun8i-r16-parrot: use AXP223 DTSI
From: Chen-Yu Tsai @ 2016-11-25 10:05 UTC (permalink / raw)
To: Quentin Schulz
Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Chen-Yu Tsai,
Russell King, Maxime Ripard, Lee Jones, open list:THERMAL,
devicetree, linux-kernel, linux-arm-kernel, Thomas Petazzoni
In-Reply-To: <20161125090921.23138-10-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Previously, the Allwinner Parrot R16 used everything declared in AXP221
> DTSI while it has an AXP223 PMIC.
>
> This corrects that so the Allwinner Parrot R16 can get some features the
> AXP223 has (at the moment, ability to have 100mA as maximal current on
> VBUS power supply).
>
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
--
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 08/10] ARM: dts: sun8i-a33-sinlinx-sina33: use AXP223 DTSI
From: Chen-Yu Tsai @ 2016-11-25 10:05 UTC (permalink / raw)
To: Quentin Schulz
Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Chen-Yu Tsai,
Russell King, Maxime Ripard, Lee Jones, open list:THERMAL,
devicetree, linux-kernel, linux-arm-kernel, Thomas Petazzoni
In-Reply-To: <20161125090921.23138-9-quentin.schulz@free-electrons.com>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Previously, the Sinlinx SinA33 used everything declared in AXP221 DTSI
> while it has an AXP223 PMIC.
>
> This corrects that so the Sinlinx SinA33 can get some features the
> AXP223 has (at the moment, ability to have 100mA as maximal current on
> VBUS power supply).
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
^ permalink raw reply
* Re: [PATCH 07/10] ARM: dts: sun8i-a33-olinuxino: use AXP223 DTSI
From: Chen-Yu Tsai @ 2016-11-25 10:05 UTC (permalink / raw)
To: Quentin Schulz
Cc: Mark Rutland, devicetree, open list:THERMAL, linux-kernel,
Sebastian Reichel, Russell King, Chen-Yu Tsai, Rob Herring,
Maxime Ripard, Lee Jones, Thomas Petazzoni, linux-arm-kernel
In-Reply-To: <20161125090921.23138-8-quentin.schulz@free-electrons.com>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Previously, the Olimex A33-OlinuXino used everything declared in AXP221
> DTSI while it has an AXP223 PMIC.
>
> This corrects that so the Olimex A33-OlinuXino can get some features the
> AXP223 has (at the moment, ability to have 100mA as maximal current on
> VBUS power supply).
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
^ permalink raw reply
* Re: [PATCH 06/10] ARM: dtsi: add DTSI for AXP223
From: Chen-Yu Tsai @ 2016-11-25 10:04 UTC (permalink / raw)
To: Quentin Schulz
Cc: Mark Rutland, devicetree, open list:THERMAL, linux-kernel,
Sebastian Reichel, Russell King, Chen-Yu Tsai, Rob Herring,
Maxime Ripard, Lee Jones, Thomas Petazzoni, linux-arm-kernel
In-Reply-To: <20161125090921.23138-7-quentin.schulz@free-electrons.com>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> The AXP223 shares most of its logic with the AXP221 but it has some
> differences for the VBUS driver.
You could also mention this in the dtsi file itself.
Otherwise,
Acked-by: Chen-Yu Tsai <wens@csie.org>
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> arch/arm/boot/dts/axp223.dtsi | 55 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 arch/arm/boot/dts/axp223.dtsi
>
> diff --git a/arch/arm/boot/dts/axp223.dtsi b/arch/arm/boot/dts/axp223.dtsi
> new file mode 100644
> index 0000000..f8ce55c
> --- /dev/null
> +++ b/arch/arm/boot/dts/axp223.dtsi
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright 2016 Free Electrons
> + *
> + * Quentin Schulz <quentin.schulz@free-electrons.com>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file 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.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/*
> + * AXP223 Integrated Power Management Chip
> + * http://www.x-powers.com/product/AXP22X.php
> + * http://dl.linux-sunxi.org/AXP/AXP223-en.pdf
> + */
> +
> +#include "axp22x.dtsi"
> +
> +&usb_power_supply {
> + compatible = "x-powers,axp223-usb-power-supply";
> +};
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH 05/10] mfd: axp20x: add separate MFD cell for AXP223
From: Chen-Yu Tsai @ 2016-11-25 10:03 UTC (permalink / raw)
To: Quentin Schulz
Cc: Mark Rutland, devicetree, open list:THERMAL, linux-kernel,
Sebastian Reichel, Russell King, Chen-Yu Tsai, Rob Herring,
Maxime Ripard, Lee Jones, Thomas Petazzoni, linux-arm-kernel
In-Reply-To: <20161125090921.23138-6-quentin.schulz@free-electrons.com>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> The AXP223 shares most of its logic with the AXP221 but has some
> differences for the VBUS power supply driver. Thus, to probe the driver
> with the correct compatible, the AXP221 and the AXP223 now have separate
> MFD cells.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> drivers/mfd/axp20x.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index ba130be..989d568 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -602,6 +602,21 @@ static struct mfd_cell axp22x_cells[] = {
> },
> };
>
> +static struct mfd_cell axp223_cells[] = {
> + {
> + .name = "axp20x-pek",
> + .num_resources = ARRAY_SIZE(axp22x_pek_resources),
> + .resources = axp22x_pek_resources,
> + }, {
> + .name = "axp20x-regulator",
> + }, {
> + .name = "axp20x-usb-power-supply",
> + .of_compatible = "x-powers,axp223-usb-power-supply",
> + .num_resources = ARRAY_SIZE(axp22x_usb_power_supply_resources),
> + .resources = axp22x_usb_power_supply_resources,
Nit: Please align the statements. And you might want to rename the original
axp22x_cells to axp221_cells to avoid confusion. Otherwise,
Acked-by: Chen-Yu Tsai <wens@csie.org>
> + },
> +};
> +
> static struct mfd_cell axp152_cells[] = {
> {
> .name = "axp20x-pek",
> @@ -789,12 +804,17 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> axp20x->regmap_irq_chip = &axp20x_regmap_irq_chip;
> break;
> case AXP221_ID:
> - case AXP223_ID:
> axp20x->nr_cells = ARRAY_SIZE(axp22x_cells);
> axp20x->cells = axp22x_cells;
> axp20x->regmap_cfg = &axp22x_regmap_config;
> axp20x->regmap_irq_chip = &axp22x_regmap_irq_chip;
> break;
> + case AXP223_ID:
> + axp20x->nr_cells = ARRAY_SIZE(axp223_cells);
> + axp20x->cells = axp223_cells;
> + axp20x->regmap_cfg = &axp22x_regmap_config;
> + axp20x->regmap_irq_chip = &axp22x_regmap_irq_chip;
> + break;
> case AXP288_ID:
> axp20x->cells = axp288_cells;
> axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH v2 0/3] increase TSCADC clock to 24MHz and fix ti,charge-delay to represent in nS
From: Lee Jones @ 2016-11-25 9:59 UTC (permalink / raw)
To: Mugunthan V N
Cc: linux-input, Dmitry Torokhov, Jonathan Cameron, Rob Herring,
Mark Rutland, Sekhar Nori, Vignesh R, devicetree, linux-omap,
linux-kernel
In-Reply-To: <71482721-fd60-65b2-d601-f1698ce5fa0a@ti.com>
On Fri, 25 Nov 2016, Mugunthan V N wrote:
> Hi Dmitry Torokhov,
>
> On Thursday 10 November 2016 10:05 PM, Mugunthan V N wrote:
> > This patch series enables ADC to be clocked at 24MHz as the
> > TI AM335x ADC driver has already adopted to use DMA to transfer
> > ADC samples. Now ADC can generated upto 800K Samples per second
> > with the patch [1] on AM335x BBB and AM437x GP EVM.
> >
> > when ADC ref clock is set at 24MHz, I am seeing some issue with
> > touch screen pointer as the pointer jumps to random locations
> > with free draw application. The issue is due to increase in ADC
> > clock and charge delay for the touchscreen ADC line duration
> > reduced.
> >
> > So the notation of ti,charge-delay in terms of ADC clock is
> > wrong, it has to be represented in time and driver has to convert
> > the charge delay time to ADC clocks based on what ADC clock
> > frequency is set.
> >
> > Measured the performance with the iio_generic_buffer with the
> > patch [2] applied
> >
> > Verified the touch screen on AM335x GP EVM and AM335x BBB LCD7
> > cape with [3] dts for display and touch screen to work.
> >
>
> Since there are acks from DT and MFD maintainers, can you pull the patch
> series if you do not have any more comments.
Cant do anything without *all* Acks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH V4 2/2] pinctrl: tegra: Add driver to configure voltage and power of io pads
From: Thierry Reding @ 2016-11-25 9:57 UTC (permalink / raw)
To: Laxman Dewangan
Cc: linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
swarren-3lzwWm7+Weoh9ZMKESR00Q, gnurou-Re5JQEeQqe8AvxtiuMwx3w,
jonathanh-DDmLM1+adcrQT0dZR+AlfA, joe-6d6DIl74uiNBDgjK7y7TUQ,
yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1479976734-30498-3-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 24860 bytes --]
On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
> NVIDIA Tegra124 and later SoCs support the multi-voltage level and
> low power state of some of its IO pads. The IO pads can work in
> the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
> sources. When IO interfaces are not used then IO pads can be
> configure in low power state to reduce the power consumption from
> that IO pads.
>
> On Tegra124, the voltage level of IO power rail source is auto
> detected by hardware(SoC) and hence it is only require to configure
> in low power mode if IO pads are not used.
>
> On T210 onwards, the auto-detection of voltage level from IO power
> rail is removed from SoC and hence SW need to configure the PMC
> register explicitly to set proper voltage in IO pads based on
> IO rail power source voltage.
>
> This driver adds the IO pad driver to configure the power state and
> IO pad voltage based on the usage and power tree via pincontrol
> framework. The configuration can be static and dynamic.
>
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> ---
> Changes from V1:
> - Dropped the custom properties to set pad voltage and use regulator.
> - Added support for regulator to get vottage in boot and configure IO
> pad voltage.
> - Add support for callback to handle regulator notification and configure
> IO pad voltage based on voltage change.
>
> Changes from V2:
> Mostly nit changes per Jon's feedback i.e. use macros for voltage, added
> comment on macros, reduce the structure and variable name size, optimise
> number of variables, and allocate memory for regulator info when it needed.
>
> Changes from V3:
> Use the devm_regulator_get() instead of devm_regulator_get_optional().
>
> drivers/pinctrl/tegra/Kconfig | 12 +
> drivers/pinctrl/tegra/Makefile | 1 +
> drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c | 530 +++++++++++++++++++++++++++
> 3 files changed, 543 insertions(+)
> create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>
> diff --git a/drivers/pinctrl/tegra/Kconfig b/drivers/pinctrl/tegra/Kconfig
> index 24e20cc..6004e5c 100644
> --- a/drivers/pinctrl/tegra/Kconfig
> +++ b/drivers/pinctrl/tegra/Kconfig
> @@ -23,6 +23,18 @@ config PINCTRL_TEGRA210
> bool
> select PINCTRL_TEGRA
>
> +config PINCTRL_TEGRA_IO_PAD
> + bool "Tegra IO pad Control Driver"
> + depends on ARCH_TEGRA && REGULATOR
> + select PINCONF
> + select PINMUX
> + help
> + NVIDIA Tegra124/210 SoC has IO pads which supports multi-voltage
> + level of interfacing and deep power down mode of IO pads. The
> + voltage of IO pads are SW configurable based on IO rail of that
> + pads on T210. This driver provides the interface to change IO pad
> + voltage and power state via pincontrol interface.
This has a lot of chip-specific text. Will all of that have to be
updated if support for new chips is added?
> +
> config PINCTRL_TEGRA_XUSB
> def_bool y if ARCH_TEGRA
> select GENERIC_PHY
> diff --git a/drivers/pinctrl/tegra/Makefile b/drivers/pinctrl/tegra/Makefile
> index d9ea2be..3ebaaa2 100644
> --- a/drivers/pinctrl/tegra/Makefile
> +++ b/drivers/pinctrl/tegra/Makefile
> @@ -4,4 +4,5 @@ obj-$(CONFIG_PINCTRL_TEGRA30) += pinctrl-tegra30.o
> obj-$(CONFIG_PINCTRL_TEGRA114) += pinctrl-tegra114.o
> obj-$(CONFIG_PINCTRL_TEGRA124) += pinctrl-tegra124.o
> obj-$(CONFIG_PINCTRL_TEGRA210) += pinctrl-tegra210.o
> +obj-$(CONFIG_PINCTRL_TEGRA_IO_PAD) += pinctrl-tegra-io-pad.o
> obj-$(CONFIG_PINCTRL_TEGRA_XUSB) += pinctrl-tegra-xusb.o
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
> new file mode 100644
> index 0000000..aab02d0
> --- /dev/null
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
> @@ -0,0 +1,530 @@
> +/*
> + * pinctrl-tegra-io-pad: IO PAD driver for configuration of IO rail and deep
> + * Power Down mode via pinctrl framework.
> + *
> + * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
> + *
> + * Author: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <soc/tegra/pmc.h>
Have you considered moving this code into the PMC driver? It seems a
little over the top to go through all of the platform device creation
and driver registration dance only to call into a public API later on.
> +#include "../core.h"
> +#include "../pinconf.h"
> +#include "../pinctrl-utils.h"
> +
> +#define TEGRA_IO_RAIL_1800000UV 1800000
> +#define TEGRA_IO_RAIL_3300000UV 3300000
Is there really a point in having these defines? They are really long
and effectively don't carry more information than just the plain
numbers.
> +
> +/* Covert IO voltage to IO pad voltage enum */
Convert
> +#define tegra_io_uv_to_io_pads_uv(io_uv) \
> + (((io_uv) == TEGRA_IO_RAIL_1800000UV) ? \
> + TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV)
> +
> +#define tegra_io_voltage_is_valid(io_uv) \
> + ({ typeof(io_uv) io_uv_ = (io_uv); \
> + ((io_uv_ == TEGRA_IO_RAIL_1800000UV) || \
> + (io_uv_ == TEGRA_IO_RAIL_3300000UV)); })
>
Maybe make both of these static inline functions to improve readability?
I find the above very hard to read.
> +struct tegra_io_pads_cfg {
> + const char *name;
> + const unsigned int pins[1];
> + const char *vsupply;
> + enum tegra_io_pad id;
> + bool supports_low_power;
> +};
Pretty much everything in this driver operates on single pads, so it's a
little confusing to use the "pads" in the names. I think it would be
more appropriate to name this structure tegra_io_pad_cfg because it is
configuration data associated with a single pad.
> +
> +struct tegra_io_pads_soc_data {
I think the _data suffix is redundant and can be dropped.
The use of "pads" in this structure name is fine because it really does
contain data for multiple pads.
> + const struct tegra_io_pads_cfg *cfg;
> + int num_cfg;
This can be unsigned int. Also I think it's more common to use the
plural in these (cfgs, num_cfgs).
> + const struct pinctrl_pin_desc *desc;
> + int num_desc;
Same here.
> +};
> +
> +struct tegra_io_pads_info {
> + struct device *dev;
> + struct pinctrl_dev *pctl;
> + const struct tegra_io_pads_soc_data *soc_data;
If you drop the _data suffix from the type I think you can also drop it
from the field name.
"pads" is fine here as well because, again, this deals with multiple
pads.
> +};
> +
> +struct tegra_io_pads_regulator_info {
> + struct tegra_io_pads_info *tiopi;
> + const struct tegra_io_pads_cfg *cfg;
> + struct regulator *regulator;
> + struct notifier_block regulator_nb;
The regulator_ prefix is redundent here. It's contained within a
structure named tegra_io_pads_regulator_info, so it's fairly obvious
that this is related to regulators.
This deals only with a single pad, so tegra_io_pad_regulator_info would
be less confusing, I think. Perhaps even drop the _info suffix while at
it because it doesn't add anything useful.
> +};
> +
> +static int tegra_io_pads_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> + struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +
> + return tiopi->soc_data->num_cfg;
> +}
> +
> +static const char *tegra_io_pads_pinctrl_get_group_name(
> + struct pinctrl_dev *pctldev, unsigned int group)
> +{
> + struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +
> + return tiopi->soc_data->cfg[group].name;
> +}
> +
> +static int tegra_io_pads_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +
> + *pins = tiopi->soc_data->cfg[group].pins;
> + *num_pins = 1;
> +
> + return 0;
> +}
> +
> +static const struct pinctrl_ops tegra_io_pads_pinctrl_ops = {
> + .get_groups_count = tegra_io_pads_pinctrl_get_groups_count,
> + .get_group_name = tegra_io_pads_pinctrl_get_group_name,
> + .get_group_pins = tegra_io_pads_pinctrl_get_group_pins,
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
> + .dt_free_map = pinctrl_utils_free_map,
> +};
Nit: I don't think this padding using tabs increases readability.
> +
> +static int tegra_io_pads_pinconf_get(struct pinctrl_dev *pctldev,
> + unsigned int pin, unsigned long *config)
> +{
> + struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> + int param = pinconf_to_config_param(*config);
> + const struct tegra_io_pads_cfg *cfg = &tiopi->soc_data->cfg[pin];
> + int arg = 0;
Why not make that a u16...
> + int ret;
> +
> + switch (param) {
> + case PIN_CONFIG_LOW_POWER_MODE:
> + if (!cfg->supports_low_power) {
> + dev_err(tiopi->dev,
> + "IO pad %s does not support low power\n",
> + cfg->name);
> + return -EINVAL;
> + }
> +
> + ret = tegra_io_pad_power_get_status(cfg->id);
> + if (ret < 0)
> + return ret;
> + arg = !ret;
> + break;
> +
> + default:
> + dev_err(tiopi->dev, "The parameter %d not supported\n", param);
> + return -EINVAL;
> + }
> +
> + *config = pinconf_to_config_packed(param, (u16)arg);
... and avoid the cast here?
> +
> + return 0;
> +}
> +
> +static int tegra_io_pads_pinconf_set(struct pinctrl_dev *pctldev,
> + unsigned int pin, unsigned long *configs,
> + unsigned int num_configs)
This last line is not quite properly aligned.
> +{
> + struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> + const struct tegra_io_pads_cfg *cfg = &tiopi->soc_data->cfg[pin];
> + int i;
This should be unsigned.
> +
> + for (i = 0; i < num_configs; i++) {
> + int ret;
> + int param = pinconf_to_config_param(configs[i]);
The function returns an enum pin_config_param, why not use that as the
type?
> + u16 param_val = pinconf_to_config_argument(configs[i]);
> +
> + switch (param) {
> + case PIN_CONFIG_LOW_POWER_MODE:
> + if (!cfg->supports_low_power) {
> + dev_err(tiopi->dev,
> + "IO pad %s does not support low power\n",
> + cfg->name);
> + return -EINVAL;
> + }
> + if (param_val)
> + ret = tegra_io_pad_power_disable(cfg->id);
> + else
> + ret = tegra_io_pad_power_enable(cfg->id);
> + if (ret < 0) {
> + dev_err(tiopi->dev,
> + "Failed to set DPD %d of io-pad %s: %d\n",
> + param_val, cfg->name, ret);
> + return ret;
> + }
> + break;
> +
> + default:
> + dev_err(tiopi->dev, "The parameter %d not supported\n",
> + param);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct pinconf_ops tegra_io_pads_pinconf_ops = {
> + .pin_config_get = tegra_io_pads_pinconf_get,
> + .pin_config_set = tegra_io_pads_pinconf_set,
> +};
> +
> +static struct pinctrl_desc tegra_io_pads_pinctrl_desc = {
> + .name = "pinctrl-tegra-io-pads",
> + .pctlops = &tegra_io_pads_pinctrl_ops,
> + .confops = &tegra_io_pads_pinconf_ops,
> +};
> +
> +static int tegra_io_pads_rail_change_notify_cb(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct tegra_io_pads_regulator_info *rinfo;
> + struct pre_voltage_change_data *vdata;
> + unsigned long int io_volt_uv;
You can drop the int, it's implied by long.
> + enum tegra_io_pad_voltage pad_volt;
> + int ret;
> +
> + rinfo = container_of(nb, struct tegra_io_pads_regulator_info,
> + regulator_nb);
> +
> + switch (event) {
> + case REGULATOR_EVENT_PRE_VOLTAGE_CHANGE:
> + vdata = data;
> +
> + if (!tegra_io_voltage_is_valid(vdata->old_uV) ||
> + !tegra_io_voltage_is_valid(vdata->min_uV)) {
> + dev_err(rinfo->tiopi->dev,
> + "IO rail %s voltage is not 1.8/3.3V: %lu:%lu\n",
> + rinfo->cfg->name, vdata->old_uV, vdata->min_uV);
> + return -EINVAL;
> + }
> +
> + /**
> + * Change IO pad voltage before changing IO voltage when it
> + * changes from 1.8V to 3.3V
> + */
> + if (vdata->min_uV == TEGRA_IO_RAIL_1800000UV)
> + break;
> +
> + ret = tegra_io_pad_set_voltage(rinfo->cfg->id,
> + TEGRA_IO_PAD_3300000UV);
> + if (ret < 0) {
> + dev_err(rinfo->tiopi->dev,
> + "Failed to set voltage %lu of pad %s: %d\n",
> + vdata->min_uV, rinfo->cfg->name, ret);
> + return ret;
> + }
> + break;
> +
> + case REGULATOR_EVENT_VOLTAGE_CHANGE:
> + io_volt_uv = (unsigned long)data;
> + ret = tegra_io_pad_get_voltage(rinfo->cfg->id);
> + if (ret < 0) {
> + dev_err(rinfo->tiopi->dev,
> + "Failed to get IO pad voltage: %d\n", ret);
> + return ret;
> + }
Might be worth reassigning ret to a variable of type enum
tegra_io_pad_voltage because...
> +
> + if (!tegra_io_voltage_is_valid(io_volt_uv)) {
> + dev_err(rinfo->tiopi->dev,
> + "IO rail %s voltage is not 1.8/3.3V: %lu\n",
> + rinfo->cfg->name, io_volt_uv);
> + return -EINVAL;
> + }
> +
> + /*
> + * If IO pad configuration matching with IO rail voltage then
> + * do nothing.
> + */
> + if (((io_volt_uv == TEGRA_IO_RAIL_1800000UV) &&
> + (ret == TEGRA_IO_PAD_1800000UV)) ||
> + ((io_volt_uv == TEGRA_IO_RAIL_3300000UV) &&
> + (ret == TEGRA_IO_PAD_3300000UV)))
> + break;
... if somebody ever inserted code between the above and this, they
might be overwriting ret.
> +
> + ret = tegra_io_pad_set_voltage(rinfo->cfg->id,
> + TEGRA_IO_PAD_1800000UV);
> + if (ret < 0) {
> + dev_err(rinfo->tiopi->dev,
> + "Failed to set voltage %lu of pad %s: %d\n",
> + vdata->min_uV, rinfo->cfg->name, ret);
You might want to add the units of the voltage to avoid confusion. Same
in a couple more messages above and below.
> + return ret;
> + }
> + break;
> +
> + case REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE:
> + io_volt_uv = (unsigned long)data;
> +
> + if (!tegra_io_voltage_is_valid(io_volt_uv)) {
> + dev_err(rinfo->tiopi->dev,
> + "IO rail %s voltage is not 1.8/3.3V: %lu\n",
> + rinfo->cfg->name, io_volt_uv);
> + return -EINVAL;
> + }
> +
> + pad_volt = tegra_io_uv_to_io_pads_uv(io_volt_uv);
> + ret = tegra_io_pad_set_voltage(rinfo->cfg->id, pad_volt);
> + if (ret < 0) {
> + dev_err(rinfo->tiopi->dev,
> + "Failed to set voltage %lu of pad %s: %d\n",
> + io_volt_uv, rinfo->cfg->name, ret);
> + return ret;
> + }
> + break;
> +
> + default:
> + break;
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static int tegra_io_pads_pinctrl_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct platform_device_id *id = platform_get_device_id(pdev);
> + const struct tegra_io_pads_soc_data *soc_data =
> + (const struct tegra_io_pads_soc_data *)id->driver_data;
> + struct tegra_io_pads_info *tiopi;
> + int ret, i;
> +
> + if (!pdev->dev.parent->of_node) {
> + dev_err(dev, "PMC should be register from DT\n");
> + return -ENODEV;
> + }
> +
> + tiopi = devm_kzalloc(dev, sizeof(*tiopi), GFP_KERNEL);
> + if (!tiopi)
> + return -ENOMEM;
> +
> + tiopi->dev = &pdev->dev;
> + pdev->dev.of_node = pdev->dev.parent->of_node;
> + tiopi->soc_data = soc_data;
> +
> + for (i = 0; i < soc_data->num_cfg; ++i) {
> + struct tegra_io_pads_regulator_info *rinfo;
> + enum tegra_io_pad_voltage pad_volt;
> + int io_volt_uv;
> +
> + if (!soc_data->cfg[i].vsupply)
> + continue;
> +
> + rinfo = devm_kzalloc(dev, sizeof(*rinfo), GFP_KERNEL);
> + if (!rinfo)
> + return -ENOMEM;
> +
> + rinfo->tiopi = tiopi;
> + rinfo->cfg = &soc_data->cfg[i];
> +
> + rinfo->regulator = devm_regulator_get(dev,
> + soc_data->cfg[i].vsupply);
> + if (IS_ERR(rinfo->regulator)) {
> + ret = PTR_ERR(rinfo->regulator);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> + continue;
> + }
> +
> + io_volt_uv = regulator_get_voltage(rinfo->regulator);
> + if (io_volt_uv < 0) {
> + dev_warn(dev, "Failed to get voltage for rail %s: %d\n",
> + soc_data->cfg[i].vsupply, io_volt_uv);
> + continue;
> + }
> +
> + if (!tegra_io_voltage_is_valid(io_volt_uv)) {
> + dev_warn(dev, "IO rail %s voltage is not 1.8/3.3V: %d\n",
> + soc_data->cfg[i].vsupply, io_volt_uv);
> + continue;
> + }
> +
> + pad_volt = tegra_io_uv_to_io_pads_uv(io_volt_uv);
> + ret = tegra_io_pad_set_voltage(soc_data->cfg[i].id, pad_volt);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set voltage %d of pad %s: %d\n",
> + io_volt_uv, soc_data->cfg[i].name, ret);
> + return ret;
> + }
> +
> + rinfo->regulator_nb.notifier_call =
> + tegra_io_pads_rail_change_notify_cb;
> + ret = devm_regulator_register_notifier(rinfo->regulator,
> + &rinfo->regulator_nb);
> + if (ret < 0) {
> + dev_err(dev, "Failed to register regulator %s notifier: %d\n",
> + soc_data->cfg[i].name, ret);
> + return ret;
> + }
> + }
> +
> + tegra_io_pads_pinctrl_desc.pins = tiopi->soc_data->desc;
> + tegra_io_pads_pinctrl_desc.npins = tiopi->soc_data->num_desc;
This modifies global data. Can we avoid that? I think the easiest would
be to make tegra_io_pads_pinctrl_desc a field of the tegra_io_pads_info
struct.
> + platform_set_drvdata(pdev, tiopi);
> +
> + tiopi->pctl = devm_pinctrl_register(dev, &tegra_io_pads_pinctrl_desc,
> + tiopi);
> + if (IS_ERR(tiopi->pctl)) {
> + ret = PTR_ERR(tiopi->pctl);
> + dev_err(dev, "Failed to register io-pad pinctrl driver: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +#define TEGRA124_PAD_INFO_TABLE(_entry_) \
> + _entry_(0, "audio", AUDIO, true, NULL), \
> + _entry_(1, "bb", BB, true, NULL), \
> + _entry_(2, "cam", CAM, true, NULL), \
> + _entry_(3, "comp", COMP, true, NULL), \
> + _entry_(4, "csia", CSIA, true, NULL), \
> + _entry_(5, "csib", CSIB, true, NULL), \
> + _entry_(6, "csie", CSIE, true, NULL), \
> + _entry_(7, "dsi", DSI, true, NULL), \
> + _entry_(8, "dsib", DSIB, true, NULL), \
> + _entry_(9, "dsic", DSIC, true, NULL), \
> + _entry_(10, "dsid", DSID, true, NULL), \
> + _entry_(11, "hdmi", HDMI, true, NULL), \
> + _entry_(12, "hsic", HSIC, true, NULL), \
> + _entry_(13, "hv", HV, true, NULL), \
> + _entry_(14, "lvds", LVDS, true, NULL), \
> + _entry_(15, "mipi-bias", MIPI_BIAS, true, NULL), \
> + _entry_(16, "nand", NAND, true, NULL), \
> + _entry_(17, "pex-bias", PEX_BIAS, true, NULL), \
> + _entry_(18, "pex-clk1", PEX_CLK1, true, NULL), \
> + _entry_(19, "pex-clk2", PEX_CLK2, true, NULL), \
> + _entry_(20, "pex-ctrl", PEX_CNTRL, true, NULL), \
> + _entry_(21, "sdmmc1", SDMMC1, true, NULL), \
> + _entry_(22, "sdmmc3", SDMMC3, true, NULL), \
> + _entry_(23, "sdmmc4", SDMMC4, true, NULL), \
> + _entry_(24, "sys-ddc", SYS_DDC, true, NULL), \
> + _entry_(25, "uart", UART, true, NULL), \
> + _entry_(26, "usb0", USB0, true, NULL), \
> + _entry_(27, "usb1", USB1, true, NULL), \
> + _entry_(28, "usb2", USB2, true, NULL), \
> + _entry_(29, "usb-bias", USB_BIAS, true, NULL)
> +
> +#define TEGRA210_PAD_INFO_TABLE(_entry_) \
> + _entry_(0, "audio", AUDIO, true, "vddio-audio"), \
> + _entry_(1, "audio-hv", AUDIO_HV, true, "vddio-audio-hv"), \
> + _entry_(2, "cam", CAM, true, "vddio-cam"), \
> + _entry_(3, "csia", CSIA, true, NULL), \
> + _entry_(4, "csib", CSIB, true, NULL), \
> + _entry_(5, "csic", CSIC, true, NULL), \
> + _entry_(6, "csid", CSID, true, NULL), \
> + _entry_(7, "csie", CSIE, true, NULL), \
> + _entry_(8, "csif", CSIF, true, NULL), \
> + _entry_(9, "dbg", DBG, true, "vddio-dbg"), \
> + _entry_(10, "debug-nonao", DEBUG_NONAO, true, NULL), \
> + _entry_(11, "dmic", DMIC, true, "vddio-dmic"), \
> + _entry_(12, "dp", DP, true, NULL), \
> + _entry_(13, "dsi", DSI, true, NULL), \
> + _entry_(14, "dsib", DSIB, true, NULL), \
> + _entry_(15, "dsic", DSIC, true, NULL), \
> + _entry_(16, "dsid", DSID, true, NULL), \
> + _entry_(17, "emmc", SDMMC4, true, NULL), \
> + _entry_(18, "emmc2", EMMC2, true, NULL), \
> + _entry_(19, "gpio", GPIO, true, "vddio-gpio"), \
> + _entry_(20, "hdmi", HDMI, true, NULL), \
> + _entry_(21, "hsic", HSIC, true, NULL), \
> + _entry_(22, "lvds", LVDS, true, NULL), \
> + _entry_(23, "mipi-bias", MIPI_BIAS, true, NULL), \
> + _entry_(24, "pex-bias", PEX_BIAS, true, NULL), \
> + _entry_(25, "pex-clk1", PEX_CLK1, true, NULL), \
> + _entry_(26, "pex-clk2", PEX_CLK2, true, NULL), \
> + _entry_(27, "pex-ctrl", PEX_CNTRL, false, "vddio-pex-ctrl"), \
> + _entry_(28, "sdmmc1", SDMMC1, true, "vddio-sdmmc1"), \
> + _entry_(29, "sdmmc3", SDMMC3, true, "vddio-sdmmc3"), \
> + _entry_(30, "spi", SPI, true, "vddio-spi"), \
> + _entry_(31, "spi-hv", SPI_HV, true, "vddio-spi-hv"), \
> + _entry_(32, "uart", UART, true, "vddio-uart"), \
> + _entry_(33, "usb0", USB0, true, NULL), \
> + _entry_(34, "usb1", USB1, true, NULL), \
> + _entry_(35, "usb2", USB2, true, NULL), \
> + _entry_(36, "usb3", USB3, true, NULL), \
> + _entry_(37, "usb-bias", USB_BIAS, true, NULL)
> +
> +#define TEGRA_IO_PAD_INFO(_pin, _name, _id, _lpstate, _vsupply) \
> + { \
> + .name = _name, \
> + .pins = {(_pin)}, \
> + .id = TEGRA_IO_PAD_##_id, \
> + .vsupply = (_vsupply), \
> + .supports_low_power = (_lpstate), \
> + }
> +
> +static const struct tegra_io_pads_cfg tegra124_io_pads_cfg_info[] = {
> + TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
> +};
> +
> +static const struct tegra_io_pads_cfg tegra210_io_pads_cfg_info[] = {
> + TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
> +};
That's a weird way of writing these tables. Why not do something simpler
and much more common such as:
#define TEGRA_IO_PAD_INFO(...) ...
static const struct tegra_io_pads_cfg tegra124_io_pads_cfgs[] = {
TEGRA_IO_PAD_INFO(...),
...
};
static const struct tegra_io_pads_cfg tegra210_io_pad_cfgs[] = {
TEGRA_IO_PAD_INFO(...),
...
};
> +
> +#define TEGRA_IO_PAD_DESC(_pin, _name, _id, _lpstate, _vsupply) \
> + PINCTRL_PIN(_pin, _name)
> +
> +static const struct pinctrl_pin_desc tegra124_io_pads_pinctrl_desc[] = {
> + TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_DESC),
> +};
> +
> +static const struct pinctrl_pin_desc tegra210_io_pads_pinctrl_desc[] = {
> + TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_DESC),
> +};
> +
> +static const struct tegra_io_pads_soc_data tegra124_io_pad_soc_data = {
> + .desc = tegra124_io_pads_pinctrl_desc,
> + .num_desc = ARRAY_SIZE(tegra124_io_pads_pinctrl_desc),
> + .cfg = tegra124_io_pads_cfg_info,
> + .num_cfg = ARRAY_SIZE(tegra124_io_pads_cfg_info),
> +};
> +
> +static const struct tegra_io_pads_soc_data tegra210_io_pad_soc_data = {
> + .desc = tegra210_io_pads_pinctrl_desc,
> + .num_desc = ARRAY_SIZE(tegra210_io_pads_pinctrl_desc),
> + .cfg = tegra210_io_pads_cfg_info,
> + .num_cfg = ARRAY_SIZE(tegra210_io_pads_cfg_info),
> +};
> +
> +static const struct platform_device_id tegra_io_pads_dev_id[] = {
> + {
> + .name = "pinctrl-t124-io-pad",
> + .driver_data = (kernel_ulong_t)&tegra124_io_pad_soc_data,
> + }, {
> + .name = "pinctrl-t210-io-pad",
> + .driver_data = (kernel_ulong_t)&tegra210_io_pad_soc_data,
> + }, {
> + },
> +};
> +MODULE_DEVICE_TABLE(platform, tegra_io_pads_dev_id);
> +
> +static struct platform_driver tegra_io_pads_pinctrl_driver = {
> + .driver = {
> + .name = "pinctrl-tegra-io-pad",
> + },
> + .probe = tegra_io_pads_pinctrl_probe,
> + .id_table = tegra_io_pads_dev_id,
> +};
> +
> +module_platform_driver(tegra_io_pads_pinctrl_driver);
> +
> +MODULE_DESCRIPTION("NVIDIA TEGRA IO pad Control Driver");
> +MODULE_AUTHOR("Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
Like I said above, I think there's a lot of boilerplate in here that's
simply there to create a virtual device and bind a driver to it. All of
that comes with very little to no benefit. I think this could all just
be moved into the PMC driver and be simplified quite a bit.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 04/10] power: supply: axp20x_usb_power: add 100mA max current limit for AXP223
From: Chen-Yu Tsai @ 2016-11-25 9:56 UTC (permalink / raw)
To: Quentin Schulz
Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Chen-Yu Tsai,
Russell King, Maxime Ripard, Lee Jones, open list:THERMAL,
devicetree, linux-kernel, linux-arm-kernel, Thomas Petazzoni
In-Reply-To: <20161125090921.23138-5-quentin.schulz@free-electrons.com>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> The X-Powers AXP223 shares most of its behaviour with the AXP221 PMIC
> but allows the VBUS power supply max current to be set to 100mA (like
> the AXP209 PMIC).
>
> This basically adds a new compatible to the VBUS power supply driver and
> adds a check on the compatible when setting current max limit.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
^ permalink raw reply
* Re: [RFC PATCH net v2 0/3] Fix OdroidC2 Gigabit Tx link issue
From: Jerome Brunet @ 2016-11-25 9:55 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
Florian Fainelli, Carlo Caione, Kevin Hilman, Giuseppe Cavallaro,
Alexandre TORGUE, Andre Roth, Neil Armstrong,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAFBinCCg8cLuQdrRsxHA53JAvyWgfW7vfVMho0tj3bCCcu-YxA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, 2016-11-24 at 18:10 +0100, Martin Blumenstingl wrote:
> On Thu, Nov 24, 2016 at 5:01 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> wrote:
> >
> > On Thu, 2016-11-24 at 15:40 +0100, Martin Blumenstingl wrote:
> > >
> > > Hi Jerome,
> > >
> > > On Mon, Nov 21, 2016 at 4:35 PM, Jerome Brunet <jbrunet@baylibre.
> > > com>
> > > wrote:
> > > >
> > > >
> > > > This patchset fixes an issue with the OdroidC2 board (DWMAC +
> > > > RTL8211F).
> > > > Initially reported as a low Tx throughput issue at gigabit
> > > > speed,
> > > > the
> > > > platform enters LPI too often. This eventually break the link
> > > > (both
> > > > Tx
> > > > and Rx), and require to bring the interface down and up again
> > > > to
> > > > get the
> > > > Rx path working again.
> > > >
> > > > The root cause of this issue is not fully understood yet but
> > > > disabling EEE
> > > > advertisement on the PHY prevent this feature to be negotiated.
> > > > With this change, the link is stable and reliable, with the
> > > > expected
> > > > throughput performance.
> > > I have just sent a series which allows configuring the TX delay
> > > on
> > > the
> > > MAC (dwmac-meson8b glue) side: [0]
> > > Disabling the TX delay generated by the MAC fixes TX throughput
> > > for
> > > me, even when leaving EEE enabled in the RTL8211F PHY driver!
> > >
> > > Unfortunately the RTL8211F PHY is a black-box for the community
> > > because there is no public datasheeet available.
> > > *maybe* (pure speculation!) they're enabling the TX delay based
> > > on
> > > some internal magic only when EEE is enabled.
> >
> > Hi already tried acting on the register setting the TX_delay. I
> > also
> > tried on the PHY. I never been able to improve situation on the
> > Odroic2. Only disabling EEE improved the situation.
> OK, thanks for clarifying this!
>
> >
> > To make sure, i tried again with your patch but the result remains
> > unchanged. With Tx_delay disabled (either the mac or the phy), the
> > situation is even worse, it seems that nothing gets through
> This is interesting, because in your case you should have a 4ns TX
> delay (2ns from the MAC and presumably 2ns from the PHY).
> Maybe that is also the reason why the TX delay is configurable in 2ns
> steps in PRG_ETHERNET0 on Amlogic SoCs.
>
> out of curiosity: have you tried setting a 4ns (half clock-cycle) TX
> delay for the MAC and disabling it in the PHY?
Just replied on the other thread. Long story short, Odroidc2 seems to
really require EEE to be switched off.
Again, thx for your help Martin
>
>
> Regards,
> Martin
--
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: [net-next PATCH v1 0/2] stmmac: dwmac-meson8b: configurable RGMII TX delay
From: Jerome Brunet @ 2016-11-25 9:53 UTC (permalink / raw)
To: Martin Blumenstingl, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, khilman-rdvid1DuHRBWk0Htik3J/w,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
alexandre.torgue-qxv4g6HH51o, peppe.cavallaro-qxv4g6HH51o,
carlo-KA+7E9HrN00dnm+yROfE0A
In-Reply-To: <CAFBinCB7sXjXor++W+PW0-j_VxATRzhexjqHgXj2jD10tBpZFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, 2016-11-24 at 18:05 +0100, Martin Blumenstingl wrote:
> On Thu, Nov 24, 2016 at 4:56 PM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> wrote:
> >
> > On Thu, 2016-11-24 at 15:34 +0100, Martin Blumenstingl wrote:
> > >
> > > Currently the dwmac-meson8b stmmac glue driver uses a hardcoded
> > > 1/4
> > > cycle TX clock delay. This seems to work fine for many boards
> > > (for
> > > example Odroid-C2 or Amlogic's reference boards) but there are
> > > some
> > > others where TX traffic is simply broken.
> > > There are probably multiple reasons why it's working on some
> > > boards
> > > while it's broken on others:
> > > - some of Amlogic's reference boards are using a Micrel PHY
> > > - hardware circuit design
> > > - maybe more...
> > >
> > > This raises a question though:
> > > Which device is supposed to enable the TX delay when both MAC and
> > > PHY
> > > support it? And should we implement it for each PHY / MAC
> > > separately
> > > or should we think about a more generic solution (currently it's
> > > not
> > > possible to disable the TX delay generated by the RTL8211F PHY
> > > via
> > > devicetree when using phy-mode "rgmii")?
> >
> > Actually you can skip the part which activate the Tx-delay on the
> > phy
> > by setting "phy-mode = "rgmii-id" instead of "rgmii"
> >
> > phy->interface will no longer be PHY_INTERFACE_MODE_RGMII
> > but PHY_INTERFACE_MODE_RGMII_ID.
> unfortunately this is not true for RTL8211F (I did my previous tests
> with the same expectation in mind)!
> the code seems to suggest that TX-delay is disabled whenever mode !=
> PHY_INTERFACE_MODE_RGMII.
> BUT: on my device RTL8211F_TX_DELAY is set even before
> "phy_write(phydev, 0x11, reg);"!
Thx a lot for pointing this out. I wrongly assumed that is was off by
default. It turns out it is ON on the OdroidC2 as well.
So I re-did all my test:
On the OdroidC2, as long as eee is not disabled (using ethtool or
through my patch) I can't get iperf to work properly. Disabling EEE,
the link works for the following configurations:
PHY TX Delay bit set : MAC delay 0ns, 2ns.
PHY TX Delay bit cleared : MAC delay 2ns, 4ns.
This seems to confirm that the PHY adds a 2ns delay when the bit is
set.
I could not test on my MXQPRO2.1 ... the PHY is no longer found during
the boot. Something died overnight :(
>
> Based on what I found it seems that rgmii-id, rgmii-txid and
> rgmii-rxid are supposed to be handled by the PHY.
> That would mean that we have two problems here:
> 1) drivers/net/phy/realtek.c:rtl8211f_config_init should check for
> PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_TXID and
> enable the TX-delay in that case - otherwise explicitly disable it
> 2) dwmac-meson8b.c should only use the configured TX-delay for
> PHY_INTERFACE_MODE_RGMII
> @Florian: could you please share your thoughts on this (who handles
> the TX delay in which case)?
>
>
> Regards,
> Martin
--
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 02/10] power: supply: axp20x_usb_power: set min voltage and max current from sysfs
From: Thomas Petazzoni @ 2016-11-25 9:48 UTC (permalink / raw)
To: Quentin Schulz
Cc: mark.rutland, devicetree, linux-pm, linux-kernel, sre, linux,
wens, robh+dt, maxime.ripard, lee.jones, linux-arm-kernel
In-Reply-To: <20161125090921.23138-3-quentin.schulz@free-electrons.com>
Quentin,
On Fri, 25 Nov 2016 10:09:13 +0100, Quentin Schulz wrote:
> +static int axp20x_usb_power_set_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> + int ret, val1;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> + switch (val->intval) {
This nested switch construct doesn't look very pretty. What about
instead:
switch(psp) {
case POWER_SUPPLY_PROP_VOLTAGE_MIN:
return axp20x_usb_power_set_prop_voltage_min(...);
case POWER_SUPPLY_PROP_CURRENT_MAX:
return axp20x_usb_power_set_prop_current_max(...);
default:
return -EINVAL;
}
> + case 4000000:
> + case 4100000:
> + case 4200000:
> + case 4300000:
> + case 4400000:
> + case 4500000:
> + case 4600000:
> + case 4700000:
> + val1 = (val->intval - 4000000) / 100000;
> + ret = regmap_update_bits(power->regmap,
> + AXP20X_VBUS_IPSOUT_MGMT,
> + AXP20X_VBUS_VHOLD_MASK,
> + val1 << 3);
> + if (ret)
> + return ret;
> +
> + return 0;
Just do:
return regmap_update_bits(...);
The dance to test ret is useless, since you anyway return ret when
non-zero, or return zero when ret was zero.
While you're at it, maybe make val1 a u32, since I guess it's written
to a 32-bit register (unless u32 is not commonly used in this driver
already, of course).
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 02/10] power: supply: axp20x_usb_power: set min voltage and max current from sysfs
From: Chen-Yu Tsai @ 2016-11-25 9:42 UTC (permalink / raw)
To: Quentin Schulz
Cc: Mark Rutland, devicetree, open list:THERMAL, linux-kernel,
Sebastian Reichel, Russell King, Chen-Yu Tsai, Rob Herring,
Maxime Ripard, Lee Jones, Thomas Petazzoni, linux-arm-kernel
In-Reply-To: <20161125090921.23138-3-quentin.schulz@free-electrons.com>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> AXP20X and AXP22X PMICs allow setting the min voltage and max current of
> VBUS power supply. This adds entries in sysfs to allow to do so.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> drivers/power/supply/axp20x_usb_power.c | 72 +++++++++++++++++++++++++++++++++
> include/linux/mfd/axp20x.h | 3 ++
> 2 files changed, 75 insertions(+)
>
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index b19754e..638cb52 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -155,6 +155,74 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
> return 0;
> }
>
> +static int axp20x_usb_power_set_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> + int ret, val1;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> + switch (val->intval) {
> + case 4000000:
> + case 4100000:
> + case 4200000:
> + case 4300000:
> + case 4400000:
> + case 4500000:
> + case 4600000:
> + case 4700000:
> + val1 = (val->intval - 4000000) / 100000;
> + ret = regmap_update_bits(power->regmap,
> + AXP20X_VBUS_IPSOUT_MGMT,
> + AXP20X_VBUS_VHOLD_MASK,
> + val1 << 3);
Please also define the shift offset.
> + if (ret)
> + return ret;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +
> + case POWER_SUPPLY_PROP_CURRENT_MAX:
> + switch (val->intval) {
> + case 100000:
> + if (power->axp20x_id == AXP221_ID)
> + return -EINVAL;
> + case 500000:
> + case 900000:
> + val1 = (900000 - val->intval) / 400000;
> + ret = regmap_update_bits(power->regmap,
> + AXP20X_VBUS_IPSOUT_MGMT,
> + AXP20X_VBUS_CLIMIT_MASK, val1);
> + if (ret)
> + return ret;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
> + enum power_supply_property psp)
> +{
> + return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
> + psp == POWER_SUPPLY_PROP_CURRENT_MAX;
> +}
> +
> static enum power_supply_property axp20x_usb_power_properties[] = {
> POWER_SUPPLY_PROP_HEALTH,
> POWER_SUPPLY_PROP_PRESENT,
> @@ -178,7 +246,9 @@ static const struct power_supply_desc axp20x_usb_power_desc = {
> .type = POWER_SUPPLY_TYPE_USB,
> .properties = axp20x_usb_power_properties,
> .num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
> + .property_is_writeable = axp20x_usb_power_prop_writeable,
> .get_property = axp20x_usb_power_get_property,
> + .set_property = axp20x_usb_power_set_property,
> };
>
> static const struct power_supply_desc axp22x_usb_power_desc = {
> @@ -186,7 +256,9 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
> .type = POWER_SUPPLY_TYPE_USB,
> .properties = axp22x_usb_power_properties,
> .num_properties = ARRAY_SIZE(axp22x_usb_power_properties),
> + .property_is_writeable = axp20x_usb_power_prop_writeable,
> .get_property = axp20x_usb_power_get_property,
> + .set_property = axp20x_usb_power_set_property,
> };
>
> static const struct of_device_id axp20x_usb_power_match[] = {
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index fec597f..8883595 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -56,6 +56,9 @@ enum {
> #define AXP20X_LDO24_V_OUT 0x28
> #define AXP20X_LDO3_V_OUT 0x29
> #define AXP20X_VBUS_IPSOUT_MGMT 0x30
> +
> +#define AXP20X_VBUS_VHOLD_MASK GENMASK(5, 3)
> +
For the AXP drivers, we keep the per register bit field
definitions in the drivers that use them.
Regards
ChenYu
> #define AXP20X_V_OFF 0x31
> #define AXP20X_OFF_CTRL 0x32
> #define AXP20X_CHRG_CTRL1 0x33
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH v6 3/5] ARM: dts: sun8i-h3: add HDMI video nodes
From: Icenowy Zheng @ 2016-11-25 9:41 UTC (permalink / raw)
To: Jean-Francois Moine, Dave Airlie, Maxime Ripard, Rob Herring
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
In-Reply-To: <cdf50b8433e2d9a053e35f8788bfcd5d41504312.1479641523.git.moinejf-GANU6spQydw@public.gmane.org>
20.11.2016, 20:12, "Jean-Francois Moine" <moinejf@free.fr>:
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
> arch/arm/boot/dts/sun8i-h3.dtsi | 51 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 416b825..7c6b1d5 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -140,6 +140,16 @@
> #size-cells = <1>;
> ranges;
>
> + de: de-controller@01000000 {
> + compatible = "allwinner,sun8i-h3-display-engine";
> + reg = <0x01000000 0x400000>;
> + clocks = <&ccu CLK_BUS_DE>, <&ccu CLK_DE>;
> + clock-names = "bus", "clock";
> + resets = <&ccu RST_BUS_DE>;
> + ports = <&lcd0_p>;
> + status = "disabled";
> + };
> +
> dma: dma-controller@01c02000 {
> compatible = "allwinner,sun8i-h3-dma";
> reg = <0x01c02000 0x1000>;
> @@ -149,6 +159,23 @@
> #dma-cells = <1>;
> };
>
> + lcd0: lcd-controller@01c0c000 {
> + compatible = "allwinner,sun8i-a83t-tcon";
> + reg = <0x01c0c000 0x400>;
> + clocks = <&ccu CLK_BUS_TCON0>, <&ccu CLK_TCON0>;
> + clock-names = "bus", "clock";
> + resets = <&ccu RST_BUS_TCON0>;
> + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + lcd0_p: port {
> + lcd0_hdmi: endpoint {
> + remote-endpoint = <&hdmi_lcd0>;
> + };
> + };
> + };
> +
> mmc0: mmc@01c0f000 {
> compatible = "allwinner,sun7i-a20-mmc";
> reg = <0x01c0f000 0x1000>;
> @@ -314,6 +341,11 @@
> clock-names = "hosc", "losc";
> #clock-cells = <1>;
> #reset-cells = <1>;
> +
> + assigned-clocks = <&ccu CLK_PLL_DE>,
> + <&ccu CLK_DE>;
> + assigned-clock-rates = <864000000>,
> + <432000000>;
> };
>
> pio: pinctrl@01c20800 {
> @@ -564,6 +596,25 @@
> interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> };
>
> + hdmi: hdmi@01ee0000 {
> + compatible = "allwinner,sun8i-h3-hdmi";
> + reg = <0x01ee0000 0x20000>;
> + clocks = <&ccu CLK_BUS_HDMI>, <&ccu CLK_HDMI>,
> + <&ccu CLK_HDMI_DDC>;
> + clock-names = "bus", "clock", "ddc-clock";
> + resets = <&ccu RST_BUS_HDMI0>, <&ccu RST_BUS_HDMI1>;
> + reset-names = "hdmi0", "hdmi1";
> + status = "disabled";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 { /* video */
> + reg = <0>;
> + hdmi_lcd0: endpoint {
> + remote-endpoint = <&lcd0_hdmi>;
> + };
> + };
> + };
> +
> rtc: rtc@01f00000 {
> compatible = "allwinner,sun6i-a31-rtc";
> reg = <0x01f00000 0x54>;
After removing CLK_PLL_DE's assigned-clock, the kernel passes compilation.
However, it cannot recognize any HDMI screen...
(My board is Orange Pi One, and I manually added status="okay"; to &lcd0, &de, &hdmi)
[ 16.507802] sun8i-de2 1000000.de-controller: bound 1c0c000.lcd-controller (ops de2_lcd_ops [sun8i_de2_drm])
[ 16.675948] sun8i-de2 1000000.de-controller: bound 1ee0000.hdmi (ops de2_hdmi_fini [sun8i_de2_hdmi])
[ 16.685120] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[ 16.695876] [drm] No driver support for vblank timestamp query.
[ 16.701862] sun8i-de2 1000000.de-controller: No connectors reported connected with modes
[ 16.713061] [drm] Cannot find any crtc or sizes - going 1024x768
[ 16.734214] Console: switching to colour frame buffer device 128x48
[ 16.751022] sun8i-de2 1000000.de-controller: fb0: frame buffer device
> --
> 2.10.2
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH 03/10] Documentation: DT: binding: axp20x_usb_power: add axp223 compatible
From: Chen-Yu Tsai @ 2016-11-25 9:25 UTC (permalink / raw)
To: Quentin Schulz
Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Chen-Yu Tsai,
Russell King, Maxime Ripard, Lee Jones, open list:THERMAL,
devicetree, linux-kernel, linux-arm-kernel, Thomas Petazzoni
In-Reply-To: <20161125090921.23138-4-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Fri, Nov 25, 2016 at 5:09 PM, Quentin Schulz
<quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> This adds the "x-powers,axp223-usb-power-supply" to the list of
> compatibles for AXP20X VBUS power supply driver.
Please explain why we need this. Basically the part about
AXP221 vs AXP223 from your cover letter.
ChenYu
>
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> Documentation/devicetree/bindings/power/supply/axp20x_usb_power.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/axp20x_usb_power.txt b/Documentation/devicetree/bindings/power/supply/axp20x_usb_power.txt
> index f1d7bee..bf3953c 100644
> --- a/Documentation/devicetree/bindings/power/supply/axp20x_usb_power.txt
> +++ b/Documentation/devicetree/bindings/power/supply/axp20x_usb_power.txt
> @@ -3,6 +3,7 @@ AXP20x USB power supply
> Required Properties:
> -compatible: One of: "x-powers,axp202-usb-power-supply"
> "x-powers,axp221-usb-power-supply"
> + "x-powers,axp223-usb-power-supply"
>
> This node is a subnode of the axp20x PMIC.
>
> --
> 2.9.3
>
--
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] dt-bindings: clarify compatible field usage for rockchip timers
From: Alexander Kochetkov @ 2016-11-25 9:23 UTC (permalink / raw)
To: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
Caesar Wang, daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A,
huangtao-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480025455-6797-1-git-send-email-al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hello,
Here the thread where the patch was discussed:
http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013174.html
Regards,
Alexander.
--
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
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