[RFC 0/5] drivers: Add boot constraints core

[RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

Hi,

I am sending this RFC to get early comments before I put too much
effort in the solution proposed here. The solution isn't fully complete
yet.


Problem statement:

Some devices are powered ON by the bootloaders before the bootloader
handovers control to Linux. It maybe important for those devices to keep
working until the time a Linux device driver probes the device and
reconfigure its resources.

A typical example of that can be the LCD controller, which is used by
the bootloaders to show image(s) while the machine is booting into
Linux. The LCD controller can be using some resources, like clk,
regulators, etc, that are shared between several devices. These shared
resources should be programmed so that all the users of them are
satisfied. If a user (X) driver gets probed before the LCD controller
driver in this case, then it may end up reconfiguring these resources to
ranges satisfying the current users (only user X) and that can make the
LCD screen unstable.

Of course we can have more complex cases where the same resource is
getting used by two devices while the kernel boots and the order in
which devices get probed wouldn't matter as the other device will surely
break then.


Proposed solution:

This patchset introduces the concept of boot-constraints, which are set
by the different parts of the kernel (on behalf of the bootloaders) and
the kernel will satisfy them until the time driver for such a device is
probed (successfully or unsuccessfully). Once the driver's probe()
routine is called, the driver core removes the constraints set for the
particular device. Only the power-supply constraint type is supported
for now.

This can be used across platforms working with DT, ACPI, etc and has no
dependency on those.


What's left ?

There are a couple of problems which aren't solved yet:

o EPROBE_DEFER: Deferred probing is a major problem that needs to be
  solved. Because we want to add the constraint ASAP (i.e. before most
  of the drivers are registered), we would end up doing that right after
  the device is created. That is very early in the kernel boot cycle and
  its very much possible that the resource we need to get (in order to
  set the constraint) isn't available yet. Like regulator in case of
  supply constraint.
  
  Now how do we control that the constraint is set right after the
  resource is available, and before any other user of the resource comes
  up ?

  One way out is to set "driver_deferred_probe_enable" to 'true'
  (drivers/base/dd.c), after we got EPROBE_DEFER for any of the
  constraints. That would make sure that all the deferred drivers get a
  chance to get probed again, after addition of every new device.
  Without this change, we probe all the deferred devices only after
  late_init.

  But that may not work as it depends on a new workqueue thread for
  doing this work, and no one is stopping another driver to get
  registered by that time.

  I also thought about using the "functional dependency" stuff [1] that
  Rafael introduced earlier, but that too has its own challenges.
  Specifically, we may not have the device structures available for all
  the consumers/suppliers to start with.

o DT support will be added at first and I am planning to add the
  constraints right from drivers/of/platform.c after the AMBA and
  platform devices are created automatically from DT. Some sort of
  bindings (per constraint type) are required to be defined for that of
  course. We can think about other interfaces (like ACPI) as well, if we
  have any users right now.


But yeah, the first thing is to get some sort of feedback for the
proposed solution. :)

FWIW, I started discussing this with Mark Brown earlier [2] and this
series is a follow-up to that discussion.

--
viresh

[1] commit 9ed9895370ae ("driver core: Functional dependencies tracking support")
[2] https://marc.info/?l=linux-kernel&m=149423887617635

Viresh Kumar (5):
  drivers: Add boot constraints core
  drivers: boot_constraint: Add support for supply constraints
  drivers: boot_constraint: Add boot_constraints_disable kernel
    parameter
  drivers: boot_constraint: Add debugfs support
  drivers: Code to test boot constraints

 Documentation/admin-guide/kernel-parameters.txt |   2 +
 drivers/base/Kconfig                            |  11 +
 drivers/base/Makefile                           |   1 +
 drivers/base/boot_constraint.c                  | 402 ++++++++++++++++++++++++
 drivers/base/dd.c                               |  20 +-
 drivers/base/test_plat_boot_constraint.c        |  51 +++
 include/linux/boot_constraint.h                 |  35 +++
 7 files changed, 515 insertions(+), 7 deletions(-)
 create mode 100644 drivers/base/boot_constraint.c
 create mode 100644 drivers/base/test_plat_boot_constraint.c
 create mode 100644 include/linux/boot_constraint.h

-- 
2.13.0.71.gd7076ec9c9cb

[RFC 1/5] drivers: Add boot constraints core

[Viresh Kumar]

Some devices are powered ON by the bootloaders before the bootloader
handovers control to Linux. It maybe important for those devices to keep
working until the time a Linux device driver probes the device and
reconfigure its resources.

A typical example of that can be the LCD controller, which is used by
the bootloaders to show image(s) while the device is booting into Linux.
The LCD controller can be using some resources, like clk, regulators,
etc, that are shared between several devices. These shared resources
should be programmed so that all the users of them are satisfied. If
some user (X) driver gets probed before the LCD controller driver in
this case, then it may end up reconfiguring these resources to ranges
satisfying the current users (only user X) and that can make the LCD
screen unstable.

This patch introduces the concept of boot-constraints, which will be set
by the bootloaders and the kernel will satisfy them until the time
driver for such a device is probed (successfully or unsuccessfully).

The list of boot constraint types is empty for now, and will be added by
a later patch.

Only two routines are exposed by the boot constraints core for now:

- boot_constraint_add(): This will be called by parts of the kernel
  (before the device is probed) to set the constraints.

- boot_constraints_remove(): This is called only by the driver core
  after a device is probed successfully or unsuccessfully. Special
  handling is done here for deffered probing.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/Kconfig            |  11 +++
 drivers/base/Makefile           |   1 +
 drivers/base/boot_constraint.c  | 210 ++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c               |  20 ++--
 include/linux/boot_constraint.h |  28 ++++++
 5 files changed, 263 insertions(+), 7 deletions(-)
 create mode 100644 drivers/base/boot_constraint.c
 create mode 100644 include/linux/boot_constraint.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index d718ae4b907a..d71217a91793 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -339,4 +339,15 @@ config CMA_ALIGNMENT
 
 endif
 
+config BOOT_CONSTRAINTS
+	bool "Boot constraints for devices"
+	default y
+	help
+	  This enables boot constraints detection for devices. These constraints
+	  are (normally) set by the Bootloader and must be satisfied by the
+	  kernel until the relevant device driver is probed. Once the driver is
+	  probed, the constraint is dropped.
+
+	  If unsure, say Y.
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index f2816f6ff76a..6094b3b75184 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -5,6 +5,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
 			   topology.o container.o property.o cacheinfo.o
+obj-$(CONFIG_BOOT_CONSTRAINTS) += boot_constraint.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/boot_constraint.c b/drivers/base/boot_constraint.c
new file mode 100644
index 000000000000..38740b8499ba
--- /dev/null
+++ b/drivers/base/boot_constraint.c
@@ -0,0 +1,210 @@
+/*
+ * This takes care of boot time constraints, normally set by the Bootloader.
+ *
+ * Copyright (C) 2017 Linaro.
+ * Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * This file is released under the GPLv2.
+ */
+
+#define pr_fmt(fmt) "Boot Constraints: " fmt
+
+#include <linux/boot_constraint.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+
+struct constraint {
+	struct constraint_dev *cdev;
+	struct list_head node;
+	enum boot_constraint_type type;
+
+	int (*add)(struct constraint *constraint, void *data);
+	void (*remove)(struct constraint *constraint);
+	void *private;
+};
+
+struct constraint_dev {
+	struct device *dev;
+	struct list_head node;
+	struct list_head constraints;
+};
+
+#define for_each_constraint(_constraint, _temp, _cdev)		\
+	list_for_each_entry_safe(_constraint, _temp, &_cdev->constraints, node)
+
+/* Global list of all constraint devices currently registered */
+static LIST_HEAD(constraint_devices);
+static DEFINE_MUTEX(constraint_devices_mutex);
+
+/* Forward declarations of constraints */
+
+
+/* Boot constraints core */
+
+static struct constraint_dev *constraint_device_find(struct device *dev)
+{
+	struct constraint_dev *cdev;
+
+	list_for_each_entry(cdev, &constraint_devices, node) {
+		if (cdev->dev == dev)
+			return cdev;
+	}
+
+	return NULL;
+}
+
+static struct constraint_dev *constraint_device_allocate(struct device *dev)
+{
+	struct constraint_dev *cdev;
+
+	cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
+	if (!cdev)
+		return ERR_PTR(-ENOMEM);
+
+	cdev->dev = dev;
+	INIT_LIST_HEAD(&cdev->node);
+	INIT_LIST_HEAD(&cdev->constraints);
+
+	list_add(&cdev->node, &constraint_devices);
+
+	return cdev;
+}
+
+static void constraint_device_free(struct constraint_dev *cdev)
+{
+	list_del(&cdev->node);
+	kfree(cdev);
+}
+
+static struct constraint_dev *constraint_device_get(struct device *dev)
+{
+	struct constraint_dev *cdev;
+
+	cdev = constraint_device_find(dev);
+	if (cdev)
+		return cdev;
+
+	cdev = constraint_device_allocate(dev);
+	if (IS_ERR(cdev)) {
+		dev_err(dev, "Failed to add constraint dev (%ld)\n",
+			PTR_ERR(cdev));
+	}
+
+	return cdev;
+}
+
+static void constraint_device_put(struct constraint_dev *cdev)
+{
+	if (!list_empty(&cdev->constraints))
+		return;
+
+	constraint_device_free(cdev);
+}
+
+static struct constraint *constraint_allocate(struct constraint_dev *cdev,
+					      enum boot_constraint_type type)
+{
+	struct constraint *constraint;
+	int (*add)(struct constraint *constraint, void *data);
+	void (*remove)(struct constraint *constraint);
+
+	switch (type) {
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	constraint = kzalloc(sizeof(*constraint), GFP_KERNEL);
+	if (!constraint)
+		return ERR_PTR(-ENOMEM);
+
+	constraint->cdev = cdev;
+	constraint->type = type;
+	constraint->add = add;
+	constraint->remove = remove;
+	INIT_LIST_HEAD(&constraint->node);
+
+	list_add(&constraint->node, &cdev->constraints);
+
+	return constraint;
+}
+
+static void constraint_free(struct constraint *constraint)
+{
+	list_del(&constraint->node);
+	kfree(constraint);
+}
+
+int boot_constraint_add(struct device *dev, enum boot_constraint_type type,
+			void *data)
+{
+	struct constraint_dev *cdev;
+	struct constraint *constraint;
+	int ret;
+
+	mutex_lock(&constraint_devices_mutex);
+
+	/* Find or add the cdev type first */
+	cdev = constraint_device_get(dev);
+	if (IS_ERR(cdev)) {
+		ret = PTR_ERR(cdev);
+		goto unlock;
+	}
+
+	constraint = constraint_allocate(cdev, type);
+	if (IS_ERR(constraint)) {
+		dev_err(dev, "Failed to add constraint type: %d (%ld)\n", type,
+			PTR_ERR(constraint));
+		ret = PTR_ERR(constraint);
+		goto put_cdev;
+	}
+
+	/* Set constraint */
+	ret = constraint->add(constraint, data);
+	if (ret)
+		goto free_constraint;
+
+	dev_dbg(dev, "Added boot constraint-type (%d)\n", type);
+
+	mutex_unlock(&constraint_devices_mutex);
+
+	return 0;
+
+free_constraint:
+	constraint_free(constraint);
+put_cdev:
+	constraint_device_put(cdev);
+unlock:
+	mutex_unlock(&constraint_devices_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(boot_constraint_add);
+
+static void constraint_remove(struct constraint *constraint)
+{
+	constraint->remove(constraint);
+	constraint_free(constraint);
+}
+
+void boot_constraints_remove(struct device *dev)
+{
+	struct constraint_dev *cdev;
+	struct constraint *constraint, *temp;
+
+	mutex_lock(&constraint_devices_mutex);
+
+	cdev = constraint_device_find(dev);
+	if (!cdev)
+		goto unlock;
+
+	for_each_constraint(constraint, temp, cdev)
+		constraint_remove(constraint);
+
+	constraint_device_put(cdev);
+unlock:
+	mutex_unlock(&constraint_devices_mutex);
+}
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 4882f06d12df..4eb9d183d647 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -17,6 +17,7 @@
  * This file is released under the GPLv2
  */
 
+#include <linux/boot_constraint.h>
 #include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
@@ -381,15 +382,20 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	 */
 	devices_kset_move_last(dev);
 
-	if (dev->bus->probe) {
+	if (dev->bus->probe)
 		ret = dev->bus->probe(dev);
-		if (ret)
-			goto probe_failed;
-	} else if (drv->probe) {
+	else if (drv->probe)
 		ret = drv->probe(dev);
-		if (ret)
-			goto probe_failed;
-	}
+
+	/*
+	 * Remove boot constraints for both successful and unsuccessful probe(),
+	 * except for the case where EPROBE_DEFER is returned by probe().
+	 */
+	if (ret != -EPROBE_DEFER)
+		boot_constraints_remove(dev);
+
+	if (ret)
+		goto probe_failed;
 
 	if (test_remove) {
 		test_remove = false;
diff --git a/include/linux/boot_constraint.h b/include/linux/boot_constraint.h
new file mode 100644
index 000000000000..41b5a62d2dbb
--- /dev/null
+++ b/include/linux/boot_constraint.h
@@ -0,0 +1,28 @@
+/*
+ * Boot constraints header.
+ *
+ * Copyright (C) 2017 Linaro.
+ * Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/err.h>
+#include <linux/types.h>
+
+struct device;
+
+enum boot_constraint_type {
+	BOOT_CONSTRAINT_NONE,
+};
+
+#ifdef CONFIG_BOOT_CONSTRAINTS
+int boot_constraint_add(struct device *dev, enum boot_constraint_type type,
+			void *data);
+void boot_constraints_remove(struct device *dev);
+#else
+static inline int boot_constraint_add(struct device *dev,
+				      enum boot_constraint_type type, void *data)
+{ return -EINVAL; }
+static inline void boot_constraints_remove(struct device *dev) {}
+#endif
-- 
2.13.0.71.gd7076ec9c9cb

[RFC 2/5] drivers: boot_constraint: Add support for supply constraints

[Viresh Kumar]

This patch adds the first constraint type: power-supply.

The constraint is set by setting a voltage range for the respective
regulator device, which will be honored by the regulator core even if
more users turn up. Once the device is probed, the regulator is
released and the constraint is removed.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/boot_constraint.c  | 92 +++++++++++++++++++++++++++++++++++++++++
 include/linux/boot_constraint.h |  9 +++-
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/base/boot_constraint.c b/drivers/base/boot_constraint.c
index 38740b8499ba..495344e6405b 100644
--- a/drivers/base/boot_constraint.c
+++ b/drivers/base/boot_constraint.c
@@ -15,6 +15,7 @@
 #include <linux/export.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 struct constraint {
@@ -41,6 +42,8 @@ static LIST_HEAD(constraint_devices);
 static DEFINE_MUTEX(constraint_devices_mutex);
 
 /* Forward declarations of constraints */
+static int constraint_supply_add(struct constraint *constraint, void *data);
+static void constraint_supply_remove(struct constraint *constraint);
 
 
 /* Boot constraints core */
@@ -113,6 +116,10 @@ static struct constraint *constraint_allocate(struct constraint_dev *cdev,
 	void (*remove)(struct constraint *constraint);
 
 	switch (type) {
+	case BOOT_CONSTRAINT_SUPPLY:
+		add = constraint_supply_add;
+		remove = constraint_supply_remove;
+		break;
 	default:
 		return ERR_PTR(-EINVAL);
 	}
@@ -208,3 +215,88 @@ void boot_constraints_remove(struct device *dev)
 unlock:
 	mutex_unlock(&constraint_devices_mutex);
 }
+
+
+/* Boot constraints */
+
+/* Boot constraint - Supply */
+
+struct constraint_supply {
+	struct boot_constraint_supply_info supply;
+	struct regulator *reg;
+};
+
+static int constraint_supply_add(struct constraint *constraint, void *data)
+{
+	struct boot_constraint_supply_info *supply = data;
+	struct constraint_supply *csupply;
+	struct device *dev = constraint->cdev->dev;
+	int ret;
+
+	csupply = kzalloc(sizeof(*csupply), GFP_KERNEL);
+	if (!csupply)
+		return -ENOMEM;
+
+	csupply->reg = regulator_get(dev, supply->name);
+	if (IS_ERR(csupply->reg)) {
+		ret = PTR_ERR(csupply->reg);
+		if (ret != -EPROBE_DEFER) {
+			dev_err(dev, "regulator_get() failed for %s (%d)\n",
+				supply->name, ret);
+		}
+		goto free;
+	}
+
+	ret = regulator_set_voltage(csupply->reg, supply->u_volt_min,
+				    supply->u_volt_max);
+	if (ret) {
+		dev_err(dev, "regulator_set_voltage %s failed (%d)\n",
+			supply->name, ret);
+		goto free_regulator;
+	}
+
+	if (supply->enable) {
+		ret = regulator_enable(csupply->reg);
+		if (ret) {
+			dev_err(dev, "regulator_enable %s failed (%d)\n",
+				supply->name, ret);
+			goto remove_voltage;
+		}
+	}
+
+	memcpy(&csupply->supply, supply, sizeof(*supply));
+	csupply->supply.name = kstrdup_const(supply->name, GFP_KERNEL);
+	constraint->private = csupply;
+
+	return 0;
+
+remove_voltage:
+	regulator_set_voltage(csupply->reg, 0, INT_MAX);
+free_regulator:
+	regulator_put(csupply->reg);
+free:
+	kfree(csupply);
+
+	return ret;
+}
+
+static void constraint_supply_remove(struct constraint *constraint)
+{
+	struct constraint_supply *csupply = constraint->private;
+	struct device *dev = constraint->cdev->dev;
+	int ret;
+
+	if (csupply->supply.enable) {
+		ret = regulator_disable(csupply->reg);
+		if (ret)
+			dev_err(dev, "regulator_disable failed (%d)\n", ret);
+	}
+
+	ret = regulator_set_voltage(csupply->reg, 0, INT_MAX);
+	if (ret)
+		dev_err(dev, "regulator_set_voltage failed (%d)\n", ret);
+
+	regulator_put(csupply->reg);
+	kfree_const(csupply->supply.name);
+	kfree(csupply);
+}
diff --git a/include/linux/boot_constraint.h b/include/linux/boot_constraint.h
index 41b5a62d2dbb..a98fdb403e8d 100644
--- a/include/linux/boot_constraint.h
+++ b/include/linux/boot_constraint.h
@@ -13,7 +13,14 @@
 struct device;
 
 enum boot_constraint_type {
-	BOOT_CONSTRAINT_NONE,
+	BOOT_CONSTRAINT_SUPPLY,
+};
+
+struct boot_constraint_supply_info {
+	bool enable;
+	const char *name;
+	unsigned long u_volt_min;
+	unsigned long u_volt_max;
 };
 
 #ifdef CONFIG_BOOT_CONSTRAINTS
-- 
2.13.0.71.gd7076ec9c9cb

[RFC 3/5] drivers: boot_constraint: Add boot_constraints_disable kernel parameter

[Viresh Kumar]

Users must be given an option to discard any constraints set by
bootloaders. For example, consider that a constraint is set for the LCD
controller's supply and the LCD driver isn't loaded by the kernel. If
the user doesn't need to use the LCD device, then he shouldn't be forced
to honour the constraint.

We can also think about finer control of such constraints with help of
some sysfs files, but a kernel parameter is fine to begin with.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/admin-guide/kernel-parameters.txt |  2 ++
 drivers/base/boot_constraint.c                  | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7737ab5d04b2..35e8a298bfc1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -426,6 +426,8 @@
 			embedded devices based on command line input.
 			See Documentation/block/cmdline-partition.txt
 
+	boot_constraints_disable = Do not set any boot constraints for devices.
+
 	boot_delay=	Milliseconds to delay each printk during boot.
 			Values larger than 10 seconds (10000) are changed to
 			no delay (0).
diff --git a/drivers/base/boot_constraint.c b/drivers/base/boot_constraint.c
index 495344e6405b..ab766d60191a 100644
--- a/drivers/base/boot_constraint.c
+++ b/drivers/base/boot_constraint.c
@@ -45,6 +45,17 @@ static DEFINE_MUTEX(constraint_devices_mutex);
 static int constraint_supply_add(struct constraint *constraint, void *data);
 static void constraint_supply_remove(struct constraint *constraint);
 
+static bool constraints_disabled;
+
+static int __init constraints_disable(char *str)
+{
+	constraints_disabled = true;
+	pr_debug("disabled\n");
+
+	return 0;
+}
+early_param("boot_constraints_disable", constraints_disable);
+
 
 /* Boot constraints core */
 
@@ -152,6 +163,9 @@ int boot_constraint_add(struct device *dev, enum boot_constraint_type type,
 	struct constraint *constraint;
 	int ret;
 
+	if (constraints_disabled)
+		return -ENODEV;
+
 	mutex_lock(&constraint_devices_mutex);
 
 	/* Find or add the cdev type first */
@@ -202,6 +216,9 @@ void boot_constraints_remove(struct device *dev)
 	struct constraint_dev *cdev;
 	struct constraint *constraint, *temp;
 
+	if (constraints_disabled)
+		return;
+
 	mutex_lock(&constraint_devices_mutex);
 
 	cdev = constraint_device_find(dev);
-- 
2.13.0.71.gd7076ec9c9cb

[RFC 4/5] drivers: boot_constraint: Add debugfs support

[Viresh Kumar]

This patch adds debugfs support for boot constraints. This is how it
looks for a "vmmc-supply" constraint for the MMC device.

$ ls -R /sys/kernel/debug/boot_constraints/
/sys/kernel/debug/boot_constraints/:
f723d000.dwmmc0

/sys/kernel/debug/boot_constraints/f723d000.dwmmc0:
supply-vmmc

/sys/kernel/debug/boot_constraints/f723d000.dwmmc0/supply-vmmc:
enable  u_volt_max  u_volt_min

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/boot_constraint.c | 83 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/base/boot_constraint.c b/drivers/base/boot_constraint.c
index ab766d60191a..ff1c63b34458 100644
--- a/drivers/base/boot_constraint.c
+++ b/drivers/base/boot_constraint.c
@@ -10,6 +10,7 @@
 #define pr_fmt(fmt) "Boot Constraints: " fmt
 
 #include <linux/boot_constraint.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/export.h>
@@ -22,6 +23,7 @@ struct constraint {
 	struct constraint_dev *cdev;
 	struct list_head node;
 	enum boot_constraint_type type;
+	struct dentry *dentry;
 
 	int (*add)(struct constraint *constraint, void *data);
 	void (*remove)(struct constraint *constraint);
@@ -32,6 +34,7 @@ struct constraint_dev {
 	struct device *dev;
 	struct list_head node;
 	struct list_head constraints;
+	struct dentry *dentry;
 };
 
 #define for_each_constraint(_constraint, _temp, _cdev)		\
@@ -57,6 +60,71 @@ static int __init constraints_disable(char *str)
 early_param("boot_constraints_disable", constraints_disable);
 
 
+/* Debugfs */
+
+static struct dentry *rootdir;
+
+static void constraint_device_add_debugfs(struct constraint_dev *cdev)
+{
+	struct device *dev = cdev->dev;
+
+	cdev->dentry = debugfs_create_dir(dev_name(dev), rootdir);
+	if (!cdev->dentry)
+		dev_err(dev, "Failed to create constraint dev debugfs dir\n");
+}
+
+static void constraint_device_remove_debugfs(struct constraint_dev *cdev)
+{
+	debugfs_remove_recursive(cdev->dentry);
+}
+
+static void constraint_add_debugfs(struct constraint *constraint,
+				   const char *suffix)
+{
+	struct device *dev = constraint->cdev->dev;
+	const char *prefix;
+	char name[NAME_MAX];
+
+	switch (constraint->type) {
+	case BOOT_CONSTRAINT_SUPPLY:
+		prefix = "supply";
+		break;
+	default:
+		dev_err(dev, "%s: Constraint type (%d) not supported\n",
+			__func__, constraint->type);
+		return;
+	}
+
+	snprintf(name, NAME_MAX, "%s-%s", prefix, suffix);
+
+	constraint->dentry = debugfs_create_dir(name, constraint->cdev->dentry);
+	if (!constraint->dentry)
+		dev_err(dev, "Failed to create constraint (%s) debugfs dir\n",
+			name);
+}
+
+static void constraint_remove_debugfs(struct constraint *constraint)
+{
+	debugfs_remove_recursive(constraint->dentry);
+}
+
+static int __init constraint_debugfs_init(void)
+{
+	if (constraints_disabled)
+		return -ENODEV;
+
+	/* Create /sys/kernel/debug/opp directory */
+	rootdir = debugfs_create_dir("boot_constraints", NULL);
+	if (!rootdir) {
+		pr_err("Failed to create root directory\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+core_initcall(constraint_debugfs_init);
+
+
 /* Boot constraints core */
 
 static struct constraint_dev *constraint_device_find(struct device *dev)
@@ -84,12 +152,14 @@ static struct constraint_dev *constraint_device_allocate(struct device *dev)
 	INIT_LIST_HEAD(&cdev->constraints);
 
 	list_add(&cdev->node, &constraint_devices);
+	constraint_device_add_debugfs(cdev);
 
 	return cdev;
 }
 
 static void constraint_device_free(struct constraint_dev *cdev)
 {
+	constraint_device_remove_debugfs(cdev);
 	list_del(&cdev->node);
 	kfree(cdev);
 }
@@ -285,6 +355,18 @@ static int constraint_supply_add(struct constraint *constraint, void *data)
 	csupply->supply.name = kstrdup_const(supply->name, GFP_KERNEL);
 	constraint->private = csupply;
 
+	/* Debugfs */
+	constraint_add_debugfs(constraint, supply->name);
+
+	debugfs_create_ulong("u_volt_min", 0444, constraint->dentry,
+			     &csupply->supply.u_volt_min);
+
+	debugfs_create_ulong("u_volt_max", 0444, constraint->dentry,
+			     &csupply->supply.u_volt_max);
+
+	debugfs_create_bool("enable", 0444, constraint->dentry,
+			    &csupply->supply.enable);
+
 	return 0;
 
 remove_voltage:
@@ -314,6 +396,7 @@ static void constraint_supply_remove(struct constraint *constraint)
 		dev_err(dev, "regulator_set_voltage failed (%d)\n", ret);
 
 	regulator_put(csupply->reg);
+	constraint_remove_debugfs(constraint);
 	kfree_const(csupply->supply.name);
 	kfree(csupply);
 }
-- 
2.13.0.71.gd7076ec9c9cb

[RFC 5/5] drivers: Code to test boot constraints

[Viresh Kumar]

NOT FOR MERGE

This is test code to show how this is tested for now on the hikey
platform with the MMC device.

Not-signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/Makefile                    |  2 +-
 drivers/base/test_plat_boot_constraint.c | 51 ++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/test_plat_boot_constraint.c

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 6094b3b75184..a1ffa9f1a0b6 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -5,7 +5,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
 			   topology.o container.o property.o cacheinfo.o
-obj-$(CONFIG_BOOT_CONSTRAINTS) += boot_constraint.o
+obj-$(CONFIG_BOOT_CONSTRAINTS) += boot_constraint.o test_plat_boot_constraint.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/test_plat_boot_constraint.c b/drivers/base/test_plat_boot_constraint.c
new file mode 100644
index 000000000000..498f87056409
--- /dev/null
+++ b/drivers/base/test_plat_boot_constraint.c
@@ -0,0 +1,51 @@
+#include <linux/boot_constraint.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+static int test_constraints_probe(struct platform_device *platform_dev)
+{
+	struct device_node *np;
+	struct boot_constraint_supply_info info = {
+		.enable = true,
+		.name = "vmmc",
+		.u_volt_min = 1800000,
+		.u_volt_max = 3000000,
+	};
+	struct platform_device *pdev;
+	int ret;
+
+	np = of_find_compatible_node(NULL, NULL, "hisilicon,hi6220-dw-mshc");
+	if (!np)
+		return -ENODEV;
+
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+
+	if (!pdev) {
+		pr_err("%s: device not found\n", __func__);
+		return -ENODEV;
+	}
+
+	ret = boot_constraint_add(&pdev->dev, BOOT_CONSTRAINT_SUPPLY, &info);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static struct platform_driver test_constraints_driver = {
+	.driver = {
+		.name	= "test-constraints",
+	},
+	.probe	= test_constraints_probe,
+};
+
+static int __init test_constraints_init(void)
+{
+	platform_device_register_data(NULL, "test-constraints", -1, NULL, 0);
+
+	return platform_driver_register(&test_constraints_driver);
+}
+subsys_initcall(test_constraints_init);
-- 
2.13.0.71.gd7076ec9c9cb

Re: [RFC 4/5] drivers: boot_constraint: Add debugfs support

[Randy Dunlap]

On 06/28/2017 03:26 AM, Viresh Kumar wrote:
> This patch adds debugfs support for boot constraints. This is how it
> looks for a "vmmc-supply" constraint for the MMC device.
> 

Hi,
Does this build OK when DEBUG_FS is not enabled in kernel .config?

-- 
~Randy

Re: [RFC 3/5] drivers: boot_constraint: Add boot_constraints_disable kernel parameter

[Randy Dunlap]

On 06/28/2017 03:26 AM, Viresh Kumar wrote:
> Users must be given an option to discard any constraints set by
> bootloaders. For example, consider that a constraint is set for the LCD
> controller's supply and the LCD driver isn't loaded by the kernel. If
> the user doesn't need to use the LCD device, then he shouldn't be forced
> to honour the constraint.
> 
> We can also think about finer control of such constraints with help of
> some sysfs files, but a kernel parameter is fine to begin with.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  2 ++
>  drivers/base/boot_constraint.c                  | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 7737ab5d04b2..35e8a298bfc1 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -426,6 +426,8 @@
>  			embedded devices based on command line input.
>  			See Documentation/block/cmdline-partition.txt
>  
> +	boot_constraints_disable = Do not set any boot constraints for devices.
> +

No '=' sign.  That is only used when there is a following parameter value.


>  	boot_delay=	Milliseconds to delay each printk during boot.
>  			Values larger than 10 seconds (10000) are changed to
>  			no delay (0).


-- 
~Randy

Re: [RFC 1/5] drivers: Add boot constraints core

[Randy Dunlap]

On 06/28/2017 03:26 AM, Viresh Kumar wrote:
> Some devices are powered ON by the bootloaders before the bootloader
> handovers control to Linux. It maybe important for those devices to keep
> working until the time a Linux device driver probes the device and
> reconfigure its resources.
> 
> A typical example of that can be the LCD controller, which is used by
> the bootloaders to show image(s) while the device is booting into Linux.
> The LCD controller can be using some resources, like clk, regulators,
> etc, that are shared between several devices. These shared resources
> should be programmed so that all the users of them are satisfied. If
> some user (X) driver gets probed before the LCD controller driver in
> this case, then it may end up reconfiguring these resources to ranges
> satisfying the current users (only user X) and that can make the LCD
> screen unstable.
> 
> This patch introduces the concept of boot-constraints, which will be set
> by the bootloaders and the kernel will satisfy them until the time
> driver for such a device is probed (successfully or unsuccessfully).
> 
> The list of boot constraint types is empty for now, and will be added by
> a later patch.
> 
> Only two routines are exposed by the boot constraints core for now:
> 
> - boot_constraint_add(): This will be called by parts of the kernel
>   (before the device is probed) to set the constraints.
> 
> - boot_constraints_remove(): This is called only by the driver core
>   after a device is probed successfully or unsuccessfully. Special
>   handling is done here for deffered probing.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/Kconfig            |  11 +++
>  drivers/base/Makefile           |   1 +
>  drivers/base/boot_constraint.c  | 210 ++++++++++++++++++++++++++++++++++++++++
>  drivers/base/dd.c               |  20 ++--
>  include/linux/boot_constraint.h |  28 ++++++
>  5 files changed, 263 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/base/boot_constraint.c
>  create mode 100644 include/linux/boot_constraint.h
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index d718ae4b907a..d71217a91793 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -339,4 +339,15 @@ config CMA_ALIGNMENT
>  
>  endif
>  
> +config BOOT_CONSTRAINTS
> +	bool "Boot constraints for devices"
> +	default y

Why default y?

As Linus just wrote yesterday:

No. We've tried. The only sensible default (and that I try to enforce)
is "new featrures default to 'n'"

> +	help
> +	  This enables boot constraints detection for devices. These constraints
> +	  are (normally) set by the Bootloader and must be satisfied by the
> +	  kernel until the relevant device driver is probed. Once the driver is
> +	  probed, the constraint is dropped.
> +
> +	  If unsure, say Y.
> +
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index f2816f6ff76a..6094b3b75184 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -5,6 +5,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
>  			   cpu.o firmware.o init.o map.o devres.o \
>  			   attribute_container.o transport_class.o \
>  			   topology.o container.o property.o cacheinfo.o
> +obj-$(CONFIG_BOOT_CONSTRAINTS) += boot_constraint.o
>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>  obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
>  obj-y			+= power/


-- 
~Randy

Re: [RFC 1/5] drivers: Add boot constraints core

[Viresh Kumar]

On 28-06-17, 08:55, Randy Dunlap wrote:
> On 06/28/2017 03:26 AM, Viresh Kumar wrote:

> > +config BOOT_CONSTRAINTS
> > +	bool "Boot constraints for devices"
> > +	default y
> 
> Why default y?
> 
> As Linus just wrote yesterday:
> 
> No. We've tried. The only sensible default (and that I try to enforce)
> is "new featrures default to 'n'"

Yeah, this should have been n really.

-- 
viresh

Re: [RFC 4/5] drivers: boot_constraint: Add debugfs support

[Viresh Kumar]

On 28-06-17, 08:46, Randy Dunlap wrote:
> On 06/28/2017 03:26 AM, Viresh Kumar wrote:
> > This patch adds debugfs support for boot constraints. This is how it
> > looks for a "vmmc-supply" constraint for the MMC device.
> > 
> 
> Hi,
> Does this build OK when DEBUG_FS is not enabled in kernel .config?

TBH, I haven't tried this earlier as I believed dummy implementation
of all the debugfs helpers is available.

It needs the following debugfs patch (that I sent just now), which
adds the dummy implementation of debugfs_create_ulong().

https://marc.info/?l=linux-kernel&m=149870936319587

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Enrico Weigelt, metux IT consult]

On 28.06.2017 10:26, Viresh Kumar wrote:

Hi,

> Some devices are powered ON by the bootloaders before the bootloader
> handovers control to Linux. It maybe important for those devices to keep
> working until the time a Linux device driver probes the device and
> reconfigure its resources.

Just curious: aren't the devices (at least w/ DT) only initialized after
dependencies (eg. regulators) are already up ?

Let's imagine a LCD panel driven by a regulator behind SPI. The panel
driver would ask the regulator framework to switch on, which would
call the regulator driver. This one now would talk to SPI framework,
which finally calls the SPI driver. If SPI isn't up yet, it would all
be deferred, leaving the panel driver uninitialized (tried again later).

Am I wrong here ?

If the bootloader already switched on the panel (therefore already
enabled SPI), why does it matter that the panel driver isn't up yet ?

Is there anything that accidentially switches it off again (eg. by
resetting the regulator) ? If so, shouldn't the corresponding drivers
make sure that all depencies are met before doing anyhing w/ the
device, not even attempting a reset ?


--mtx

Re: [RFC 0/5] drivers: Add boot constraints core

[Russell King - ARM Linux]

On Wed, Jun 28, 2017 at 03:56:33PM +0530, Viresh Kumar wrote:
> A typical example of that can be the LCD controller, which is used by
> the bootloaders to show image(s) while the machine is booting into
> Linux. The LCD controller can be using some resources, like clk,
> regulators, etc, that are shared between several devices. These shared
> resources should be programmed so that all the users of them are
> satisfied. If a user (X) driver gets probed before the LCD controller
> driver in this case, then it may end up reconfiguring these resources to
> ranges satisfying the current users (only user X) and that can make the
> LCD screen unstable.

The thing that concerns me most about this is that typically the LCD
controller will be performing DMA to system RAM.

The location of the frame buffer is unknown to the decompressor - and
as the decompressor self-relocates itself (using purely assembly code),
it could relocate itself on top of the frame buffer, causing the "nice"
image to become very colourful.

The decompressor doesn't have the information from DT at that point to
know what are safe locations, so it's up to the boot loader to place
the frame buffer somewhere out of the way.  (If people want to write
a DT parser in position independent ARM assembly code that may change.)

As long as people realise this, then it's not a problem, but given the
number of problems that we've already encountered with boot loaders and
memory space layout, I don't trust them to get this right.

Right now, the ARM kernel booting document requires:

- Quiesce all DMA capable devices so that memory does not get
  corrupted by bogus network packets or disk data. This will save
  you many hours of debug.

so we would need to modify that to make an exception for LCD controllers.
However, we definitely can't have devices left enabled which are capable
of writing to system memory, or which changing system memory is likely
to cause bad effects (eg, packet ring buffers, USB buffers etc, which is
really what the above requirement is about.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC 1/5] drivers: Add boot constraints core

[Russell King - ARM Linux]

On Thu, Jun 29, 2017 at 09:21:57AM +0530, Viresh Kumar wrote:
> On 28-06-17, 08:55, Randy Dunlap wrote:
> > On 06/28/2017 03:26 AM, Viresh Kumar wrote:
> 
> > > +config BOOT_CONSTRAINTS
> > > +	bool "Boot constraints for devices"
> > > +	default y
> > 
> > Why default y?
> > 
> > As Linus just wrote yesterday:
> > 
> > No. We've tried. The only sensible default (and that I try to enforce)
> > is "new featrures default to 'n'"
> 
> Yeah, this should have been n really.

Given that the default default is to default to n, you don't need to
supply a default that just says what the default default actually is.
Please also avoid silly defaults.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC 0/5] drivers: Add boot constraints core

[Enrico Weigelt, metux IT consult]

On 29.06.2017 12:49, Russell King - ARM Linux wrote:

> The location of the frame buffer is unknown to the decompressor - and
> as the decompressor self-relocates itself (using purely assembly code),
> it could relocate itself on top of the frame buffer, causing the "nice"
> image to become very colourful.

Could the bootloader pass safe (or blocked) memory regions to the
decompressor ?

--mtx

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 29-06-17, 12:40, Enrico Weigelt, metux IT consult wrote:
> Just curious: aren't the devices (at least w/ DT) only initialized after
> dependencies (eg. regulators) are already up ?

No. Drivers are registered to the kernel (randomly, though we can know
their order) and devices are registered separately (platform/amba
devices get registered automatically with DT, hint:
drivers/of/platform.c). The device core checks while registering
devices/drivers if their drivers/devices are available or not. If
yes, then the devices are probed using the drivers. Now the drivers
must make sure all the dependencies are met at this point, else they
can return -EPROBE_DEFER and the kernel will try probing them again.

> Let's imagine a LCD panel driven by a regulator behind SPI. The panel
> driver would ask the regulator framework to switch on, which would
> call the regulator driver. This one now would talk to SPI framework,
> which finally calls the SPI driver. If SPI isn't up yet, it would all
> be deferred, leaving the panel driver uninitialized (tried again later).

This should happen in probe, otherwise we are screwed.

> If the bootloader already switched on the panel (therefore already
> enabled SPI), why does it matter that the panel driver isn't up yet ?

But the kernel doesn't know how it is configured, there can be so many
configurable parameters. The kernel needs to do it again by itself.

> Is there anything that accidentially switches it off again (eg. by
> resetting the regulator) ?

It is not just about switching it off, but the configuration here.

Let me try with an example. A regulator is shared between LCD and DMA
controller.

Operable ranges of the regulator: 1.8 - 3.0 V
Range required by LCD: 2.0 - 3.0 V
Range required by DMA: 1.8 - 2.5 V

Of course the right range for both of them to work here is 2.0 - 2.5
V.

Now the LCD is already enabled by the bootloader and kernel doesn't
know the ranges. DMA requests for the regulator and we set its voltage
to 1.8 V. LCD is screwed while we are still booting.

This will go worse if we add more devices to this example that share
the same regulator.

-- 
viresh

Re: [RFC 1/5] drivers: Add boot constraints core

[Viresh Kumar]

On 29-06-17, 13:50, Russell King - ARM Linux wrote:
> On Thu, Jun 29, 2017 at 09:21:57AM +0530, Viresh Kumar wrote:
> > On 28-06-17, 08:55, Randy Dunlap wrote:
> > > On 06/28/2017 03:26 AM, Viresh Kumar wrote:
> > 
> > > > +config BOOT_CONSTRAINTS
> > > > +	bool "Boot constraints for devices"
> > > > +	default y
> > > 
> > > Why default y?
> > > 
> > > As Linus just wrote yesterday:
> > > 
> > > No. We've tried. The only sensible default (and that I try to enforce)
> > > is "new featrures default to 'n'"
> > 
> > Yeah, this should have been n really.
> 
> Given that the default default is to default to n, you don't need to
> supply a default that just says what the default default actually is.
> Please also avoid silly defaults.

That was nice :)

Yeah, will get rid of the default statement here.

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 29-06-17, 13:49, Russell King - ARM Linux wrote:
> The thing that concerns me most about this is that typically the LCD
> controller will be performing DMA to system RAM.
> 
> The location of the frame buffer is unknown to the decompressor - and
> as the decompressor self-relocates itself (using purely assembly code),
> it could relocate itself on top of the frame buffer, causing the "nice"
> image to become very colourful.
> 
> The decompressor doesn't have the information from DT at that point to
> know what are safe locations, so it's up to the boot loader to place
> the frame buffer somewhere out of the way.  (If people want to write
> a DT parser in position independent ARM assembly code that may change.)
> 
> As long as people realise this, then it's not a problem, but given the
> number of problems that we've already encountered with boot loaders and
> memory space layout, I don't trust them to get this right.
> 
> Right now, the ARM kernel booting document requires:
> 
> - Quiesce all DMA capable devices so that memory does not get
>   corrupted by bogus network packets or disk data. This will save
>   you many hours of debug.
> 
> so we would need to modify that to make an exception for LCD controllers.
> However, we definitely can't have devices left enabled which are capable
> of writing to system memory, or which changing system memory is likely
> to cause bad effects (eg, packet ring buffers, USB buffers etc, which is
> really what the above requirement is about.)

Well, LCD was just an example here. But yeah, it is one of the most
probable case we have.

So, this thing is already working for sure, of course with some out of
tree hacks. Every smart phone shows their company's logo (some kind of
flash) while the phone boots. How do they get around such issues?

@Stephen: Any idea how Qcom does it ? :)

Must be fixing some area in RAM for this purpose, isn't it ?

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Enrico Weigelt, metux IT consult]

On 29.06.2017 14:47, Viresh Kumar wrote:

> No. Drivers are registered to the kernel (randomly, though we can know
> their order) and devices are registered separately (platform/amba
> devices get registered automatically with DT, hint:
> drivers/of/platform.c). The device core checks while registering
> devices/drivers if their drivers/devices are available or not. If
> yes, then the devices are probed using the drivers. Now the drivers
> must make sure all the dependencies are met at this point, else they
> can return -EPROBE_DEFER and the kernel will try probing them again.

Could we somehow introduce an strict ordering ?
Maybe by letting the device core know of the dependencies, before
individual probe()'s explicitly ask for them ?

>> Let's imagine a LCD panel driven by a regulator behind SPI. The panel
>> driver would ask the regulator framework to switch on, which would
>> call the regulator driver. This one now would talk to SPI framework,
>> which finally calls the SPI driver. If SPI isn't up yet, it would all
>> be deferred, leaving the panel driver uninitialized (tried again later).
>
> This should happen in probe, otherwise we are screwed.

Yes, but the probe result may be deferred, so it's tried again in the
next round. Correct ?

>> If the bootloader already switched on the panel (therefore already
>> enabled SPI), why does it matter that the panel driver isn't up yet ?
>
> But the kernel doesn't know how it is configured, there can be so many
> configurable parameters. The kernel needs to do it again by itself.

Could it read back the config ?

By the way: I've got a similar problem w/ gpmc right now: uboot already
sets it up, but the kernel only knows about one CS (for the nand) and
screwes up the others (eg. fpga), so it cant access the fpga . Until
I've sorted out all the parameters for DT (unfortunately, only have the
raw register values), I'll have to rely on an userland test program
to set it all up ...

> Let me try with an example. A regulator is shared between LCD and DMA
> controller.
>
> Operable ranges of the regulator: 1.8 - 3.0 V
> Range required by LCD: 2.0 - 3.0 V
> Range required by DMA: 1.8 - 2.5 V

Would a config readback help here ?

The regulator core then should know that we're already in proper
range for DMA and no need to touch the regulator.


--mtx

Re: [RFC 0/5] drivers: Add boot constraints core

[Russell King - ARM Linux]

On Thu, Jun 29, 2017 at 08:28:08PM +0530, Viresh Kumar wrote:
> On 29-06-17, 13:49, Russell King - ARM Linux wrote:
> > The thing that concerns me most about this is that typically the LCD
> > controller will be performing DMA to system RAM.
> > 
> > The location of the frame buffer is unknown to the decompressor - and
> > as the decompressor self-relocates itself (using purely assembly code),
> > it could relocate itself on top of the frame buffer, causing the "nice"
> > image to become very colourful.
> > 
> > The decompressor doesn't have the information from DT at that point to
> > know what are safe locations, so it's up to the boot loader to place
> > the frame buffer somewhere out of the way.  (If people want to write
> > a DT parser in position independent ARM assembly code that may change.)
> > 
> > As long as people realise this, then it's not a problem, but given the
> > number of problems that we've already encountered with boot loaders and
> > memory space layout, I don't trust them to get this right.
> > 
> > Right now, the ARM kernel booting document requires:
> > 
> > - Quiesce all DMA capable devices so that memory does not get
> >   corrupted by bogus network packets or disk data. This will save
> >   you many hours of debug.
> > 
> > so we would need to modify that to make an exception for LCD controllers.
> > However, we definitely can't have devices left enabled which are capable
> > of writing to system memory, or which changing system memory is likely
> > to cause bad effects (eg, packet ring buffers, USB buffers etc, which is
> > really what the above requirement is about.)
> 
> Well, LCD was just an example here. But yeah, it is one of the most
> probable case we have.
> 
> So, this thing is already working for sure, of course with some out of
> tree hacks. Every smart phone shows their company's logo (some kind of
> flash) while the phone boots. How do they get around such issues?

As far as the memory being used goes, they probably locate the frame
buffer well away from the kernel or any area that the kernel is likely
to use during decompression.

It's probably also marked as a reserved memory region in DT to avoid
the kernel touching it during boot, or _maybe_ they just locate it
somewhere in memory that they've tested that the kernel doesn't touch
until after their kernel has initialised the LCD controller (thereby
avoiding the memory being permanently consumed.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC 0/5] drivers: Add boot constraints core

[Stephen Boyd]

On 06/29, Russell King - ARM Linux wrote:
> On Thu, Jun 29, 2017 at 08:28:08PM +0530, Viresh Kumar wrote:
> > On 29-06-17, 13:49, Russell King - ARM Linux wrote:
> > > The thing that concerns me most about this is that typically the LCD
> > > controller will be performing DMA to system RAM.
> > > 
> > > The location of the frame buffer is unknown to the decompressor - and
> > > as the decompressor self-relocates itself (using purely assembly code),
> > > it could relocate itself on top of the frame buffer, causing the "nice"
> > > image to become very colourful.
> > > 
> > > The decompressor doesn't have the information from DT at that point to
> > > know what are safe locations, so it's up to the boot loader to place
> > > the frame buffer somewhere out of the way.  (If people want to write
> > > a DT parser in position independent ARM assembly code that may change.)
> > > 
> > > As long as people realise this, then it's not a problem, but given the
> > > number of problems that we've already encountered with boot loaders and
> > > memory space layout, I don't trust them to get this right.
> > > 
> > > Right now, the ARM kernel booting document requires:
> > > 
> > > - Quiesce all DMA capable devices so that memory does not get
> > >   corrupted by bogus network packets or disk data. This will save
> > >   you many hours of debug.
> > > 
> > > so we would need to modify that to make an exception for LCD controllers.
> > > However, we definitely can't have devices left enabled which are capable
> > > of writing to system memory, or which changing system memory is likely
> > > to cause bad effects (eg, packet ring buffers, USB buffers etc, which is
> > > really what the above requirement is about.)
> > 
> > Well, LCD was just an example here. But yeah, it is one of the most
> > probable case we have.
> > 
> > So, this thing is already working for sure, of course with some out of
> > tree hacks. Every smart phone shows their company's logo (some kind of
> > flash) while the phone boots. How do they get around such issues?
> 
> As far as the memory being used goes, they probably locate the frame
> buffer well away from the kernel or any area that the kernel is likely
> to use during decompression.
> 
> It's probably also marked as a reserved memory region in DT to avoid
> the kernel touching it during boot, or _maybe_ they just locate it
> somewhere in memory that they've tested that the kernel doesn't touch
> until after their kernel has initialised the LCD controller (thereby
> avoiding the memory being permanently consumed.)
> 

Yes. The display controller is typically pointed to a memory
carveout that we treat as reserved in the kernel. I'm fairly
certain that we avoid the "permanently consumed" problem by
making it a carveout for the display controller, so that when the
display controller probes it can take ownership of the memory
from the bootloader.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 29-06-17, 15:06, Enrico Weigelt, metux IT consult wrote:
> On 29.06.2017 14:47, Viresh Kumar wrote:
> 
> >No. Drivers are registered to the kernel (randomly, though we can know
> >their order) and devices are registered separately (platform/amba
> >devices get registered automatically with DT, hint:
> >drivers/of/platform.c). The device core checks while registering
> >devices/drivers if their drivers/devices are available or not. If
> >yes, then the devices are probed using the drivers. Now the drivers
> >must make sure all the dependencies are met at this point, else they
> >can return -EPROBE_DEFER and the kernel will try probing them again.
> 
> Could we somehow introduce an strict ordering ?

The problem I am trying to solve isn't really related to ordering.

Consider this for example:

A supply shared between LCD and I2C controller (Not sure if such
configurations are there in any of the hardware we have), where the
same I2C controller is used to access the LCD controller's registers.
Both are enabled at boot and the supply is configured to satisfy both.
If the voltage requirements of the I2C controller are below that of
LCD, then we can't decide on which one to probe first. We can't probe
LCD first as its bus isn't active yt and if we probe I2C first, then
it may take the supply down to a level that isn't acceptable for the
LCD (which was on from boot).

> Maybe by letting the device core know of the dependencies, before
> individual probe()'s explicitly ask for them ?

That's what we are sorting out in probe() and I am not sure if we need
any more intelligence on that. Though, you may want to look at the
"functional dependency" stuff, which can be of some help in such
cases. Its mentioned in cover-letter as well.

> >This should happen in probe, otherwise we are screwed.
> 
> Yes, but the probe result may be deferred, so it's tried again in the
> next round. Correct ?

Right.

> >But the kernel doesn't know how it is configured, there can be so many
> >configurable parameters. The kernel needs to do it again by itself.
> 
> Could it read back the config ?

First, it may not always be possible to do that. And even if the
kernel reads it all well, then it wouldn't know why things are
configured the way they are. And trying to read the config in drivers
is going to be so so hacky, that we wouldn't want to do it anyway. We
need a clean way of doing this, so that the kernel knows of what's
going on and that's what this series is targeting here.

> By the way: I've got a similar problem w/ gpmc right now: uboot already
> sets it up, but the kernel only knows about one CS (for the nand) and
> screwes up the others (eg. fpga), so it cant access the fpga . Until
> I've sorted out all the parameters for DT (unfortunately, only have the
> raw register values), I'll have to rely on an userland test program
> to set it all up ...
> 
> >Let me try with an example. A regulator is shared between LCD and DMA
> >controller.
> >
> >Operable ranges of the regulator: 1.8 - 3.0 V
> >Range required by LCD: 2.0 - 3.0 V
> >Range required by DMA: 1.8 - 2.5 V
> 
> Would a config readback help here ?
> 
> The regulator core then should know that we're already in proper
> range for DMA and no need to touch the regulator.

No body is going to allow that kind of hacky code to get merged :)

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Chen-Yu Tsai]

On Fri, Jun 30, 2017 at 11:16 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 29-06-17, 15:06, Enrico Weigelt, metux IT consult wrote:
>> On 29.06.2017 14:47, Viresh Kumar wrote:
>>
>> >No. Drivers are registered to the kernel (randomly, though we can know
>> >their order) and devices are registered separately (platform/amba
>> >devices get registered automatically with DT, hint:
>> >drivers/of/platform.c). The device core checks while registering
>> >devices/drivers if their drivers/devices are available or not. If
>> >yes, then the devices are probed using the drivers. Now the drivers
>> >must make sure all the dependencies are met at this point, else they
>> >can return -EPROBE_DEFER and the kernel will try probing them again.
>>
>> Could we somehow introduce an strict ordering ?
>
> The problem I am trying to solve isn't really related to ordering.
>
> Consider this for example:
>
> A supply shared between LCD and I2C controller (Not sure if such
> configurations are there in any of the hardware we have), where the
> same I2C controller is used to access the LCD controller's registers.
> Both are enabled at boot and the supply is configured to satisfy both.
> If the voltage requirements of the I2C controller are below that of
> LCD, then we can't decide on which one to probe first. We can't probe
> LCD first as its bus isn't active yt and if we probe I2C first, then
> it may take the supply down to a level that isn't acceptable for the
> LCD (which was on from boot).

AFAIK regulator constraints are supposed to satisfy all users of it.

>> Maybe by letting the device core know of the dependencies, before
>> individual probe()'s explicitly ask for them ?
>
> That's what we are sorting out in probe() and I am not sure if we need
> any more intelligence on that. Though, you may want to look at the
> "functional dependency" stuff, which can be of some help in such
> cases. Its mentioned in cover-letter as well.
>
>> >This should happen in probe, otherwise we are screwed.
>>
>> Yes, but the probe result may be deferred, so it's tried again in the
>> next round. Correct ?
>
> Right.
>
>> >But the kernel doesn't know how it is configured, there can be so many
>> >configurable parameters. The kernel needs to do it again by itself.
>>
>> Could it read back the config ?
>
> First, it may not always be possible to do that. And even if the
> kernel reads it all well, then it wouldn't know why things are
> configured the way they are. And trying to read the config in drivers
> is going to be so so hacky, that we wouldn't want to do it anyway. We
> need a clean way of doing this, so that the kernel knows of what's
> going on and that's what this series is targeting here.
>
>> By the way: I've got a similar problem w/ gpmc right now: uboot already
>> sets it up, but the kernel only knows about one CS (for the nand) and
>> screwes up the others (eg. fpga), so it cant access the fpga . Until
>> I've sorted out all the parameters for DT (unfortunately, only have the
>> raw register values), I'll have to rely on an userland test program
>> to set it all up ...
>>
>> >Let me try with an example. A regulator is shared between LCD and DMA
>> >controller.
>> >
>> >Operable ranges of the regulator: 1.8 - 3.0 V
>> >Range required by LCD: 2.0 - 3.0 V
>> >Range required by DMA: 1.8 - 2.5 V

So for the example here, the regulator constraint should be 2.5 - 3.0 V,
or the intersection of all voltage requirements.

ChenYu

>>
>> Would a config readback help here ?
>>
>> The regulator core then should know that we're already in proper
>> range for DMA and no need to touch the regulator.
>
> No body is going to allow that kind of hacky code to get merged :)
>
> --
> viresh
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 30-06-17, 11:33, Chen-Yu Tsai wrote:
> AFAIK regulator constraints are supposed to satisfy all users of it.

Right.

> >> >Let me try with an example. A regulator is shared between LCD and DMA
> >> >controller.
> >> >
> >> >Operable ranges of the regulator: 1.8 - 3.0 V
> >> >Range required by LCD: 2.0 - 3.0 V
> >> >Range required by DMA: 1.8 - 2.5 V
> 
> So for the example here, the regulator constraint should be 2.5 - 3.0 V,
> or the intersection of all voltage requirements.

Had a look at regulator_check_consumers() and the range selected by it
is the *highest* min_uV and *lowest* max_uV, to find that intersection
point.

For LCD: min_uV = 2.0 V, max_uV = 3.0 V
For DMA: min_uV = 1.8 V, max_uV = 2.5 V

Highest min_uV = 2.0 V
Lowest max_uV = 2.5 V

And so I mentioned the regulator's final range (that satisfies all
consumers) is 2 - 2.5 V.

Why do you say it should be 2.5 - 3.0 V ?

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Chen-Yu Tsai]

On Fri, Jun 30, 2017 at 11:55 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 30-06-17, 11:33, Chen-Yu Tsai wrote:
>> AFAIK regulator constraints are supposed to satisfy all users of it.
>
> Right.
>
>> >> >Let me try with an example. A regulator is shared between LCD and DMA
>> >> >controller.
>> >> >
>> >> >Operable ranges of the regulator: 1.8 - 3.0 V
>> >> >Range required by LCD: 2.0 - 3.0 V
>> >> >Range required by DMA: 1.8 - 2.5 V
>>
>> So for the example here, the regulator constraint should be 2.5 - 3.0 V,
>> or the intersection of all voltage requirements.
>
> Had a look at regulator_check_consumers() and the range selected by it
> is the *highest* min_uV and *lowest* max_uV, to find that intersection
> point.
>
> For LCD: min_uV = 2.0 V, max_uV = 3.0 V
> For DMA: min_uV = 1.8 V, max_uV = 2.5 V
>
> Highest min_uV = 2.0 V
> Lowest max_uV = 2.5 V
>
> And so I mentioned the regulator's final range (that satisfies all
> consumers) is 2 - 2.5 V.
>
> Why do you say it should be 2.5 - 3.0 V ?

You are right. It should be 2.0 - 2.5 V. Haven't had my coffee this
morning. :(

I also want to mention that for DT based platforms, this constraint
should already be set in the device tree for the regulator, so the
scenario where DMA comes up and sets a voltage level that LCD cannot
use should not even be possible.

ChenYu

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 30-06-17, 12:05, Chen-Yu Tsai wrote:
> On Fri, Jun 30, 2017 at 11:55 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 30-06-17, 11:33, Chen-Yu Tsai wrote:
> >> AFAIK regulator constraints are supposed to satisfy all users of it.
> >
> > Right.
> >
> >> >> >Let me try with an example. A regulator is shared between LCD and DMA
> >> >> >controller.
> >> >> >
> >> >> >Operable ranges of the regulator: 1.8 - 3.0 V
> >> >> >Range required by LCD: 2.0 - 3.0 V
> >> >> >Range required by DMA: 1.8 - 2.5 V
> >>
> >> So for the example here, the regulator constraint should be 2.5 - 3.0 V,
> >> or the intersection of all voltage requirements.
> >
> > Had a look at regulator_check_consumers() and the range selected by it
> > is the *highest* min_uV and *lowest* max_uV, to find that intersection
> > point.
> >
> > For LCD: min_uV = 2.0 V, max_uV = 3.0 V
> > For DMA: min_uV = 1.8 V, max_uV = 2.5 V
> >
> > Highest min_uV = 2.0 V
> > Lowest max_uV = 2.5 V
> >
> > And so I mentioned the regulator's final range (that satisfies all
> > consumers) is 2 - 2.5 V.
> >
> > Why do you say it should be 2.5 - 3.0 V ?
> 
> You are right. It should be 2.0 - 2.5 V. Haven't had my coffee this
> morning. :(

And I was worrying if I had something else in my coffee :)

> I also want to mention that for DT based platforms, this constraint
> should already be set in the device tree for the regulator, so the
> scenario where DMA comes up and sets a voltage level that LCD cannot
> use should not even be possible.

Yes, such constraints are already present. But the problem (this
series is trying to solve) is that the kernel doesn't know if the LCD
is already powered ON. And so when DMA gets probed first, the kernel
thinks that DMA is the only user of the regulator and the voltage is
set to 1.8-2.5 V. And so this series is somehow trying to make the
kernel aware about the constraints of the LCD controller which was
enabled in the bootloader.

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Chen-Yu Tsai]

On Fri, Jun 30, 2017 at 12:12 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 30-06-17, 12:05, Chen-Yu Tsai wrote:
>> On Fri, Jun 30, 2017 at 11:55 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 30-06-17, 11:33, Chen-Yu Tsai wrote:
>> >> AFAIK regulator constraints are supposed to satisfy all users of it.
>> >
>> > Right.
>> >
>> >> >> >Let me try with an example. A regulator is shared between LCD and DMA
>> >> >> >controller.
>> >> >> >
>> >> >> >Operable ranges of the regulator: 1.8 - 3.0 V
>> >> >> >Range required by LCD: 2.0 - 3.0 V
>> >> >> >Range required by DMA: 1.8 - 2.5 V
>> >>
>> >> So for the example here, the regulator constraint should be 2.5 - 3.0 V,
>> >> or the intersection of all voltage requirements.
>> >
>> > Had a look at regulator_check_consumers() and the range selected by it
>> > is the *highest* min_uV and *lowest* max_uV, to find that intersection
>> > point.
>> >
>> > For LCD: min_uV = 2.0 V, max_uV = 3.0 V
>> > For DMA: min_uV = 1.8 V, max_uV = 2.5 V
>> >
>> > Highest min_uV = 2.0 V
>> > Lowest max_uV = 2.5 V
>> >
>> > And so I mentioned the regulator's final range (that satisfies all
>> > consumers) is 2 - 2.5 V.
>> >
>> > Why do you say it should be 2.5 - 3.0 V ?
>>
>> You are right. It should be 2.0 - 2.5 V. Haven't had my coffee this
>> morning. :(
>
> And I was worrying if I had something else in my coffee :)
>
>> I also want to mention that for DT based platforms, this constraint
>> should already be set in the device tree for the regulator, so the
>> scenario where DMA comes up and sets a voltage level that LCD cannot
>> use should not even be possible.
>
> Yes, such constraints are already present. But the problem (this
> series is trying to solve) is that the kernel doesn't know if the LCD
> is already powered ON. And so when DMA gets probed first, the kernel
> thinks that DMA is the only user of the regulator and the voltage is
> set to 1.8-2.5 V. And so this series is somehow trying to make the
> kernel aware about the constraints of the LCD controller which was
> enabled in the bootloader.

What I'm saying is for the DT case, the constraints are already limited
to the intersection of all users, regardless of whether they are turned
on or not. At least this is what I believe makes sense. You really don't
want to set a regulator such that it over voltages for a subset of its
consumers. Consumers might not have proper power isolation for this.

I think what you mean is that the DT constraints are the union of all
consumer constraints (1.8 - 3.0 V in this case), then each consumer
comes in and adds its own constraints. And for such a design, the kernel
needs to know which and what constraints to apply.

Either way regulators already support constraints, so they are easier
to deal with. Clocks on the other hand, while the core does support
clock rate constraints, AFAIK no one really uses or supports them.

ChenYu

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 30-06-17, 12:22, Chen-Yu Tsai wrote:
> On Fri, Jun 30, 2017 at 12:12 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 30-06-17, 12:05, Chen-Yu Tsai wrote:

> >> I also want to mention that for DT based platforms, this constraint
> >> should already be set in the device tree for the regulator, so the
> >> scenario where DMA comes up and sets a voltage level that LCD cannot
> >> use should not even be possible.
> 
> What I'm saying is for the DT case, the constraints are already limited
> to the intersection of all users, regardless of whether they are turned
> on or not.

Right, but someone needs to get the regulator first to have that
considered by the regulator core while deciding the final range.

Both DMA and LCD driver do regulator_get() for their devices but if
only DMA driver is probed until now, then the regulator core wouldn't
consider LCD as regulator_get() is never called for LCD.

> I think what you mean is that the DT constraints are the union of all
> consumer constraints (1.8 - 3.0 V in this case), then each consumer
> comes in and adds its own constraints. And for such a design, the kernel
> needs to know which and what constraints to apply.

Sorry, I am confused with what you just said and not sure if I
understand it completely.

Each consumer DT node will have its own set of constraints for the
regulator device. The kernel will do regulator_get() for them one by
one, based on when their drivers get probed. And an intersection of
those constraints (which already did regulator_get()) will be used by
the regulator core.

Now this series is saying that even if the driver didn't come up (for
LCD) and haven't done its regulator_get() yet, consider that device's
constraint while calculating the target voltage for the regulator.

> Either way regulators already support constraints, so they are easier
> to deal with. Clocks on the other hand, while the core does support
> clock rate constraints, AFAIK no one really uses or supports them.

Yeah, so I started with just regulators and that's when Mark suggested
to do something generic which can be reused by other resource types.
We may end up covering clk for sure I believe. Not sure yet about
other resource types though.

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Chen-Yu Tsai]

 = On Fri, Jun 30, 2017 at 1:12 PM, Viresh Kumar
<viresh.kumar@linaro.org> wrote:
> On 30-06-17, 12:22, Chen-Yu Tsai wrote:
>> On Fri, Jun 30, 2017 at 12:12 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 30-06-17, 12:05, Chen-Yu Tsai wrote:
>
>> >> I also want to mention that for DT based platforms, this constraint
>> >> should already be set in the device tree for the regulator, so the
>> >> scenario where DMA comes up and sets a voltage level that LCD cannot
>> >> use should not even be possible.
>>
>> What I'm saying is for the DT case, the constraints are already limited
>> to the intersection of all users, regardless of whether they are turned
>> on or not.
>
> Right, but someone needs to get the regulator first to have that
> considered by the regulator core while deciding the final range.

AFAIK the regulator core automatically corrects any voltages outside
its constraints when the regulator is first registered. This is
independent of any consumer constraints.

> Both DMA and LCD driver do regulator_get() for their devices but if
> only DMA driver is probed until now, then the regulator core wouldn't
> consider LCD as regulator_get() is never called for LCD.
>
>> I think what you mean is that the DT constraints are the union of all
>> consumer constraints (1.8 - 3.0 V in this case), then each consumer
>> comes in and adds its own constraints. And for such a design, the kernel
>> needs to know which and what constraints to apply.
>
> Sorry, I am confused with what you just said and not sure if I
> understand it completely.
>
> Each consumer DT node will have its own set of constraints for the
> regulator device. The kernel will do regulator_get() for them one by
> one, based on when their drivers get probed. And an intersection of
> those constraints (which already did regulator_get()) will be used by
> the regulator core.

No. In the device tree, the only constraints (per the current state
of the bindings) is for the regulator supply. Any consumer constraints
are programmed purely by the driver, by using regulator_set_voltage().
All of them are considered by the core before setting the real voltage.

> Now this series is saying that even if the driver didn't come up (for
> LCD) and haven't done its regulator_get() yet, consider that device's
> constraint while calculating the target voltage for the regulator.

What I'm saying is that, for the constraints in the regulator supply node,
you would have already considered all consumer constraints. If one of its
consumers can't take power above 2.5 V, surely you don't want the regulator
sending power above that, so you would have

    regulator-max-microvolt = <2500000>;

for that regulator node. You would do something similar for the lower
limit of the voltage range.

You don't even need any actual consumers. Using regulator-min-microvolt,
regulator-max-microvolt, and regulator-always-on, you can have a regulator
provide power at a suitable voltage. We do this for various power rails
on Allwinner SoCs that don't really have proper consumers, like power
for internal logic, PLL, and others. I'm not saying this is a good solution,
because you lose runtime control of the regulator. It's just something
we came up with in lieu of any proper consumers.

>> Either way regulators already support constraints, so they are easier
>> to deal with. Clocks on the other hand, while the core does support
>> clock rate constraints, AFAIK no one really uses or supports them.
>
> Yeah, so I started with just regulators and that's when Mark suggested
> to do something generic which can be reused by other resource types.
> We may end up covering clk for sure I believe. Not sure yet about
> other resource types though.

This might be unrelated, but I think it is a similar problem. When a
clk rate change is propagated up the clk tree, any affected sibling
clks aren't automatically readjusted, i.e. try to keep roughly the
same output clk rate by adjusting its own dividers. This might be
one side of the problem you are trying to solve.

ChenYu

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 30-06-17, 14:36, Chen-Yu Tsai wrote:
>  = On Fri, Jun 30, 2017 at 1:12 PM, Viresh Kumar
> <viresh.kumar@linaro.org> wrote:

> > Right, but someone needs to get the regulator first to have that
> > considered by the regulator core while deciding the final range.
> 
> AFAIK the regulator core automatically corrects any voltages outside
> its constraints when the regulator is first registered. This is
> independent of any consumer constraints.

Right, so the kernel checks if the current voltage value set for the
supply is within valid range as per the constraints present in
regulator node.

> > Both DMA and LCD driver do regulator_get() for their devices but if
> > only DMA driver is probed until now, then the regulator core wouldn't
> > consider LCD as regulator_get() is never called for LCD.
> >
> >> I think what you mean is that the DT constraints are the union of all
> >> consumer constraints (1.8 - 3.0 V in this case), then each consumer
> >> comes in and adds its own constraints. And for such a design, the kernel
> >> needs to know which and what constraints to apply.
> >
> > Sorry, I am confused with what you just said and not sure if I
> > understand it completely.
> >
> > Each consumer DT node will have its own set of constraints for the
> > regulator device. The kernel will do regulator_get() for them one by
> > one, based on when their drivers get probed. And an intersection of
> > those constraints (which already did regulator_get()) will be used by
> > the regulator core.
> 
> No. In the device tree, the only constraints (per the current state
> of the bindings) is for the regulator supply. Any consumer constraints
> are programmed purely by the driver, by using regulator_set_voltage().
> All of them are considered by the core before setting the real voltage.

That's right. I had a bit of misunderstanding here. By default min/max
for the consumers is set to zero and they are ignored in
regulator_check_consumers().

> > Now this series is saying that even if the driver didn't come up (for
> > LCD) and haven't done its regulator_get() yet, consider that device's
> > constraint while calculating the target voltage for the regulator.
> 
> What I'm saying is that, for the constraints in the regulator supply node,
> you would have already considered all consumer constraints.

Yes, so whatever voltage the bootloader has programmed for LCD should
fit within constraints present in supply's node.

> If one of its
> consumers can't take power above 2.5 V, surely you don't want the regulator
> sending power above that, so you would have
> 
>     regulator-max-microvolt = <2500000>;
>
> for that regulator node. You would do something similar for the lower
> limit of the voltage range.
> 

I am no regulators expert, but AFAIU that's not true. What's wrong
with the following scenario:

Operable ranges of the regulator: 1.8 - 3.0 V
Range required by LCD: 2.0 - 3.0 V
Range required by DMA: 1.8 - 2.5 V

Here DMA can't work with regulator voltages > 2.5 V, but regulator can
go max to 3.0 V. Of course if the DMA driver has done
regulator_set_voltage(), then we will be within 2.5 V range. But that
doesn't force us to have regulator-max-microvolt set to 2.5 V.

And so the DT node shall have this:

        regulator-min-microvolt = <1800000>;
        regulator-max-microvolt = <3000000>;

Isn't it ?

> This might be unrelated, but I think it is a similar problem. When a
> clk rate change is propagated up the clk tree, any affected sibling
> clks aren't automatically readjusted, i.e. try to keep roughly the
> same output clk rate by adjusting its own dividers. This might be
> one side of the problem you are trying to solve.

I am not sure how this problem can be solved with what this set is
proposing. :(

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

On Fri, Jun 30, 2017 at 02:13:30PM +0530, Viresh Kumar wrote:
> On 30-06-17, 14:36, Chen-Yu Tsai wrote:

> Operable ranges of the regulator: 1.8 - 3.0 V
> Range required by LCD: 2.0 - 3.0 V
> Range required by DMA: 1.8 - 2.5 V

> Here DMA can't work with regulator voltages > 2.5 V, but regulator can
> go max to 3.0 V. Of course if the DMA driver has done
> regulator_set_voltage(), then we will be within 2.5 V range. But that
> doesn't force us to have regulator-max-microvolt set to 2.5 V.

> And so the DT node shall have this:

>         regulator-min-microvolt = <1800000>;
>         regulator-max-microvolt = <3000000>;

> Isn't it ?

If the DMA can't tolerate more than 2.5V then why would the constraints
allow the voltage to float that far?  Similarly on the low end?

Please remember that devices shouldn't be managing their voltages unless
they are actively changing them at runtime, simply setting them at
startup is the job of the constraints.  I would be very surprised to see
a DMA controller doing anything like DVFS.

> 
> > This might be unrelated, but I think it is a similar problem. When a
> > clk rate change is propagated up the clk tree, any affected sibling
> > clks aren't automatically readjusted, i.e. try to keep roughly the
> > same output clk rate by adjusting its own dividers. This might be
> > one side of the problem you are trying to solve.
> 
> I am not sure how this problem can be solved with what this set is
> proposing. :(
> 
> -- 
> viresh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/5] drivers: Add boot constraints core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On Fri, Jun 30, 2017 at 12:22:13PM +0800, Chen-Yu Tsai wrote:

> What I'm saying is for the DT case, the constraints are already limited
> to the intersection of all users, regardless of whether they are turned
> on or not. At least this is what I believe makes sense. You really don't
> want to set a regulator such that it over voltages for a subset of its
> consumers. Consumers might not have proper power isolation for this.

Right, and we also shouldn't have voltage setting code in consumers that
don't need to vary their voltage at runtime.  This keeps complexity out
of drivers and avoids the need to handle things like variants that have
different power requirements.

> I think what you mean is that the DT constraints are the union of all
> consumer constraints (1.8 - 3.0 V in this case), then each consumer
> comes in and adds its own constraints. And for such a design, the kernel
> needs to know which and what constraints to apply.

That's the broad idea.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 30-06-17, 13:10, Mark Brown wrote:
> On Fri, Jun 30, 2017 at 02:13:30PM +0530, Viresh Kumar wrote:
> > On 30-06-17, 14:36, Chen-Yu Tsai wrote:
> 
> > Operable ranges of the regulator: 1.8 - 3.0 V
> > Range required by LCD: 2.0 - 3.0 V
> > Range required by DMA: 1.8 - 2.5 V
> 
> > Here DMA can't work with regulator voltages > 2.5 V, but regulator can
> > go max to 3.0 V. Of course if the DMA driver has done
> > regulator_set_voltage(), then we will be within 2.5 V range. But that
> > doesn't force us to have regulator-max-microvolt set to 2.5 V.
> 
> > And so the DT node shall have this:
> 
> >         regulator-min-microvolt = <1800000>;
> >         regulator-max-microvolt = <3000000>;
> 
> > Isn't it ?
> 
> If the DMA can't tolerate more than 2.5V then why would the constraints
> allow the voltage to float that far?  Similarly on the low end?

The above regulator-min/max-microvolt values I mentioned were for the regulator
device and not what the consumers would request. Yes, DMA will request something
between 1.8 to 2.5 V, but in the above example LCD can request from 2.0 to 3.0 V
and so I had those limits for the regulator device (in DT).

> Please remember that devices shouldn't be managing their voltages unless
> they are actively changing them at runtime, simply setting them at
> startup is the job of the constraints.  I would be very surprised to see
> a DMA controller doing anything like DVFS.

Sure, DMA would most likely set a constraint from probe. Maybe I could have used
MMC in the above example, which may actually do DVFS at runtime.

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

On Mon, Jul 03, 2017 at 11:45:52AM +0530, Viresh Kumar wrote:
> On 30-06-17, 13:10, Mark Brown wrote:
> > On Fri, Jun 30, 2017 at 02:13:30PM +0530, Viresh Kumar wrote:
> > > On 30-06-17, 14:36, Chen-Yu Tsai wrote:

> > > And so the DT node shall have this:

> > >         regulator-min-microvolt = <1800000>;
> > >         regulator-max-microvolt = <3000000>;

> > > Isn't it ?

> > If the DMA can't tolerate more than 2.5V then why would the constraints
> > allow the voltage to float that far?  Similarly on the low end?

> The above regulator-min/max-microvolt values I mentioned were for the regulator
> device and not what the consumers would request. Yes, DMA will request something

If you're putting the maximum possible range that the physical regulator
can supply into machine constraints then you really haven't understood
what machine constraints are at all.

> > Please remember that devices shouldn't be managing their voltages unless
> > they are actively changing them at runtime, simply setting them at
> > startup is the job of the constraints.  I would be very surprised to see
> > a DMA controller doing anything like DVFS.

> Sure, DMA would most likely set a constraint from probe. Maybe I could have used

No, it really shouldn't.  Please read what I wrote.

> MMC in the above example, which may actually do DVFS at runtime.

Yes, MMC does vary voltage.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/5] drivers: Add boot constraints core

[Viresh Kumar]

On 03-07-17, 16:07, Mark Brown wrote:
> On Mon, Jul 03, 2017 at 11:45:52AM +0530, Viresh Kumar wrote:
> > The above regulator-min/max-microvolt values I mentioned were for the regulator
> > device and not what the consumers would request. Yes, DMA will request something
> 
> If you're putting the maximum possible range that the physical regulator
> can supply into machine constraints then you really haven't understood
> what machine constraints are at all.

I wasn't referring to the limits of the physical regulators but the min/max that
the consumers can set on a particular platform.

> No, it really shouldn't.  Please read what I wrote.

Sorry about that. Understood it now.

-- 
viresh

Re: [RFC 0/5] drivers: Add boot constraints core

[Rob Clark]

On Thu, Jun 29, 2017 at 5:00 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/29, Russell King - ARM Linux wrote:
>> On Thu, Jun 29, 2017 at 08:28:08PM +0530, Viresh Kumar wrote:
>> > On 29-06-17, 13:49, Russell King - ARM Linux wrote:
>> > > The thing that concerns me most about this is that typically the LCD
>> > > controller will be performing DMA to system RAM.
>> > >
>> > > The location of the frame buffer is unknown to the decompressor - and
>> > > as the decompressor self-relocates itself (using purely assembly code),
>> > > it could relocate itself on top of the frame buffer, causing the "nice"
>> > > image to become very colourful.
>> > >
>> > > The decompressor doesn't have the information from DT at that point to
>> > > know what are safe locations, so it's up to the boot loader to place
>> > > the frame buffer somewhere out of the way.  (If people want to write
>> > > a DT parser in position independent ARM assembly code that may change.)
>> > >
>> > > As long as people realise this, then it's not a problem, but given the
>> > > number of problems that we've already encountered with boot loaders and
>> > > memory space layout, I don't trust them to get this right.
>> > >
>> > > Right now, the ARM kernel booting document requires:
>> > >
>> > > - Quiesce all DMA capable devices so that memory does not get
>> > >   corrupted by bogus network packets or disk data. This will save
>> > >   you many hours of debug.
>> > >
>> > > so we would need to modify that to make an exception for LCD controllers.
>> > > However, we definitely can't have devices left enabled which are capable
>> > > of writing to system memory, or which changing system memory is likely
>> > > to cause bad effects (eg, packet ring buffers, USB buffers etc, which is
>> > > really what the above requirement is about.)
>> >
>> > Well, LCD was just an example here. But yeah, it is one of the most
>> > probable case we have.
>> >
>> > So, this thing is already working for sure, of course with some out of
>> > tree hacks. Every smart phone shows their company's logo (some kind of
>> > flash) while the phone boots. How do they get around such issues?
>>
>> As far as the memory being used goes, they probably locate the frame
>> buffer well away from the kernel or any area that the kernel is likely
>> to use during decompression.
>>
>> It's probably also marked as a reserved memory region in DT to avoid
>> the kernel touching it during boot, or _maybe_ they just locate it
>> somewhere in memory that they've tested that the kernel doesn't touch
>> until after their kernel has initialised the LCD controller (thereby
>> avoiding the memory being permanently consumed.)
>>
>
> Yes. The display controller is typically pointed to a memory
> carveout that we treat as reserved in the kernel. I'm fairly
> certain that we avoid the "permanently consumed" problem by
> making it a carveout for the display controller, so that when the
> display controller probes it can take ownership of the memory
> from the bootloader.
>

For aarch64/arm64 booting with EFI, the bootloader passes info about
memory layout to the kernel (including the fact that the framebuffer
is carved out) so kernel doesn't clobber the scanout buffer.  The
non-EFI case, the bootloader should (not that lk does) patch up the
fdt reserved memory node w/ correct address/size.  I think
lk+downstream just relies on luck.

(I have a patch that makes lk insert a framebuffer-simple node, which
I think should also solve it in the non-bootefi case)

BR,
-R

Re: [RFC 0/5] drivers: Add boot constraints core

[Stephen Boyd]

On 07/05, Rob Clark wrote:
> On Thu, Jun 29, 2017 at 5:00 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/29, Russell King - ARM Linux wrote:
> >>
> >> As far as the memory being used goes, they probably locate the frame
> >> buffer well away from the kernel or any area that the kernel is likely
> >> to use during decompression.
> >>
> >> It's probably also marked as a reserved memory region in DT to avoid
> >> the kernel touching it during boot, or _maybe_ they just locate it
> >> somewhere in memory that they've tested that the kernel doesn't touch
> >> until after their kernel has initialised the LCD controller (thereby
> >> avoiding the memory being permanently consumed.)
> >>
> >
> > Yes. The display controller is typically pointed to a memory
> > carveout that we treat as reserved in the kernel. I'm fairly
> > certain that we avoid the "permanently consumed" problem by
> > making it a carveout for the display controller, so that when the
> > display controller probes it can take ownership of the memory
> > from the bootloader.
> >
> 
> For aarch64/arm64 booting with EFI, the bootloader passes info about
> memory layout to the kernel (including the fact that the framebuffer
> is carved out) so kernel doesn't clobber the scanout buffer.  The
> non-EFI case, the bootloader should (not that lk does) patch up the
> fdt reserved memory node w/ correct address/size.  I think
> lk+downstream just relies on luck.
> 

Downstream+lk seems to be doing a carveout with reserved-memory
in DT[1]. The bootloader isn't updating the DT to indicate this,
instead we rely on an agreement between bootloader and kernel's
DT file to have the carveout. Once the display driver configures
things enough to allocate another display buffer it actually
frees the memory back to the system[2]. Most of the code for this
splash screen stuff is in that mdss_mdp_splash_logo.c file.

[1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8996.dtsi?h=rel/msm-4.4#n231
[2] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/video/fbdev/msm/mdss_mdp_splash_logo.c?h=rel/msm-4.4#n252

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project