LinuxLists.cc - [PATCH 0/4] Device Tree Quirks & the Beaglebone

[permalink] [raw]

Subject: [PATCH 1/4] arm: of: Add a DT quirk method after unflattening

We need a per-machine dt_quirk() method to allow us to perform
device tree quirk processing after unflattening.

Signed-off-by: Pantelis Antoniou <[email protected]>
---
arch/arm/include/asm/mach/arch.h | 1 +
arch/arm/kernel/setup.c | 3 +++
2 files changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 0406cb3..e873434 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -51,6 +51,7 @@ struct machine_desc {
bool (*smp_init)(void);
void (*fixup)(struct tag *, char **);
void (*dt_fixup)(void);
+ void (*dt_quirk)(void);
void (*init_meminfo)(void);
void (*reserve)(void);/* reserve mem blocks */
void (*map_io)(void);/* IO mapping function */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index e55408e..e4c6eb5 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -934,6 +934,9 @@ void __init setup_arch(char **cmdline_p)

unflatten_device_tree();

+ if (mdesc->dt_quirk)
+ mdesc->dt_quirk();
+
arm_dt_init_cpu_maps();
psci_init();
#ifdef CONFIG_SMP
--
1.7.12

2015-02-18 15:01:26

[permalink] [raw]

Subject: [PATCH 2/4] of: DT quirks infrastructure

Implement a method of applying DT quirks early in the boot sequence.

A DT quirk is a subtree of the boot DT that can be applied to
a target in the base DT resulting in a modification of the live
tree. The format of the quirk nodes is that of a device tree overlay.

For details please refer to Documentation/devicetree/quirks.txt

Signed-off-by: Pantelis Antoniou <[email protected]>
---
Documentation/devicetree/quirks.txt | 101 ++++++++++
drivers/of/dynamic.c | 358 ++++++++++++++++++++++++++++++++++++
include/linux/of.h | 16 ++
3 files changed, 475 insertions(+)
create mode 100644 Documentation/devicetree/quirks.txt

diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
new file mode 100644
index 0000000..789319a
--- /dev/null
+++ b/Documentation/devicetree/quirks.txt
@@ -0,0 +1,101 @@
+A Device Tree quirk is the way which allows modification of the
+boot device tree under the control of a per-platform specific method.
+
+Take for instance the case of a board family that comprises of a
+number of different board revisions, all being incremental changes
+after an initial release.
+
+Since all board revisions must be supported via a single software image
+the only way to support this scheme is by having a different DTB for each
+revision with the bootloader selecting which one to use at boot time.
+
+While this may in theory work, in practice it is very cumbersome
+for the following reasons:
+
+1. The act of selecting a different boot device tree blob requires
+a reasonably advanced bootloader with some kind of configuration or
+scripting capabilities. Sadly this is not the case many times, the
+bootloader is extremely dumb and can only use a single dt blob.
+
+2. On many instances boot time is extremely critical; in some cases
+there are hard requirements like having working video feeds in under
+2 seconds from power-up. This leaves an extremely small time budget for
+boot-up, as low as 500ms to kernel entry. The sanest way to get there
+is by removing the standard bootloader from the normal boot sequence
+altogether by having a very small boot shim that loads the kernel and
+immediately jumps to kernel, like falcon-boot mode in u-boot does.
+
+3. Having different DTBs/DTSs for different board revisions easily leads to
+drift between versions. Since no developer is expected to have every single
+board revision at hand, things are easy to get out of sync, with board versions
+failing to boot even though the kernel is up to date.
+
+4. One final problem is the static way that device tree works.
+For example it might be desirable for various boards to have a way to
+selectively configure the boot device tree, possibly by the use of command
+line options. For instance a device might be disabled if a given command line
+option is present, different configuration to various devices for debugging
+purposes can be selected and so on. Currently the only way to do so is by
+recompiling the DTS and installing, which is an chore for developers and
+a completely unreasonable expectation from end-users.
+
+Device Tree quirks solve all those problems by having an in-kernel interface
+which per-board/platform method can use to selectively modify the device tree
+right after unflattening.
+
+A DT quirk is a subtree of the boot DT that can be applied to
+a target in the base DT resulting in a modification of the live
+tree. The format of the quirk nodes is that of a device tree overlay.
+
+As an example the following DTS contains a quirk.
+
+/ {
+ foo: foo-node {
+ bar = <10>;
+ };
+
+ select-quirk = <&quirk>;
+
+ quirk: quirk {
+ fragment@0 {
+ target = <&foo>;
+ __overlay {
+ bar = <0xf00>;
+ baz = <11>;
+ };
+ };
+ };
+};
+
+The quirk when applied would result at the following tree:
+
+/ {
+ foo: foo-node {
+ bar = <0xf00>;
+ baz = <11>;
+ };
+
+ select-quirk = <&quirk>;
+
+ quirk: quirk {
+ fragment@0 {
+ target = <&foo>;
+ __overlay {
+ bar = <0xf00>;
+ baz = <11>;
+ };
+ };
+ };
+
+};
+
+The two public method used to accomplish this are of_quirk_apply_by_node()
+and of_quirk_apply_by_phandle();
+
+To apply the quirk, a per-platform method can retrieve the phandle from the
+select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
+
+The method which the per-platform method is using to select the quirk to apply
+is out of the scope of the DT quirk definition, but possible methods include
+and are not limited to: revision encoding in a GPIO input range, board id
+located in external or internal EEPROM or flash, DMI board ids, etc.
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 3351ef4..d275dc7 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -7,6 +7,7 @@
*/

#include <linux/of.h>
+#include <linux/of_fdt.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/string.h>
@@ -779,3 +780,360 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
list_add_tail(&ce->node, &ocs->entries);
return 0;
}
+
+/* fixup a symbol entry for a quirk if it exists */
+static int quirk_fixup_symbol(struct device_node *dns, struct device_node *dnp)
+{
+ struct device_node *dn;
+ struct property *prop;
+ const char *names, *namep;
+ int lens, lenp;
+ char *p;
+
+ dn = of_find_node_by_path("/__symbols__");
+ if (!dn)
+ return 0;
+
+ names = of_node_full_name(dns);
+ lens = strlen(names);
+ namep = of_node_full_name(dnp);
+ lenp = strlen(namep);
+ for_each_property_of_node(dn, prop) {
+ /* be very concervative at matching */
+ if (lens == (prop->length - 1) &&
+ ((const char *)prop->value)[prop->length] == '\0' &&
+ strcmp(prop->value, names) == 0)
+ break;
+ }
+ if (prop == NULL)
+ return 0;
+ p = early_init_dt_alloc_memory_arch(lenp + 1, __alignof__(char));
+ if (!p) {
+ pr_err("%s: symbol fixup %s failed\n", __func__, prop->name);
+ return -ENOMEM;
+ }
+ strcpy(p, namep);
+
+ pr_debug("%s: symbol fixup %s: %s -> %s\n", __func__,
+ prop->name, names, namep);
+
+ prop->value = p;
+ prop->length = lenp + 1;
+
+ return 0;
+}
+
+/* create a new quirk node */
+static struct device_node *new_quirk_node(
+ struct device_node *dns,
+ struct device_node *dnt,
+ const char *name)
+{
+ struct device_node *dnp;
+ int dnlen, len, ret;
+ struct property **pp, *prop;
+ char *p;
+
+ dnlen = strlen(dnt->full_name);
+ len = dnlen + 1 + strlen(name) + 1;
+ dnp = early_init_dt_alloc_memory_arch(
+ sizeof(struct device_node) + len,
+ __alignof__(struct device_node));
+ if (dnp == NULL) {
+ pr_err("%s: allocation failure at %pO\n", __func__,
+ dns);
+ return NULL;
+ }
+ memset(dnp, 0, sizeof(*dnp));
+ of_node_init(dnp);
+ p = (char *)dnp + sizeof(*dnp);
+
+ /* build full name */
+ dnp->full_name = p;
+ memcpy(p, dnt->full_name, dnlen);
+ p += dnlen;
+ if (dnlen != 1)
+ *p++ = '/';
+ strcpy(p, name);
+
+ dnp->parent = dnt;
+
+ /* we now move the phandle properties */
+ for (pp = &dns->properties; (prop = *pp) != NULL; ) {
+
+ /* do not touch normal properties */
+ if (strcmp(prop->name, "name") &&
+ strcmp(prop->name, "phandle") &&
+ strcmp(prop->name, "linux,phandle") &&
+ strcmp(prop->name, "ibm,phandle")) {
+ pp = &(*pp)->next;
+ continue;
+ }
+
+ /* move to the new node */
+ *pp = prop->next;
+ /* don't advance */
+
+ prop->next = dnp->properties;
+ dnp->properties = prop;
+
+ if ((strcmp(prop->name, "phandle") == 0 ||
+ strcmp(prop->name, "linux,phandle") == 0 ||
+ strcmp(prop->name, "ibm,phandle") == 0) &&
+ dnp->phandle == 0) {
+ dnp->phandle = be32_to_cpup(prop->value);
+ /* remove the phandle from the source */
+ dns->phandle = 0;
+ }
+ }
+
+ dnp->name = of_get_property(dnp, "name", NULL);
+ dnp->type = of_get_property(dnp, "device_type", NULL);
+ if (!dnp->name)
+ dnp->name = "<NULL>";
+ if (!dnp->type)
+ dnp->type = "<NULL>";
+
+ ret = quirk_fixup_symbol(dns, dnp);
+ if (ret != 0)
+ pr_warn("%s: Failed to fixup symbol %pO\n", __func__, dnp);
+
+ return dnp;
+}
+
+/* apply a quirk fragment node recursively */
+static int of_apply_quirk_fragment_node(struct device_node *dn,
+ struct device_node *dnt)
+{
+ struct property *prop, *tprop, **pp;
+ struct device_node *dnp, **dnpp, *child;
+ const char *name, *namet;
+ int i, ret;
+
+ if (!dn || !dnt)
+ return -EINVAL;
+
+ /* iterate over all properties */
+ for (pp = &dn->properties; (prop = *pp) != NULL; pp = &prop->next) {
+
+ /* do not touch auto-generated properties */
+ if (!strcmp(prop->name, "name") ||
+ !strcmp(prop->name, "phandle") ||
+ !strcmp(prop->name, "linux,phandle") ||
+ !strcmp(prop->name, "ibm,phandle") ||
+ !strcmp(prop->name, "__remove_property__") ||
+ !strcmp(prop->name, "__remove_node__"))
+ continue;
+
+ pr_debug("%s: change property %s from %pO to %pO\n",
+ __func__, prop->name, dn, dnt);
+
+ tprop = of_find_property(dnt, prop->name, NULL);
+ if (tprop) {
+ tprop->value = prop->value;
+ tprop->length = prop->length;
+ continue;
+ }
+ tprop = early_init_dt_alloc_memory_arch(
+ sizeof(struct property),
+ __alignof__(struct property));
+ if (!tprop) {
+ pr_err("%s: allocation failure at %pO\n", __func__,
+ dn);
+ return -ENOMEM;
+ }
+ tprop->name = prop->name;
+ tprop->value = prop->value;
+ tprop->length = prop->length;
+
+ /* link */
+ tprop->next = dnt->properties;
+ dnt->properties = tprop;
+ }
+
+ /* now handle property removals (if any) */
+ for (i = 0; of_property_read_string_index(dn, "__remove_property__",
+ i, &name) == 0; i++) {
+
+ /* remove property directly (we don't care about dead props) */
+ for (pp = &dnt->properties; (prop = *pp) != NULL;
+ pp = &prop->next) {
+ if (!strcmp(prop->name, name)) {
+ *pp = prop->next;
+ pr_info("%s: remove property %s at %pO\n",
+ __func__, name, dnt);
+ break;
+ }
+ }
+ }
+
+ /* now handle node removals (if any) */
+ for (i = 0; of_property_read_string_index(dn, "__remove_node__",
+ i, &name) == 0; i++) {
+
+ /* remove node directly (we don't care about dead props) */
+ for (dnpp = &dnt->child; (dnp = *dnpp) != NULL;
+ dnpp = &dnp->sibling) {
+
+ /* find path component */
+ namet = strrchr(dnp->full_name, '/');
+ if (!namet) /* root */
+ namet = dnp->full_name;
+ else
+ namet++;
+ if (!strcmp(namet, name)) {
+ *dnpp = dnp->sibling;
+ pr_info("%s: remove node %s at %pO\n",
+ __func__, namet, dnt);
+ break;
+ }
+ }
+ }
+
+ /* now iterate over childen */
+ for_each_child_of_node(dn, child) {
+ /* locate path component */
+ name = strrchr(child->full_name, '/');
+ if (name == NULL) /* root? */
+ name = child->full_name;
+ else
+ name++;
+
+ /* find node (if it exists) */
+ for (dnpp = &dnt->child; (dnp = *dnpp) != NULL;
+ dnpp = &dnp->sibling) {
+
+ namet = strrchr(dnp->full_name, '/');
+ if (!namet) /* root */
+ namet = dnp->full_name;
+ else
+ namet++;
+
+ if (!strcmp(namet, name))
+ break;
+ }
+
+ /* not found, create node */
+ if (dnp == NULL) {
+ dnp = new_quirk_node(child, dnt, name);
+ if (dnp == NULL) {
+ pr_err("%s: allocation failure at %pO\n",
+ __func__, dn);
+ of_node_put(child);
+ return -ENOMEM;
+ }
+ dnp->sibling = *dnpp;
+ *dnpp = dnp;
+
+ pr_debug("%s: new node %pO\n", __func__, dnp);
+ }
+ pr_debug("%s: recursing %pO\n", __func__, dnp);
+
+ ret = of_apply_quirk_fragment_node(child, dnp);
+ if (ret != 0) {
+ of_node_put(child);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+/* apply a single quirk fragment located at dn */
+static int of_apply_single_quirk_fragment(struct device_node *dn)
+{
+ struct device_node *dnt, *dno;
+ const char *path;
+ u32 val;
+ int ret;
+
+ /* first try to go by using the target as a phandle */
+ dno = NULL;
+ dnt = NULL;
+ ret = of_property_read_u32(dn, "target", &val);
+ if (ret == 0)
+ dnt = of_find_node_by_phandle(val);
+
+ if (dnt == NULL) {
+ /* now try to locate by path */
+ ret = of_property_read_string(dn, "target-path",
+ &path);
+ if (ret == 0)
+ dnt = of_find_node_by_path(path);
+ }
+
+ if (dnt == NULL) {
+ pr_err("%s: Failed to find target for node %pO\n",
+ __func__, dn);
+ ret = -ENODEV;
+ goto out;
+ }
+
+ pr_debug("%s: Found target at %pO\n", __func__, dnt);
+ dno = of_get_child_by_name(dn, "__overlay__");
+ if (!dno) {
+ pr_err("%s: Failed to find overlay node %pO\n", __func__, dn);
+ ret = -ENODEV;
+ goto out;
+ }
+
+ ret = of_apply_quirk_fragment_node(dno, dnt);
+out:
+ of_node_put(dno);
+ of_node_put(dnt);
+
+ return ret;
+}
+
+/**
+ * of_quirk_apply_by_node - Apply a DT quirk found at the given node
+ *
+ * @dn: device node pointer to the quirk
+ *
+ * Returns 0 on success, a negative error value in case of an error.
+ */
+int of_quirk_apply_by_node(struct device_node *dn)
+{
+ struct device_node *child;
+ int ret;
+
+ if (!dn)
+ return -ENODEV;
+
+ pr_debug("Apply quirk at %pO\n", dn);
+
+ ret = 0;
+ for_each_child_of_node(dn, child) {
+ ret = of_apply_single_quirk_fragment(child);
+ if (ret != 0) {
+ of_node_put(child);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+/**
+ * of_quirk_apply_by_node - Apply a DT quirk found by the given phandle
+ *
+ * ph: phandle of the quirk node
+ *
+ * Returns 0 on success, a negative error value in case of an error.
+ */
+int of_quirk_apply_by_phandle(phandle ph)
+{
+ struct device_node *dn;
+ int ret;
+
+ dn = of_find_node_by_phandle(ph);
+ if (!dn) {
+ pr_err("Failed to find node with phandle %u\n", ph);
+ return -ENODEV;
+ }
+
+ ret = of_quirk_apply_by_node(dn);
+ of_node_put(dn);
+
+ return ret;
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index 7ede449..02d8988 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1075,4 +1075,20 @@ static inline int of_overlay_destroy_all(void)

#endif

+/* early boot quirks */
+#ifdef CONFIG_OF_DYNAMIC
+int of_quirk_apply_by_node(struct device_node *dn);
+int of_quirk_apply_by_phandle(phandle ph);
+#else
+static inline int of_quirk_apply_by_node(struct device_node *dn)
+{
+ return -ENOTSUPP;
+}
+
+int of_quirk_apply_by_phandle(phandle ph)
+{
+ return -ENOTSUPP;
+}
+#endif
+
#endif /* _LINUX_OF_H */
--
1.7.12

2015-02-18 14:59:58

[permalink] [raw]

Subject: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

Implement DT quirks for the am33xx beaglebone boards.

A single boot DTB supports both the beaglebone white and the beaglebone
black, and on boot time the identifying EEPROM is read and the info
is used to modify the live tree to match the board detected.

Refer to Documentation/devicetree/bindings/quirks/am33xx-bone-quirk.txt
for detailed information.

Signed-off-by: Pantelis Antoniou <[email protected]>
---
.../bindings/quirks/am33xx-bone-quirk.txt | 82 ++++
arch/arm/mach-omap2/Makefile | 5 +
arch/arm/mach-omap2/am33xx-dt-quirks.c | 498 +++++++++++++++++++++
arch/arm/mach-omap2/am33xx-dt-quirks.h | 10 +
arch/arm/mach-omap2/board-generic.c | 1 +
arch/arm/mach-omap2/common.h | 8 +
6 files changed, 604 insertions(+)
create mode 100644 Documentation/devicetree/bindings/quirks/am33xx-bone-quirk.txt
create mode 100644 arch/arm/mach-omap2/am33xx-dt-quirks.c
create mode 100644 arch/arm/mach-omap2/am33xx-dt-quirks.h

diff --git a/Documentation/devicetree/bindings/quirks/am33xx-bone-quirk.txt b/Documentation/devicetree/bindings/quirks/am33xx-bone-quirk.txt
new file mode 100644
index 0000000..239dd8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/quirks/am33xx-bone-quirk.txt
@@ -0,0 +1,82 @@
+am33xx Beaglebone White/Black Device Tree Bindings
+--------------------------------------------------
+
+Beaglebone boards that boot with a common device tree blob contain
+a DT quirk entry with the following properties:
+
+Required properties:
+
+- compatible : should be "ti,am33xx-bone-quirk"
+
+Required child node:
+
+- revs : Contains children nodes that identify boards
+
+ Each child node of revs must be of the following form:
+
+ Required properties:
+
+ - board-id : Contain the board ID as it appears in the EEPROM
+ - board-apply : Should contain the phandle of the quirk to apply
+
+ Optional child node:
+
+ - options : Contain a number of properties that identify options
+ that can be enabled or disabled by external means. For Linux the
+ options are selected by parsing the boot command line.
+ The properties contain a tupple of quirk phandles, the first containing
+ the enable quirk and the second the disable quirk. If the phandle is zero
+ then no quirk is applied.
+
+Any other nodes (like the quirk definitions are ignored).
+
+The following example defines two revisions (white/black) with
+board-id A335BONE for the white and A335BNLT for the black.
+For the beaglebone black we also define two options
+no-emmc and no-hdmi which when are present in the boot command line
+the emmc and hdmi are disabled.
+
+ beaglebone-quirks {
+ compatible = "ti,am33xx-bone-quirk";
+ status = "okay";
+
+ revs {
+ white {
+ board-id = "A335BONE";
+ board-apply = <&bonewhite>;
+ };
+ black {
+ board-id = "A335BNLT";
+ board-apply = <&boneblack>;
+ options {
+ no-emmc = <0 &black_enable_emmc>;
+ no-hdmi = <0 &black_enable_hdmi>;
+ };
+ };
+ };
+
+ overlays {
+ bonewhite: bonewhite {
+ /* quirk for white */
+ ...
+ };
+
+ boneblack: boneblack {
+ /* quirk for black */
+ ...
+ };
+
+ black_enable_hdmi: black_hdmi {
+ /* quirk to enable hdmi */
+ ...
+ };
+
+ black_enable_emmc: black_emmc {
+ /* quirk to enable emmc */
+ ...
+ };
+ };
+ };
+
+
+
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 5d27dfd..02129e7 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -259,6 +259,11 @@ obj-$(CONFIG_MACH_CRANEBOARD) += board-am3517crane.o

obj-$(CONFIG_MACH_SBC3530) += board-omap3stalker.o

+# DT quirks
+ifneq ($(CONFIG_OF_DYNAMIC),)
+obj-$(CONFIG_SOC_AM33XX) += am33xx-dt-quirks.o
+endif
+
# Platform specific device init code

omap-flash-$(CONFIG_MTD_NAND_OMAP2) := board-flash.o
diff --git a/arch/arm/mach-omap2/am33xx-dt-quirks.c b/arch/arm/mach-omap2/am33xx-dt-quirks.c
new file mode 100644
index 0000000..668235c
--- /dev/null
+++ b/arch/arm/mach-omap2/am33xx-dt-quirks.c
@@ -0,0 +1,498 @@
+/*
+ * arch/arm/mach-omap2/am33xx-dt-quirks.c
+ *
+ * AM33xx variant DT quirks
+ *
+ * Copyright (C) 2015 Konsulko Group
+ *
+ * Author:
+ * Pantelis Antoniou <[email protected]>
+ *
+ * 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/module.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <asm/io.h>
+#include <asm/delay.h>
+
+#include "am33xx-dt-quirks.h"
+
+/*
+ * The board IDs for am33xx board are in an I2C EEPROM
+ * We are very early in the boot process so we have to
+ * read the EEPROM directly without using the I2C layer.
+ *
+ * Note that we rely on the bootloader setting up the muxes
+ * (which is the case for u-boot).
+ */
+
+/* I2C Status Register (OMAP_I2C_STAT): */
+#define OMAP_I2C_STAT_XDR (1 << 14) /* TX Buffer draining */
+#define OMAP_I2C_STAT_RDR (1 << 13) /* RX Buffer draining */
+#define OMAP_I2C_STAT_BB (1 << 12) /* Bus busy */
+#define OMAP_I2C_STAT_ROVR (1 << 11) /* Receive overrun */
+#define OMAP_I2C_STAT_XUDF (1 << 10) /* Transmit underflow */
+#define OMAP_I2C_STAT_AAS (1 << 9) /* Address as slave */
+#define OMAP_I2C_STAT_BF (1 << 8) /* Bus Free */
+#define OMAP_I2C_STAT_XRDY (1 << 4) /* Transmit data ready */
+#define OMAP_I2C_STAT_RRDY (1 << 3) /* Receive data ready */
+#define OMAP_I2C_STAT_ARDY (1 << 2) /* Register access ready */
+#define OMAP_I2C_STAT_NACK (1 << 1) /* No ack interrupt enable */
+#define OMAP_I2C_STAT_AL (1 << 0) /* Arbitration lost int ena */
+
+/* I2C Configuration Register (OMAP_I2C_CON): */
+#define OMAP_I2C_CON_EN (1 << 15) /* I2C module enable */
+#define OMAP_I2C_CON_BE (1 << 14) /* Big endian mode */
+#define OMAP_I2C_CON_OPMODE_HS (1 << 12) /* High Speed support */
+#define OMAP_I2C_CON_STB (1 << 11) /* Start byte mode (master) */
+#define OMAP_I2C_CON_MST (1 << 10) /* Master/slave mode */
+#define OMAP_I2C_CON_TRX (1 << 9) /* TX/RX mode (master only) */
+#define OMAP_I2C_CON_XA (1 << 8) /* Expand address */
+#define OMAP_I2C_CON_RM (1 << 2) /* Repeat mode (master only) */
+#define OMAP_I2C_CON_STP (1 << 1) /* Stop cond (master only) */
+#define OMAP_I2C_CON_STT (1 << 0) /* Start condition (master) */
+
+/* register definitions */
+#define I2C_REVNB_LO 0x00
+#define I2C_REVNB_HI 0x04
+#define I2C_SYSC 0x10
+#define I2C_IRQSTATUS_RAW 0x24
+#define I2C_IRQSTATUS 0x28
+#define I2C_IRQENABLE_SET 0x2C
+#define I2C_IRQENABLE_CLR 0x30
+#define I2C_WE 0x34
+#define I2C_DMARXENABLE_SET 0x38
+#define I2C_DMATXENABLE_SET 0x3C
+#define I2C_DMARXENABLE_CLR 0x40
+#define I2C_DMATXENABLE_CLR 0x44
+#define I2C_DMARXWAKE_EN 0x48
+#define I2C_DMATXWAKE_EN 0x4C
+#define I2C_SYSS 0x90
+#define I2C_BUF 0x94
+#define I2C_CNT 0x98
+#define I2C_DATA 0x9C
+#define I2C_CON 0xA4
+#define I2C_OA 0xA8
+#define I2C_SA 0xAC
+#define I2C_PSC 0xB0
+#define I2C_SCLL 0xB4
+#define I2C_SCLH 0xB8
+#define I2C_SYSTEST 0xBC
+#define I2C_BUFSTAT 0xC0
+#define I2C_OA1 0xC4
+#define I2C_OA2 0xC8
+#define I2C_OA3 0xCC
+#define I2C_ACTOA 0xD0
+#define I2C_SBLOCK 0xD4
+
+static inline void i2c_reg_write(void __iomem *base, unsigned int reg, u16 val)
+{
+ writew_relaxed(val, base + reg);
+}
+
+static inline u16 i2c_reg_read(void __iomem *base, unsigned int reg)
+{
+ return readw_relaxed(base + reg);
+}
+
+static void flush_fifo(void __iomem *base)
+{
+ u16 stat;
+
+ while ((stat = i2c_reg_read(base, I2C_IRQSTATUS_RAW))
+ & OMAP_I2C_STAT_RRDY) {
+ (void)i2c_reg_read(base, I2C_DATA);
+ i2c_reg_write(base, I2C_IRQSTATUS, OMAP_I2C_STAT_RRDY);
+ udelay(1000);
+ };
+}
+
+static void wait_delay(void __iomem *base)
+{
+ udelay((10000000 / 100000) * 2);
+}
+
+static int wait_for_bb(void __iomem *base)
+{
+ int timeout;
+ u16 stat;
+
+ timeout = 1000;
+ while (((stat = i2c_reg_read(base, I2C_IRQSTATUS_RAW)) &
+ OMAP_I2C_STAT_BB) && timeout--) {
+ i2c_reg_write(base, I2C_IRQSTATUS, stat);
+ wait_delay(base);
+ }
+
+ if (timeout <= 0) {
+ pr_err("%s: Timeout while waiting for bus\n", __func__);
+ return -1;
+ }
+ i2c_reg_write(base, I2C_IRQSTATUS, 0xffff);
+ return 0;
+}
+
+static u16 wait_for_event(void __iomem *base)
+{
+ u16 status;
+ int timeout = 10000;
+
+ do {
+ wait_delay(base);
+ status = i2c_reg_read(base, I2C_IRQSTATUS_RAW);
+ } while (!(status & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_XUDF |
+ OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_RRDY |
+ OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
+ OMAP_I2C_STAT_AL)) &&
+ timeout--);
+
+ if (timeout <= 0) {
+ pr_err("%s: Timeout status=%04x\n", __func__, status);
+ i2c_reg_write(base, I2C_IRQSTATUS, 0xffff);
+ status = 0;
+ }
+
+ return status;
+}
+
+static int i2c_init(void __iomem *base, u16 psc, u16 scll, u16 sclh)
+{
+ int timeout;
+
+ /* init */
+ if (i2c_reg_read(base, I2C_CON) & OMAP_I2C_CON_EN) {
+ i2c_reg_write(base, I2C_CON, 0);
+ for (timeout = 0; timeout <= 50000; timeout += 1000)
+ udelay(1000);
+ }
+
+ /* soft reset */
+ i2c_reg_write(base, I2C_SYSC, 0x02);
+ udelay(1000);
+ i2c_reg_write(base, I2C_CON, OMAP_I2C_CON_EN);
+
+ timeout = 1000;
+ while (!(i2c_reg_read(base, I2C_SYSS) & 0x0001) && timeout--) {
+ if (timeout <= 0) {
+ pr_err("%s: Timeout in soft reset\n", __func__);
+ return -ENODEV;
+ }
+ udelay(1000);
+ }
+
+ i2c_reg_write(base, I2C_CON, 0x0000);
+ i2c_reg_write(base, I2C_PSC, psc);
+ i2c_reg_write(base, I2C_SCLL, scll);
+ i2c_reg_write(base, I2C_SCLH, sclh);
+ i2c_reg_write(base, I2C_CON, 0x8000);
+ udelay(1000);
+
+ i2c_reg_write(base, I2C_OA, 1);
+
+ /* flush fifo */
+ flush_fifo(base);
+ i2c_reg_write(base, I2C_IRQSTATUS, 0xffff);
+
+ return 0;
+}
+
+static int i2c_read(void __iomem *base, u8 chip, u16 addr,
+ unsigned int alen,
+ void *buffer, int len)
+{
+ u8 *s = buffer;
+ unsigned int i;
+ u16 status;
+ int timeout, ret;
+
+ if (alen < 0 || len < 0 || !buffer || alen > 2 ||
+ (addr + len > 0x10000))
+ return -EINVAL;
+
+ if (wait_for_bb(base) != 0) {
+ pr_err("%s: wait for bb fail\n", __func__);
+ return -ENODEV;
+ }
+
+ ret = -EINVAL;
+ i2c_reg_write(base, I2C_SA, chip);
+
+ if (alen) {
+ i2c_reg_write(base, I2C_CNT, alen);
+
+ /* Stop - Start (P-S) */
+ i2c_reg_write(base, I2C_CON, OMAP_I2C_CON_EN |
+ OMAP_I2C_CON_MST | OMAP_I2C_CON_STT |
+ OMAP_I2C_CON_STP | OMAP_I2C_CON_TRX);
+ while (alen > 0) {
+ status = wait_for_event(base);
+ if (status == 0 || (status & OMAP_I2C_STAT_NACK)) {
+ ret = -ENODEV;
+ pr_err("%s: error waiting for addr ACK\n",
+ __func__);
+ goto out;
+ }
+ if (status & OMAP_I2C_STAT_XRDY) {
+ alen--;
+ i2c_reg_write(base, I2C_DATA,
+ (addr >> (8 * alen)) & 0xff);
+ i2c_reg_write(base, I2C_IRQSTATUS,
+ OMAP_I2C_STAT_XRDY);
+ }
+ }
+
+ /* poll ARDY for last byte going out */
+ timeout = 1000;
+ do {
+ status = wait_for_event(base);
+ if (status & OMAP_I2C_STAT_ARDY)
+ break;
+ udelay(1000);
+ } while (timeout--);
+
+ if (timeout <= 0) {
+ ret = -ENODEV;
+ pr_err("%s: timeout waiting for ARDY\n",
+ __func__);
+ goto out;
+ }
+
+ i2c_reg_write(base, I2C_IRQSTATUS, OMAP_I2C_STAT_ARDY);
+
+ wait_delay(base);
+ }
+
+ i2c_reg_write(base, I2C_CNT, len);
+
+ i2c_reg_write(base, I2C_CON, OMAP_I2C_CON_EN | OMAP_I2C_CON_MST |
+ OMAP_I2C_CON_STT | OMAP_I2C_CON_STP);
+
+ i = 0;
+ while (i < len) {
+ status = wait_for_event(base);
+ if (status == 0 || (status & OMAP_I2C_STAT_NACK)) {
+ ret = -ENODEV;
+ pr_err("%s: error waiting for data ACK\n",
+ __func__);
+ goto out;
+ }
+ if (status & OMAP_I2C_STAT_RRDY) {
+ *s++ = (u8)i2c_reg_read(base, I2C_DATA);
+ i2c_reg_write(base, I2C_IRQSTATUS,
+ OMAP_I2C_STAT_RRDY);
+ i++;
+ }
+ if (status & OMAP_I2C_STAT_ARDY) {
+ i2c_reg_write(base, I2C_IRQSTATUS,
+ OMAP_I2C_STAT_ARDY);
+ break;
+ }
+ }
+
+ if (i < len)
+ pr_err("%s: short read (%u < %u)\n", __func__, i, len);
+ ret = i;
+
+out:
+ flush_fifo(base);
+ i2c_reg_write(base, I2C_IRQSTATUS, 0xffff);
+ return ret;
+}
+
+struct am335x_baseboard_id {
+ u8 magic[4];
+ u8 name[8];
+ u8 version[4];
+ u8 serial[12];
+ u8 config[32];
+ u8 mac_addr[3][6];
+};
+
+static int beaglebone_read_header(struct am335x_baseboard_id *hdr)
+{
+ void __iomem *base;
+ u32 magic;
+ int ret;
+
+ /* map I2C0 */
+ base = ioremap((phys_addr_t)0x44E0B000, 0x1000);
+ if (base == NULL) {
+ pr_err("%s: failed to ioremap\n", __func__);
+ return -ENOMEM;
+ }
+
+ ret = i2c_init(base, 0x0000, 0x00ea, 0x00ea);
+ if (ret != 0) {
+ pr_err("%s: i2c_init failed\n", __func__);
+ return ret;
+ }
+
+ /* the EEPROM is at 0x50 */
+ ret = i2c_read(base, 0x50, 0, 2, hdr, sizeof(*hdr));
+ if (ret != sizeof(*hdr)) {
+ pr_err("%s: Failed to read EEPROM\n", __func__);
+ ret = ret >= 0 ? -EINVAL : ret;
+ goto out;
+ }
+
+ print_hex_dump(KERN_DEBUG, "EEPROM: ", DUMP_PREFIX_OFFSET,
+ 16, 1, hdr, sizeof(*hdr), true);
+
+ /* magic value is LE */
+ magic = ((u32)hdr->magic[3] << 24) | ((u32)hdr->magic[2] << 16) |
+ ((u32)hdr->magic[1] << 8) | hdr->magic[0];
+ if (magic != 0xEE3355AA) {
+ pr_err("%s: Bad EEPROM (0x%08x) %02x %02x %02x %02x\n",
+ __func__, magic,
+ hdr->magic[0], hdr->magic[1],
+ hdr->magic[2], hdr->magic[3]);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = 0;
+
+out:
+ iounmap(base);
+ return ret;
+}
+
+/* find whether a command line argument exists */
+static const char *command_line_arg(const char *bootargs, const char *what)
+{
+ const char *s, *e, *p;
+ int len;
+
+ len = strlen(what);
+ s = bootargs;
+ e = bootargs + strlen(bootargs);
+ for (; s < e && (p = strstr(s, what)) != NULL; s += len) {
+
+ /* if not the first arguments a space must precede */
+ if (p > bootargs && p[-1] != ' ')
+ continue;
+
+ /* skip over what */
+ p += len;
+
+ /* if not the last argument a space must follow */
+ if (p < e && *p != ' ')
+ continue;
+
+ return p;
+ }
+
+ return NULL;
+}
+
+static void __init beaglebone_dt_quirk(void)
+{
+ struct am335x_baseboard_id header;
+ struct device_node *np, *revnp, *child, *optnp, *chosen;
+ struct property *prop;
+ char name[8 + 1];
+ const char *detected_board_id, *board_id, *bootargs;
+ int i, ret;
+ phandle ph;
+
+ np = NULL;
+ revnp = NULL;
+ optnp = NULL;
+ chosen = NULL;
+
+ /* beaglebone quirks */
+ np = of_find_compatible_node(NULL, NULL, "ti,am33xx-bone-quirk");
+ if (!np || !of_device_is_available(np))
+ goto out;
+
+ revnp = of_get_child_by_name(np, "revs");
+ if (!revnp) {
+ pr_err("%s: no revs node at %pO\n", __func__, np);
+ goto out;
+ }
+ child = NULL;
+ optnp = NULL;
+ chosen = NULL;
+
+ ret = beaglebone_read_header(&header);
+ if (ret != 0) {
+ pr_err("%s: Failed to read EEPROM\n", __func__);
+ goto out;
+ }
+ memcpy(name, header.name, sizeof(header.name));
+ name[sizeof(header.name)] = '\0';
+
+ detected_board_id = name;
+
+ pr_debug("%s: Finding quirks for board_id=%s\n", __func__,
+ detected_board_id);
+ for_each_child_of_node(revnp, child) {
+ if (!of_property_read_string(child, "board-id", &board_id) &&
+ !strcmp(board_id, detected_board_id))
+ goto found;
+ }
+ pr_warn("%s: No quirks for board_id=%s\n", __func__, detected_board_id);
+ goto out;
+found:
+ pr_debug("%s: Applying quirks for board_id=%s\n", __func__,
+ board_id);
+ ret = 0;
+ for (i = 0; of_property_read_u32_index(child,
+ "board-apply", i, &ph) == 0; i++) {
+
+ ret = of_quirk_apply_by_phandle(ph);
+ if (ret != 0)
+ break;
+ }
+ if (ret != 0) {
+ pr_err("%s: Failed to apply quirk at %pO\n", __func__, child);
+ goto out;
+ }
+
+ optnp = of_get_child_by_name(child, "options");
+ chosen = of_find_node_by_path("/chosen");
+ bootargs = NULL;
+ if (chosen) {
+ if (of_property_read_string(chosen, "bootargs", &bootargs))
+ bootargs = NULL;
+ of_node_put(chosen);
+ }
+ if (optnp && bootargs) {
+ /* iterate on properties */
+ for_each_property_of_node(optnp, prop) {
+ if (!strcmp(prop->name, "name"))
+ continue;
+
+ if (command_line_arg(bootargs, prop->name))
+ i = 0;
+ else
+ i = 1;
+ if (of_property_read_u32_index(optnp,
+ prop->name, i, &ph) != 0) {
+ pr_err("%s: Failed to get phandle at %pO/%s\n",
+ __func__, optnp, prop->name);
+ continue;
+ }
+ ret = of_quirk_apply_by_phandle(ph);
+ if (ret != 0)
+ break;
+ }
+ }
+
+out:
+ of_node_put(optnp);
+ of_node_put(child);
+ of_node_put(revnp);
+ of_node_put(np);
+}
+
+void __init am33xx_dt_quirk(void)
+{
+ /* Manually issue calls for each supported board variant */
+
+ /* the beaglebone is the one supported board for now */
+ beaglebone_dt_quirk();
+}
diff --git a/arch/arm/mach-omap2/am33xx-dt-quirks.h b/arch/arm/mach-omap2/am33xx-dt-quirks.h
new file mode 100644
index 0000000..8c7dbec
--- /dev/null
+++ b/arch/arm/mach-omap2/am33xx-dt-quirks.h
@@ -0,0 +1,10 @@
+#ifndef AM33XX_DT_QUIRKS_H
+#define AM33XX_DT_QUIRKS_H
+
+#if IS_ENABLED(OF_DYNAMIC)
+extern void __init am33xx_dt_quirk(void);
+#else
+#define am33xx_dt_quirk NULL
+#endif
+
+#endif
diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index b61c049..9922c61 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -177,6 +177,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)")
.init_time = omap3_gptimer_timer_init,
.dt_compat = am33xx_boards_compat,
.restart = am33xx_restart,
+ .dt_quirk = am33xx_dt_quirk,
MACHINE_END
#endif

diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 64e44d6..6b4d6e0 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -155,6 +155,14 @@ static inline void am33xx_restart(enum reboot_mode mode, const char *cmd)
}
#endif

+#ifdef CONFIG_SOC_AM33XX
+void am33xx_dt_quirk(void);
+#else
+static inline void am33xx_dt_quirk(void)
+{
+}
+#endif
+
#ifdef CONFIG_ARCH_OMAP3
void omap3xxx_restart(enum reboot_mode mode, const char *cmd);
#else
--
1.7.12

2015-02-18 15:00:56

[permalink] [raw]

Subject: [PATCH 4/4] arm: dts: Common Black/White Beaglebone DTS using quirks

A common DTS for both beaglebone white/black, using DT quirks
for board selection at runtime.

Signed-off-by: Pantelis Antoniou <[email protected]>
---
arch/arm/boot/dts/Makefile | 3 +-
arch/arm/boot/dts/am335x-bone-all.dts | 157 ++++++++++++++++++++++++++++++++++
2 files changed, 159 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/boot/dts/am335x-bone-all.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index e8ce368..fade89b 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -359,7 +359,8 @@ dtb-$(CONFIG_SOC_AM33XX) += am335x-base0033.dtb \
am335x-evmsk.dtb \
am335x-nano.dtb \
am335x-pepper.dtb \
- am335x-lxm.dtb
+ am335x-lxm.dtb \
+ am335x-bone-all.dtb
dtb-$(CONFIG_ARCH_OMAP4) += omap4-duovero-parlor.dtb \
omap4-panda.dtb \
omap4-panda-a4.dtb \
diff --git a/arch/arm/boot/dts/am335x-bone-all.dts b/arch/arm/boot/dts/am335x-bone-all.dts
new file mode 100644
index 0000000..1f96903
--- /dev/null
+++ b/arch/arm/boot/dts/am335x-bone-all.dts
@@ -0,0 +1,157 @@
+/*
+ */
+/dts-v1/;
+
+#include "am33xx.dtsi"
+#include "am335x-bone-common.dtsi"
+
+/* XXX boneblack */
+
+/ {
+ model = "TI AM335x BeagleBone White/Black";
+ compatible = "ti,am335x-bone", "ti,am33xx";
+
+ beaglebone-quirks {
+ compatible = "ti,am33xx-bone-quirk";
+ status = "okay";
+
+ revs {
+ white {
+ board-id = "A335BONE";
+ board-apply = <&bonewhite>;
+ };
+ black {
+ board-id = "A335BNLT";
+ board-apply = <&boneblack>;
+ options {
+ no-emmc = <0 &black_enable_emmc>;
+ no-hdmi = <0 &black_enable_hdmi>;
+ };
+ };
+ };
+
+ overlays {
+ bonewhite: bonewhite {
+ fragment@0 {
+ target = <&ldo3_reg>;
+ __overlay__ {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+ };
+ fragment@1 {
+ target = <&mmc1>;
+ __overlay__ {
+ vmmc-supply = <&ldo3_reg>;
+ };
+ };
+ fragment@2 {
+ target = <&sham>;
+ __overlay__ {
+ status = "okay";
+ };
+ };
+ fragment@3 {
+ target = <&aes>;
+ __overlay__ {
+ status = "okay";
+ };
+ };
+ };
+
+ boneblack: boneblack {
+ fragment@0 {
+ target = <&ldo3_reg>;
+ __overlay__ {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ };
+ };
+ fragment@1 {
+ target = <&mmc1>;
+ __overlay__ {
+ vmmc-supply = <&vmmcsd_fixed>;
+ };
+ };
+ fragment@6 {
+ target = <&rtc>;
+ __overlay__ {
+ system-power-controller;
+ };
+ };
+ };
+
+ black_enable_hdmi: black_hdmi {
+ fragment@0 {
+ target = <&am33xx_pinmux>;
+ __overlay__ {
+ nxp_hdmi_bonelt_pins: nxp_hdmi_bonelt_pins {
+ pinctrl-single,pins = <
+ 0x1b0 0x03 /* xdma_event_intr0, OMAP_MUX_MODE3 | AM33XX_PIN_OUTPUT */
+ 0xa0 0x08 /* lcd_data0.lcd_data0, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xa4 0x08 /* lcd_data1.lcd_data1, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xa8 0x08 /* lcd_data2.lcd_data2, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xac 0x08 /* lcd_data3.lcd_data3, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xb0 0x08 /* lcd_data4.lcd_data4, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xb4 0x08 /* lcd_data5.lcd_data5, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xb8 0x08 /* lcd_data6.lcd_data6, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xbc 0x08 /* lcd_data7.lcd_data7, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xc0 0x08 /* lcd_data8.lcd_data8, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xc4 0x08 /* lcd_data9.lcd_data9, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xc8 0x08 /* lcd_data10.lcd_data10, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xcc 0x08 /* lcd_data11.lcd_data11, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xd0 0x08 /* lcd_data12.lcd_data12, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xd4 0x08 /* lcd_data13.lcd_data13, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xd8 0x08 /* lcd_data14.lcd_data14, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xdc 0x08 /* lcd_data15.lcd_data15, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA */
+ 0xe0 0x00 /* lcd_vsync.lcd_vsync, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT */
+ 0xe4 0x00 /* lcd_hsync.lcd_hsync, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT */
+ 0xe8 0x00 /* lcd_pclk.lcd_pclk, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT */
+ 0xec 0x00 /* lcd_ac_bias_en.lcd_ac_bias_en, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT */
+ >;
+ };
+ nxp_hdmi_bonelt_off_pins: nxp_hdmi_bonelt_off_pins {
+ pinctrl-single,pins = <
+ 0x1b0 0x03 /* xdma_event_intr0, OMAP_MUX_MODE3 | AM33XX_PIN_OUTPUT */
+ >;
+ };
+ };
+ };
+ fragment@1 {
+ target-path = "/";
+ __overlay__ {
+ hdmi {
+ compatible = "ti,tilcdc,slave";
+ i2c = <&i2c0>;
+ pinctrl-names = "default", "off";
+ pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
+ pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
+ status = "okay";
+ };
+ };
+ };
+ fragment@2 {
+ target = <&lcdc>;
+ __overlay__ {
+ status = "okay";
+ };
+ };
+ };
+
+ black_enable_emmc: black_emmc {
+ fragment@0 {
+ target = <&mmc2>;
+ __overlay__ {
+ vmmc-supply = <&vmmcsd_fixed>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&emmc_pins>;
+ bus-width = <8>;
+ status = "okay";
+ };
+ };
+ };
+ };
+ };
+};
--
1.7.12

2015-02-18 15:41:41

by Mark Rutland

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Pantelis,

On Wed, Feb 18, 2015 at 02:59:34PM +0000, Pantelis Antoniou wrote:
> Implement a method of applying DT quirks early in the boot sequence.
>
> A DT quirk is a subtree of the boot DT that can be applied to
> a target in the base DT resulting in a modification of the live
> tree. The format of the quirk nodes is that of a device tree overlay.
>
> For details please refer to Documentation/devicetree/quirks.txt
>
> Signed-off-by: Pantelis Antoniou <[email protected]>
> ---
> Documentation/devicetree/quirks.txt | 101 ++++++++++
> drivers/of/dynamic.c | 358 ++++++++++++++++++++++++++++++++++++
> include/linux/of.h | 16 ++
> 3 files changed, 475 insertions(+)
> create mode 100644 Documentation/devicetree/quirks.txt
>
> diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
> new file mode 100644
> index 0000000..789319a
> --- /dev/null
> +++ b/Documentation/devicetree/quirks.txt
> @@ -0,0 +1,101 @@
> +A Device Tree quirk is the way which allows modification of the
> +boot device tree under the control of a per-platform specific method.
> +
> +Take for instance the case of a board family that comprises of a
> +number of different board revisions, all being incremental changes
> +after an initial release.
> +
> +Since all board revisions must be supported via a single software image
> +the only way to support this scheme is by having a different DTB for each
> +revision with the bootloader selecting which one to use at boot time.

Not necessarily at boot time. The boards don't have to run the exact
same FW/bootloader binary, so the relevant DTB could be programmed onto
each board.

> +While this may in theory work, in practice it is very cumbersome
> +for the following reasons:
> +
> +1. The act of selecting a different boot device tree blob requires
> +a reasonably advanced bootloader with some kind of configuration or
> +scripting capabilities. Sadly this is not the case many times, the
> +bootloader is extremely dumb and can only use a single dt blob.

You can have several bootloader builds, or even a single build with
something like appended DTB to get an appropriate DTB if the same binary
will otherwise work across all variants of a board.

So it's not necessarily true that you need a complex bootloader.

> +2. On many instances boot time is extremely critical; in some cases
> +there are hard requirements like having working video feeds in under
> +2 seconds from power-up. This leaves an extremely small time budget for
> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> +is by removing the standard bootloader from the normal boot sequence
> +altogether by having a very small boot shim that loads the kernel and
> +immediately jumps to kernel, like falcon-boot mode in u-boot does.

Given my previous comments above I don't see why this is relevant.
You're already passing _some_ DTB here, so if you can organise for the
board to statically provide a sane DTB that's fine, or you can resort to
appended DTB if it's not possible to update the board configuration.

> +3. Having different DTBs/DTSs for different board revisions easily leads to
> +drift between versions. Since no developer is expected to have every single
> +board revision at hand, things are easy to get out of sync, with board versions
> +failing to boot even though the kernel is up to date.

I'm not sure I follow. Surely if you don't have every board revision at
hand you can't test quirks exhaustively either?

Additionally you face the problem that two boards of the same variant
could have different base DTBs that you would need to test that each
board's quirks worked for a range of base DTBs.

> +4. One final problem is the static way that device tree works.
> +For example it might be desirable for various boards to have a way to
> +selectively configure the boot device tree, possibly by the use of command
> +line options. For instance a device might be disabled if a given command line
> +option is present, different configuration to various devices for debugging
> +purposes can be selected and so on. Currently the only way to do so is by
> +recompiling the DTS and installing, which is an chore for developers and
> +a completely unreasonable expectation from end-users.

I'm not sure I follow here.

Which devices do you envisage this being the case for?

Outside of debug scenarios when would you envisage we do this?

For the debug case it seems reasonable to have command line parameters
to get the kernel to do what we want. Just because there's a device in
the DTB that's useful in a debug scenario doesn't mean we have to use it
by default.

> +Device Tree quirks solve all those problems by having an in-kernel interface
> +which per-board/platform method can use to selectively modify the device tree
> +right after unflattening.
> +
> +A DT quirk is a subtree of the boot DT that can be applied to
> +a target in the base DT resulting in a modification of the live
> +tree. The format of the quirk nodes is that of a device tree overlay.
> +
> +As an example the following DTS contains a quirk.
> +
> +/ {
> + foo: foo-node {
> + bar = <10>;
> + };
> +
> + select-quirk = <&quirk>;
> +
> + quirk: quirk {
> + fragment@0 {
> + target = <&foo>;
> + __overlay {
> + bar = <0xf00>;
> + baz = <11>;
> + };
> + };
> + };
> +};
> +
> +The quirk when applied would result at the following tree:
> +
> +/ {
> + foo: foo-node {
> + bar = <0xf00>;
> + baz = <11>;
> + };
> +
> + select-quirk = <&quirk>;
> +
> + quirk: quirk {
> + fragment@0 {
> + target = <&foo>;
> + __overlay {
> + bar = <0xf00>;
> + baz = <11>;
> + };
> + };
> + };
> +
> +};
> +
> +The two public method used to accomplish this are of_quirk_apply_by_node()
> +and of_quirk_apply_by_phandle();
> +
> +To apply the quirk, a per-platform method can retrieve the phandle from the
> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
> +
> +The method which the per-platform method is using to select the quirk to apply
> +is out of the scope of the DT quirk definition, but possible methods include
> +and are not limited to: revision encoding in a GPIO input range, board id
> +located in external or internal EEPROM or flash, DMI board ids, etc.

It seems rather unfortunate that to get a useful device tree we have to
resort to board-specific mechanisms. That means yet more platform code,
which is rather unfortunate. This would also require any other DT users
to understand and potentially have to sanitize any quirks (e.g. in the
case of Xen).

Mark.

2015-02-18 15:54:00

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Mark,

> On Feb 18, 2015, at 17:41 , Mark Rutland <[email protected]> wrote:
>
> Hi Pantelis,
>
> On Wed, Feb 18, 2015 at 02:59:34PM +0000, Pantelis Antoniou wrote:
>> Implement a method of applying DT quirks early in the boot sequence.
>>
>> A DT quirk is a subtree of the boot DT that can be applied to
>> a target in the base DT resulting in a modification of the live
>> tree. The format of the quirk nodes is that of a device tree overlay.
>>
>> For details please refer to Documentation/devicetree/quirks.txt
>>
>> Signed-off-by: Pantelis Antoniou <[email protected]>
>> ---
>> Documentation/devicetree/quirks.txt | 101 ++++++++++
>> drivers/of/dynamic.c | 358 ++++++++++++++++++++++++++++++++++++
>> include/linux/of.h | 16 ++
>> 3 files changed, 475 insertions(+)
>> create mode 100644 Documentation/devicetree/quirks.txt
>>
>> diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
>> new file mode 100644
>> index 0000000..789319a
>> --- /dev/null
>> +++ b/Documentation/devicetree/quirks.txt
>> @@ -0,0 +1,101 @@
>> +A Device Tree quirk is the way which allows modification of the
>> +boot device tree under the control of a per-platform specific method.
>> +
>> +Take for instance the case of a board family that comprises of a
>> +number of different board revisions, all being incremental changes
>> +after an initial release.
>> +
>> +Since all board revisions must be supported via a single software image
>> +the only way to support this scheme is by having a different DTB for each
>> +revision with the bootloader selecting which one to use at boot time.
>
> Not necessarily at boot time. The boards don't have to run the exact
> same FW/bootloader binary, so the relevant DTB could be programmed onto
> each board.
>

That has not been the case in any kind of board I’ve worked with.

A special firmware image that requires a different programming step at
the factory to select the correct DTB for each is always one more thing
that can go wrong.

>> +While this may in theory work, in practice it is very cumbersome
>> +for the following reasons:
>> +
>> +1. The act of selecting a different boot device tree blob requires
>> +a reasonably advanced bootloader with some kind of configuration or
>> +scripting capabilities. Sadly this is not the case many times, the
>> +bootloader is extremely dumb and can only use a single dt blob.
>
> You can have several bootloader builds, or even a single build with
> something like appended DTB to get an appropriate DTB if the same binary
> will otherwise work across all variants of a board.
>

No, the same DTB will not work across all the variants of a board.

> So it's not necessarily true that you need a complex bootloader.
>

>> +2. On many instances boot time is extremely critical; in some cases
>> +there are hard requirements like having working video feeds in under
>> +2 seconds from power-up. This leaves an extremely small time budget for
>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>> +is by removing the standard bootloader from the normal boot sequence
>> +altogether by having a very small boot shim that loads the kernel and
>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>
> Given my previous comments above I don't see why this is relevant.
> You're already passing _some_ DTB here, so if you can organise for the
> board to statically provide a sane DTB that's fine, or you can resort to
> appended DTB if it's not possible to update the board configuration.
>

You’re missing the point. I can’t use the same DTB for each revision of the
board. Each board is similar but it’s not identical.

>> +3. Having different DTBs/DTSs for different board revisions easily leads to
>> +drift between versions. Since no developer is expected to have every single
>> +board revision at hand, things are easy to get out of sync, with board versions
>> +failing to boot even though the kernel is up to date.
>
> I'm not sure I follow. Surely if you don't have every board revision at
> hand you can't test quirks exhaustively either?
>

It’s one less thing to worry about. For example in the current mainline kernel
already there is a drift between the beaglebone white and the beaglebone black.

Having the same DTS is just easier to keep things in sync.

> Additionally you face the problem that two boards of the same variant
> could have different base DTBs that you would need to test that each
> board's quirks worked for a range of base DTBs.
>

This is not a valid case. This patch is about boards that have the same base DTB.

>> +4. One final problem is the static way that device tree works.
>> +For example it might be desirable for various boards to have a way to
>> +selectively configure the boot device tree, possibly by the use of command
>> +line options. For instance a device might be disabled if a given command line
>> +option is present, different configuration to various devices for debugging
>> +purposes can be selected and so on. Currently the only way to do so is by
>> +recompiling the DTS and installing, which is an chore for developers and
>> +a completely unreasonable expectation from end-users.
>
> I'm not sure I follow here.
>
> Which devices do you envisage this being the case for?
>
> Outside of debug scenarios when would you envisage we do this?
>

We already have to do this on the beaglebone black. The onboard EMMC and HDMI
devices conflict with any capes that use the same pins. So you have to
have a way to disable them so that the attached cape will work.

> For the debug case it seems reasonable to have command line parameters
> to get the kernel to do what we want. Just because there's a device in
> the DTB that's useful in a debug scenario doesn't mean we have to use it
> by default.

I don’t follow. Users need this functionality to work. I.e. pass a command
line option to use different OPPs etc. Real world usage is messy.

>
>> +Device Tree quirks solve all those problems by having an in-kernel interface
>> +which per-board/platform method can use to selectively modify the device tree
>> +right after unflattening.
>> +
>> +A DT quirk is a subtree of the boot DT that can be applied to
>> +a target in the base DT resulting in a modification of the live
>> +tree. The format of the quirk nodes is that of a device tree overlay.
>> +
>> +As an example the following DTS contains a quirk.
>> +
>> +/ {
>> + foo: foo-node {
>> + bar = <10>;
>> + };
>> +
>> + select-quirk = <&quirk>;
>> +
>> + quirk: quirk {
>> + fragment@0 {
>> + target = <&foo>;
>> + __overlay {
>> + bar = <0xf00>;
>> + baz = <11>;
>> + };
>> + };
>> + };
>> +};
>> +
>> +The quirk when applied would result at the following tree:
>> +
>> +/ {
>> + foo: foo-node {
>> + bar = <0xf00>;
>> + baz = <11>;
>> + };
>> +
>> + select-quirk = <&quirk>;
>> +
>> + quirk: quirk {
>> + fragment@0 {
>> + target = <&foo>;
>> + __overlay {
>> + bar = <0xf00>;
>> + baz = <11>;
>> + };
>> + };
>> + };
>> +
>> +};
>> +
>> +The two public method used to accomplish this are of_quirk_apply_by_node()
>> +and of_quirk_apply_by_phandle();
>> +
>> +To apply the quirk, a per-platform method can retrieve the phandle from the
>> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
>> +
>> +The method which the per-platform method is using to select the quirk to apply
>> +is out of the scope of the DT quirk definition, but possible methods include
>> +and are not limited to: revision encoding in a GPIO input range, board id
>> +located in external or internal EEPROM or flash, DMI board ids, etc.
>
> It seems rather unfortunate that to get a useful device tree we have to
> resort to board-specific mechanisms. That means yet more platform code,
> which is rather unfortunate. This would also require any other DT users
> to understand and potentially have to sanitize any quirks (e.g. in the
> case of Xen).

The original internal version of the patches used early platform devices and
a generic firmware quirk mechanism, but I was directed to the per-platform
method instead. It is perfectly doable to go back at the original implementation
but I need to get the ball rolling with a discussion about the internals.

>
> Mark.

Regards

— Pantelis

2015-02-18 16:32:59

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi,

Great something we are waiting for a long time!

On Wed, Feb 18, 2015 at 05:53:50PM +0200, Pantelis Antoniou wrote:
> Hi Mark,
>
> > On Feb 18, 2015, at 17:41 , Mark Rutland <[email protected]> wrote:
> >
> > Hi Pantelis,
> >
> > On Wed, Feb 18, 2015 at 02:59:34PM +0000, Pantelis Antoniou wrote:
> >> Implement a method of applying DT quirks early in the boot sequence.
> >>
> >> A DT quirk is a subtree of the boot DT that can be applied to
> >> a target in the base DT resulting in a modification of the live
> >> tree. The format of the quirk nodes is that of a device tree overlay.
> >>
> >> For details please refer to Documentation/devicetree/quirks.txt
> >>
> >> Signed-off-by: Pantelis Antoniou <[email protected]>
> >> ---
> >> Documentation/devicetree/quirks.txt | 101 ++++++++++
> >> drivers/of/dynamic.c | 358 ++++++++++++++++++++++++++++++++++++
> >> include/linux/of.h | 16 ++
> >> 3 files changed, 475 insertions(+)
> >> create mode 100644 Documentation/devicetree/quirks.txt
> >>
> >> diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
> >> new file mode 100644
> >> index 0000000..789319a
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/quirks.txt
> >> @@ -0,0 +1,101 @@
> >> +A Device Tree quirk is the way which allows modification of the
> >> +boot device tree under the control of a per-platform specific method.
> >> +
> >> +Take for instance the case of a board family that comprises of a
> >> +number of different board revisions, all being incremental changes
> >> +after an initial release.
> >> +
> >> +Since all board revisions must be supported via a single software image
> >> +the only way to support this scheme is by having a different DTB for each
> >> +revision with the bootloader selecting which one to use at boot time.
> >
> > Not necessarily at boot time. The boards don't have to run the exact
> > same FW/bootloader binary, so the relevant DTB could be programmed onto
> > each board.
> >
>
> That has not been the case in any kind of board I’ve worked with.
>
> A special firmware image that requires a different programming step at
> the factory to select the correct DTB for each is always one more thing
> that can go wrong.
>

I agree. We have boards with several display modules, even if it seems
quite easy to know which dtb has to be loaded since we use a prefix
describing the display module (_pda4, _pda7, etc.) it is a pain for
customers. Moreover you can add the revision of the board, we have a
mother board and a cpu module so it can quickly lead to something like
this:
at91-sama5d31ek_mb-revc_cm-revd_pda7.

It is only an example, at the moment it is a bit less complicated but I
am not so far from the reality: sama5d31ek_revc_pda7.dts,
sama5d33ek_revc_pda4.dts, etc. For a SoC family, we have 27 DTS files...

As for the single zImage, we should find a way to have a single DTB.

> >> +While this may in theory work, in practice it is very cumbersome
> >> +for the following reasons:
> >> +
> >> +1. The act of selecting a different boot device tree blob requires
> >> +a reasonably advanced bootloader with some kind of configuration or
> >> +scripting capabilities. Sadly this is not the case many times, the
> >> +bootloader is extremely dumb and can only use a single dt blob.
> >
> > You can have several bootloader builds, or even a single build with
> > something like appended DTB to get an appropriate DTB if the same binary
> > will otherwise work across all variants of a board.
> >
>
> No, the same DTB will not work across all the variants of a board.
>
> > So it's not necessarily true that you need a complex bootloader.
> >
>
> >> +2. On many instances boot time is extremely critical; in some cases
> >> +there are hard requirements like having working video feeds in under
> >> +2 seconds from power-up. This leaves an extremely small time budget for
> >> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> >> +is by removing the standard bootloader from the normal boot sequence
> >> +altogether by having a very small boot shim that loads the kernel and
> >> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> >
> > Given my previous comments above I don't see why this is relevant.
> > You're already passing _some_ DTB here, so if you can organise for the
> > board to statically provide a sane DTB that's fine, or you can resort to
> > appended DTB if it's not possible to update the board configuration.
> >
>
> You’re missing the point. I can’t use the same DTB for each revision of the
> board. Each board is similar but it’s not identical.
>
> >> +3. Having different DTBs/DTSs for different board revisions easily leads to
> >> +drift between versions. Since no developer is expected to have every single
> >> +board revision at hand, things are easy to get out of sync, with board versions
> >> +failing to boot even though the kernel is up to date.
> >
> > I'm not sure I follow. Surely if you don't have every board revision at
> > hand you can't test quirks exhaustively either?
> >
>
> It’s one less thing to worry about. For example in the current mainline kernel
> already there is a drift between the beaglebone white and the beaglebone black.
>
> Having the same DTS is just easier to keep things in sync.
>
> > Additionally you face the problem that two boards of the same variant
> > could have different base DTBs that you would need to test that each
> > board's quirks worked for a range of base DTBs.
> >
>
> This is not a valid case. This patch is about boards that have the same base DTB.
>
> >> +4. One final problem is the static way that device tree works.
> >> +For example it might be desirable for various boards to have a way to
> >> +selectively configure the boot device tree, possibly by the use of command
> >> +line options. For instance a device might be disabled if a given command line
> >> +option is present, different configuration to various devices for debugging
> >> +purposes can be selected and so on. Currently the only way to do so is by
> >> +recompiling the DTS and installing, which is an chore for developers and
> >> +a completely unreasonable expectation from end-users.
> >
> > I'm not sure I follow here.
> >
> > Which devices do you envisage this being the case for?
> >
> > Outside of debug scenarios when would you envisage we do this?
> >
>
> We already have to do this on the beaglebone black. The onboard EMMC and HDMI
> devices conflict with any capes that use the same pins. So you have to
> have a way to disable them so that the attached cape will work.
>
> > For the debug case it seems reasonable to have command line parameters
> > to get the kernel to do what we want. Just because there's a device in
> > the DTB that's useful in a debug scenario doesn't mean we have to use it
> > by default.
>
> I don’t follow. Users need this functionality to work. I.e. pass a command
> line option to use different OPPs etc. Real world usage is messy.
>
> >
> >> +Device Tree quirks solve all those problems by having an in-kernel interface
> >> +which per-board/platform method can use to selectively modify the device tree
> >> +right after unflattening.
> >> +
> >> +A DT quirk is a subtree of the boot DT that can be applied to
> >> +a target in the base DT resulting in a modification of the live
> >> +tree. The format of the quirk nodes is that of a device tree overlay.
> >> +
> >> +As an example the following DTS contains a quirk.
> >> +
> >> +/ {
> >> + foo: foo-node {
> >> + bar = <10>;
> >> + };
> >> +
> >> + select-quirk = <&quirk>;
> >> +
> >> + quirk: quirk {
> >> + fragment@0 {
> >> + target = <&foo>;
> >> + __overlay {
> >> + bar = <0xf00>;
> >> + baz = <11>;
> >> + };
> >> + };
> >> + };
> >> +};
> >> +
> >> +The quirk when applied would result at the following tree:
> >> +
> >> +/ {
> >> + foo: foo-node {
> >> + bar = <0xf00>;
> >> + baz = <11>;
> >> + };
> >> +
> >> + select-quirk = <&quirk>;
> >> +
> >> + quirk: quirk {
> >> + fragment@0 {
> >> + target = <&foo>;
> >> + __overlay {
> >> + bar = <0xf00>;
> >> + baz = <11>;
> >> + };
> >> + };
> >> + };
> >> +
> >> +};
> >> +
> >> +The two public method used to accomplish this are of_quirk_apply_by_node()
> >> +and of_quirk_apply_by_phandle();
> >> +
> >> +To apply the quirk, a per-platform method can retrieve the phandle from the
> >> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
> >> +
> >> +The method which the per-platform method is using to select the quirk to apply
> >> +is out of the scope of the DT quirk definition, but possible methods include
> >> +and are not limited to: revision encoding in a GPIO input range, board id
> >> +located in external or internal EEPROM or flash, DMI board ids, etc.
> >
> > It seems rather unfortunate that to get a useful device tree we have to
> > resort to board-specific mechanisms. That means yet more platform code,
> > which is rather unfortunate. This would also require any other DT users
> > to understand and potentially have to sanitize any quirks (e.g. in the
> > case of Xen).
>
> The original internal version of the patches used early platform devices and
> a generic firmware quirk mechanism, but I was directed to the per-platform
> method instead. It is perfectly doable to go back at the original implementation
> but I need to get the ball rolling with a discussion about the internals.

I also think we should used early platform devices to not add platform
specific code. What were the cons to swith to per-platform method?

>
> >
> > Mark.
>
> Regards
>
> — Pantelis
>

Regards

Ludovic

2015-02-18 16:39:12

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Ludovic,

> On Feb 18, 2015, at 18:32 , Ludovic Desroches <[email protected]> wrote:
>
> Hi,
>
> Great something we are waiting for a long time!
>
> On Wed, Feb 18, 2015 at 05:53:50PM +0200, Pantelis Antoniou wrote:
>> Hi Mark,
>>
>>> On Feb 18, 2015, at 17:41 , Mark Rutland <[email protected]> wrote:
>>>
>>> Hi Pantelis,
>>>
>>> On Wed, Feb 18, 2015 at 02:59:34PM +0000, Pantelis Antoniou wrote:
>>>> Implement a method of applying DT quirks early in the boot sequence.
>>>>
>>>> A DT quirk is a subtree of the boot DT that can be applied to
>>>> a target in the base DT resulting in a modification of the live
>>>> tree. The format of the quirk nodes is that of a device tree overlay.
>>>>
>>>> For details please refer to Documentation/devicetree/quirks.txt
>>>>
>>>> Signed-off-by: Pantelis Antoniou <[email protected]>
>>>> ---
>>>> Documentation/devicetree/quirks.txt | 101 ++++++++++
>>>> drivers/of/dynamic.c | 358 ++++++++++++++++++++++++++++++++++++
>>>> include/linux/of.h | 16 ++
>>>> 3 files changed, 475 insertions(+)
>>>> create mode 100644 Documentation/devicetree/quirks.txt
>>>>
>>>> diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
>>>> new file mode 100644
>>>> index 0000000..789319a
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/quirks.txt
>>>> @@ -0,0 +1,101 @@
>>>> +A Device Tree quirk is the way which allows modification of the
>>>> +boot device tree under the control of a per-platform specific method.
>>>> +
>>>> +Take for instance the case of a board family that comprises of a
>>>> +number of different board revisions, all being incremental changes
>>>> +after an initial release.
>>>> +
>>>> +Since all board revisions must be supported via a single software image
>>>> +the only way to support this scheme is by having a different DTB for each
>>>> +revision with the bootloader selecting which one to use at boot time.
>>>
>>> Not necessarily at boot time. The boards don't have to run the exact
>>> same FW/bootloader binary, so the relevant DTB could be programmed onto
>>> each board.
>>>
>>
>> That has not been the case in any kind of board I’ve worked with.
>>
>> A special firmware image that requires a different programming step at
>> the factory to select the correct DTB for each is always one more thing
>> that can go wrong.
>>
>
> I agree. We have boards with several display modules, even if it seems
> quite easy to know which dtb has to be loaded since we use a prefix
> describing the display module (_pda4, _pda7, etc.) it is a pain for
> customers. Moreover you can add the revision of the board, we have a
> mother board and a cpu module so it can quickly lead to something like
> this:
> at91-sama5d31ek_mb-revc_cm-revd_pda7.
>
> It is only an example, at the moment it is a bit less complicated but I
> am not so far from the reality: sama5d31ek_revc_pda7.dts,
> sama5d33ek_revc_pda4.dts, etc. For a SoC family, we have 27 DTS files…
>

I bet it’s easy to screw up no? Any horror stories from the factory floor? :)

> As for the single zImage, we should find a way to have a single DTB.
>
>>>> +While this may in theory work, in practice it is very cumbersome
>>>> +for the following reasons:
>>>> +
>>>> +1. The act of selecting a different boot device tree blob requires
>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>
>>> You can have several bootloader builds, or even a single build with
>>> something like appended DTB to get an appropriate DTB if the same binary
>>> will otherwise work across all variants of a board.
>>>
>>
>> No, the same DTB will not work across all the variants of a board.
>>
>>> So it's not necessarily true that you need a complex bootloader.
>>>
>>
>>>> +2. On many instances boot time is extremely critical; in some cases
>>>> +there are hard requirements like having working video feeds in under
>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>> +is by removing the standard bootloader from the normal boot sequence
>>>> +altogether by having a very small boot shim that loads the kernel and
>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>
>>> Given my previous comments above I don't see why this is relevant.
>>> You're already passing _some_ DTB here, so if you can organise for the
>>> board to statically provide a sane DTB that's fine, or you can resort to
>>> appended DTB if it's not possible to update the board configuration.
>>>
>>
>> You’re missing the point. I can’t use the same DTB for each revision of the
>> board. Each board is similar but it’s not identical.
>>
>>>> +3. Having different DTBs/DTSs for different board revisions easily leads to
>>>> +drift between versions. Since no developer is expected to have every single
>>>> +board revision at hand, things are easy to get out of sync, with board versions
>>>> +failing to boot even though the kernel is up to date.
>>>
>>> I'm not sure I follow. Surely if you don't have every board revision at
>>> hand you can't test quirks exhaustively either?
>>>
>>
>> It’s one less thing to worry about. For example in the current mainline kernel
>> already there is a drift between the beaglebone white and the beaglebone black.
>>
>> Having the same DTS is just easier to keep things in sync.
>>
>>> Additionally you face the problem that two boards of the same variant
>>> could have different base DTBs that you would need to test that each
>>> board's quirks worked for a range of base DTBs.
>>>
>>
>> This is not a valid case. This patch is about boards that have the same base DTB.
>>
>>>> +4. One final problem is the static way that device tree works.
>>>> +For example it might be desirable for various boards to have a way to
>>>> +selectively configure the boot device tree, possibly by the use of command
>>>> +line options. For instance a device might be disabled if a given command line
>>>> +option is present, different configuration to various devices for debugging
>>>> +purposes can be selected and so on. Currently the only way to do so is by
>>>> +recompiling the DTS and installing, which is an chore for developers and
>>>> +a completely unreasonable expectation from end-users.
>>>
>>> I'm not sure I follow here.
>>>
>>> Which devices do you envisage this being the case for?
>>>
>>> Outside of debug scenarios when would you envisage we do this?
>>>
>>
>> We already have to do this on the beaglebone black. The onboard EMMC and HDMI
>> devices conflict with any capes that use the same pins. So you have to
>> have a way to disable them so that the attached cape will work.
>>
>>> For the debug case it seems reasonable to have command line parameters
>>> to get the kernel to do what we want. Just because there's a device in
>>> the DTB that's useful in a debug scenario doesn't mean we have to use it
>>> by default.
>>
>> I don’t follow. Users need this functionality to work. I.e. pass a command
>> line option to use different OPPs etc. Real world usage is messy.
>>
>>>
>>>> +Device Tree quirks solve all those problems by having an in-kernel interface
>>>> +which per-board/platform method can use to selectively modify the device tree
>>>> +right after unflattening.
>>>> +
>>>> +A DT quirk is a subtree of the boot DT that can be applied to
>>>> +a target in the base DT resulting in a modification of the live
>>>> +tree. The format of the quirk nodes is that of a device tree overlay.
>>>> +
>>>> +As an example the following DTS contains a quirk.
>>>> +
>>>> +/ {
>>>> + foo: foo-node {
>>>> + bar = <10>;
>>>> + };
>>>> +
>>>> + select-quirk = <&quirk>;
>>>> +
>>>> + quirk: quirk {
>>>> + fragment@0 {
>>>> + target = <&foo>;
>>>> + __overlay {
>>>> + bar = <0xf00>;
>>>> + baz = <11>;
>>>> + };
>>>> + };
>>>> + };
>>>> +};
>>>> +
>>>> +The quirk when applied would result at the following tree:
>>>> +
>>>> +/ {
>>>> + foo: foo-node {
>>>> + bar = <0xf00>;
>>>> + baz = <11>;
>>>> + };
>>>> +
>>>> + select-quirk = <&quirk>;
>>>> +
>>>> + quirk: quirk {
>>>> + fragment@0 {
>>>> + target = <&foo>;
>>>> + __overlay {
>>>> + bar = <0xf00>;
>>>> + baz = <11>;
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> +};
>>>> +
>>>> +The two public method used to accomplish this are of_quirk_apply_by_node()
>>>> +and of_quirk_apply_by_phandle();
>>>> +
>>>> +To apply the quirk, a per-platform method can retrieve the phandle from the
>>>> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
>>>> +
>>>> +The method which the per-platform method is using to select the quirk to apply
>>>> +is out of the scope of the DT quirk definition, but possible methods include
>>>> +and are not limited to: revision encoding in a GPIO input range, board id
>>>> +located in external or internal EEPROM or flash, DMI board ids, etc.
>>>
>>> It seems rather unfortunate that to get a useful device tree we have to
>>> resort to board-specific mechanisms. That means yet more platform code,
>>> which is rather unfortunate. This would also require any other DT users
>>> to understand and potentially have to sanitize any quirks (e.g. in the
>>> case of Xen).
>>
>> The original internal version of the patches used early platform devices and
>> a generic firmware quirk mechanism, but I was directed to the per-platform
>> method instead. It is perfectly doable to go back at the original implementation
>> but I need to get the ball rolling with a discussion about the internals.
>
> I also think we should used early platform devices to not add platform
> specific code. What were the cons to swith to per-platform method?
>

Supposedly easier to accept. /me shrugs

>>
>>>
>>> Mark.
>>
>> Regards
>>
>> — Pantelis
>>
>
> Regards
>
> Ludovic

Regards

— Pantelis

2015-02-18 16:46:35

by Matt Porter

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Wed, Feb 18, 2015 at 05:53:50PM +0200, Pantelis Antoniou wrote:
> Hi Mark,
>
> > On Feb 18, 2015, at 17:41 , Mark Rutland <[email protected]> wrote:
> >
> > Hi Pantelis,
> >
> > On Wed, Feb 18, 2015 at 02:59:34PM +0000, Pantelis Antoniou wrote:
> >> Implement a method of applying DT quirks early in the boot sequence.
> >>
> >> A DT quirk is a subtree of the boot DT that can be applied to
> >> a target in the base DT resulting in a modification of the live
> >> tree. The format of the quirk nodes is that of a device tree overlay.
> >>
> >> For details please refer to Documentation/devicetree/quirks.txt
> >>
> >> Signed-off-by: Pantelis Antoniou <[email protected]>
> >> ---
> >> Documentation/devicetree/quirks.txt | 101 ++++++++++
> >> drivers/of/dynamic.c | 358 ++++++++++++++++++++++++++++++++++++
> >> include/linux/of.h | 16 ++
> >> 3 files changed, 475 insertions(+)
> >> create mode 100644 Documentation/devicetree/quirks.txt
> >>
> >> diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
> >> new file mode 100644
> >> index 0000000..789319a
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/quirks.txt
> >> @@ -0,0 +1,101 @@
> >> +A Device Tree quirk is the way which allows modification of the
> >> +boot device tree under the control of a per-platform specific method.
> >> +
> >> +Take for instance the case of a board family that comprises of a
> >> +number of different board revisions, all being incremental changes
> >> +after an initial release.
> >> +
> >> +Since all board revisions must be supported via a single software image
> >> +the only way to support this scheme is by having a different DTB for each
> >> +revision with the bootloader selecting which one to use at boot time.
> >
> > Not necessarily at boot time. The boards don't have to run the exact
> > same FW/bootloader binary, so the relevant DTB could be programmed onto
> > each board.
> >
>
> That has not been the case in any kind of board I’ve worked with.
>
> A special firmware image that requires a different programming step at
> the factory to select the correct DTB for each is always one more thing
> that can go wrong.
>
> >> +While this may in theory work, in practice it is very cumbersome
> >> +for the following reasons:
> >> +
> >> +1. The act of selecting a different boot device tree blob requires
> >> +a reasonably advanced bootloader with some kind of configuration or
> >> +scripting capabilities. Sadly this is not the case many times, the
> >> +bootloader is extremely dumb and can only use a single dt blob.
> >
> > You can have several bootloader builds, or even a single build with
> > something like appended DTB to get an appropriate DTB if the same binary
> > will otherwise work across all variants of a board.
> >
>
> No, the same DTB will not work across all the variants of a board.
>
> > So it's not necessarily true that you need a complex bootloader.
> >
>
> >> +2. On many instances boot time is extremely critical; in some cases
> >> +there are hard requirements like having working video feeds in under
> >> +2 seconds from power-up. This leaves an extremely small time budget for
> >> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> >> +is by removing the standard bootloader from the normal boot sequence
> >> +altogether by having a very small boot shim that loads the kernel and
> >> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> >
> > Given my previous comments above I don't see why this is relevant.
> > You're already passing _some_ DTB here, so if you can organise for the
> > board to statically provide a sane DTB that's fine, or you can resort to
> > appended DTB if it's not possible to update the board configuration.
> >
>
> You’re missing the point. I can’t use the same DTB for each revision of the
> board. Each board is similar but it’s not identical.
>
> >> +3. Having different DTBs/DTSs for different board revisions easily leads to
> >> +drift between versions. Since no developer is expected to have every single
> >> +board revision at hand, things are easy to get out of sync, with board versions
> >> +failing to boot even though the kernel is up to date.
> >
> > I'm not sure I follow. Surely if you don't have every board revision at
> > hand you can't test quirks exhaustively either?
> >
>
> It’s one less thing to worry about. For example in the current mainline kernel
> already there is a drift between the beaglebone white and the beaglebone black.
>
> Having the same DTS is just easier to keep things in sync.

Absolutely, we have the problem in the dts files today where we have
lots of duplicated bits. I know one case that I think you are alluding
to is how BBW has the aes and sham enabled but BBB does not. That's with
just two variants. :( This would definitely drive better organization
of dtses and cure a lot of bitrot if they could share a common file.
OTOH, some might argue that the bone common dtsi should just enable those
nodes staticly for this case.

-Matt

2015-02-18 16:47:50

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Wed, Feb 18, 2015 at 06:39:01PM +0200, Pantelis Antoniou wrote:
> Hi Ludovic,
>
> > On Feb 18, 2015, at 18:32 , Ludovic Desroches <[email protected]> wrote:
> >
> > Hi,
> >
> > Great something we are waiting for a long time!
> >
> > On Wed, Feb 18, 2015 at 05:53:50PM +0200, Pantelis Antoniou wrote:
> >> Hi Mark,
> >>
> >>> On Feb 18, 2015, at 17:41 , Mark Rutland <[email protected]> wrote:
> >>>
> >>> Hi Pantelis,
> >>>
> >>> On Wed, Feb 18, 2015 at 02:59:34PM +0000, Pantelis Antoniou wrote:
> >>>> Implement a method of applying DT quirks early in the boot sequence.
> >>>>
> >>>> A DT quirk is a subtree of the boot DT that can be applied to
> >>>> a target in the base DT resulting in a modification of the live
> >>>> tree. The format of the quirk nodes is that of a device tree overlay.
> >>>>
> >>>> For details please refer to Documentation/devicetree/quirks.txt
> >>>>
> >>>> Signed-off-by: Pantelis Antoniou <[email protected]>
> >>>> ---
> >>>> Documentation/devicetree/quirks.txt | 101 ++++++++++
> >>>> drivers/of/dynamic.c | 358 ++++++++++++++++++++++++++++++++++++
> >>>> include/linux/of.h | 16 ++
> >>>> 3 files changed, 475 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/quirks.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
> >>>> new file mode 100644
> >>>> index 0000000..789319a
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/quirks.txt
> >>>> @@ -0,0 +1,101 @@
> >>>> +A Device Tree quirk is the way which allows modification of the
> >>>> +boot device tree under the control of a per-platform specific method.
> >>>> +
> >>>> +Take for instance the case of a board family that comprises of a
> >>>> +number of different board revisions, all being incremental changes
> >>>> +after an initial release.
> >>>> +
> >>>> +Since all board revisions must be supported via a single software image
> >>>> +the only way to support this scheme is by having a different DTB for each
> >>>> +revision with the bootloader selecting which one to use at boot time.
> >>>
> >>> Not necessarily at boot time. The boards don't have to run the exact
> >>> same FW/bootloader binary, so the relevant DTB could be programmed onto
> >>> each board.
> >>>
> >>
> >> That has not been the case in any kind of board I’ve worked with.
> >>
> >> A special firmware image that requires a different programming step at
> >> the factory to select the correct DTB for each is always one more thing
> >> that can go wrong.
> >>
> >
> > I agree. We have boards with several display modules, even if it seems
> > quite easy to know which dtb has to be loaded since we use a prefix
> > describing the display module (_pda4, _pda7, etc.) it is a pain for
> > customers. Moreover you can add the revision of the board, we have a
> > mother board and a cpu module so it can quickly lead to something like
> > this:
> > at91-sama5d31ek_mb-revc_cm-revd_pda7.
> >
> > It is only an example, at the moment it is a bit less complicated but I
> > am not so far from the reality: sama5d31ek_revc_pda7.dts,
> > sama5d33ek_revc_pda4.dts, etc. For a SoC family, we have 27 DTS files…
> >
>
> I bet it’s easy to screw up no? Any horror stories from the factory floor? :)

Yes it is.

I keep these horror stories for Halloween! At the moment no incident because
we are trying hard to find workarounds with the help of our
In-system programmer or U-boot but the goal is to not rely on a particular
component to do this job.

>
> > As for the single zImage, we should find a way to have a single DTB.
> >
> >>>> +While this may in theory work, in practice it is very cumbersome
> >>>> +for the following reasons:
> >>>> +
> >>>> +1. The act of selecting a different boot device tree blob requires
> >>>> +a reasonably advanced bootloader with some kind of configuration or
> >>>> +scripting capabilities. Sadly this is not the case many times, the
> >>>> +bootloader is extremely dumb and can only use a single dt blob.
> >>>
> >>> You can have several bootloader builds, or even a single build with
> >>> something like appended DTB to get an appropriate DTB if the same binary
> >>> will otherwise work across all variants of a board.
> >>>
> >>
> >> No, the same DTB will not work across all the variants of a board.
> >>
> >>> So it's not necessarily true that you need a complex bootloader.
> >>>
> >>
> >>>> +2. On many instances boot time is extremely critical; in some cases
> >>>> +there are hard requirements like having working video feeds in under
> >>>> +2 seconds from power-up. This leaves an extremely small time budget for
> >>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> >>>> +is by removing the standard bootloader from the normal boot sequence
> >>>> +altogether by having a very small boot shim that loads the kernel and
> >>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> >>>
> >>> Given my previous comments above I don't see why this is relevant.
> >>> You're already passing _some_ DTB here, so if you can organise for the
> >>> board to statically provide a sane DTB that's fine, or you can resort to
> >>> appended DTB if it's not possible to update the board configuration.
> >>>
> >>
> >> You’re missing the point. I can’t use the same DTB for each revision of the
> >> board. Each board is similar but it’s not identical.
> >>
> >>>> +3. Having different DTBs/DTSs for different board revisions easily leads to
> >>>> +drift between versions. Since no developer is expected to have every single
> >>>> +board revision at hand, things are easy to get out of sync, with board versions
> >>>> +failing to boot even though the kernel is up to date.
> >>>
> >>> I'm not sure I follow. Surely if you don't have every board revision at
> >>> hand you can't test quirks exhaustively either?
> >>>
> >>
> >> It’s one less thing to worry about. For example in the current mainline kernel
> >> already there is a drift between the beaglebone white and the beaglebone black.
> >>
> >> Having the same DTS is just easier to keep things in sync.
> >>
> >>> Additionally you face the problem that two boards of the same variant
> >>> could have different base DTBs that you would need to test that each
> >>> board's quirks worked for a range of base DTBs.
> >>>
> >>
> >> This is not a valid case. This patch is about boards that have the same base DTB.
> >>
> >>>> +4. One final problem is the static way that device tree works.
> >>>> +For example it might be desirable for various boards to have a way to
> >>>> +selectively configure the boot device tree, possibly by the use of command
> >>>> +line options. For instance a device might be disabled if a given command line
> >>>> +option is present, different configuration to various devices for debugging
> >>>> +purposes can be selected and so on. Currently the only way to do so is by
> >>>> +recompiling the DTS and installing, which is an chore for developers and
> >>>> +a completely unreasonable expectation from end-users.
> >>>
> >>> I'm not sure I follow here.
> >>>
> >>> Which devices do you envisage this being the case for?
> >>>
> >>> Outside of debug scenarios when would you envisage we do this?
> >>>
> >>
> >> We already have to do this on the beaglebone black. The onboard EMMC and HDMI
> >> devices conflict with any capes that use the same pins. So you have to
> >> have a way to disable them so that the attached cape will work.
> >>
> >>> For the debug case it seems reasonable to have command line parameters
> >>> to get the kernel to do what we want. Just because there's a device in
> >>> the DTB that's useful in a debug scenario doesn't mean we have to use it
> >>> by default.
> >>
> >> I don’t follow. Users need this functionality to work. I.e. pass a command
> >> line option to use different OPPs etc. Real world usage is messy.
> >>
> >>>
> >>>> +Device Tree quirks solve all those problems by having an in-kernel interface
> >>>> +which per-board/platform method can use to selectively modify the device tree
> >>>> +right after unflattening.
> >>>> +
> >>>> +A DT quirk is a subtree of the boot DT that can be applied to
> >>>> +a target in the base DT resulting in a modification of the live
> >>>> +tree. The format of the quirk nodes is that of a device tree overlay.
> >>>> +
> >>>> +As an example the following DTS contains a quirk.
> >>>> +
> >>>> +/ {
> >>>> + foo: foo-node {
> >>>> + bar = <10>;
> >>>> + };
> >>>> +
> >>>> + select-quirk = <&quirk>;
> >>>> +
> >>>> + quirk: quirk {
> >>>> + fragment@0 {
> >>>> + target = <&foo>;
> >>>> + __overlay {
> >>>> + bar = <0xf00>;
> >>>> + baz = <11>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +};
> >>>> +
> >>>> +The quirk when applied would result at the following tree:
> >>>> +
> >>>> +/ {
> >>>> + foo: foo-node {
> >>>> + bar = <0xf00>;
> >>>> + baz = <11>;
> >>>> + };
> >>>> +
> >>>> + select-quirk = <&quirk>;
> >>>> +
> >>>> + quirk: quirk {
> >>>> + fragment@0 {
> >>>> + target = <&foo>;
> >>>> + __overlay {
> >>>> + bar = <0xf00>;
> >>>> + baz = <11>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +
> >>>> +};
> >>>> +
> >>>> +The two public method used to accomplish this are of_quirk_apply_by_node()
> >>>> +and of_quirk_apply_by_phandle();
> >>>> +
> >>>> +To apply the quirk, a per-platform method can retrieve the phandle from the
> >>>> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
> >>>> +
> >>>> +The method which the per-platform method is using to select the quirk to apply
> >>>> +is out of the scope of the DT quirk definition, but possible methods include
> >>>> +and are not limited to: revision encoding in a GPIO input range, board id
> >>>> +located in external or internal EEPROM or flash, DMI board ids, etc.
> >>>
> >>> It seems rather unfortunate that to get a useful device tree we have to
> >>> resort to board-specific mechanisms. That means yet more platform code,
> >>> which is rather unfortunate. This would also require any other DT users
> >>> to understand and potentially have to sanitize any quirks (e.g. in the
> >>> case of Xen).
> >>
> >> The original internal version of the patches used early platform devices and
> >> a generic firmware quirk mechanism, but I was directed to the per-platform
> >> method instead. It is perfectly doable to go back at the original implementation
> >> but I need to get the ball rolling with a discussion about the internals.
> >
> > I also think we should used early platform devices to not add platform
> > specific code. What were the cons to swith to per-platform method?
> >
>
> Supposedly easier to accept. /me shrugs
>
> >>
> >>>
> >>> Mark.
> >>
> >> Regards
> >>
> >> — Pantelis
> >>
> >
> > Regards
> >
> > Ludovic
>
> Regards
>
> — Pantelis
>

2015-02-18 17:31:52

by Mark Rutland

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

> >> +While this may in theory work, in practice it is very cumbersome
> >> +for the following reasons:
> >> +
> >> +1. The act of selecting a different boot device tree blob requires
> >> +a reasonably advanced bootloader with some kind of configuration or
> >> +scripting capabilities. Sadly this is not the case many times, the
> >> +bootloader is extremely dumb and can only use a single dt blob.
> >
> > You can have several bootloader builds, or even a single build with
> > something like appended DTB to get an appropriate DTB if the same binary
> > will otherwise work across all variants of a board.
> >
>
> No, the same DTB will not work across all the variants of a board.

I wasn't on about the DTB. I was on about the loader binary, in the case
the FW/bootloader could be common even if the DTB couldn't.

To some extent there must be a DTB that will work across all variants
(albeit with limited utility) or the quirk approach wouldn't work...

> > So it's not necessarily true that you need a complex bootloader.
> >
>
> >> +2. On many instances boot time is extremely critical; in some cases
> >> +there are hard requirements like having working video feeds in under
> >> +2 seconds from power-up. This leaves an extremely small time budget for
> >> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> >> +is by removing the standard bootloader from the normal boot sequence
> >> +altogether by having a very small boot shim that loads the kernel and
> >> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> >
> > Given my previous comments above I don't see why this is relevant.
> > You're already passing _some_ DTB here, so if you can organise for the
> > board to statically provide a sane DTB that's fine, or you can resort to
> > appended DTB if it's not possible to update the board configuration.
> >
>
> You’re missing the point. I can’t use the same DTB for each revision of the
> board. Each board is similar but it’s not identical.

I think you've misunderstood my point. If you program the board with the
relevant DTB, or use appended DTB, then you will pass the correct DTB to
the kernel without need for quirks.

I understand that each variant is somewhat incompatible (and hence needs
its own DTB).

> >> +3. Having different DTBs/DTSs for different board revisions easily leads to
> >> +drift between versions. Since no developer is expected to have every single
> >> +board revision at hand, things are easy to get out of sync, with board versions
> >> +failing to boot even though the kernel is up to date.
> >
> > I'm not sure I follow. Surely if you don't have every board revision at
> > hand you can't test quirks exhaustively either?
> >
>
> It’s one less thing to worry about. For example in the current mainline kernel
> already there is a drift between the beaglebone white and the beaglebone black.

What's currently out of sync?

> Having the same DTS is just easier to keep things in sync.

We have dtsi files to keep this common stuff in sync currently. It
sounds like we need to factor something out to bring beaglebone black
and beaglebone white back in sync.

> > Additionally you face the problem that two boards of the same variant
> > could have different base DTBs that you would need to test that each
> > board's quirks worked for a range of base DTBs.
> >
>
> This is not a valid case. This patch is about boards that have the same base DTB.

Hmm. I mangled my words here.

Say you have a DTB for some set of boards, relying on quirks. A new
board comes out requiring both updates to the DT and the quirk detection
mechanism. Now you need to test that the new detection code works with
the old boards with the old DTB, in addition to all the boards with the
new DTB.

If anything is updated, testing can't be avoided, regardless of this
quirks mechanism.

> >> +4. One final problem is the static way that device tree works.
> >> +For example it might be desirable for various boards to have a way to
> >> +selectively configure the boot device tree, possibly by the use of command
> >> +line options. For instance a device might be disabled if a given command line
> >> +option is present, different configuration to various devices for debugging
> >> +purposes can be selected and so on. Currently the only way to do so is by
> >> +recompiling the DTS and installing, which is an chore for developers and
> >> +a completely unreasonable expectation from end-users.
> >
> > I'm not sure I follow here.
> >
> > Which devices do you envisage this being the case for?
> >
> > Outside of debug scenarios when would you envisage we do this?
> >
>
> We already have to do this on the beaglebone black. The onboard EMMC and HDMI
> devices conflict with any capes that use the same pins. So you have to
> have a way to disable them so that the attached cape will work.

Surely the boot-time quirks are independent of the cape/overlay cases?

If you pass a sane DTB to begin with for the board then things should
work, and if you override with an overlay then that should work
regardless of whatever the boot-time configuration was, no?

> > For the debug case it seems reasonable to have command line parameters
> > to get the kernel to do what we want. Just because there's a device in
> > the DTB that's useful in a debug scenario doesn't mean we have to use it
> > by default.
>
> I don’t follow. Users need this functionality to work. I.e. pass a command
> line option to use different OPPs etc. Real world usage is messy.

This was all about the debug case; for anything that's required for
normal operation I agree that a command line argument doesn't work.

> >> +Device Tree quirks solve all those problems by having an in-kernel interface
> >> +which per-board/platform method can use to selectively modify the device tree
> >> +right after unflattening.
> >> +
> >> +A DT quirk is a subtree of the boot DT that can be applied to
> >> +a target in the base DT resulting in a modification of the live
> >> +tree. The format of the quirk nodes is that of a device tree overlay.
> >> +
> >> +As an example the following DTS contains a quirk.
> >> +
> >> +/ {
> >> + foo: foo-node {
> >> + bar = <10>;
> >> + };
> >> +
> >> + select-quirk = <&quirk>;
> >> +
> >> + quirk: quirk {
> >> + fragment@0 {
> >> + target = <&foo>;
> >> + __overlay {
> >> + bar = <0xf00>;
> >> + baz = <11>;
> >> + };
> >> + };
> >> + };
> >> +};
> >> +
> >> +The quirk when applied would result at the following tree:
> >> +
> >> +/ {
> >> + foo: foo-node {
> >> + bar = <0xf00>;
> >> + baz = <11>;
> >> + };
> >> +
> >> + select-quirk = <&quirk>;
> >> +
> >> + quirk: quirk {
> >> + fragment@0 {
> >> + target = <&foo>;
> >> + __overlay {
> >> + bar = <0xf00>;
> >> + baz = <11>;
> >> + };
> >> + };
> >> + };
> >> +
> >> +};
> >> +
> >> +The two public method used to accomplish this are of_quirk_apply_by_node()
> >> +and of_quirk_apply_by_phandle();
> >> +
> >> +To apply the quirk, a per-platform method can retrieve the phandle from the
> >> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
> >> +
> >> +The method which the per-platform method is using to select the quirk to apply
> >> +is out of the scope of the DT quirk definition, but possible methods include
> >> +and are not limited to: revision encoding in a GPIO input range, board id
> >> +located in external or internal EEPROM or flash, DMI board ids, etc.
> >
> > It seems rather unfortunate that to get a useful device tree we have to
> > resort to board-specific mechanisms. That means yet more platform code,
> > which is rather unfortunate. This would also require any other DT users
> > to understand and potentially have to sanitize any quirks (e.g. in the
> > case of Xen).
>
> The original internal version of the patches used early platform devices and
> a generic firmware quirk mechanism, but I was directed to the per-platform
> method instead. It is perfectly doable to go back at the original implementation
> but I need to get the ball rolling with a discussion about the internals.

Either way my concern is the same; some platform-specific code (be it in
arch/arm or drivers/) is potentially required in order to get a usable
DTB of any level of functionality.

Thanks,
Mark.

2015-02-18 19:33:25

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Wed, Feb 18, 2015 at 05:31:16PM +0000, Mark Rutland wrote:
> > >> +While this may in theory work, in practice it is very cumbersome
> > >> +for the following reasons:
> > >> +
> > >> +1. The act of selecting a different boot device tree blob requires
> > >> +a reasonably advanced bootloader with some kind of configuration or
> > >> +scripting capabilities. Sadly this is not the case many times, the
> > >> +bootloader is extremely dumb and can only use a single dt blob.
> > >
> > > You can have several bootloader builds, or even a single build with
> > > something like appended DTB to get an appropriate DTB if the same binary
> > > will otherwise work across all variants of a board.
> > >
> >
> > No, the same DTB will not work across all the variants of a board.
>
> I wasn't on about the DTB. I was on about the loader binary, in the case
> the FW/bootloader could be common even if the DTB couldn't.
>
> To some extent there must be a DTB that will work across all variants
> (albeit with limited utility) or the quirk approach wouldn't work...
>

Not necessarily. I have another use case: A cpu card can be plugged into
multiple carrier card variants, each with different functionality.
At production time, it is not known which carrier card the CPU card
will be plugged in. It is not feasible for manufacturing to update
the dtb files after the cpu card has been plugged into the carrier,
since both are manufactured separately and plugged together at a
later step in the production process (and may be swapped around later).

External overlays do not work in this case because those have to be loaded
through the carrier card. A common dtb file as proposed here would be an
elegant solution for that problem.

Guenter

2015-02-19 02:08:33

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
> Implement a method of applying DT quirks early in the boot sequence.
>
> A DT quirk is a subtree of the boot DT that can be applied to
> a target in the base DT resulting in a modification of the live
> tree. The format of the quirk nodes is that of a device tree overlay.

The use of the word "quirk" is a different mental model for me than what
this patch series appears to be addressing. I would suggest totally
removing the word "quirk" from this proposal to avoid confusing the
mental models of future generations of kernel folks.

What this patch series seems to be proposing is a method to apply DT
overlays as soon as unflatten_device_tree() completes. In other words,
making the device tree a dynamic object, that is partially defined by
the kernel during boot. Well, to be fair, the kernel chooses among
several possible alternatives encoded in the DT blob. So the device
tree is no longer a static object that describes the hardware of the
system. It may not sound like a big deal, but it seems to me to be
a fundamental shift in what the device tree blob is. Something that
should be thought about carefully and not just applied as a patch to
solve a point problem.

The stated use of this proposal is to create dynamic DT blobs that can
describe many similar variants of a given system instead of creating
unique DT blobs for each different system.

I obviously have not thought through the architectural implications yet,
but just a quick example. One of the issues we have been trying to fix
is device tree validation. The not yet existent (except as a few proof
of concept attempts) validator would need to validate a device tree
for each dynamic variant. Probably not a big deal, but an example of
the ripple effects this conceptual change implies.

A second function that this patch is proposing is a method to enable
or disable devices via command line options. If I understand
correctly, this is meant to solve a problem with run time overlays
that require disabling a device previously enabled by the DT blob.
If so, it seems like it could easily be implemented in a simpler
generic way than in the board specific code in this patch series.

I share the concerns that Mark Rutland has expressed in his comments
about this series.

< snip >

I have read through the patches and will have comments on the code
later if this proposal is seen as a good idea.

-Frank

2015-02-19 14:29:56

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Mark,

> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>
>>>> +While this may in theory work, in practice it is very cumbersome
>>>> +for the following reasons:
>>>> +
>>>> +1. The act of selecting a different boot device tree blob requires
>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>
>>> You can have several bootloader builds, or even a single build with
>>> something like appended DTB to get an appropriate DTB if the same binary
>>> will otherwise work across all variants of a board.
>>>
>>
>> No, the same DTB will not work across all the variants of a board.
>
> I wasn't on about the DTB. I was on about the loader binary, in the case
> the FW/bootloader could be common even if the DTB couldn't.
>
> To some extent there must be a DTB that will work across all variants
> (albeit with limited utility) or the quirk approach wouldn't work…
>

That’s not correct; the only part of the DTB that needs to be common
is the model property that would allow the quirk detection logic to fire.

So, there is a base DTB that will work on all variants, but that only means
that it will work only up to the point that the quirk detector method
can work. So while in recommended practice there are common subsets
of the DTB that might work, they might be unsafe.

For instance on the beaglebone the regulator configuration is different
between white and black, it is imperative you get them right otherwise
you risk board damage.

>>> So it's not necessarily true that you need a complex bootloader.
>>>
>>
>>>> +2. On many instances boot time is extremely critical; in some cases
>>>> +there are hard requirements like having working video feeds in under
>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>> +is by removing the standard bootloader from the normal boot sequence
>>>> +altogether by having a very small boot shim that loads the kernel and
>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>
>>> Given my previous comments above I don't see why this is relevant.
>>> You're already passing _some_ DTB here, so if you can organise for the
>>> board to statically provide a sane DTB that's fine, or you can resort to
>>> appended DTB if it's not possible to update the board configuration.
>>>
>>
>> You’re missing the point. I can’t use the same DTB for each revision of the
>> board. Each board is similar but it’s not identical.
>
> I think you've misunderstood my point. If you program the board with the
> relevant DTB, or use appended DTB, then you will pass the correct DTB to
> the kernel without need for quirks.
>
> I understand that each variant is somewhat incompatible (and hence needs
> its own DTB).

In theory it might work, in practice this does not. Ludovic mentioned that they
have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
that’s 27x60k = 1.6MB of DTBs, that need to be installed.

>
>>>> +3. Having different DTBs/DTSs for different board revisions easily leads to
>>>> +drift between versions. Since no developer is expected to have every single
>>>> +board revision at hand, things are easy to get out of sync, with board versions
>>>> +failing to boot even though the kernel is up to date.
>>>
>>> I'm not sure I follow. Surely if you don't have every board revision at
>>> hand you can't test quirks exhaustively either?
>>>
>>
>> It’s one less thing to worry about. For example in the current mainline kernel
>> already there is a drift between the beaglebone white and the beaglebone black.
>
> What's currently out of sync?
>
>> Having the same DTS is just easier to keep things in sync.
>
> We have dtsi files to keep this common stuff in sync currently. It
> sounds like we need to factor something out to bring beaglebone black
> and beaglebone white back in sync.
>

Yes, they need to. Unfortunately it is very easy for DTBs to get out of sync
as it appears in practice.

>>> Additionally you face the problem that two boards of the same variant
>>> could have different base DTBs that you would need to test that each
>>> board's quirks worked for a range of base DTBs.
>>>
>>
>> This is not a valid case. This patch is about boards that have the same base DTB.
>
> Hmm. I mangled my words here.
>
> Say you have a DTB for some set of boards, relying on quirks. A new
> board comes out requiring both updates to the DT and the quirk detection
> mechanism. Now you need to test that the new detection code works with
> the old boards with the old DTB, in addition to all the boards with the
> new DTB.
>
> If anything is updated, testing can't be avoided, regardless of this
> quirks mechanism.
>

Obviously testing is needed everytime something is updated. In practice
the way the boards are identified tend to be similar; on am33xx boards it’s
a standard formatted EEPROM at a known address on a specific i2c bus.

On ATMEL boards it’s the same, but the EEPROM is 1wire, and so on.

>>>> +4. One final problem is the static way that device tree works.
>>>> +For example it might be desirable for various boards to have a way to
>>>> +selectively configure the boot device tree, possibly by the use of command
>>>> +line options. For instance a device might be disabled if a given command line
>>>> +option is present, different configuration to various devices for debugging
>>>> +purposes can be selected and so on. Currently the only way to do so is by
>>>> +recompiling the DTS and installing, which is an chore for developers and
>>>> +a completely unreasonable expectation from end-users.
>>>
>>> I'm not sure I follow here.
>>>
>>> Which devices do you envisage this being the case for?
>>>
>>> Outside of debug scenarios when would you envisage we do this?
>>>
>>
>> We already have to do this on the beaglebone black. The onboard EMMC and HDMI
>> devices conflict with any capes that use the same pins. So you have to
>> have a way to disable them so that the attached cape will work.
>
> Surely the boot-time quirks are independent of the cape/overlay cases?
>
> If you pass a sane DTB to begin with for the board then things should
> work, and if you override with an overlay then that should work
> regardless of whatever the boot-time configuration was, no?
>

No. There is no such thing as a ‘sane’ DTB that will work under every case.

Let me expand on the specific HDMI case for the beaglebone black.

The black has an on board HDMI framer that’s connected to the LCDC interface.
However the LCDC pins are brought out to the dual connectors and a add-on
LCD cape can be attached.

For it to work, the LCDC driver must be disabled in the boot DTB so that
the overlay that enables the LCD cape can work.

But since the image drop for both case is the same you have to be able to
disable the HDMI using a command line option, instead of installing a different
DTB.

>>> For the debug case it seems reasonable to have command line parameters
>>> to get the kernel to do what we want. Just because there's a device in
>>> the DTB that's useful in a debug scenario doesn't mean we have to use it
>>> by default.
>>
>> I don’t follow. Users need this functionality to work. I.e. pass a command
>> line option to use different OPPs etc. Real world usage is messy.
>
> This was all about the debug case; for anything that's required for
> normal operation I agree that a command line argument doesn't work.
>
>>>> +Device Tree quirks solve all those problems by having an in-kernel interface
>>>> +which per-board/platform method can use to selectively modify the device tree
>>>> +right after unflattening.
>>>> +
>>>> +A DT quirk is a subtree of the boot DT that can be applied to
>>>> +a target in the base DT resulting in a modification of the live
>>>> +tree. The format of the quirk nodes is that of a device tree overlay.
>>>> +
>>>> +As an example the following DTS contains a quirk.
>>>> +
>>>> +/ {
>>>> + foo: foo-node {
>>>> + bar = <10>;
>>>> + };
>>>> +
>>>> + select-quirk = <&quirk>;
>>>> +
>>>> + quirk: quirk {
>>>> + fragment@0 {
>>>> + target = <&foo>;
>>>> + __overlay {
>>>> + bar = <0xf00>;
>>>> + baz = <11>;
>>>> + };
>>>> + };
>>>> + };
>>>> +};
>>>> +
>>>> +The quirk when applied would result at the following tree:
>>>> +
>>>> +/ {
>>>> + foo: foo-node {
>>>> + bar = <0xf00>;
>>>> + baz = <11>;
>>>> + };
>>>> +
>>>> + select-quirk = <&quirk>;
>>>> +
>>>> + quirk: quirk {
>>>> + fragment@0 {
>>>> + target = <&foo>;
>>>> + __overlay {
>>>> + bar = <0xf00>;
>>>> + baz = <11>;
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> +};
>>>> +
>>>> +The two public method used to accomplish this are of_quirk_apply_by_node()
>>>> +and of_quirk_apply_by_phandle();
>>>> +
>>>> +To apply the quirk, a per-platform method can retrieve the phandle from the
>>>> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
>>>> +
>>>> +The method which the per-platform method is using to select the quirk to apply
>>>> +is out of the scope of the DT quirk definition, but possible methods include
>>>> +and are not limited to: revision encoding in a GPIO input range, board id
>>>> +located in external or internal EEPROM or flash, DMI board ids, etc.
>>>
>>> It seems rather unfortunate that to get a useful device tree we have to
>>> resort to board-specific mechanisms. That means yet more platform code,
>>> which is rather unfortunate. This would also require any other DT users
>>> to understand and potentially have to sanitize any quirks (e.g. in the
>>> case of Xen).
>>
>> The original internal version of the patches used early platform devices and
>> a generic firmware quirk mechanism, but I was directed to the per-platform
>> method instead. It is perfectly doable to go back at the original implementation
>> but I need to get the ball rolling with a discussion about the internals.
>
> Either way my concern is the same; some platform-specific code (be it in
> arch/arm or drivers/) is potentially required in order to get a usable
> DTB of any level of functionality.
>

Yes, platform specific code is required, but that’s a given for any platform no?

> Thanks,
> Mark.

Regards

— Pantelis

2015-02-19 14:41:52

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Frank,

> On Feb 19, 2015, at 04:08 , Frank Rowand <[email protected]> wrote:
>
> On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
>> Implement a method of applying DT quirks early in the boot sequence.
>>
>> A DT quirk is a subtree of the boot DT that can be applied to
>> a target in the base DT resulting in a modification of the live
>> tree. The format of the quirk nodes is that of a device tree overlay.
>
> The use of the word "quirk" is a different mental model for me than what
> this patch series appears to be addressing. I would suggest totally
> removing the word "quirk" from this proposal to avoid confusing the
> mental models of future generations of kernel folks.
>

Naming things is hard to do. Suggestions?

> What this patch series seems to be proposing is a method to apply DT
> overlays as soon as unflatten_device_tree() completes. In other words,
> making the device tree a dynamic object, that is partially defined by
> the kernel during boot. Well, to be fair, the kernel chooses among
> several possible alternatives encoded in the DT blob. So the device
> tree is no longer a static object that describes the hardware of the
> system. It may not sound like a big deal, but it seems to me to be
> a fundamental shift in what the device tree blob is. Something that
> should be thought about carefully and not just applied as a patch to
> solve a point problem.
>

There is a fundamental shift going on about what hardware is. It is nowhere
as static as it used to be. It is time for the kernel to keep up.

> The stated use of this proposal is to create dynamic DT blobs that can
> describe many similar variants of a given system instead of creating
> unique DT blobs for each different system.
>

Yes.

> I obviously have not thought through the architectural implications yet,
> but just a quick example. One of the issues we have been trying to fix
> is device tree validation. The not yet existent (except as a few proof
> of concept attempts) validator would need to validate a device tree
> for each dynamic variant. Probably not a big deal, but an example of
> the ripple effects this conceptual change implies.
>

I don?t see what the big problem with the validator is. The ?quirk?
are easily identified by the presence of the __overlay__ nodes and
the validator can apply each overlay and perform the validation check
at each resultant tree.

> A second function that this patch is proposing is a method to enable
> or disable devices via command line options. If I understand
> correctly, this is meant to solve a problem with run time overlays
> that require disabling a device previously enabled by the DT blob.
> If so, it seems like it could easily be implemented in a simpler
> generic way than in the board specific code in this patch series.
>

Disabling a device is the most common case, but other options are desired
too. For instance changing OPPs by a command line option, etc.

> I share the concerns that Mark Rutland has expressed in his comments
> about this series.
>
> < snip >
>
> I have read through the patches and will have comments on the code
> later if this proposal is seen as a good idea.
>

OK

> -Frank

Regards

? Pantelis

2015-02-19 16:40:38

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 2/19/2015 6:41 AM, Pantelis Antoniou wrote:
> Hi Frank,
>
>> On Feb 19, 2015, at 04:08 , Frank Rowand <[email protected]> wrote:
>>
>> On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
>>> Implement a method of applying DT quirks early in the boot sequence.
>>>
>>> A DT quirk is a subtree of the boot DT that can be applied to
>>> a target in the base DT resulting in a modification of the live
>>> tree. The format of the quirk nodes is that of a device tree overlay.
>>
>> The use of the word "quirk" is a different mental model for me than what
>> this patch series appears to be addressing. I would suggest totally
>> removing the word "quirk" from this proposal to avoid confusing the
>> mental models of future generations of kernel folks.
>>
>
> Naming things is hard to do. Suggestions?

You are inviting me to bikeshed. I'll leave that to you.

>
>> What this patch series seems to be proposing is a method to apply DT
>> overlays as soon as unflatten_device_tree() completes. In other words,
>> making the device tree a dynamic object, that is partially defined by
>> the kernel during boot. Well, to be fair, the kernel chooses among
>> several possible alternatives encoded in the DT blob. So the device
>> tree is no longer a static object that describes the hardware of the
>> system. It may not sound like a big deal, but it seems to me to be
>> a fundamental shift in what the device tree blob is. Something that
>> should be thought about carefully and not just applied as a patch to
>> solve a point problem.
>>
>
> There is a fundamental shift going on about what hardware is. It is nowhere
> as static as it used to be. It is time for the kernel to keep up.

Run time overlays do that.

The problem you seem to be dealing with here is that you want a single
DT blob that can be installed on many different _physical_ boards.

>
>> The stated use of this proposal is to create dynamic DT blobs that can
>> describe many similar variants of a given system instead of creating
>> unique DT blobs for each different system.
>>
>
> Yes.
>
>> I obviously have not thought through the architectural implications yet,
>> but just a quick example. One of the issues we have been trying to fix
>> is device tree validation. The not yet existent (except as a few proof
>> of concept attempts) validator would need to validate a device tree
>> for each dynamic variant. Probably not a big deal, but an example of
>> the ripple effects this conceptual change implies.
>>
>
> I don?t see what the big problem with the validator is. The ?quirk?
> are easily identified by the presence of the __overlay__ nodes and
> the validator can apply each overlay and perform the validation check
> at each resultant tree.

I said "not a big deal". I was trying to make people think about the
bigger picture. Defending that this is a non-issue for the validator
is totally missing my point. Step back and think architecturally
about the big picture. I do _not_ know if this is a problem, but
they will be ripples from this proposal.

>
>> A second function that this patch is proposing is a method to enable
>> or disable devices via command line options. If I understand
>> correctly, this is meant to solve a problem with run time overlays
>> that require disabling a device previously enabled by the DT blob.
>> If so, it seems like it could easily be implemented in a simpler
>> generic way than in the board specific code in this patch series.
>>
>
> Disabling a device is the most common case, but other options are desired
> too. For instance changing OPPs by a command line option, etc.

The device tree is supposed to describe what the hardware is. Why would
you want a command line option to change what OPPs are possible for the
hardware?

>
>> I share the concerns that Mark Rutland has expressed in his comments
>> about this series.
>>
>> < snip >
>>
>> I have read through the patches and will have comments on the code
>> later if this proposal is seen as a good idea.
>>
>
> OK
>
>> -Frank
>
> Regards
>
> ? Pantelis
>
>

2015-02-19 16:48:44

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
> Hi Mark,
>
>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>
>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>> +for the following reasons:
>>>>> +
>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>
>>>> You can have several bootloader builds, or even a single build with
>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>> will otherwise work across all variants of a board.
>>>>
>>>
>>> No, the same DTB will not work across all the variants of a board.
>>
>> I wasn't on about the DTB. I was on about the loader binary, in the case
>> the FW/bootloader could be common even if the DTB couldn't.
>>
>> To some extent there must be a DTB that will work across all variants
>> (albeit with limited utility) or the quirk approach wouldn't work…
>>
>
> That’s not correct; the only part of the DTB that needs to be common
> is the model property that would allow the quirk detection logic to fire.
>
> So, there is a base DTB that will work on all variants, but that only means
> that it will work only up to the point that the quirk detector method
> can work. So while in recommended practice there are common subsets
> of the DTB that might work, they might be unsafe.
>
> For instance on the beaglebone the regulator configuration is different
> between white and black, it is imperative you get them right otherwise
> you risk board damage.
>
>>>> So it's not necessarily true that you need a complex bootloader.
>>>>
>>>
>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>> +there are hard requirements like having working video feeds in under
>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>
>>>> Given my previous comments above I don't see why this is relevant.
>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>> appended DTB if it's not possible to update the board configuration.
>>>>
>>>
>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>> board. Each board is similar but it’s not identical.
>>
>> I think you've misunderstood my point. If you program the board with the
>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>> the kernel without need for quirks.
>>
>> I understand that each variant is somewhat incompatible (and hence needs
>> its own DTB).
>
> In theory it might work, in practice this does not. Ludovic mentioned that they
> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
> that’s 27x60k = 1.6MB of DTBs, that need to be installed.

< snip >

Or you can install the correct DTB on the board. You trust your manufacturing line
to install the correct resistors. You trust your manufacturing line to install the
correct kernel version (eg an updated version to resolve a security issue).

I thought the DT blob was supposed to follow the same standard that other OS's or
bootloaders understood. Are you willing to break that? (This is one of those
ripples I mentioned in my other emails.)

-Frank

2015-02-19 16:51:22

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 2/19/2015 8:40 AM, Frank Rowand wrote:
> On 2/19/2015 6:41 AM, Pantelis Antoniou wrote:
>> Hi Frank,
>>
>>> On Feb 19, 2015, at 04:08 , Frank Rowand <[email protected]> wrote:
>>>
>>> On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
>>>> Implement a method of applying DT quirks early in the boot sequence.
>>>>
>>>> A DT quirk is a subtree of the boot DT that can be applied to
>>>> a target in the base DT resulting in a modification of the live
>>>> tree. The format of the quirk nodes is that of a device tree overlay.
>>>
>>> The use of the word "quirk" is a different mental model for me than what
>>> this patch series appears to be addressing. I would suggest totally
>>> removing the word "quirk" from this proposal to avoid confusing the
>>> mental models of future generations of kernel folks.
>>>
>>
>> Naming things is hard to do. Suggestions?
>
> You are inviting me to bikeshed. I'll leave that to you.
>
>>
>>> What this patch series seems to be proposing is a method to apply DT
>>> overlays as soon as unflatten_device_tree() completes. In other words,
>>> making the device tree a dynamic object, that is partially defined by
>>> the kernel during boot. Well, to be fair, the kernel chooses among
>>> several possible alternatives encoded in the DT blob. So the device
>>> tree is no longer a static object that describes the hardware of the
>>> system. It may not sound like a big deal, but it seems to me to be
>>> a fundamental shift in what the device tree blob is. Something that
>>> should be thought about carefully and not just applied as a patch to
>>> solve a point problem.
>>>
>>
>> There is a fundamental shift going on about what hardware is. It is nowhere
>> as static as it used to be. It is time for the kernel to keep up.
>
> Run time overlays do that.
>
> The problem you seem to be dealing with here is that you want a single
> DT blob that can be installed on many different _physical_ boards.
>
>
>>
>>> The stated use of this proposal is to create dynamic DT blobs that can
>>> describe many similar variants of a given system instead of creating
>>> unique DT blobs for each different system.
>>>
>>
>> Yes.
>>
>>> I obviously have not thought through the architectural implications yet,
>>> but just a quick example. One of the issues we have been trying to fix
>>> is device tree validation. The not yet existent (except as a few proof
>>> of concept attempts) validator would need to validate a device tree
>>> for each dynamic variant. Probably not a big deal, but an example of
>>> the ripple effects this conceptual change implies.
>>>
>>
>> I don?t see what the big problem with the validator is. The ?quirk?
>> are easily identified by the presence of the __overlay__ nodes and
>> the validator can apply each overlay and perform the validation check
>> at each resultant tree.
>
> I said "not a big deal". I was trying to make people think about the
> bigger picture. Defending that this is a non-issue for the validator
> is totally missing my point. Step back and think architecturally
> about the big picture. I do _not_ know if this is a problem, but
> they will be ripples from this proposal.

oops: there will be ripples from this proposal.

>
>>
>>> A second function that this patch is proposing is a method to enable
>>> or disable devices via command line options. If I understand
>>> correctly, this is meant to solve a problem with run time overlays
>>> that require disabling a device previously enabled by the DT blob.
>>> If so, it seems like it could easily be implemented in a simpler
>>> generic way than in the board specific code in this patch series.
>>>
>>
>> Disabling a device is the most common case, but other options are desired
>> too. For instance changing OPPs by a command line option, etc.
>
> The device tree is supposed to describe what the hardware is. Why would
> you want a command line option to change what OPPs are possible for the
> hardware?
>
>>
>>> I share the concerns that Mark Rutland has expressed in his comments
>>> about this series.
>>>
>>> < snip >
>>>
>>> I have read through the patches and will have comments on the code
>>> later if this proposal is seen as a good idea.
>>>
>>
>> OK
>>
>>> -Frank
>>
>> Regards
>>
>> ? Pantelis
>>
>>
>

2015-02-19 17:00:27

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Frank,

> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>
> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>> Hi Mark,
>>
>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>
>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>> +for the following reasons:
>>>>>> +
>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>
>>>>> You can have several bootloader builds, or even a single build with
>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>> will otherwise work across all variants of a board.
>>>>>
>>>>
>>>> No, the same DTB will not work across all the variants of a board.
>>>
>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>> the FW/bootloader could be common even if the DTB couldn't.
>>>
>>> To some extent there must be a DTB that will work across all variants
>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>
>>
>> That’s not correct; the only part of the DTB that needs to be common
>> is the model property that would allow the quirk detection logic to fire.
>>
>> So, there is a base DTB that will work on all variants, but that only means
>> that it will work only up to the point that the quirk detector method
>> can work. So while in recommended practice there are common subsets
>> of the DTB that might work, they might be unsafe.
>>
>> For instance on the beaglebone the regulator configuration is different
>> between white and black, it is imperative you get them right otherwise
>> you risk board damage.
>>
>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>
>>>>
>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>> +there are hard requirements like having working video feeds in under
>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>
>>>>> Given my previous comments above I don't see why this is relevant.
>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>
>>>>
>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>> board. Each board is similar but it’s not identical.
>>>
>>> I think you've misunderstood my point. If you program the board with the
>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>> the kernel without need for quirks.
>>>
>>> I understand that each variant is somewhat incompatible (and hence needs
>>> its own DTB).
>>
>> In theory it might work, in practice this does not. Ludovic mentioned that they
>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>
> < snip >
>
> Or you can install the correct DTB on the board. You trust your manufacturing line
> to install the correct resistors. You trust your manufacturing line to install the
> correct kernel version (eg an updated version to resolve a security issue).
>
> I thought the DT blob was supposed to follow the same standard that other OS's or
> bootloaders understood. Are you willing to break that? (This is one of those
> ripples I mentioned in my other emails.)
>

Trust no-one.

This is one of those things that the kernel community doesn’t understand which makes people
who push product quite mad.

Engineering a product is not only about meeting customer spec, in order to turn a profit
the whole endeavor must be engineered as well for manufacturability.

Yes, you can always manually install files in the bootloader. For 1 board no problem.
For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
instead of turning a profit you’re losing money if you only have a few cents of profit
per unit.

No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
for a few million units.

And frankly I don’t care what other OSes do. If you were to take a look at the sorry DT support
they have you’d be amazed.

I would be very surprised if there’s another OS out there that can boot with a late Linux DTB.

> -Frank

Regards

— Pantelis

PS. For a real use case please take a look at the answer Guenter gave on this thread a little
while back.-

2015-02-19 17:31:05

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
> Hi Frank,
>
>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>
>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>> Hi Mark,
>>>
>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>
>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>> +for the following reasons:
>>>>>>> +
>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>
>>>>>> You can have several bootloader builds, or even a single build with
>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>> will otherwise work across all variants of a board.
>>>>>>
>>>>>
>>>>> No, the same DTB will not work across all the variants of a board.
>>>>
>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>
>>>> To some extent there must be a DTB that will work across all variants
>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>
>>>
>>> That’s not correct; the only part of the DTB that needs to be common
>>> is the model property that would allow the quirk detection logic to fire.
>>>
>>> So, there is a base DTB that will work on all variants, but that only means
>>> that it will work only up to the point that the quirk detector method
>>> can work. So while in recommended practice there are common subsets
>>> of the DTB that might work, they might be unsafe.
>>>
>>> For instance on the beaglebone the regulator configuration is different
>>> between white and black, it is imperative you get them right otherwise
>>> you risk board damage.
>>>
>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>
>>>>>
>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>
>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>
>>>>>
>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>> board. Each board is similar but it’s not identical.
>>>>
>>>> I think you've misunderstood my point. If you program the board with the
>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>> the kernel without need for quirks.
>>>>
>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>> its own DTB).
>>>
>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>
>> < snip >
>>
>> Or you can install the correct DTB on the board. You trust your manufacturing line
>> to install the correct resistors. You trust your manufacturing line to install the
>> correct kernel version (eg an updated version to resolve a security issue).
>>
>> I thought the DT blob was supposed to follow the same standard that other OS's or
>> bootloaders understood. Are you willing to break that? (This is one of those
>> ripples I mentioned in my other emails.)
>>
>
> Trust no-one.
>
> This is one of those things that the kernel community doesn’t understand which makes people
> who push product quite mad.
>
> Engineering a product is not only about meeting customer spec, in order to turn a profit
> the whole endeavor must be engineered as well for manufacturability.
>
> Yes, you can always manually install files in the bootloader. For 1 board no problem.
> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
> instead of turning a profit you’re losing money if you only have a few cents of profit
> per unit.

I'm not installing physical components manually. Why would I be installing software
manually? (rhetorical question)

>
> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
> for a few million units.

And you produce a few million units before testing that the first one off the line works?

>
> And frankly I don’t care what other OSes do. If you were to take a look at the sorry DT support
> they have you’d be amazed.
>
> I would be very surprised if there’s another OS out there that can boot with a late Linux DTB.
>
>> -Frank
>
> Regards
>
> — Pantelis
>
> PS. For a real use case please take a look at the answer Guenter gave on this thread a little
> while back.
>

My previous comments were written after reading Guenter's comment.

-Frank

2015-02-19 17:39:00

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>
> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>> Hi Frank,
>>
>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>>
>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>>> Hi Mark,
>>>>
>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>>
>>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>>> +for the following reasons:
>>>>>>>> +
>>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>>
>>>>>>> You can have several bootloader builds, or even a single build with
>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>>> will otherwise work across all variants of a board.
>>>>>>>
>>>>>>
>>>>>> No, the same DTB will not work across all the variants of a board.
>>>>>
>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>>
>>>>> To some extent there must be a DTB that will work across all variants
>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>>
>>>>
>>>> That’s not correct; the only part of the DTB that needs to be common
>>>> is the model property that would allow the quirk detection logic to fire.
>>>>
>>>> So, there is a base DTB that will work on all variants, but that only means
>>>> that it will work only up to the point that the quirk detector method
>>>> can work. So while in recommended practice there are common subsets
>>>> of the DTB that might work, they might be unsafe.
>>>>
>>>> For instance on the beaglebone the regulator configuration is different
>>>> between white and black, it is imperative you get them right otherwise
>>>> you risk board damage.
>>>>
>>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>>
>>>>>>
>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>>
>>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>>
>>>>>>
>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>>> board. Each board is similar but it’s not identical.
>>>>>
>>>>> I think you've misunderstood my point. If you program the board with the
>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>>> the kernel without need for quirks.
>>>>>
>>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>>> its own DTB).
>>>>
>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>>
>>> < snip >
>>>
>>> Or you can install the correct DTB on the board. You trust your manufacturing line
>>> to install the correct resistors. You trust your manufacturing line to install the
>>> correct kernel version (eg an updated version to resolve a security issue).
>>>
>>> I thought the DT blob was supposed to follow the same standard that other OS's or
>>> bootloaders understood. Are you willing to break that? (This is one of those
>>> ripples I mentioned in my other emails.)
>>>
>>
>> Trust no-one.
>>
>> This is one of those things that the kernel community doesn’t understand which makes people
>> who push product quite mad.
>>
>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>> the whole endeavor must be engineered as well for manufacturability.
>>
>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>> instead of turning a profit you’re losing money if you only have a few cents of profit
>> per unit.
>
> I'm not installing physical components manually. Why would I be installing software
> manually? (rhetorical question)
>

Because on high volume product runs the flash comes preprogrammed and is soldered as is.

Having a single binary to flash to every revision of the board makes logistics considerably
easier.

Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
on the flash medium) takes time and is error-prone.

Factory time == money, errors == money.

>>
>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>> for a few million units.
>
> And you produce a few million units before testing that the first one off the line works?
>

The first one off the line works. The rest will get some burn in and functional testing if you’re
lucky. In many cases where the product is very cheap it might make financial sense to just ship
as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.

Hardware is hard :)

>>
>> And frankly I don’t care what other OSes do. If you were to take a look at the sorry DT support
>> they have you’d be amazed.
>>
>> I would be very surprised if there’s another OS out there that can boot with a late Linux DTB.
>>
>>> -Frank
>>
>> Regards
>>
>> — Pantelis
>>
>> PS. For a real use case please take a look at the answer Guenter gave on this thread a little
>> while back.
>>
>
> My previous comments were written after reading Guenter's comment.
>

OK, sorry for coming off a bit strong there :)

> -Frank
>

Regards

— Pantelis

2015-02-19 18:01:37

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Wed, Feb 18, 2015 at 8:08 PM, Frank Rowand <[email protected]> wrote:
> On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
>> Implement a method of applying DT quirks early in the boot sequence.
>>
>> A DT quirk is a subtree of the boot DT that can be applied to
>> a target in the base DT resulting in a modification of the live
>> tree. The format of the quirk nodes is that of a device tree overlay.
>
> The use of the word "quirk" is a different mental model for me than what
> this patch series appears to be addressing. I would suggest totally
> removing the word "quirk" from this proposal to avoid confusing the
> mental models of future generations of kernel folks.

This comes from me as quirks are a different usecase I had in mind,
but one that could use a similar mechanism. Although, in the case of
quirks, I would expect them to be overlays built into the kernel. It
would be more a way to update old dtbs.

> What this patch series seems to be proposing is a method to apply DT
> overlays as soon as unflatten_device_tree() completes. In other words,
> making the device tree a dynamic object, that is partially defined by
> the kernel during boot. Well, to be fair, the kernel chooses among
> several possible alternatives encoded in the DT blob. So the device
> tree is no longer a static object that describes the hardware of the
> system. It may not sound like a big deal, but it seems to me to be
> a fundamental shift in what the device tree blob is. Something that
> should be thought about carefully and not just applied as a patch to
> solve a point problem.

I agree. I would not want to see every board for an SOC become an
overlay for example. I think it has to be limited to truly plugable
h/w (e.g. capes) or minor changes. We just have to define what is
minor. :)

Rob

2015-02-19 18:09:36

by Maxime Bizon

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Thu, 2015-02-19 at 19:38 +0200, Pantelis Antoniou wrote:

> Having to boot and tweak the bootloader settings to select the correct
> dtb (even if it’s present on the flash medium) takes time and is
> error-prone.

Dedicate a set of GPIO for board/PCB revision detection (it only costs a
few resistors connected to VCC or GND), then use that information to
choose the correct DTB.

--
Maxime

2015-02-19 18:13:21

by Sylvain Rochet

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hello,

On Thu, Feb 19, 2015 at 07:01:59PM +0100, Maxime Bizon wrote:
> On Thu, 2015-02-19 at 19:38 +0200, Pantelis Antoniou wrote:
>
> > Having to boot and tweak the bootloader settings to select the correct
> > dtb (even if it’s present on the flash medium) takes time and is
> > error-prone.
>
> Dedicate a set of GPIO for board/PCB revision detection (it only costs a
> few resistors connected to VCC or GND), then use that information to
> choose the correct DTB.

Or use a 1-wire or I2C EEPROM to store your board information.

Or, even better, if you have an I2C device, just chose a different
address on each board for this device and then probe I2C devices in your
boot loader until you found one you are looking for, this way, you don't
need spare GPIO at all. You don't even need to populate the same I2C
device on all boards, you can actually probe anything.

Sylvain

Attachments:

(No filename) (888.00 B)
signature.asc (198.00 B)
Digital signature Download all attachments

2015-02-19 18:13:03

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Thu, Feb 19, 2015 at 12:01:14PM -0600, Rob Herring wrote:
> On Wed, Feb 18, 2015 at 8:08 PM, Frank Rowand <[email protected]> wrote:
> > On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
> >> Implement a method of applying DT quirks early in the boot sequence.
> >>
> >> A DT quirk is a subtree of the boot DT that can be applied to
> >> a target in the base DT resulting in a modification of the live
> >> tree. The format of the quirk nodes is that of a device tree overlay.
> >
> > The use of the word "quirk" is a different mental model for me than what
> > this patch series appears to be addressing. I would suggest totally
> > removing the word "quirk" from this proposal to avoid confusing the
> > mental models of future generations of kernel folks.
>
> This comes from me as quirks are a different usecase I had in mind,
> but one that could use a similar mechanism. Although, in the case of
> quirks, I would expect them to be overlays built into the kernel. It
> would be more a way to update old dtbs.
>
> > What this patch series seems to be proposing is a method to apply DT
> > overlays as soon as unflatten_device_tree() completes. In other words,
> > making the device tree a dynamic object, that is partially defined by
> > the kernel during boot. Well, to be fair, the kernel chooses among
> > several possible alternatives encoded in the DT blob. So the device
> > tree is no longer a static object that describes the hardware of the
> > system. It may not sound like a big deal, but it seems to me to be
> > a fundamental shift in what the device tree blob is. Something that
> > should be thought about carefully and not just applied as a patch to
> > solve a point problem.
>
> I agree. I would not want to see every board for an SOC become an
> overlay for example. I think it has to be limited to truly plugable
> h/w (e.g. capes) or minor changes. We just have to define what is
> minor. :)
>

Everything that isn't OIR capable but fixed at boot time. OIR can and
should be handled with "real" overlays. OIR implies removal; I would
assume that what is discused here is insertion-only, and runtime removal
is neither required nor wanted. Or at least that is our use case.

Guenter

2015-02-19 18:41:31

by Tony Lindgren

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

* Pantelis Antoniou <[email protected]> [150218 07:03]:
> Implement DT quirks for the am33xx beaglebone boards.
> --- /dev/null
> +++ b/arch/arm/mach-omap2/am33xx-dt-quirks.c
...
> +
> +/*
> + * The board IDs for am33xx board are in an I2C EEPROM
> + * We are very early in the boot process so we have to
> + * read the EEPROM directly without using the I2C layer.
> + *
> + * Note that we rely on the bootloader setting up the muxes
> + * (which is the case for u-boot).
> + */
> +
> +/* I2C Status Register (OMAP_I2C_STAT): */
> +#define OMAP_I2C_STAT_XDR (1 << 14) /* TX Buffer draining */
> +#define OMAP_I2C_STAT_RDR (1 << 13) /* RX Buffer draining */
> +#define OMAP_I2C_STAT_BB (1 << 12) /* Bus busy */
> +#define OMAP_I2C_STAT_ROVR (1 << 11) /* Receive overrun */
> +#define OMAP_I2C_STAT_XUDF (1 << 10) /* Transmit underflow */
> +#define OMAP_I2C_STAT_AAS (1 << 9) /* Address as slave */
> +#define OMAP_I2C_STAT_BF (1 << 8) /* Bus Free */
> +#define OMAP_I2C_STAT_XRDY (1 << 4) /* Transmit data ready */
> +#define OMAP_I2C_STAT_RRDY (1 << 3) /* Receive data ready */
> +#define OMAP_I2C_STAT_ARDY (1 << 2) /* Register access ready */
> +#define OMAP_I2C_STAT_NACK (1 << 1) /* No ack interrupt enable */
> +#define OMAP_I2C_STAT_AL (1 << 0) /* Arbitration lost int ena */
...

Uhh I don't like the idea of duplicating the i2c-omap.c driver under
arch/arm.. And in general we should initialize things later rather
than earlier.

What's stopping doing these quirk checks later on time with just
a regular device driver, something like drivers/misc/bbone-quirks.c?

Regards,

Tony

2015-02-19 18:22:58

by Maxime Bizon

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Thu, 2015-02-19 at 19:12 +0100, Sylvain Rochet wrote:

> Or use a 1-wire or I2C EEPROM to store your board information.

no, you don't reduce the human error probability.

eeprom needs to be preprogrammed, factory will at some point have a lot
of eeprom with different version, and will potentially equip the wrong
one.

much more error prone than them putting the resistor at the wrong place.

> Or, even better, if you have an I2C device, just chose a different
> address on each board for this device and then probe I2C devices in your
> boot loader until you found one you are looking for, this way, you don't
> need spare GPIO at all. You don't even need to populate the same I2C
> device on all boards, you can actually probe anything.

I'm not a fan of schemes where not probing something is not a definitive
sign of hardware failure.

--
Maxime

2015-02-19 18:28:26

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

Hi Tony,

> On Feb 19, 2015, at 20:16 , Tony Lindgren <[email protected]> wrote:
>
> * Pantelis Antoniou <[email protected]> [150218 07:03]:
>> Implement DT quirks for the am33xx beaglebone boards.
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/am33xx-dt-quirks.c
> ...
>> +
>> +/*
>> + * The board IDs for am33xx board are in an I2C EEPROM
>> + * We are very early in the boot process so we have to
>> + * read the EEPROM directly without using the I2C layer.
>> + *
>> + * Note that we rely on the bootloader setting up the muxes
>> + * (which is the case for u-boot).
>> + */
>> +
>> +/* I2C Status Register (OMAP_I2C_STAT): */
>> +#define OMAP_I2C_STAT_XDR (1 << 14) /* TX Buffer draining */
>> +#define OMAP_I2C_STAT_RDR (1 << 13) /* RX Buffer draining */
>> +#define OMAP_I2C_STAT_BB (1 << 12) /* Bus busy */
>> +#define OMAP_I2C_STAT_ROVR (1 << 11) /* Receive overrun */
>> +#define OMAP_I2C_STAT_XUDF (1 << 10) /* Transmit underflow */
>> +#define OMAP_I2C_STAT_AAS (1 << 9) /* Address as slave */
>> +#define OMAP_I2C_STAT_BF (1 << 8) /* Bus Free */
>> +#define OMAP_I2C_STAT_XRDY (1 << 4) /* Transmit data ready */
>> +#define OMAP_I2C_STAT_RRDY (1 << 3) /* Receive data ready */
>> +#define OMAP_I2C_STAT_ARDY (1 << 2) /* Register access ready */
>> +#define OMAP_I2C_STAT_NACK (1 << 1) /* No ack interrupt enable */
>> +#define OMAP_I2C_STAT_AL (1 << 0) /* Arbitration lost int ena */
> ...
>
> Uhh I don't like the idea of duplicating the i2c-omap.c driver under
> arch/arm.. And in general we should initialize things later rather
> than earlier.
>
> What's stopping doing these quirk checks later on time with just
> a regular device driver, something like drivers/misc/bbone-quirks.c?
>

We have no choice; we are way early in the boot process, right after
the device tree unflattening step.

I’ve toyed with the idea of using early platform devices but the omap-i2c driver
would need some tender love and care to make it work, and I didn’t want to get
bogged down with i2c driver details at this point.

> Regards,
>
> Tony

Regards

— Pantelis

2015-02-19 18:41:30

by Tony Lindgren

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

* Pantelis Antoniou <[email protected]> [150219 10:32]:
> > On Feb 19, 2015, at 20:16 , Tony Lindgren <[email protected]> wrote:
> >
> > Uhh I don't like the idea of duplicating the i2c-omap.c driver under
> > arch/arm.. And in general we should initialize things later rather
> > than earlier.
> >
> > What's stopping doing these quirk checks later on time with just
> > a regular device driver, something like drivers/misc/bbone-quirks.c?
> >
>
> We have no choice; we are way early in the boot process, right after
> the device tree unflattening step.

To me it seems the dt patching part should be done with minimal
code before any driver like features..

> I’ve toyed with the idea of using early platform devices but the omap-i2c driver
> would need some tender love and care to make it work, and I didn’t want to get
> bogged down with i2c driver details at this point.

..so how about just parse a kernel cmdline for the quirks to apply
based on a version string or similar? That can be easily populated
by u-boot or set manually with setenv.

That leaves out the need for tinkering with i2c super early in
the kernel for revision detection.

Regards,

Tony

2015-02-19 18:44:51

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

Hi Tony,

> On Feb 19, 2015, at 20:36 , Tony Lindgren <[email protected]> wrote:
>
> * Pantelis Antoniou <[email protected]> [150219 10:32]:
>>> On Feb 19, 2015, at 20:16 , Tony Lindgren <[email protected]> wrote:
>>>
>>> Uhh I don't like the idea of duplicating the i2c-omap.c driver under
>>> arch/arm.. And in general we should initialize things later rather
>>> than earlier.
>>>
>>> What's stopping doing these quirk checks later on time with just
>>> a regular device driver, something like drivers/misc/bbone-quirks.c?
>>>
>>
>> We have no choice; we are way early in the boot process, right after
>> the device tree unflattening step.
>
> To me it seems the dt patching part should be done with minimal
> code before any driver like features..
>

The way it’s done right now is with minimal code. Reading the EEPROM
is required.

>> I’ve toyed with the idea of using early platform devices but the omap-i2c driver
>> would need some tender love and care to make it work, and I didn’t want to get
>> bogged down with i2c driver details at this point.
>
> ..so how about just parse a kernel cmdline for the quirks to apply
> based on a version string or similar? That can be easily populated
> by u-boot or set manually with setenv.
>
> That leaves out the need for tinkering with i2c super early in
> the kernel for revision detection.
>

You assume there’s going to be a bootloader…

> Regards,
>
> Tony

Regards

— Pantelis

2015-02-19 18:58:10

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

On Thu, Feb 19, 2015 at 10:36:00AM -0800, Tony Lindgren wrote:
> * Pantelis Antoniou <[email protected]> [150219 10:32]:
> > > On Feb 19, 2015, at 20:16 , Tony Lindgren <[email protected]> wrote:
> > >
> > > Uhh I don't like the idea of duplicating the i2c-omap.c driver under
> > > arch/arm.. And in general we should initialize things later rather
> > > than earlier.
> > >
> > > What's stopping doing these quirk checks later on time with just
> > > a regular device driver, something like drivers/misc/bbone-quirks.c?
> > >
> >
> > We have no choice; we are way early in the boot process, right after
> > the device tree unflattening step.
>
> To me it seems the dt patching part should be done with minimal
> code before any driver like features..
>
> > I’ve toyed with the idea of using early platform devices but the omap-i2c driver
> > would need some tender love and care to make it work, and I didn’t want to get
> > bogged down with i2c driver details at this point.
>
> ..so how about just parse a kernel cmdline for the quirks to apply
> based on a version string or similar? That can be easily populated
> by u-boot or set manually with setenv.
>
That would not work for my use case. Again, this is a CPU card
plugged into a carrier card, of which there are several variants.
The CPU card is similar to a Com Express card (not exactly the same,
but the same idea), so it may even be manufactured by a third party.
Actually, in some cases it is. Modifying the kernel command line
based on the carrier card it is connected to is simply not possible.

Guenter

2015-02-20 08:05:12

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Thu, Feb 19, 2015 at 09:30:58AM -0800, Frank Rowand wrote:
> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
> > Hi Frank,
> >
> >> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
> >>
> >> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
> >>> Hi Mark,
> >>>
> >>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
> >>>>
> >>>>>>> +While this may in theory work, in practice it is very cumbersome
> >>>>>>> +for the following reasons:
> >>>>>>> +
> >>>>>>> +1. The act of selecting a different boot device tree blob requires
> >>>>>>> +a reasonably advanced bootloader with some kind of configuration or
> >>>>>>> +scripting capabilities. Sadly this is not the case many times, the
> >>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
> >>>>>>
> >>>>>> You can have several bootloader builds, or even a single build with
> >>>>>> something like appended DTB to get an appropriate DTB if the same binary
> >>>>>> will otherwise work across all variants of a board.
> >>>>>>
> >>>>>
> >>>>> No, the same DTB will not work across all the variants of a board.
> >>>>
> >>>> I wasn't on about the DTB. I was on about the loader binary, in the case
> >>>> the FW/bootloader could be common even if the DTB couldn't.
> >>>>
> >>>> To some extent there must be a DTB that will work across all variants
> >>>> (albeit with limited utility) or the quirk approach wouldn't work…
> >>>>
> >>>
> >>> That’s not correct; the only part of the DTB that needs to be common
> >>> is the model property that would allow the quirk detection logic to fire.
> >>>
> >>> So, there is a base DTB that will work on all variants, but that only means
> >>> that it will work only up to the point that the quirk detector method
> >>> can work. So while in recommended practice there are common subsets
> >>> of the DTB that might work, they might be unsafe.
> >>>
> >>> For instance on the beaglebone the regulator configuration is different
> >>> between white and black, it is imperative you get them right otherwise
> >>> you risk board damage.
> >>>
> >>>>>> So it's not necessarily true that you need a complex bootloader.
> >>>>>>
> >>>>>
> >>>>>>> +2. On many instances boot time is extremely critical; in some cases
> >>>>>>> +there are hard requirements like having working video feeds in under
> >>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
> >>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> >>>>>>> +is by removing the standard bootloader from the normal boot sequence
> >>>>>>> +altogether by having a very small boot shim that loads the kernel and
> >>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> >>>>>>
> >>>>>> Given my previous comments above I don't see why this is relevant.
> >>>>>> You're already passing _some_ DTB here, so if you can organise for the
> >>>>>> board to statically provide a sane DTB that's fine, or you can resort to
> >>>>>> appended DTB if it's not possible to update the board configuration.
> >>>>>>
> >>>>>
> >>>>> You’re missing the point. I can’t use the same DTB for each revision of the
> >>>>> board. Each board is similar but it’s not identical.
> >>>>
> >>>> I think you've misunderstood my point. If you program the board with the
> >>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
> >>>> the kernel without need for quirks.
> >>>>
> >>>> I understand that each variant is somewhat incompatible (and hence needs
> >>>> its own DTB).
> >>>
> >>> In theory it might work, in practice this does not. Ludovic mentioned that they
> >>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
> >>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
> >>
> >> < snip >
> >>
> >> Or you can install the correct DTB on the board. You trust your manufacturing line
> >> to install the correct resistors. You trust your manufacturing line to install the
> >> correct kernel version (eg an updated version to resolve a security issue).
> >>
> >> I thought the DT blob was supposed to follow the same standard that other OS's or
> >> bootloaders understood. Are you willing to break that? (This is one of those
> >> ripples I mentioned in my other emails.)
> >>
> >
> > Trust no-one.
> >
> > This is one of those things that the kernel community doesn’t understand which makes people
> > who push product quite mad.
> >
> > Engineering a product is not only about meeting customer spec, in order to turn a profit
> > the whole endeavor must be engineered as well for manufacturability.
> >
> > Yes, you can always manually install files in the bootloader. For 1 board no problem.
> > For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
> > instead of turning a profit you’re losing money if you only have a few cents of profit
> > per unit.
>
> I'm not installing physical components manually. Why would I be installing software
> manually? (rhetorical question)

It is not only about manufacturing. You can provide software updates and
trust me even if it seems easy to identify which dtb you have to load
with a good naming, some customers will use the bad one.

Other use case, we have a cpu module with the nand flash and a mother
board, we put the cpu module on another mother board with a different
revision, you don't have to update your dtb.

Maybe it is not necessary but it is a ease of use. I don't understand
how we could want a single zImage (even if it helps to clean the code) and we
don't take care about dtb stuff.

>
> >
> > No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
> > for a few million units.
>
> And you produce a few million units before testing that the first one off the line works?
>
> >
> > And frankly I don’t care what other OSes do. If you were to take a look at the sorry DT support
> > they have you’d be amazed.
> >
> > I would be very surprised if there’s another OS out there that can boot with a late Linux DTB.
> >
> >> -Frank
> >
> > Regards
> >
> > — Pantelis
> >
> > PS. For a real use case please take a look at the answer Guenter gave on this thread a little
> > while back.
> >
>
> My previous comments were written after reading Guenter's comment.
>
> -Frank
>

2015-02-20 08:17:15

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Thu, Feb 19, 2015 at 12:01:14PM -0600, Rob Herring wrote:
> On Wed, Feb 18, 2015 at 8:08 PM, Frank Rowand <[email protected]> wrote:
> > On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
> >> Implement a method of applying DT quirks early in the boot sequence.
> >>
> >> A DT quirk is a subtree of the boot DT that can be applied to
> >> a target in the base DT resulting in a modification of the live
> >> tree. The format of the quirk nodes is that of a device tree overlay.
> >
> > The use of the word "quirk" is a different mental model for me than what
> > this patch series appears to be addressing. I would suggest totally
> > removing the word "quirk" from this proposal to avoid confusing the
> > mental models of future generations of kernel folks.
>
> This comes from me as quirks are a different usecase I had in mind,
> but one that could use a similar mechanism. Although, in the case of
> quirks, I would expect them to be overlays built into the kernel. It
> would be more a way to update old dtbs.
>
> > What this patch series seems to be proposing is a method to apply DT
> > overlays as soon as unflatten_device_tree() completes. In other words,
> > making the device tree a dynamic object, that is partially defined by
> > the kernel during boot. Well, to be fair, the kernel chooses among
> > several possible alternatives encoded in the DT blob. So the device
> > tree is no longer a static object that describes the hardware of the
> > system. It may not sound like a big deal, but it seems to me to be
> > a fundamental shift in what the device tree blob is. Something that
> > should be thought about carefully and not just applied as a patch to
> > solve a point problem.
>
> I agree. I would not want to see every board for an SOC become an
> overlay for example. I think it has to be limited to truly plugable
> h/w (e.g. capes) or minor changes. We just have to define what is
> minor. :)

I don't think it is the goal. It should allow to deal with board
revisions, for example some components put on an other i2c bus to
isolate an i2c device corrupting the bus in some cases. It should
concern plugable h/w which is only needed at boot time for instance a
display panel if we want to show a splash screen.

Ludovic

2015-02-20 13:23:57

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 02/19/2015 09:41 AM, Pantelis Antoniou wrote:
> Hi Frank,
>
>> On Feb 19, 2015, at 04:08 , Frank Rowand <[email protected]> wrote:
>>
>> On 2/18/2015 6:59 AM, Pantelis Antoniou wrote:
>>> Implement a method of applying DT quirks early in the boot sequence.
>>>
>>> A DT quirk is a subtree of the boot DT that can be applied to
>>> a target in the base DT resulting in a modification of the live
>>> tree. The format of the quirk nodes is that of a device tree overlay.
>>
>> The use of the word "quirk" is a different mental model for me than what
>> this patch series appears to be addressing. I would suggest totally
>> removing the word "quirk" from this proposal to avoid confusing the
>> mental models of future generations of kernel folks.
>>
>
> Naming things is hard to do. Suggestions?

variant

2015-02-20 14:21:50

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>
>> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>>
>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>>> Hi Frank,
>>>
>>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>>>
>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>>>> Hi Mark,
>>>>>
>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>>>
>>>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>>>> +for the following reasons:
>>>>>>>>> +
>>>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>>>
>>>>>>>> You can have several bootloader builds, or even a single build with
>>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>>>> will otherwise work across all variants of a board.
>>>>>>>>
>>>>>>>
>>>>>>> No, the same DTB will not work across all the variants of a board.
>>>>>>
>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>>>
>>>>>> To some extent there must be a DTB that will work across all variants
>>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>>>
>>>>>
>>>>> That’s not correct; the only part of the DTB that needs to be common
>>>>> is the model property that would allow the quirk detection logic to fire.
>>>>>
>>>>> So, there is a base DTB that will work on all variants, but that only means
>>>>> that it will work only up to the point that the quirk detector method
>>>>> can work. So while in recommended practice there are common subsets
>>>>> of the DTB that might work, they might be unsafe.
>>>>>
>>>>> For instance on the beaglebone the regulator configuration is different
>>>>> between white and black, it is imperative you get them right otherwise
>>>>> you risk board damage.
>>>>>
>>>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>>>
>>>>>>>
>>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>>>
>>>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>>>
>>>>>>>
>>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>>>> board. Each board is similar but it’s not identical.
>>>>>>
>>>>>> I think you've misunderstood my point. If you program the board with the
>>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>>>> the kernel without need for quirks.
>>>>>>
>>>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>>>> its own DTB).
>>>>>
>>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>>>
>>>> < snip >
>>>>
>>>> Or you can install the correct DTB on the board. You trust your manufacturing line
>>>> to install the correct resistors. You trust your manufacturing line to install the
>>>> correct kernel version (eg an updated version to resolve a security issue).
>>>>
>>>> I thought the DT blob was supposed to follow the same standard that other OS's or
>>>> bootloaders understood. Are you willing to break that? (This is one of those
>>>> ripples I mentioned in my other emails.)
>>>>
>>>
>>> Trust no-one.
>>>
>>> This is one of those things that the kernel community doesn’t understand which makes people
>>> who push product quite mad.
>>>
>>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>>> the whole endeavor must be engineered as well for manufacturability.
>>>
>>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>>> instead of turning a profit you’re losing money if you only have a few cents of profit
>>> per unit.
>>
>> I'm not installing physical components manually. Why would I be installing software
>> manually? (rhetorical question)
>>
>
> Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>
> Having a single binary to flash to every revision of the board makes logistics considerably
> easier.
>
> Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
> on the flash medium) takes time and is error-prone.
>
> Factory time == money, errors == money.
>
>>>
>>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>>> for a few million units.
>>
>> And you produce a few million units before testing that the first one off the line works?
>>
>
> The first one off the line works. The rest will get some burn in and functional testing if you’re
> lucky. In many cases where the product is very cheap it might make financial sense to just ship
> as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>
> Hardware is hard :)

I'm failing to see how this series improves your manufacturing process at all.

1. Won't you have to provide the factory with different eeprom images for the
White and Black? You _trust_ them to get that right, or more likely, you
have process control procedures in place so that you don't get 1 million Blacks
flashed with the White eeprom image.

2. The White and Black use different memory technology so it's not as if the
eMMC from the Black will end up on the White SMT line (or vice versa).

3 For that matter, why wouldn't you worry that all the microSD cards intended
for the White were accidentally assembled with the first 50,000 Blacks; at
that point you're losing a lot more than a few cents of profit. And that has
nothing to do with what image you provided.

3. The factory is just as likely to use some other customer's image by accident,
so you're just as likely to have the same failure rate if you have no test
process at the factory.

4. If you're using offline programming, the image has to be tested after
reflow anyway.

IOW, your QA process will not change at all == same cost.

Regards,
Peter Hurley

2015-02-20 14:35:51

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
> >
> >> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
> >>
> >> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
> >>> Hi Frank,
> >>>
> >>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
> >>>>
> >>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
> >>>>> Hi Mark,
> >>>>>
> >>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
> >>>>>>
> >>>>>>>>> +While this may in theory work, in practice it is very cumbersome
> >>>>>>>>> +for the following reasons:
> >>>>>>>>> +
> >>>>>>>>> +1. The act of selecting a different boot device tree blob requires
> >>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
> >>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
> >>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
> >>>>>>>>
> >>>>>>>> You can have several bootloader builds, or even a single build with
> >>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
> >>>>>>>> will otherwise work across all variants of a board.
> >>>>>>>>
> >>>>>>>
> >>>>>>> No, the same DTB will not work across all the variants of a board.
> >>>>>>
> >>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
> >>>>>> the FW/bootloader could be common even if the DTB couldn't.
> >>>>>>
> >>>>>> To some extent there must be a DTB that will work across all variants
> >>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
> >>>>>>
> >>>>>
> >>>>> That’s not correct; the only part of the DTB that needs to be common
> >>>>> is the model property that would allow the quirk detection logic to fire.
> >>>>>
> >>>>> So, there is a base DTB that will work on all variants, but that only means
> >>>>> that it will work only up to the point that the quirk detector method
> >>>>> can work. So while in recommended practice there are common subsets
> >>>>> of the DTB that might work, they might be unsafe.
> >>>>>
> >>>>> For instance on the beaglebone the regulator configuration is different
> >>>>> between white and black, it is imperative you get them right otherwise
> >>>>> you risk board damage.
> >>>>>
> >>>>>>>> So it's not necessarily true that you need a complex bootloader.
> >>>>>>>>
> >>>>>>>
> >>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
> >>>>>>>>> +there are hard requirements like having working video feeds in under
> >>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
> >>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> >>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
> >>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
> >>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> >>>>>>>>
> >>>>>>>> Given my previous comments above I don't see why this is relevant.
> >>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
> >>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
> >>>>>>>> appended DTB if it's not possible to update the board configuration.
> >>>>>>>>
> >>>>>>>
> >>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
> >>>>>>> board. Each board is similar but it’s not identical.
> >>>>>>
> >>>>>> I think you've misunderstood my point. If you program the board with the
> >>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
> >>>>>> the kernel without need for quirks.
> >>>>>>
> >>>>>> I understand that each variant is somewhat incompatible (and hence needs
> >>>>>> its own DTB).
> >>>>>
> >>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
> >>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
> >>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
> >>>>
> >>>> < snip >
> >>>>
> >>>> Or you can install the correct DTB on the board. You trust your manufacturing line
> >>>> to install the correct resistors. You trust your manufacturing line to install the
> >>>> correct kernel version (eg an updated version to resolve a security issue).
> >>>>
> >>>> I thought the DT blob was supposed to follow the same standard that other OS's or
> >>>> bootloaders understood. Are you willing to break that? (This is one of those
> >>>> ripples I mentioned in my other emails.)
> >>>>
> >>>
> >>> Trust no-one.
> >>>
> >>> This is one of those things that the kernel community doesn’t understand which makes people
> >>> who push product quite mad.
> >>>
> >>> Engineering a product is not only about meeting customer spec, in order to turn a profit
> >>> the whole endeavor must be engineered as well for manufacturability.
> >>>
> >>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
> >>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
> >>> instead of turning a profit you’re losing money if you only have a few cents of profit
> >>> per unit.
> >>
> >> I'm not installing physical components manually. Why would I be installing software
> >> manually? (rhetorical question)
> >>
> >
> > Because on high volume product runs the flash comes preprogrammed and is soldered as is.
> >
> > Having a single binary to flash to every revision of the board makes logistics considerably
> > easier.
> >
> > Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
> > on the flash medium) takes time and is error-prone.
> >
> > Factory time == money, errors == money.
> >
> >>>
> >>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
> >>> for a few million units.
> >>
> >> And you produce a few million units before testing that the first one off the line works?
> >>
> >
> > The first one off the line works. The rest will get some burn in and functional testing if you’re
> > lucky. In many cases where the product is very cheap it might make financial sense to just ship
> > as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
> >
> > Hardware is hard :)
>
> I'm failing to see how this series improves your manufacturing process at all.
>
> 1. Won't you have to provide the factory with different eeprom images for the
> White and Black? You _trust_ them to get that right, or more likely, you
> have process control procedures in place so that you don't get 1 million Blacks
> flashed with the White eeprom image.
>
> 2. The White and Black use different memory technology so it's not as if the
> eMMC from the Black will end up on the White SMT line (or vice versa).
>
> 3 For that matter, why wouldn't you worry that all the microSD cards intended
> for the White were accidentally assembled with the first 50,000 Blacks; at
> that point you're losing a lot more than a few cents of profit. And that has
> nothing to do with what image you provided.
>

As you said, we can imagine many reasons to have a failure during the
production, having several DTB files will increase the risk.

> 3. The factory is just as likely to use some other customer's image by accident,
> so you're just as likely to have the same failure rate if you have no test
> process at the factory.
>
> 4. If you're using offline programming, the image has to be tested after
> reflow anyway.
>
> IOW, your QA process will not change at all == same cost.
>
> Regards,
> Peter Hurley

2015-02-20 14:38:23

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Peter,

> On Feb 20, 2015, at 16:21 , Peter Hurley <[email protected]> wrote:
>
> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>>
>>> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>>>
>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>>>> Hi Frank,
>>>>
>>>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>>>>
>>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>>>>> Hi Mark,
>>>>>>
>>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>>>>
>>>>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>>>>> +for the following reasons:
>>>>>>>>>> +
>>>>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>>>>
>>>>>>>>> You can have several bootloader builds, or even a single build with
>>>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>>>>> will otherwise work across all variants of a board.
>>>>>>>>>
>>>>>>>>
>>>>>>>> No, the same DTB will not work across all the variants of a board.
>>>>>>>
>>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>>>>
>>>>>>> To some extent there must be a DTB that will work across all variants
>>>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>>>>
>>>>>>
>>>>>> That’s not correct; the only part of the DTB that needs to be common
>>>>>> is the model property that would allow the quirk detection logic to fire.
>>>>>>
>>>>>> So, there is a base DTB that will work on all variants, but that only means
>>>>>> that it will work only up to the point that the quirk detector method
>>>>>> can work. So while in recommended practice there are common subsets
>>>>>> of the DTB that might work, they might be unsafe.
>>>>>>
>>>>>> For instance on the beaglebone the regulator configuration is different
>>>>>> between white and black, it is imperative you get them right otherwise
>>>>>> you risk board damage.
>>>>>>
>>>>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>>>>
>>>>>>>>
>>>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>>>>
>>>>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>>>>
>>>>>>>>
>>>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>>>>> board. Each board is similar but it’s not identical.
>>>>>>>
>>>>>>> I think you've misunderstood my point. If you program the board with the
>>>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>>>>> the kernel without need for quirks.
>>>>>>>
>>>>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>>>>> its own DTB).
>>>>>>
>>>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>>>>
>>>>> < snip >
>>>>>
>>>>> Or you can install the correct DTB on the board. You trust your manufacturing line
>>>>> to install the correct resistors. You trust your manufacturing line to install the
>>>>> correct kernel version (eg an updated version to resolve a security issue).
>>>>>
>>>>> I thought the DT blob was supposed to follow the same standard that other OS's or
>>>>> bootloaders understood. Are you willing to break that? (This is one of those
>>>>> ripples I mentioned in my other emails.)
>>>>>
>>>>
>>>> Trust no-one.
>>>>
>>>> This is one of those things that the kernel community doesn’t understand which makes people
>>>> who push product quite mad.
>>>>
>>>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>>>> the whole endeavor must be engineered as well for manufacturability.
>>>>
>>>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>>>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>>>> instead of turning a profit you’re losing money if you only have a few cents of profit
>>>> per unit.
>>>
>>> I'm not installing physical components manually. Why would I be installing software
>>> manually? (rhetorical question)
>>>
>>
>> Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>>
>> Having a single binary to flash to every revision of the board makes logistics considerably
>> easier.
>>
>> Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
>> on the flash medium) takes time and is error-prone.
>>
>> Factory time == money, errors == money.
>>
>>>>
>>>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>>>> for a few million units.
>>>
>>> And you produce a few million units before testing that the first one off the line works?
>>>
>>
>> The first one off the line works. The rest will get some burn in and functional testing if you’re
>> lucky. In many cases where the product is very cheap it might make financial sense to just ship
>> as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>>
>> Hardware is hard :)
>
> I'm failing to see how this series improves your manufacturing process at all.
>
> 1. Won't you have to provide the factory with different eeprom images for the
> White and Black? You _trust_ them to get that right, or more likely, you
> have process control procedures in place so that you don't get 1 million Blacks
> flashed with the White eeprom image.
>
> 2. The White and Black use different memory technology so it's not as if the
> eMMC from the Black will end up on the White SMT line (or vice versa).
>
> 3 For that matter, why wouldn't you worry that all the microSD cards intended
> for the White were accidentally assembled with the first 50,000 Blacks; at
> that point you're losing a lot more than a few cents of profit. And that has
> nothing to do with what image you provided.
>
> 3. The factory is just as likely to use some other customer's image by accident,
> so you're just as likely to have the same failure rate if you have no test
> process at the factory.
>
> 4. If you're using offline programming, the image has to be tested after
> reflow anyway.
>
> IOW, your QA process will not change at all == same cost.
>

I never said this fixes every case where someone might screw up, I just said
that it makes it extremely less likely.

And no-one is holding the beaglebone as the paragon of good design process for
ease of manufacturing as far as I know.

> Regards,
> Peter Hurley

Regards

— Pantelis

2015-02-20 15:00:17

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 02/20/2015 09:35 AM, Ludovic Desroches wrote:
> On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
>> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>>>
>>>> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>>>>
>>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>>>>> Hi Frank,
>>>>>
>>>>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>>>>>
>>>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>>>>>> Hi Mark,
>>>>>>>
>>>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>>>>>
>>>>>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>>>>>> +for the following reasons:
>>>>>>>>>>> +
>>>>>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>>>>>
>>>>>>>>>> You can have several bootloader builds, or even a single build with
>>>>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>>>>>> will otherwise work across all variants of a board.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, the same DTB will not work across all the variants of a board.
>>>>>>>>
>>>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>>>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>>>>>
>>>>>>>> To some extent there must be a DTB that will work across all variants
>>>>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>>>>>
>>>>>>>
>>>>>>> That’s not correct; the only part of the DTB that needs to be common
>>>>>>> is the model property that would allow the quirk detection logic to fire.
>>>>>>>
>>>>>>> So, there is a base DTB that will work on all variants, but that only means
>>>>>>> that it will work only up to the point that the quirk detector method
>>>>>>> can work. So while in recommended practice there are common subsets
>>>>>>> of the DTB that might work, they might be unsafe.
>>>>>>>
>>>>>>> For instance on the beaglebone the regulator configuration is different
>>>>>>> between white and black, it is imperative you get them right otherwise
>>>>>>> you risk board damage.
>>>>>>>
>>>>>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>>>>>
>>>>>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>>>>>> board. Each board is similar but it’s not identical.
>>>>>>>>
>>>>>>>> I think you've misunderstood my point. If you program the board with the
>>>>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>>>>>> the kernel without need for quirks.
>>>>>>>>
>>>>>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>>>>>> its own DTB).
>>>>>>>
>>>>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>>>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>>>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>>>>>
>>>>>> < snip >
>>>>>>
>>>>>> Or you can install the correct DTB on the board. You trust your manufacturing line
>>>>>> to install the correct resistors. You trust your manufacturing line to install the
>>>>>> correct kernel version (eg an updated version to resolve a security issue).
>>>>>>
>>>>>> I thought the DT blob was supposed to follow the same standard that other OS's or
>>>>>> bootloaders understood. Are you willing to break that? (This is one of those
>>>>>> ripples I mentioned in my other emails.)
>>>>>>
>>>>>
>>>>> Trust no-one.
>>>>>
>>>>> This is one of those things that the kernel community doesn’t understand which makes people
>>>>> who push product quite mad.
>>>>>
>>>>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>>>>> the whole endeavor must be engineered as well for manufacturability.
>>>>>
>>>>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>>>>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>>>>> instead of turning a profit you’re losing money if you only have a few cents of profit
>>>>> per unit.
>>>>
>>>> I'm not installing physical components manually. Why would I be installing software
>>>> manually? (rhetorical question)
>>>>
>>>
>>> Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>>>
>>> Having a single binary to flash to every revision of the board makes logistics considerably
>>> easier.
>>>
>>> Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
>>> on the flash medium) takes time and is error-prone.
>>>
>>> Factory time == money, errors == money.
>>>
>>>>>
>>>>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>>>>> for a few million units.
>>>>
>>>> And you produce a few million units before testing that the first one off the line works?
>>>>
>>>
>>> The first one off the line works. The rest will get some burn in and functional testing if you’re
>>> lucky. In many cases where the product is very cheap it might make financial sense to just ship
>>> as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>>>
>>> Hardware is hard :)
>>
>> I'm failing to see how this series improves your manufacturing process at all.
>>
>> 1. Won't you have to provide the factory with different eeprom images for the
>> White and Black? You _trust_ them to get that right, or more likely, you
>> have process control procedures in place so that you don't get 1 million Blacks
>> flashed with the White eeprom image.
>>
>> 2. The White and Black use different memory technology so it's not as if the
>> eMMC from the Black will end up on the White SMT line (or vice versa).
>>
>> 3 For that matter, why wouldn't you worry that all the microSD cards intended
>> for the White were accidentally assembled with the first 50,000 Blacks; at
>> that point you're losing a lot more than a few cents of profit. And that has
>> nothing to do with what image you provided.
>>
>
> As you said, we can imagine many reasons to have a failure during the
> production, having several DTB files will increase the risk.

It's interesting that you don't see the added complexity of open-coding
the i2c driver or mixing DTS fragments for different designs as increased risk
(for us all).

>> 3. The factory is just as likely to use some other customer's image by accident,
>> so you're just as likely to have the same failure rate if you have no test
>> process at the factory.
>>
>> 4. If you're using offline programming, the image has to be tested after
>> reflow anyway.
>>
>> IOW, your QA process will not change at all == same cost.
>>
>> Regards,
>> Peter Hurley

2015-02-20 15:02:41

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Peter,

> On Feb 20, 2015, at 17:00 , Peter Hurley <[email protected]> wrote:
>
> On 02/20/2015 09:35 AM, Ludovic Desroches wrote:
>> On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
>>> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>>>>
>>>>> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>>>>>
>>>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>>>>>> Hi Frank,
>>>>>>
>>>>>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>>>>>>
>>>>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>>>>>>> Hi Mark,
>>>>>>>>
>>>>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>>>>>>> +for the following reasons:
>>>>>>>>>>>> +
>>>>>>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>>>>>>
>>>>>>>>>>> You can have several bootloader builds, or even a single build with
>>>>>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>>>>>>> will otherwise work across all variants of a board.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, the same DTB will not work across all the variants of a board.
>>>>>>>>>
>>>>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>>>>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>>>>>>
>>>>>>>>> To some extent there must be a DTB that will work across all variants
>>>>>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>>>>>>
>>>>>>>>
>>>>>>>> That’s not correct; the only part of the DTB that needs to be common
>>>>>>>> is the model property that would allow the quirk detection logic to fire.
>>>>>>>>
>>>>>>>> So, there is a base DTB that will work on all variants, but that only means
>>>>>>>> that it will work only up to the point that the quirk detector method
>>>>>>>> can work. So while in recommended practice there are common subsets
>>>>>>>> of the DTB that might work, they might be unsafe.
>>>>>>>>
>>>>>>>> For instance on the beaglebone the regulator configuration is different
>>>>>>>> between white and black, it is imperative you get them right otherwise
>>>>>>>> you risk board damage.
>>>>>>>>
>>>>>>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>>>>>>
>>>>>>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>>>>>>> board. Each board is similar but it’s not identical.
>>>>>>>>>
>>>>>>>>> I think you've misunderstood my point. If you program the board with the
>>>>>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>>>>>>> the kernel without need for quirks.
>>>>>>>>>
>>>>>>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>>>>>>> its own DTB).
>>>>>>>>
>>>>>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>>>>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>>>>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>>>>>>
>>>>>>> < snip >
>>>>>>>
>>>>>>> Or you can install the correct DTB on the board. You trust your manufacturing line
>>>>>>> to install the correct resistors. You trust your manufacturing line to install the
>>>>>>> correct kernel version (eg an updated version to resolve a security issue).
>>>>>>>
>>>>>>> I thought the DT blob was supposed to follow the same standard that other OS's or
>>>>>>> bootloaders understood. Are you willing to break that? (This is one of those
>>>>>>> ripples I mentioned in my other emails.)
>>>>>>>
>>>>>>
>>>>>> Trust no-one.
>>>>>>
>>>>>> This is one of those things that the kernel community doesn’t understand which makes people
>>>>>> who push product quite mad.
>>>>>>
>>>>>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>>>>>> the whole endeavor must be engineered as well for manufacturability.
>>>>>>
>>>>>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>>>>>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>>>>>> instead of turning a profit you’re losing money if you only have a few cents of profit
>>>>>> per unit.
>>>>>
>>>>> I'm not installing physical components manually. Why would I be installing software
>>>>> manually? (rhetorical question)
>>>>>
>>>>
>>>> Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>>>>
>>>> Having a single binary to flash to every revision of the board makes logistics considerably
>>>> easier.
>>>>
>>>> Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
>>>> on the flash medium) takes time and is error-prone.
>>>>
>>>> Factory time == money, errors == money.
>>>>
>>>>>>
>>>>>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>>>>>> for a few million units.
>>>>>
>>>>> And you produce a few million units before testing that the first one off the line works?
>>>>>
>>>>
>>>> The first one off the line works. The rest will get some burn in and functional testing if you’re
>>>> lucky. In many cases where the product is very cheap it might make financial sense to just ship
>>>> as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>>>>
>>>> Hardware is hard :)
>>>
>>> I'm failing to see how this series improves your manufacturing process at all.
>>>
>>> 1. Won't you have to provide the factory with different eeprom images for the
>>> White and Black? You _trust_ them to get that right, or more likely, you
>>> have process control procedures in place so that you don't get 1 million Blacks
>>> flashed with the White eeprom image.
>>>
>>> 2. The White and Black use different memory technology so it's not as if the
>>> eMMC from the Black will end up on the White SMT line (or vice versa).
>>>
>>> 3 For that matter, why wouldn't you worry that all the microSD cards intended
>>> for the White were accidentally assembled with the first 50,000 Blacks; at
>>> that point you're losing a lot more than a few cents of profit. And that has
>>> nothing to do with what image you provided.
>>>
>>
>> As you said, we can imagine many reasons to have a failure during the
>> production, having several DTB files will increase the risk.
>
> It's interesting that you don't see the added complexity of open-coding
> the i2c driver or mixing DTS fragments for different designs as increased risk
> (for us all).
>
>

You don’t have to use it. Some people really do though. As for increased risk
I expect to see arguments instead of a statement.

>>> 3. The factory is just as likely to use some other customer's image by accident,
>>> so you're just as likely to have the same failure rate if you have no test
>>> process at the factory.
>>>
>>> 4. If you're using offline programming, the image has to be tested after
>>> reflow anyway.
>>>
>>> IOW, your QA process will not change at all == same cost.
>>>
>>> Regards,
>>> Peter Hurley
>

Regards

— Pantelis

2015-02-20 15:24:58

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 02/20/2015 10:02 AM, Pantelis Antoniou wrote:
> Hi Peter,
>
>> On Feb 20, 2015, at 17:00 , Peter Hurley <[email protected]> wrote:
>>
>> On 02/20/2015 09:35 AM, Ludovic Desroches wrote:
>>> On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
>>>> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>>>>>
>>>>>> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>>>>>>
>>>>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>>>>>>> Hi Frank,
>>>>>>>
>>>>>>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>>>>>>>> Hi Mark,
>>>>>>>>>
>>>>>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>>>>>>>> +for the following reasons:
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>>>>>>>
>>>>>>>>>>>> You can have several bootloader builds, or even a single build with
>>>>>>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>>>>>>>> will otherwise work across all variants of a board.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> No, the same DTB will not work across all the variants of a board.
>>>>>>>>>>
>>>>>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>>>>>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>>>>>>>
>>>>>>>>>> To some extent there must be a DTB that will work across all variants
>>>>>>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That’s not correct; the only part of the DTB that needs to be common
>>>>>>>>> is the model property that would allow the quirk detection logic to fire.
>>>>>>>>>
>>>>>>>>> So, there is a base DTB that will work on all variants, but that only means
>>>>>>>>> that it will work only up to the point that the quirk detector method
>>>>>>>>> can work. So while in recommended practice there are common subsets
>>>>>>>>> of the DTB that might work, they might be unsafe.
>>>>>>>>>
>>>>>>>>> For instance on the beaglebone the regulator configuration is different
>>>>>>>>> between white and black, it is imperative you get them right otherwise
>>>>>>>>> you risk board damage.
>>>>>>>>>
>>>>>>>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>>>>>>>
>>>>>>>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>>>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>>>>>>>> board. Each board is similar but it’s not identical.
>>>>>>>>>>
>>>>>>>>>> I think you've misunderstood my point. If you program the board with the
>>>>>>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>>>>>>>> the kernel without need for quirks.
>>>>>>>>>>
>>>>>>>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>>>>>>>> its own DTB).
>>>>>>>>>
>>>>>>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>>>>>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>>>>>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>>>>>>>
>>>>>>>> < snip >
>>>>>>>>
>>>>>>>> Or you can install the correct DTB on the board. You trust your manufacturing line
>>>>>>>> to install the correct resistors. You trust your manufacturing line to install the
>>>>>>>> correct kernel version (eg an updated version to resolve a security issue).
>>>>>>>>
>>>>>>>> I thought the DT blob was supposed to follow the same standard that other OS's or
>>>>>>>> bootloaders understood. Are you willing to break that? (This is one of those
>>>>>>>> ripples I mentioned in my other emails.)
>>>>>>>>
>>>>>>>
>>>>>>> Trust no-one.
>>>>>>>
>>>>>>> This is one of those things that the kernel community doesn’t understand which makes people
>>>>>>> who push product quite mad.
>>>>>>>
>>>>>>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>>>>>>> the whole endeavor must be engineered as well for manufacturability.
>>>>>>>
>>>>>>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>>>>>>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>>>>>>> instead of turning a profit you’re losing money if you only have a few cents of profit
>>>>>>> per unit.
>>>>>>
>>>>>> I'm not installing physical components manually. Why would I be installing software
>>>>>> manually? (rhetorical question)
>>>>>>
>>>>>
>>>>> Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>>>>>
>>>>> Having a single binary to flash to every revision of the board makes logistics considerably
>>>>> easier.
>>>>>
>>>>> Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
>>>>> on the flash medium) takes time and is error-prone.
>>>>>
>>>>> Factory time == money, errors == money.
>>>>>
>>>>>>>
>>>>>>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>>>>>>> for a few million units.
>>>>>>
>>>>>> And you produce a few million units before testing that the first one off the line works?
>>>>>>
>>>>>
>>>>> The first one off the line works. The rest will get some burn in and functional testing if you’re
>>>>> lucky. In many cases where the product is very cheap it might make financial sense to just ship
>>>>> as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>>>>>
>>>>> Hardware is hard :)
>>>>
>>>> I'm failing to see how this series improves your manufacturing process at all.
>>>>
>>>> 1. Won't you have to provide the factory with different eeprom images for the
>>>> White and Black? You _trust_ them to get that right, or more likely, you
>>>> have process control procedures in place so that you don't get 1 million Blacks
>>>> flashed with the White eeprom image.
>>>>
>>>> 2. The White and Black use different memory technology so it's not as if the
>>>> eMMC from the Black will end up on the White SMT line (or vice versa).
>>>>
>>>> 3 For that matter, why wouldn't you worry that all the microSD cards intended
>>>> for the White were accidentally assembled with the first 50,000 Blacks; at
>>>> that point you're losing a lot more than a few cents of profit. And that has
>>>> nothing to do with what image you provided.
>>>>
>>>
>>> As you said, we can imagine many reasons to have a failure during the
>>> production, having several DTB files will increase the risk.
>>
>> It's interesting that you don't see the added complexity of open-coding
>> the i2c driver or mixing DTS fragments for different designs as increased risk
>> (for us all).
>>
>>
>
> You don’t have to use it.

> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
> index 5d27dfd..02129e7 100644
> --- a/arch/arm/mach-omap2/Makefile
> +++ b/arch/arm/mach-omap2/Makefile
> @@ -259,6 +259,11 @@ obj-$(CONFIG_MACH_CRANEBOARD) += board-am3517crane.o
>
> obj-$(CONFIG_MACH_SBC3530) += board-omap3stalker.o
>
> +# DT quirks
> +ifneq ($(CONFIG_OF_DYNAMIC),)
> +obj-$(CONFIG_SOC_AM33XX) += am33xx-dt-quirks.o
> +endif

Won't this automatically be included on my Black that supports DT overlays?

> Some people really do though. As for increased risk
> I expect to see arguments instead of a statement.

No one is wasting your time with random arguments. Please keep your tone civil.

Regards,
Peter Hurley

>>>> 3. The factory is just as likely to use some other customer's image by accident,
>>>> so you're just as likely to have the same failure rate if you have no test
>>>> process at the factory.
>>>>
>>>> 4. If you're using offline programming, the image has to be tested after
>>>> reflow anyway.
>>>>
>>>> IOW, your QA process will not change at all == same cost.
>>>>
>>>> Regards,
>>>> Peter Hurley
>>
>
> Regards
>
> — Pantelis
>

2015-02-20 15:38:27

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Peter,

> On Feb 20, 2015, at 17:24 , Peter Hurley <[email protected]> wrote:
>
> On 02/20/2015 10:02 AM, Pantelis Antoniou wrote:
>> Hi Peter,
>>
>>> On Feb 20, 2015, at 17:00 , Peter Hurley <[email protected]> wrote:
>>>
>>> On 02/20/2015 09:35 AM, Ludovic Desroches wrote:
>>>> On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
>>>>> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>>>>>>
>>>>>>> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>>>>>>>
>>>>>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>>>>>>>> Hi Frank,
>>>>>>>>
>>>>>>>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
>>>>>>>>>> Hi Mark,
>>>>>>>>>>
>>>>>>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>>> +While this may in theory work, in practice it is very cumbersome
>>>>>>>>>>>>>> +for the following reasons:
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +1. The act of selecting a different boot device tree blob requires
>>>>>>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
>>>>>>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
>>>>>>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
>>>>>>>>>>>>>
>>>>>>>>>>>>> You can have several bootloader builds, or even a single build with
>>>>>>>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
>>>>>>>>>>>>> will otherwise work across all variants of a board.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> No, the same DTB will not work across all the variants of a board.
>>>>>>>>>>>
>>>>>>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
>>>>>>>>>>> the FW/bootloader could be common even if the DTB couldn't.
>>>>>>>>>>>
>>>>>>>>>>> To some extent there must be a DTB that will work across all variants
>>>>>>>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That’s not correct; the only part of the DTB that needs to be common
>>>>>>>>>> is the model property that would allow the quirk detection logic to fire.
>>>>>>>>>>
>>>>>>>>>> So, there is a base DTB that will work on all variants, but that only means
>>>>>>>>>> that it will work only up to the point that the quirk detector method
>>>>>>>>>> can work. So while in recommended practice there are common subsets
>>>>>>>>>> of the DTB that might work, they might be unsafe.
>>>>>>>>>>
>>>>>>>>>> For instance on the beaglebone the regulator configuration is different
>>>>>>>>>> between white and black, it is imperative you get them right otherwise
>>>>>>>>>> you risk board damage.
>>>>>>>>>>
>>>>>>>>>>>>> So it's not necessarily true that you need a complex bootloader.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
>>>>>>>>>>>>>> +there are hard requirements like having working video feeds in under
>>>>>>>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
>>>>>>>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>>>>>>>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
>>>>>>>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
>>>>>>>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Given my previous comments above I don't see why this is relevant.
>>>>>>>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
>>>>>>>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
>>>>>>>>>>>>> appended DTB if it's not possible to update the board configuration.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
>>>>>>>>>>>> board. Each board is similar but it’s not identical.
>>>>>>>>>>>
>>>>>>>>>>> I think you've misunderstood my point. If you program the board with the
>>>>>>>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
>>>>>>>>>>> the kernel without need for quirks.
>>>>>>>>>>>
>>>>>>>>>>> I understand that each variant is somewhat incompatible (and hence needs
>>>>>>>>>>> its own DTB).
>>>>>>>>>>
>>>>>>>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
>>>>>>>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
>>>>>>>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
>>>>>>>>>
>>>>>>>>> < snip >
>>>>>>>>>
>>>>>>>>> Or you can install the correct DTB on the board. You trust your manufacturing line
>>>>>>>>> to install the correct resistors. You trust your manufacturing line to install the
>>>>>>>>> correct kernel version (eg an updated version to resolve a security issue).
>>>>>>>>>
>>>>>>>>> I thought the DT blob was supposed to follow the same standard that other OS's or
>>>>>>>>> bootloaders understood. Are you willing to break that? (This is one of those
>>>>>>>>> ripples I mentioned in my other emails.)
>>>>>>>>>
>>>>>>>>
>>>>>>>> Trust no-one.
>>>>>>>>
>>>>>>>> This is one of those things that the kernel community doesn’t understand which makes people
>>>>>>>> who push product quite mad.
>>>>>>>>
>>>>>>>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>>>>>>>> the whole endeavor must be engineered as well for manufacturability.
>>>>>>>>
>>>>>>>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>>>>>>>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>>>>>>>> instead of turning a profit you’re losing money if you only have a few cents of profit
>>>>>>>> per unit.
>>>>>>>
>>>>>>> I'm not installing physical components manually. Why would I be installing software
>>>>>>> manually? (rhetorical question)
>>>>>>>
>>>>>>
>>>>>> Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>>>>>>
>>>>>> Having a single binary to flash to every revision of the board makes logistics considerably
>>>>>> easier.
>>>>>>
>>>>>> Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
>>>>>> on the flash medium) takes time and is error-prone.
>>>>>>
>>>>>> Factory time == money, errors == money.
>>>>>>
>>>>>>>>
>>>>>>>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>>>>>>>> for a few million units.
>>>>>>>
>>>>>>> And you produce a few million units before testing that the first one off the line works?
>>>>>>>
>>>>>>
>>>>>> The first one off the line works. The rest will get some burn in and functional testing if you’re
>>>>>> lucky. In many cases where the product is very cheap it might make financial sense to just ship
>>>>>> as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>>>>>>
>>>>>> Hardware is hard :)
>>>>>
>>>>> I'm failing to see how this series improves your manufacturing process at all.
>>>>>
>>>>> 1. Won't you have to provide the factory with different eeprom images for the
>>>>> White and Black? You _trust_ them to get that right, or more likely, you
>>>>> have process control procedures in place so that you don't get 1 million Blacks
>>>>> flashed with the White eeprom image.
>>>>>
>>>>> 2. The White and Black use different memory technology so it's not as if the
>>>>> eMMC from the Black will end up on the White SMT line (or vice versa).
>>>>>
>>>>> 3 For that matter, why wouldn't you worry that all the microSD cards intended
>>>>> for the White were accidentally assembled with the first 50,000 Blacks; at
>>>>> that point you're losing a lot more than a few cents of profit. And that has
>>>>> nothing to do with what image you provided.
>>>>>
>>>>
>>>> As you said, we can imagine many reasons to have a failure during the
>>>> production, having several DTB files will increase the risk.
>>>
>>> It's interesting that you don't see the added complexity of open-coding
>>> the i2c driver or mixing DTS fragments for different designs as increased risk
>>> (for us all).
>>>
>>>
>>
>> You don’t have to use it.
>
>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>> index 5d27dfd..02129e7 100644
>> --- a/arch/arm/mach-omap2/Makefile
>> +++ b/arch/arm/mach-omap2/Makefile
>> @@ -259,6 +259,11 @@ obj-$(CONFIG_MACH_CRANEBOARD) += board-am3517crane.o
>>
>> obj-$(CONFIG_MACH_SBC3530) += board-omap3stalker.o
>>
>> +# DT quirks
>> +ifneq ($(CONFIG_OF_DYNAMIC),)
>> +obj-$(CONFIG_SOC_AM33XX) += am33xx-dt-quirks.o
>> +endif
>
> Won't this automatically be included on my Black that supports DT overlays?
>

Yes it will. It is a grand total of 498 lines of code, and the total size of
the code segment is about 2.2K.

You do realize that you’re probably booting a multi-platform kernel on the
black right? Including things for all 2xxx/3xxx and 44xx platforms?
For instance:

> ~/ti/kernels/linux-github.git $ wc -l arch/arm/mach-omap2/*44xx*.c
> 443 arch/arm/mach-omap2/clockdomains44xx_data.c
> 526 arch/arm/mach-omap2/cminst44xx.c
> 251 arch/arm/mach-omap2/cpuidle44xx.c
> 250 arch/arm/mach-omap2/dpll44xx.c
> 4864 arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> 295 arch/arm/mach-omap2/pm44xx.c
> 358 arch/arm/mach-omap2/powerdomains44xx_data.c
> 62 arch/arm/mach-omap2/prcm_mpu44xx.c
> 770 arch/arm/mach-omap2/prm44xx.c
> 210 arch/arm/mach-omap2/prminst44xx.c
> 117 arch/arm/mach-omap2/vc44xx_data.c
> 130 arch/arm/mach-omap2/voltagedomains44xx_data.c
> 104 arch/arm/mach-omap2/vp44xx_data.c
> 8380 total

I bet those things are far larger than 2.2K. And I also bet that in the
tradeoff analysis that the board maintainer did things came down to
increasing complexity so that he can consolidate the kernels for all the
other platforms he has to support besides the black.

>
>> Some people really do though. As for increased risk
>> I expect to see arguments instead of a statement.
>
> No one is wasting your time with random arguments. Please keep your tone civil.
>

A statement like 'increasing risk for all of us' is very open ended. What is
the risk? How much of it exists?

If I offended you I’m really sorry though, I meant no such thing.

> Regards,
> Peter Hurley
>

Regards

— Pantelis

>
>>>>> 3. The factory is just as likely to use some other customer's image by accident,
>>>>> so you're just as likely to have the same failure rate if you have no test
>>>>> process at the factory.
>>>>>
>>>>> 4. If you're using offline programming, the image has to be tested after
>>>>> reflow anyway.
>>>>>
>>>>> IOW, your QA process will not change at all == same cost.
>>>>>
>>>>> Regards,
>>>>> Peter Hurley
>>>
>>
>> Regards
>>
>> — Pantelis

2015-02-20 16:13:12

by Jon Hunter

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

On 02/19/2015 06:28 PM, Pantelis Antoniou wrote:
> Hi Tony,
>
>> On Feb 19, 2015, at 20:16 , Tony Lindgren <[email protected]> wrote:
>>
>> * Pantelis Antoniou <[email protected]> [150218 07:03]:
>>> Implement DT quirks for the am33xx beaglebone boards.
>>> --- /dev/null
>>> +++ b/arch/arm/mach-omap2/am33xx-dt-quirks.c
>> ...
>>> +
>>> +/*
>>> + * The board IDs for am33xx board are in an I2C EEPROM
>>> + * We are very early in the boot process so we have to
>>> + * read the EEPROM directly without using the I2C layer.
>>> + *
>>> + * Note that we rely on the bootloader setting up the muxes
>>> + * (which is the case for u-boot).
>>> + */
>>> +
>>> +/* I2C Status Register (OMAP_I2C_STAT): */
>>> +#define OMAP_I2C_STAT_XDR (1 << 14) /* TX Buffer draining */
>>> +#define OMAP_I2C_STAT_RDR (1 << 13) /* RX Buffer draining */
>>> +#define OMAP_I2C_STAT_BB (1 << 12) /* Bus busy */
>>> +#define OMAP_I2C_STAT_ROVR (1 << 11) /* Receive overrun */
>>> +#define OMAP_I2C_STAT_XUDF (1 << 10) /* Transmit underflow */
>>> +#define OMAP_I2C_STAT_AAS (1 << 9) /* Address as slave */
>>> +#define OMAP_I2C_STAT_BF (1 << 8) /* Bus Free */
>>> +#define OMAP_I2C_STAT_XRDY (1 << 4) /* Transmit data ready */
>>> +#define OMAP_I2C_STAT_RRDY (1 << 3) /* Receive data ready */
>>> +#define OMAP_I2C_STAT_ARDY (1 << 2) /* Register access ready */
>>> +#define OMAP_I2C_STAT_NACK (1 << 1) /* No ack interrupt enable */
>>> +#define OMAP_I2C_STAT_AL (1 << 0) /* Arbitration lost int ena */
>> ...
>>
>> Uhh I don't like the idea of duplicating the i2c-omap.c driver under
>> arch/arm.. And in general we should initialize things later rather
>> than earlier.
>>
>> What's stopping doing these quirk checks later on time with just
>> a regular device driver, something like drivers/misc/bbone-quirks.c?
>>
>
> We have no choice; we are way early in the boot process, right after
> the device tree unflattening step.

Can you elaborate with an example of why not? Why can't the overlay
happen at a later stage in the kernel boot as Tony suggests?

One thought would be that ideally devices that are dependent on a
particular board variant would be disabled in the base DT blob until you
know what board you are. However, that assumes that they can be
initialised at a later stage in the boot process and may be for some
regulators or other devices this is not possible. I know you mentioned
some time restrictions for some devices, but I still don't see why it
could not happen later.

Jon

2015-02-20 16:34:48

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On 02/20/2015 10:38 AM, Pantelis Antoniou wrote:
> Hi Peter,
>
>> On Feb 20, 2015, at 17:24 , Peter Hurley <[email protected]> wrote:
>>
>> On 02/20/2015 10:02 AM, Pantelis Antoniou wrote:
>>> Hi Peter,
>>>
>>>> On Feb 20, 2015, at 17:00 , Peter Hurley <[email protected]> wrote:
>>>>
>>>> On 02/20/2015 09:35 AM, Ludovic Desroches wrote:

[...]

>>>>> As you said, we can imagine many reasons to have a failure during the
>>>>> production, having several DTB files will increase the risk.
>>>>
>>>> It's interesting that you don't see the added complexity of open-coding
>>>> the i2c driver or mixing DTS fragments for different designs as increased risk
>>>> (for us all).
>>>>
>>>>
>>>
>>> You don’t have to use it.
>>
>>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>>> index 5d27dfd..02129e7 100644
>>> --- a/arch/arm/mach-omap2/Makefile
>>> +++ b/arch/arm/mach-omap2/Makefile
>>> @@ -259,6 +259,11 @@ obj-$(CONFIG_MACH_CRANEBOARD) += board-am3517crane.o
>>>
>>> obj-$(CONFIG_MACH_SBC3530) += board-omap3stalker.o
>>>
>>> +# DT quirks
>>> +ifneq ($(CONFIG_OF_DYNAMIC),)
>>> +obj-$(CONFIG_SOC_AM33XX) += am33xx-dt-quirks.o
>>> +endif
>>
>> Won't this automatically be included on my Black that supports DT overlays?
>>
>
> Yes it will. It is a grand total of 498 lines of code, and the total size of
> the code segment is about 2.2K.
>
> You do realize that you’re probably booting a multi-platform kernel on the
> black right? Including things for all 2xxx/3xxx and 44xx platforms?
> For instance:
>
>> ~/ti/kernels/linux-github.git $ wc -l arch/arm/mach-omap2/*44xx*.c
>> 443 arch/arm/mach-omap2/clockdomains44xx_data.c
>> 526 arch/arm/mach-omap2/cminst44xx.c
>> 251 arch/arm/mach-omap2/cpuidle44xx.c
>> 250 arch/arm/mach-omap2/dpll44xx.c
>> 4864 arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>> 295 arch/arm/mach-omap2/pm44xx.c
>> 358 arch/arm/mach-omap2/powerdomains44xx_data.c
>> 62 arch/arm/mach-omap2/prcm_mpu44xx.c
>> 770 arch/arm/mach-omap2/prm44xx.c
>> 210 arch/arm/mach-omap2/prminst44xx.c
>> 117 arch/arm/mach-omap2/vc44xx_data.c
>> 130 arch/arm/mach-omap2/voltagedomains44xx_data.c
>> 104 arch/arm/mach-omap2/vp44xx_data.c
>> 8380 total
>
> I bet those things are far larger than 2.2K. And I also bet that in the
> tradeoff analysis that the board maintainer did things came down to
> increasing complexity so that he can consolidate the kernels for all the
> other platforms he has to support besides the black.

Not that it really matters, but I'm not using any of that.

>>> Some people really do though. As for increased risk
>>> I expect to see arguments instead of a statement.
>>
>> No one is wasting your time with random arguments. Please keep your tone civil.
>>
>
> A statement like 'increasing risk for all of us' is very open ended. What is
> the risk? How much of it exists?

My point was simply that this trades reduced complexity in one area
with increased complexity in another area.

For you, that trade-off is worth it, but for others, not so much.

FWIW, I agree that some mechanism is required to support the other
use cases. I just don't think ease of manufacturing, when the
submit configuration is the BeagleBone, is where I would hang my hat.

> If I offended you I’m really sorry though, I meant no such thing.

In re-reading it, I realize I shouldn't have taken offense. Thanks anyway
for the apology.

Regards,
Peter Hurley

2015-02-20 16:48:14

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
> >
> >> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
> >>
> >> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
> >>> Hi Frank,
> >>>
> >>>> On Feb 19, 2015, at 18:48 , Frank Rowand <[email protected]> wrote:
> >>>>
> >>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote:
> >>>>> Hi Mark,
> >>>>>
> >>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland <[email protected]> wrote:
> >>>>>>
> >>>>>>>>> +While this may in theory work, in practice it is very cumbersome
> >>>>>>>>> +for the following reasons:
> >>>>>>>>> +
> >>>>>>>>> +1. The act of selecting a different boot device tree blob requires
> >>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or
> >>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the
> >>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob.
> >>>>>>>>
> >>>>>>>> You can have several bootloader builds, or even a single build with
> >>>>>>>> something like appended DTB to get an appropriate DTB if the same binary
> >>>>>>>> will otherwise work across all variants of a board.
> >>>>>>>>
> >>>>>>>
> >>>>>>> No, the same DTB will not work across all the variants of a board.
> >>>>>>
> >>>>>> I wasn't on about the DTB. I was on about the loader binary, in the case
> >>>>>> the FW/bootloader could be common even if the DTB couldn't.
> >>>>>>
> >>>>>> To some extent there must be a DTB that will work across all variants
> >>>>>> (albeit with limited utility) or the quirk approach wouldn't work…
> >>>>>>
> >>>>>
> >>>>> That’s not correct; the only part of the DTB that needs to be common
> >>>>> is the model property that would allow the quirk detection logic to fire.
> >>>>>
> >>>>> So, there is a base DTB that will work on all variants, but that only means
> >>>>> that it will work only up to the point that the quirk detector method
> >>>>> can work. So while in recommended practice there are common subsets
> >>>>> of the DTB that might work, they might be unsafe.
> >>>>>
> >>>>> For instance on the beaglebone the regulator configuration is different
> >>>>> between white and black, it is imperative you get them right otherwise
> >>>>> you risk board damage.
> >>>>>
> >>>>>>>> So it's not necessarily true that you need a complex bootloader.
> >>>>>>>>
> >>>>>>>
> >>>>>>>>> +2. On many instances boot time is extremely critical; in some cases
> >>>>>>>>> +there are hard requirements like having working video feeds in under
> >>>>>>>>> +2 seconds from power-up. This leaves an extremely small time budget for
> >>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
> >>>>>>>>> +is by removing the standard bootloader from the normal boot sequence
> >>>>>>>>> +altogether by having a very small boot shim that loads the kernel and
> >>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> >>>>>>>>
> >>>>>>>> Given my previous comments above I don't see why this is relevant.
> >>>>>>>> You're already passing _some_ DTB here, so if you can organise for the
> >>>>>>>> board to statically provide a sane DTB that's fine, or you can resort to
> >>>>>>>> appended DTB if it's not possible to update the board configuration.
> >>>>>>>>
> >>>>>>>
> >>>>>>> You’re missing the point. I can’t use the same DTB for each revision of the
> >>>>>>> board. Each board is similar but it’s not identical.
> >>>>>>
> >>>>>> I think you've misunderstood my point. If you program the board with the
> >>>>>> relevant DTB, or use appended DTB, then you will pass the correct DTB to
> >>>>>> the kernel without need for quirks.
> >>>>>>
> >>>>>> I understand that each variant is somewhat incompatible (and hence needs
> >>>>>> its own DTB).
> >>>>>
> >>>>> In theory it might work, in practice this does not. Ludovic mentioned that they
> >>>>> have 27 different DTBs in use at the moment. At a relatively common 60k per DTB
> >>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed.
> >>>>
> >>>> < snip >
> >>>>
> >>>> Or you can install the correct DTB on the board. You trust your manufacturing line
> >>>> to install the correct resistors. You trust your manufacturing line to install the
> >>>> correct kernel version (eg an updated version to resolve a security issue).
> >>>>
> >>>> I thought the DT blob was supposed to follow the same standard that other OS's or
> >>>> bootloaders understood. Are you willing to break that? (This is one of those
> >>>> ripples I mentioned in my other emails.)
> >>>>
> >>>
> >>> Trust no-one.
> >>>
> >>> This is one of those things that the kernel community doesn’t understand which makes people
> >>> who push product quite mad.
> >>>
> >>> Engineering a product is not only about meeting customer spec, in order to turn a profit
> >>> the whole endeavor must be engineered as well for manufacturability.
> >>>
> >>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
> >>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
> >>> instead of turning a profit you’re losing money if you only have a few cents of profit
> >>> per unit.
> >>
> >> I'm not installing physical components manually. Why would I be installing software
> >> manually? (rhetorical question)
> >>
> >
> > Because on high volume product runs the flash comes preprogrammed and is soldered as is.
> >
> > Having a single binary to flash to every revision of the board makes logistics considerably
> > easier.
> >
> > Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
> > on the flash medium) takes time and is error-prone.
> >
> > Factory time == money, errors == money.
> >
> >>>
> >>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
> >>> for a few million units.
> >>
> >> And you produce a few million units before testing that the first one off the line works?
> >>
> >
> > The first one off the line works. The rest will get some burn in and functional testing if you’re
> > lucky. In many cases where the product is very cheap it might make financial sense to just ship
> > as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
> >
> > Hardware is hard :)
>
> I'm failing to see how this series improves your manufacturing process at all.
>
> 1. Won't you have to provide the factory with different eeprom images for the
> White and Black? You _trust_ them to get that right, or more likely, you
> have process control procedures in place so that you don't get 1 million Blacks
> flashed with the White eeprom image.
>

I am open to hearing your suggestions for our use case, where the CPU card with
the eeprom is manufactured separately from its carier cards.

I assume you might suggest that manufacturing should (re-)program the EEPROM
on the CPU card after it was inserted into the carrier.

Problem is though that the CPU card may be inserted into ts carrier outside
manufacturing, at the final stages of assembly or in product repair. Those
groups would typically not even have the means to (re-)program the eeprom.
Besides, manufacturing would, quite understandably, go ballistic if we demand
that they start programming EEPROMs after insertion into carrier, and no longer
use pre-programmed EEPROMs.

Note that it is not feasible to put the necessary EEPROM onto the carrier
either. Maybe in a later design. Maybe that makes sense, and we will go along
that route at some point. However, forcing a specific hardware solution
due to software limitations, ie lack of ability by core software to handle
the different carries, seems to be not the right decision to make on an
OS level.

In the PCI world it has long since been accepted that the world is not perfect.
The argument here is pretty much equivalent to demanding that PCI drop its
quirks mechanism, to force the HW manufacturers to finally get it right from
the beginning. I somehow suspect that this won't happen.

Instead of questioning the need for a mechanism such as the one proposed by
Pantelis, I think our time would be better spent arguing if it is the right
mechanism and, if not, how it can be improved.

Thanks,
Guenter

2015-02-20 16:49:36

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Peter,

> On Feb 20, 2015, at 18:34 , Peter Hurley <[email protected]> wrote:
>
> On 02/20/2015 10:38 AM, Pantelis Antoniou wrote:
>> Hi Peter,
>>
>>> On Feb 20, 2015, at 17:24 , Peter Hurley <[email protected]> wrote:
>>>
>>> On 02/20/2015 10:02 AM, Pantelis Antoniou wrote:
>>>> Hi Peter,
>>>>
>>>>> On Feb 20, 2015, at 17:00 , Peter Hurley <[email protected]> wrote:
>>>>>
>>>>> On 02/20/2015 09:35 AM, Ludovic Desroches wrote:
>
> [...]
>
>>>>>> As you said, we can imagine many reasons to have a failure during the
>>>>>> production, having several DTB files will increase the risk.
>>>>>
>>>>> It's interesting that you don't see the added complexity of open-coding
>>>>> the i2c driver or mixing DTS fragments for different designs as increased risk
>>>>> (for us all).
>>>>>
>>>>>
>>>>
>>>> You don’t have to use it.
>>>
>>>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>>>> index 5d27dfd..02129e7 100644
>>>> --- a/arch/arm/mach-omap2/Makefile
>>>> +++ b/arch/arm/mach-omap2/Makefile
>>>> @@ -259,6 +259,11 @@ obj-$(CONFIG_MACH_CRANEBOARD) += board-am3517crane.o
>>>>
>>>> obj-$(CONFIG_MACH_SBC3530) += board-omap3stalker.o
>>>>
>>>> +# DT quirks
>>>> +ifneq ($(CONFIG_OF_DYNAMIC),)
>>>> +obj-$(CONFIG_SOC_AM33XX) += am33xx-dt-quirks.o
>>>> +endif
>>>
>>> Won't this automatically be included on my Black that supports DT overlays?
>>>
>>
>> Yes it will. It is a grand total of 498 lines of code, and the total size of
>> the code segment is about 2.2K.
>>
>> You do realize that you’re probably booting a multi-platform kernel on the
>> black right? Including things for all 2xxx/3xxx and 44xx platforms?
>> For instance:
>>
>>> ~/ti/kernels/linux-github.git $ wc -l arch/arm/mach-omap2/*44xx*.c
>>> 443 arch/arm/mach-omap2/clockdomains44xx_data.c
>>> 526 arch/arm/mach-omap2/cminst44xx.c
>>> 251 arch/arm/mach-omap2/cpuidle44xx.c
>>> 250 arch/arm/mach-omap2/dpll44xx.c
>>> 4864 arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>>> 295 arch/arm/mach-omap2/pm44xx.c
>>> 358 arch/arm/mach-omap2/powerdomains44xx_data.c
>>> 62 arch/arm/mach-omap2/prcm_mpu44xx.c
>>> 770 arch/arm/mach-omap2/prm44xx.c
>>> 210 arch/arm/mach-omap2/prminst44xx.c
>>> 117 arch/arm/mach-omap2/vc44xx_data.c
>>> 130 arch/arm/mach-omap2/voltagedomains44xx_data.c
>>> 104 arch/arm/mach-omap2/vp44xx_data.c
>>> 8380 total
>>
>> I bet those things are far larger than 2.2K. And I also bet that in the
>> tradeoff analysis that the board maintainer did things came down to
>> increasing complexity so that he can consolidate the kernels for all the
>> other platforms he has to support besides the black.
>
> Not that it really matters, but I'm not using any of that.
>
>
>>>> Some people really do though. As for increased risk
>>>> I expect to see arguments instead of a statement.
>>>
>>> No one is wasting your time with random arguments. Please keep your tone civil.
>>>
>>
>> A statement like 'increasing risk for all of us' is very open ended. What is
>> the risk? How much of it exists?
>
> My point was simply that this trades reduced complexity in one area
> with increased complexity in another area.
>
> For you, that trade-off is worth it, but for others, not so much.
>

Of course and that’s the point. No-one is going to remove the vanilla
DTSs for the beaglebone black. If you don’t want to use this capability
there won’t be any impact.

The EEPROM probe is only triggered by the presence of the quirk node,
so if you don’t boot with a DTB that contains it there is no change
in the boot process.

> FWIW, I agree that some mechanism is required to support the other
> use cases. I just don't think ease of manufacturing, when the
> submit configuration is the BeagleBone, is where I would hang my hat.
>

The beaglebone is just an open source friendly platform that I can
demonstrate the capability.

>
>> If I offended you I’m really sorry though, I meant no such thing.
>
> In re-reading it, I realize I shouldn't have taken offense. Thanks anyway
> for the apology.
>
> Regards,
> Peter Hurley

Regards

— Pantelis

2015-02-20 17:30:36

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Fri, Feb 20, 2015 at 8:35 AM, Ludovic Desroches
<[email protected]> wrote:
> On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
>> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>> >
>> >> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>> >>
>> >> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>> >>> Hi Frank,

[...]

>> >>> This is one of those things that the kernel community doesn’t understand which makes people
>> >>> who push product quite mad.
>> >>>
>> >>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>> >>> the whole endeavor must be engineered as well for manufacturability.
>> >>>
>> >>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>> >>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>> >>> instead of turning a profit you’re losing money if you only have a few cents of profit
>> >>> per unit.
>> >>
>> >> I'm not installing physical components manually. Why would I be installing software
>> >> manually? (rhetorical question)
>> >>
>> >
>> > Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>> >
>> > Having a single binary to flash to every revision of the board makes logistics considerably
>> > easier.
>> >
>> > Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
>> > on the flash medium) takes time and is error-prone.
>> >
>> > Factory time == money, errors == money.
>> >
>> >>>
>> >>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>> >>> for a few million units.
>> >>
>> >> And you produce a few million units before testing that the first one off the line works?
>> >>
>> >
>> > The first one off the line works. The rest will get some burn in and functional testing if you’re
>> > lucky. In many cases where the product is very cheap it might make financial sense to just ship
>> > as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>> >
>> > Hardware is hard :)
>>
>> I'm failing to see how this series improves your manufacturing process at all.
>>
>> 1. Won't you have to provide the factory with different eeprom images for the
>> White and Black? You _trust_ them to get that right, or more likely, you
>> have process control procedures in place so that you don't get 1 million Blacks
>> flashed with the White eeprom image.
>>
>> 2. The White and Black use different memory technology so it's not as if the
>> eMMC from the Black will end up on the White SMT line (or vice versa).
>>
>> 3 For that matter, why wouldn't you worry that all the microSD cards intended
>> for the White were accidentally assembled with the first 50,000 Blacks; at
>> that point you're losing a lot more than a few cents of profit. And that has
>> nothing to do with what image you provided.
>>
>
> As you said, we can imagine many reasons to have a failure during the
> production, having several DTB files will increase the risk.

Then package them as a single file. You can even use DT to do that.
See u-boot FIT image.

Rob

2015-02-20 17:37:18

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Rob,

> On Feb 20, 2015, at 19:30 , Rob Herring <[email protected]> wrote:
>
> On Fri, Feb 20, 2015 at 8:35 AM, Ludovic Desroches
> <[email protected]> wrote:
>> On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
>>> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
>>>>
>>>>> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
>>>>>
>>>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
>>>>>> Hi Frank,
>
> [...]
>
>>>>>> This is one of those things that the kernel community doesn’t understand which makes people
>>>>>> who push product quite mad.
>>>>>>
>>>>>> Engineering a product is not only about meeting customer spec, in order to turn a profit
>>>>>> the whole endeavor must be engineered as well for manufacturability.
>>>>>>
>>>>>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
>>>>>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
>>>>>> instead of turning a profit you’re losing money if you only have a few cents of profit
>>>>>> per unit.
>>>>>
>>>>> I'm not installing physical components manually. Why would I be installing software
>>>>> manually? (rhetorical question)
>>>>>
>>>>
>>>> Because on high volume product runs the flash comes preprogrammed and is soldered as is.
>>>>
>>>> Having a single binary to flash to every revision of the board makes logistics considerably
>>>> easier.
>>>>
>>>> Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
>>>> on the flash medium) takes time and is error-prone.
>>>>
>>>> Factory time == money, errors == money.
>>>>
>>>>>>
>>>>>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
>>>>>> for a few million units.
>>>>>
>>>>> And you produce a few million units before testing that the first one off the line works?
>>>>>
>>>>
>>>> The first one off the line works. The rest will get some burn in and functional testing if you’re
>>>> lucky. In many cases where the product is very cheap it might make financial sense to just ship
>>>> as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
>>>>
>>>> Hardware is hard :)
>>>
>>> I'm failing to see how this series improves your manufacturing process at all.
>>>
>>> 1. Won't you have to provide the factory with different eeprom images for the
>>> White and Black? You _trust_ them to get that right, or more likely, you
>>> have process control procedures in place so that you don't get 1 million Blacks
>>> flashed with the White eeprom image.
>>>
>>> 2. The White and Black use different memory technology so it's not as if the
>>> eMMC from the Black will end up on the White SMT line (or vice versa).
>>>
>>> 3 For that matter, why wouldn't you worry that all the microSD cards intended
>>> for the White were accidentally assembled with the first 50,000 Blacks; at
>>> that point you're losing a lot more than a few cents of profit. And that has
>>> nothing to do with what image you provided.
>>>
>>
>> As you said, we can imagine many reasons to have a failure during the
>> production, having several DTB files will increase the risk.
>
> Then package them as a single file. You can even use DT to do that.
> See u-boot FIT image.
>

In the ideal case there is no u-boot, and no bootloader.

Packaging 27 difference DTBs, as in the Atmel people case, differing in only a few
properties seems a waste of space, no?

We keep on dancing around the issue, namely that DT does not have a quirk/variant
mechanism. I feel that it is a glaring omission. We can’t keep shoveling crap over
the fence to firmware and expect it to get buried there.

> Rob

Regards

— Pantelis

2015-02-20 18:10:09

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Guenter,

On 02/20/2015 11:47 AM, Guenter Roeck wrote:

[...]

> I am open to hearing your suggestions for our use case, where the CPU card with
> the eeprom is manufactured separately from its carier cards.

I think your use case may be more compelling than two flavors of Beaglebone
(one of which is pretty much a dead stick), but it's also less clear what your
design constraints are (not that I really want to know, 'cause I don't).

But the logical extension of this is an N-way dtb that supports unrelated SOCs,
and I think most would agree that's not an acceptable outcome.

My thought was that every design that can afford an EEPROM to probe can afford
a bootloader to select the appropriate dtb, and can afford the extra space
required for multiple dtbs.

I'm not naysaying; I just want to elicit enough information so the community
can make informed decisions.

> I assume you might suggest that manufacturing should (re-)program the EEPROM
> on the CPU card after it was inserted into the carrier.
>
> Problem is though that the CPU card may be inserted into ts carrier outside
> manufacturing, at the final stages of assembly or in product repair. Those
> groups would typically not even have the means to (re-)program the eeprom.
> Besides, manufacturing would, quite understandably, go ballistic if we demand
> that they start programming EEPROMs after insertion into carrier, and no longer
> use pre-programmed EEPROMs.

I agree; that would be The Wrong Way.

> Note that it is not feasible to put the necessary EEPROM onto the carrier
> either. Maybe in a later design. Maybe that makes sense, and we will go along
> that route at some point. However, forcing a specific hardware solution
> due to software limitations, ie lack of ability by core software to handle
> the different carries, seems to be not the right decision to make on an
> OS level.

Agreed; hardware is what it is.

> In the PCI world it has long since been accepted that the world is not perfect.
> The argument here is pretty much equivalent to demanding that PCI drop its
> quirks mechanism, to force the HW manufacturers to finally get it right from
> the beginning. I somehow suspect that this won't happen.

I was thinking back to the introductions of fast DEVSEL# and AGP :(

> Instead of questioning the need for a mechanism such as the one proposed by
> Pantelis, I think our time would be better spent arguing if it is the right
> mechanism and, if not, how it can be improved.

My thoughts exactly. Apologies if something I wrote came across as
"You Shall Not Pass" :)

One issue seems to be the moving target that is the compelling use case(s).
The initial submission implied it was the Beaglebone, which comes with
4GB eMMC/microSD so naturally the argument for space-savings with DTBs doesn't
fly. That has since been clarified so no need to rehash that.

Regards,
Peter Hurley

2015-02-20 18:48:42

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Fri, Feb 20, 2015 at 01:09:58PM -0500, Peter Hurley wrote:
> Hi Guenter,
>
> On 02/20/2015 11:47 AM, Guenter Roeck wrote:
>
> [...]
>
> > I am open to hearing your suggestions for our use case, where the CPU card with
> > the eeprom is manufactured separately from its carier cards.
>
> I think your use case may be more compelling than two flavors of Beaglebone
> (one of which is pretty much a dead stick), but it's also less clear what your
> design constraints are (not that I really want to know, 'cause I don't).
>
> But the logical extension of this is an N-way dtb that supports unrelated SOCs,
> and I think most would agree that's not an acceptable outcome.
>
With this logic you can pretty much refuse to accept anything new, anywhere.
Including everything old, for that matter.

Food can be abused to poison people, therefore no one should be permitted to
distribute food. Houses can be abused to falsely imprison people, therefore
no one should live in houses. And don't even start talking about guns.
Everything can be abused, therefore we should not do anything.

Discussions about possible abuse are for sure valid, and reducing the potential
for abuse is a worthy goal, but not as argument to reject a solution for an
existing and real roblem outright.

> My thought was that every design that can afford an EEPROM to probe can afford
> a bootloader to select the appropriate dtb, and can afford the extra space
> required for multiple dtbs.
>
There are many more use cases where this is simply not the case. Another one,
for example, is a system where the devicetree is loaded through u-boot using
tftp. In this case, u-boot would have some information about the hardware
to request the correct devicetree file, but it may not know about hardware
variants.

Sure, one could solve that problem by making u-boot aware of such variants
and maintaining a large number of dtb files as you suggest. That means,
however, that there would be that need to maintain all those dtb files
just to address minor differences, and having modify every piece of the
system for each new variant. In essence you put a lot of burden on pretty
much everyone from software to manufacturing to testing, plus possibly
hardware, just for the purpose of rejecting a relatively simple solution
for the problem Pantelis' patch set is trying to address.

Ultimately, no matter what the kernel community ends up doing, Pantelis'
solution or something similar _will_ find its way into our system.
I would very much prefer to have the code upstream, but we will carry
his patches along in our vendor branch if necessary. The functionality
and its benefits for our development, manufacturing, and testing process
are way too valuable to ignore, and it actually solves problems for which
we don't have an acceptable solution today. I would be quite surprised
if other vendors would not end up doing the same.

Guenter

2015-02-23 07:00:23

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Rob,

On Fri, Feb 20, 2015 at 11:30:12AM -0600, Rob Herring wrote:
> On Fri, Feb 20, 2015 at 8:35 AM, Ludovic Desroches
> <[email protected]> wrote:
> > On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote:
> >> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote:
> >> >
> >> >> On Feb 19, 2015, at 19:30 , Frank Rowand <[email protected]> wrote:
> >> >>
> >> >> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote:
> >> >>> Hi Frank,
>
> [...]
>
> >> >>> This is one of those things that the kernel community doesn’t understand which makes people
> >> >>> who push product quite mad.
> >> >>>
> >> >>> Engineering a product is not only about meeting customer spec, in order to turn a profit
> >> >>> the whole endeavor must be engineered as well for manufacturability.
> >> >>>
> >> >>> Yes, you can always manually install files in the bootloader. For 1 board no problem.
> >> >>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? Guess what,
> >> >>> instead of turning a profit you’re losing money if you only have a few cents of profit
> >> >>> per unit.
> >> >>
> >> >> I'm not installing physical components manually. Why would I be installing software
> >> >> manually? (rhetorical question)
> >> >>
> >> >
> >> > Because on high volume product runs the flash comes preprogrammed and is soldered as is.
> >> >
> >> > Having a single binary to flash to every revision of the board makes logistics considerably
> >> > easier.
> >> >
> >> > Having to boot and tweak the bootloader settings to select the correct dtb (even if it’s present
> >> > on the flash medium) takes time and is error-prone.
> >> >
> >> > Factory time == money, errors == money.
> >> >
> >> >>>
> >> >>> No knobs to tweak means no knobs to break. And a broken knob can have pretty bad consequences
> >> >>> for a few million units.
> >> >>
> >> >> And you produce a few million units before testing that the first one off the line works?
> >> >>
> >> >
> >> > The first one off the line works. The rest will get some burn in and functional testing if you’re
> >> > lucky. In many cases where the product is very cheap it might make financial sense to just ship
> >> > as is and deal with recalls, if you’re reasonably happy after a little bit of statistical sampling.
> >> >
> >> > Hardware is hard :)
> >>
> >> I'm failing to see how this series improves your manufacturing process at all.
> >>
> >> 1. Won't you have to provide the factory with different eeprom images for the
> >> White and Black? You _trust_ them to get that right, or more likely, you
> >> have process control procedures in place so that you don't get 1 million Blacks
> >> flashed with the White eeprom image.
> >>
> >> 2. The White and Black use different memory technology so it's not as if the
> >> eMMC from the Black will end up on the White SMT line (or vice versa).
> >>
> >> 3 For that matter, why wouldn't you worry that all the microSD cards intended
> >> for the White were accidentally assembled with the first 50,000 Blacks; at
> >> that point you're losing a lot more than a few cents of profit. And that has
> >> nothing to do with what image you provided.
> >>
> >
> > As you said, we can imagine many reasons to have a failure during the
> > production, having several DTB files will increase the risk.
>
> Then package them as a single file. You can even use DT to do that.
> See u-boot FIT image.
>
> Rob

It is acualyy what we did but we are not happy with this solution
because as said previously we rely on U-Boot and on dts/dtsi side we
have too many files.

Regards

Ludovic

2015-02-23 07:30:50

[permalink] [raw]

Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

On Fri, Feb 20, 2015 at 10:48:18AM -0800, Guenter Roeck wrote:
> On Fri, Feb 20, 2015 at 01:09:58PM -0500, Peter Hurley wrote:
> > Hi Guenter,
> >
> > On 02/20/2015 11:47 AM, Guenter Roeck wrote:
> >
> > [...]
> >
> > > I am open to hearing your suggestions for our use case, where the CPU card with
> > > the eeprom is manufactured separately from its carier cards.
> >
> > I think your use case may be more compelling than two flavors of Beaglebone
> > (one of which is pretty much a dead stick), but it's also less clear what your
> > design constraints are (not that I really want to know, 'cause I don't).
> >
> > But the logical extension of this is an N-way dtb that supports unrelated SOCs,
> > and I think most would agree that's not an acceptable outcome.
> >
> With this logic you can pretty much refuse to accept anything new, anywhere.
> Including everything old, for that matter.
>
> Food can be abused to poison people, therefore no one should be permitted to
> distribute food. Houses can be abused to falsely imprison people, therefore
> no one should live in houses. And don't even start talking about guns.
> Everything can be abused, therefore we should not do anything.
>
> Discussions about possible abuse are for sure valid, and reducing the potential
> for abuse is a worthy goal, but not as argument to reject a solution for an
> existing and real roblem outright.
>
> > My thought was that every design that can afford an EEPROM to probe can afford
> > a bootloader to select the appropriate dtb, and can afford the extra space
> > required for multiple dtbs.
> >
> There are many more use cases where this is simply not the case. Another one,
> for example, is a system where the devicetree is loaded through u-boot using
> tftp. In this case, u-boot would have some information about the hardware
> to request the correct devicetree file, but it may not know about hardware
> variants.
>
> Sure, one could solve that problem by making u-boot aware of such variants
> and maintaining a large number of dtb files as you suggest. That means,
> however, that there would be that need to maintain all those dtb files
> just to address minor differences, and having modify every piece of the
> system for each new variant. In essence you put a lot of burden on pretty
> much everyone from software to manufacturing to testing, plus possibly
> hardware, just for the purpose of rejecting a relatively simple solution
> for the problem Pantelis' patch set is trying to address.
>

Other example that show how it becomes a mess. Tell me if we do that in
a wrong way but I don't think.

We have the following source files:
- a dtsi file for the SoC family
- a dtsi file for SoC variant enabling the devices available
- a dtsi file for the CPU module describing components on this module:
the ethernet phy, the nand flash, etc.
- a dtsi file for the motherboard
- several dtsi files for different kind of display modules
- many dts file for the final board, it includes the SoC variant, the
CPU module, the motherboard and one of the display module if needed.

At the end we have something like this (from our github, not all these
boards have been sent to the mainline)

arch/arm/boot/dts/sama5d3.dtsi
arch/arm/boot/dts/sama5d34ek_pda7.dts
arch/arm/boot/dts/sama5d36ek_revc_pda7.dts
arch/arm/boot/dts/sama5d31ek.dts
arch/arm/boot/dts/sama5d34ek_revc.dts
arch/arm/boot/dts/sama5d3xcm.dtsi
arch/arm/boot/dts/sama5d31ek_pda4.dts
arch/arm/boot/dts/sama5d34ek_revc_pda4.dts
arch/arm/boot/dts/sama5d3xdm.dtsi
arch/arm/boot/dts/sama5d31ek_pda7.dts
arch/arm/boot/dts/sama5d34ek_revc_pda7.dts
arch/arm/boot/dts/sama5d3xdm_pda4.dtsi
arch/arm/boot/dts/sama5d31ek_revc.dts
arch/arm/boot/dts/sama5d35ek.dts
arch/arm/boot/dts/sama5d3xdm_pda7.dtsi
arch/arm/boot/dts/sama5d31ek_revc_pda4.dts
arch/arm/boot/dts/sama5d35ek_revc.dts
arch/arm/boot/dts/sama5d3xek.its
arch/arm/boot/dts/sama5d31ek_revc_pda7.dts
arch/arm/boot/dts/sama5d36ek.dts
arch/arm/boot/dts/sama5d3xek_pda4.its
arch/arm/boot/dts/sama5d33ek.dts
arch/arm/boot/dts/sama5d36ek_cmp.dts
arch/arm/boot/dts/sama5d3xek_pda7.its
arch/arm/boot/dts/sama5d33ek_pda4.dts
arch/arm/boot/dts/sama5d36ek_cmp_pins_sleep.dtsi
arch/arm/boot/dts/sama5d3xmb.dtsi
arch/arm/boot/dts/sama5d33ek_pda7.dts
arch/arm/boot/dts/sama5d36ek_hdmi.dts
arch/arm/boot/dts/sama5d3xmb_revc.dtsi
arch/arm/boot/dts/sama5d33ek_revc.dts
arch/arm/boot/dts/sama5d36ek_pda4.dts
arch/arm/boot/dts/sama5d3xmb_revc_isi.dtsi
arch/arm/boot/dts/sama5d33ek_revc_pda4.dts
arch/arm/boot/dts/sama5d36ek_pda7.dts
arch/arm/boot/dts/sama5d4.dtsi
arch/arm/boot/dts/sama5d33ek_revc_pda7.dts
arch/arm/boot/dts/sama5d36ek_revc.dts
arch/arm/boot/dts/sama5d4ek.dts
arch/arm/boot/dts/sama5d34ek.dts
arch/arm/boot/dts/sama5d36ek_revc_cmp.dts
arch/arm/boot/dts/sama5d4ek_hdmi.dts
arch/arm/boot/dts/sama5d34ek_pda4.dts
arch/arm/boot/dts/sama5d36ek_revc_pda4.dts
arch/arm/boot/dts/sama5d4ek_pin_sleep_state.dtsi

> Ultimately, no matter what the kernel community ends up doing, Pantelis'
> solution or something similar _will_ find its way into our system.
> I would very much prefer to have the code upstream, but we will carry
> his patches along in our vendor branch if necessary. The functionality
> and its benefits for our development, manufacturing, and testing process
> are way too valuable to ignore, and it actually solves problems for which
> we don't have an acceptable solution today. I would be quite surprised
> if other vendors would not end up doing the same.

We'll probably do the same. Beaglebone is only an example.

>
> Guenter

Regards

Ludovic

2015-02-23 18:39:29

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants

Hi Pantelis,

On 02/19/2015 01:44 PM, Pantelis Antoniou wrote:
> Hi Tony,
>
>> On Feb 19, 2015, at 20:36 , Tony Lindgren <[email protected]> wrote:
>>
>> * Pantelis Antoniou <[email protected]> [150219 10:32]:
>>>> On Feb 19, 2015, at 20:16 , Tony Lindgren <[email protected]> wrote:
>>>>
>>>> Uhh I don't like the idea of duplicating the i2c-omap.c driver under
>>>> arch/arm.. And in general we should initialize things later rather
>>>> than earlier.
>>>>
>>>> What's stopping doing these quirk checks later on time with just
>>>> a regular device driver, something like drivers/misc/bbone-quirks.c?
>>>>
>>>
>>> We have no choice; we are way early in the boot process, right after
>>> the device tree unflattening step.
>>
>> To me it seems the dt patching part should be done with minimal
>> code before any driver like features..
>>
>
> The way it’s done right now is with minimal code. Reading the EEPROM
> is required.
>
>>> I’ve toyed with the idea of using early platform devices but the omap-i2c driver
>>> would need some tender love and care to make it work, and I didn’t want to get
>>> bogged down with i2c driver details at this point.
>>
>> ..so how about just parse a kernel cmdline for the quirks to apply
>> based on a version string or similar? That can be easily populated
>> by u-boot or set manually with setenv.
>>
>> That leaves out the need for tinkering with i2c super early in
>> the kernel for revision detection.
>>
>
> You assume there’s going to be a bootloader…

So does this patch.

> diff --git a/arch/arm/mach-omap2/am33xx-dt-quirks.c b/arch/arm/mach-omap2/am33xx-dt-quirks.c
[...]
> + * Note that we rely on the bootloader setting up the muxes
> + * (which is the case for u-boot).

Regards,
Peter Hurley

2015-02-23 18:48:47