| Message ID | 1a844e27ee7e0b22acf8ea582bf4e8d35f54c84a.1501578037.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drivers: Boot Constraints core | expand |
On Tue, Aug 01, 2017 at 02:53:42PM +0530, Viresh Kumar wrote: > Some devices are powered ON by the bootloader 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 platform is booting into > Linux. The LCD controller can be using some resources, like clk, > regulators, PM domain, etc, that are shared between several devices. > These shared resources should be configured to satisfy need of all the > users. If another device's (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 device 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 > incrementally updated by later patches. > > Only two routines are exposed by the boot constraints core for now: > > - dev_boot_constraint_add(): This shall be called by parts of the kernel > (before the device is probed) to set the constraints. > > - dev_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. How is this information getting to the kernel from the bootloader? I didn't see where that happened, just a single example driver that somehow "knew" what had to happen, which seems odd... This is a lot of new code for no users, I would like to see at least 3 real drivers that are using it before we merge it, as then you have a chance of getting the user/kernel api correct. thanks, greg k-h
On 29-08-17, 08:39, Greg Kroah-Hartman wrote: > How is this information getting to the kernel from the bootloader? I > didn't see where that happened, just a single example driver that > somehow "knew" what had to happen, which seems odd... I tried to do it with DT earlier, but we couldn't reach to an agreement on what bindings to add and so this series started doing things the old way. The kernel would have platform specific drivers, which would exactly know what constraints to set and we wouldn't need the bootloaders to pass anything for now. > This is a lot of new code for no users, I agree and so added the patch 8/8 to show a real user. I will convert that to a patch going forward, which can be merged along with this series. > I would like to see at least 3 > real drivers that are using it before we merge it, as then you have a > chance of getting the user/kernel api correct. Hmm, so I am quite sure that this is a fairly generic problem to solve, specifically for all the handheld devices. I can get code for few of the Qcom SoCs but they may end up using the same platform driver. Not sure if I can get code for multiple SoC families in the beginning. -- viresh
On Tue, Aug 29, 2017 at 11:52:17AM +0200, Viresh Kumar wrote: > On 29-08-17, 08:39, Greg Kroah-Hartman wrote: > > How is this information getting to the kernel from the bootloader? I > > didn't see where that happened, just a single example driver that > > somehow "knew" what had to happen, which seems odd... > > I tried to do it with DT earlier, but we couldn't reach to an agreement on what > bindings to add and so this series started doing things the old way. The kernel > would have platform specific drivers, which would exactly know what constraints > to set and we wouldn't need the bootloaders to pass anything for now. Who couldn't reach an agreement? So you gave up and decided to make a whole bunch of kernel code instead of just using new DT entries? That's crazy... > > This is a lot of new code for no users, > > I agree and so added the patch 8/8 to show a real user. I will convert that to a > patch going forward, which can be merged along with this series. > > > I would like to see at least 3 > > real drivers that are using it before we merge it, as then you have a > > chance of getting the user/kernel api correct. > > Hmm, so I am quite sure that this is a fairly generic problem to solve, > specifically for all the handheld devices. I can get code for few of the Qcom > SoCs but they may end up using the same platform driver. Not sure if I can get > code for multiple SoC families in the beginning. Let's see a working system or two first here please. But you are implying that existing handheld devices need this problem solved, how do they do it today without this code as obviously they are shipping working solutions. thanks, greg k-h
On 29-08-17, 14:03, Greg Kroah-Hartman wrote: > Who couldn't reach an agreement? Rob Herring (DT Maintainer) didn't like the first set of bindings and wasn't convinced that we need any new bindings for this purpose to begin with. > So you gave up and decided to make a > whole bunch of kernel code instead of just using new DT entries? That's > crazy... Its not a lot really. Most of the code is anyways required, the only extra part is the platform specific drivers, which are replacing what the DT would have done. So, it shouldn't be that big of a deal I suppose. > Let's see a working system or two first here please. Sure, I will get as many converted as possible. > But you are implying that existing handheld devices need this problem > solved, how do they do it today without this code as obviously they are > shipping working solutions. So yeah, LCD is a common usecase but the configurations aren't always shared. It may not be an issue with private clock/regulator resources, but with shared ones. Though even with the private resources, we may want the clock/domains/regulators to stay powered on and I assume that the platforms would be doing hacky stuff to get that all working right now. -- viresh
On Mon, Sep 04, 2017 at 11:15:47AM +0200, Viresh Kumar wrote: > On 29-08-17, 14:03, Greg Kroah-Hartman wrote: > > Who couldn't reach an agreement? > > Rob Herring (DT Maintainer) didn't like the first set of bindings and wasn't > convinced that we need any new bindings for this purpose to begin with. So Rob wanted more kernel code and less bindings? Ok, I'll ask for Rob to sign off on these patches then :) > > But you are implying that existing handheld devices need this problem > > solved, how do they do it today without this code as obviously they are > > shipping working solutions. > > So yeah, LCD is a common usecase but the configurations aren't always shared. It > may not be an issue with private clock/regulator resources, but with shared > ones. > > Though even with the private resources, we may want the clock/domains/regulators > to stay powered on and I assume that the platforms would be doing hacky stuff to > get that all working right now. As you have the source for a number of such systems, you might want to verify this please. Also, what is going to cause any new systems to use this new api? thanks, greg k-h
On 04-09-17, 11:38, Greg Kroah-Hartman wrote: > On Mon, Sep 04, 2017 at 11:15:47AM +0200, Viresh Kumar wrote: > > On 29-08-17, 14:03, Greg Kroah-Hartman wrote: > > > Who couldn't reach an agreement? > > > > Rob Herring (DT Maintainer) didn't like the first set of bindings and wasn't > > convinced that we need any new bindings for this purpose to begin with. > > So Rob wanted more kernel code and less bindings? We had a similar thread sometime back (Performance states for power domains), where we couldn't get to the bindings in the first go. Rob suggested that we can actually get the code in first and then we can look at how to get stuff from DT once we understand it more. He wasn't happy with the bindings I proposed and so I took the same approach here. Its not a lot of code we are going to have to remove once we move to DT, so it ain't that bad. > Ok, I'll ask for Rob to sign off on these patches then :) I can't give any guarantee of that ;) > As you have the source for a number of such systems, you might want to > verify this please. I will try to talk to few guys during Linaro connect next week and also look into source for such stuff. > Also, what is going to cause any new systems to use > this new api? As far as my part go, I can do my best to advertise it with articles and presentations. But eventually people have to get motivated to use it themselves :) What else can I do as an author here? I will do whatever it takes to get people to use it. -- viresh
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index f046d21de57d..2333db2a33b7 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -347,4 +347,14 @@ config GENERIC_ARCH_TOPOLOGY appropriate scaling, sysfs interface for changing capacity values at runtime. +config DEV_BOOT_CONSTRAINTS + bool "Boot constraints for devices" + 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 N. + endmenu diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 397e5c344e6a..8c37ca07114b 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_DEV_BOOT_CONSTRAINTS) += boot_constraints/ obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-$(CONFIG_DMA_CMA) += dma-contiguous.o obj-y += power/ diff --git a/drivers/base/boot_constraints/Makefile b/drivers/base/boot_constraints/Makefile new file mode 100644 index 000000000000..0f2680177974 --- /dev/null +++ b/drivers/base/boot_constraints/Makefile @@ -0,0 +1,3 @@ +# Makefile for device boot constraints + +obj-y := core.o diff --git a/drivers/base/boot_constraints/core.c b/drivers/base/boot_constraints/core.c new file mode 100644 index 000000000000..366a05d6d9ba --- /dev/null +++ b/drivers/base/boot_constraints/core.c @@ -0,0 +1,199 @@ +/* + * This takes care of boot time device 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/err.h> +#include <linux/export.h> +#include <linux/mutex.h> +#include <linux/slab.h> + +#include "core.h" + +#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); + +/* 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 dev_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 dev_boot_constraint_add(struct device *dev, + struct dev_boot_constraint_info *info) +{ + 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, info->constraint.type); + if (IS_ERR(constraint)) { + dev_err(dev, "Failed to add constraint type: %d (%ld)\n", + info->constraint.type, PTR_ERR(constraint)); + ret = PTR_ERR(constraint); + goto put_cdev; + } + + constraint->free_resources = info->free_resources; + constraint->free_resources_data = info->free_resources_data; + + /* Set constraint */ + ret = constraint->add(constraint, info->constraint.data); + if (ret) + goto free_constraint; + + dev_dbg(dev, "Added boot constraint-type (%d)\n", + info->constraint.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(dev_boot_constraint_add); + +static void constraint_remove(struct constraint *constraint) +{ + constraint->remove(constraint); + + if (constraint->free_resources) + constraint->free_resources(constraint->free_resources_data); + + constraint_free(constraint); +} + +void dev_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/boot_constraints/core.h b/drivers/base/boot_constraints/core.h new file mode 100644 index 000000000000..7ba4ac172c09 --- /dev/null +++ b/drivers/base/boot_constraints/core.h @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2017 Linaro. + * Viresh Kumar <viresh.kumar@linaro.org> + * + * This file is released under the GPLv2. + */ +#ifndef _CORE_H +#define _CORE_H + +#include <linux/boot_constraint.h> +#include <linux/device.h> +#include <linux/list.h> + +struct constraint_dev { + struct device *dev; + struct list_head node; + struct list_head constraints; +}; + +struct constraint { + struct constraint_dev *cdev; + struct list_head node; + enum dev_boot_constraint_type type; + void (*free_resources)(void *data); + void *free_resources_data; + + int (*add)(struct constraint *constraint, void *data); + void (*remove)(struct constraint *constraint); + void *private; +}; + +/* Forward declarations of constraint specific callbacks */ +#endif /* _CORE_H */ diff --git a/drivers/base/dd.c b/drivers/base/dd.c index c17fefc77345..2262a4a4c0e4 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> @@ -383,15 +384,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) + dev_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..ae34fee547c5 --- /dev/null +++ b/include/linux/boot_constraint.h @@ -0,0 +1,46 @@ +/* + * Boot constraints header. + * + * Copyright (C) 2017 Linaro. + * Viresh Kumar <viresh.kumar@linaro.org> + * + * This file is released under the GPLv2 + */ +#ifndef _LINUX_BOOT_CONSTRAINT_H +#define _LINUX_BOOT_CONSTRAINT_H + +#include <linux/err.h> +#include <linux/types.h> + +struct device; + +enum dev_boot_constraint_type { + DEV_BOOT_CONSTRAINT_NONE, +}; + +struct dev_boot_constraint { + enum dev_boot_constraint_type type; + void *data; +}; + +struct dev_boot_constraint_info { + struct dev_boot_constraint constraint; + + /* This will be called just before the constraint is removed */ + void (*free_resources)(void *data); + void *free_resources_data; +}; + +#ifdef CONFIG_DEV_BOOT_CONSTRAINTS +int dev_boot_constraint_add(struct device *dev, + struct dev_boot_constraint_info *info); +void dev_boot_constraints_remove(struct device *dev); +#else +static inline +int dev_boot_constraint_add(struct device *dev, + struct dev_boot_constraint_info *info); +{ return -EINVAL; } +static inline void dev_boot_constraints_remove(struct device *dev) {} +#endif /* CONFIG_DEV_BOOT_CONSTRAINTS */ + +#endif /* _LINUX_BOOT_CONSTRAINT_H */