Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676535 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-28 12:30 +0200
    [RFC 1/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-28 12:30 +0200
      Re: [RFC 1/5] drivers: Add boot constraints core Randy Dunlap <rdunlap@infradead.org> - 2017-06-28 18:00 +0200
        Re: [RFC 1/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-29 06:00 +0200
          Re: [RFC 1/5] drivers: Add boot constraints core Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-06-29 15:00 +0200
            Re: [RFC 1/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-29 17:00 +0200
    Re: [RFC 0/5] drivers: Add boot constraints core "Enrico Weigelt, metux IT consult" <enrico.weigelt@gr13.net> - 2017-06-29 14:50 +0200
      Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-29 16:50 +0200
        Re: [RFC 0/5] drivers: Add boot constraints core "Enrico Weigelt, metux IT consult" <enrico.weigelt@gr13.net> - 2017-06-29 17:10 +0200
          Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-30 05:20 +0200
            Re: [RFC 0/5] drivers: Add boot constraints core Chen-Yu Tsai <wens@csie.org> - 2017-06-30 05:40 +0200
              Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-30 06:00 +0200
                Re: [RFC 0/5] drivers: Add boot constraints core Chen-Yu Tsai <wens@csie.org> - 2017-06-30 06:10 +0200
                  Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-30 06:20 +0200
                    Re: [RFC 0/5] drivers: Add boot constraints core Chen-Yu Tsai <wens@csie.org> - 2017-06-30 06:30 +0200
                      Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-30 07:20 +0200
                        Re: [RFC 0/5] drivers: Add boot constraints core Chen-Yu Tsai <wens@csie.org> - 2017-06-30 08:40 +0200
                          Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-30 10:50 +0200
                            Re: [RFC 0/5] drivers: Add boot constraints core Mark Brown <broonie@kernel.org> - 2017-06-30 14:20 +0200
                              Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-07-03 08:20 +0200
                                Re: [RFC 0/5] drivers: Add boot constraints core Mark Brown <broonie@kernel.org> - 2017-07-03 17:10 +0200
                                  Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-07-04 08:50 +0200
                      Re: [RFC 0/5] drivers: Add boot constraints core Mark Brown <broonie@kernel.org> - 2017-06-30 14:20 +0200
    Re: [RFC 0/5] drivers: Add boot constraints core Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-06-29 14:50 +0200
      Re: [RFC 0/5] drivers: Add boot constraints core "Enrico Weigelt, metux IT consult" <enrico.weigelt@gr13.net> - 2017-06-29 15:10 +0200
      Re: [RFC 0/5] drivers: Add boot constraints core Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-29 17:00 +0200
        Re: [RFC 0/5] drivers: Add boot constraints core Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-06-29 17:50 +0200
          Re: [RFC 0/5] drivers: Add boot constraints core Stephen Boyd <sboyd@codeaurora.org> - 2017-06-29 23:10 +0200
            Re: [RFC 0/5] drivers: Add boot constraints core Rob Clark <robdclark@gmail.com> - 2017-07-06 00:10 +0200
              Re: [RFC 0/5] drivers: Add boot constraints core Stephen Boyd <sboyd@codeaurora.org> - 2017-07-08 00:40 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1677715

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

[toc] | [prev] | [next] | [standalone]

#1677832

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

[toc] | [prev] | [next] | [standalone]

#1677853

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

[toc] | [prev] | [next] | [standalone]

#1678427

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

[toc] | [prev] | [next] | [standalone]

#1678436

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

[toc] | [prev] | [next] | [standalone]

#1678444

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

[toc] | [prev] | [next] | [standalone]

#1678461

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

[toc] | [prev] | [next] | [standalone]

#1678465

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

[toc] | [prev] | [next] | [standalone]

#1678466

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

[toc] | [prev] | [next] | [standalone]

#1678482

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

[toc] | [prev] | [next] | [standalone]

#1678529

 = 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

[toc] | [prev] | [next] | [standalone]

#1678602

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

[toc] | [prev] | [next] | [standalone]

#1678759

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1679658

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web