| Message ID | 20240822092006.3134096-8-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
On Thu, Aug 22, 2024 at 05:20:00PM +0800, Chen-Yu Tsai wrote: > This adds regulator management to the I2C OF component prober. > Components that the prober intends to probe likely require their > regulator supplies be enabled, and GPIOs be toggled to enable them or > bring them out of reset before they will respond to probe attempts. > GPIOs will be handled in the next patch. > > Without specific knowledge of each component's resource names or > power sequencing requirements, the prober can only enable the > regulator supplies all at once, and toggle the GPIOs all at once. > Luckily, reset pins tend to be active low, while enable pins tend to > be active high, so setting the raw status of all GPIO pins to high > should work. The wait time before and after resources are enabled > are collected from existing drivers and device trees. > > The prober collects resources from all possible components and enables > them together, instead of enabling resources and probing each component > one by one. The latter approach does not provide any boot time benefits > over simply enabling each component and letting each driver probe > sequentially. > > The prober will also deduplicate the resources, since on a component > swap out or co-layout design, the resources are always the same. > While duplicate regulator supplies won't cause much issue, shared > GPIOs don't work reliably, especially with other drivers. For the > same reason, the prober will release the GPIOs before the successfully > probed component is actually enabled. ... > /* > * address responds. > * > * TODO: > - * - Support handling common regulators and GPIOs. > + * - Support handling common GPIOs. You can split this to two lines in the first place and have less churn in this patch and the other one. > * - Support I2C muxes > */ .. > +/* Returns number of regulator supplies found for node, or error. */ > +static int i2c_of_probe_get_regulator(struct device *dev, struct device_node *node, > + struct i2c_of_probe_data *data) > +{ > + struct regulator_bulk_data *tmp, *new_regulators; > + int ret; > + > + ret = of_regulator_bulk_get_all(dev, node, &tmp); > + if (ret <= 0) > + return ret; I would split this and explain 0 case. > + if (!data->regulators) { > + data->regulators = tmp; > + data->regulators_num = ret; > + return ret; > + }; > + > + new_regulators = krealloc(data->regulators, > + sizeof(*tmp) * (data->regulators_num + ret), krealloc_array() > + GFP_KERNEL); > + if (!new_regulators) { > + regulator_bulk_free(ret, tmp); > + return -ENOMEM; > + } > + > + data->regulators = new_regulators; > + for (unsigned int i = 0; i < ret; i++) > + memcpy(&data->regulators[data->regulators_num++], &tmp[i], sizeof(*tmp)); Seems like copying array to array, no? If so, can't be done in a single memcpy() call? > + return ret; > +} ... > +static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, > + struct i2c_of_probe_data *data) > +{ > + struct property *prop; > + int ret; > + > + ret = i2c_of_probe_get_regulator(dev, node, data); > + if (ret < 0) { > + dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node); > + goto err_cleanup; > + } > + > + return 0; > + > +err_cleanup: > + i2c_of_probe_free_res(data); > + return ret; > +} Hmm... why not static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, struct i2c_of_probe_data *data) { struct property *prop; int ret; ret = i2c_of_probe_get_regulator(dev, node, data); if (ret < 0) { i2c_of_probe_free_res(data); return dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node); } return 0; } ... > +static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data) > +{ > + int ret = 0; Redundant assignment. > + dev_dbg(dev, "Enabling regulator supplies\n"); > + > + ret = regulator_bulk_enable(data->regulators_num, data->regulators); > + if (ret) > + return ret; > + > + /* largest post-power-on pre-reset-deassert delay seen among drivers */ > + msleep(500); How would we monitor if any [new] driver wants to use bigger timeout? > + return 0; > +} ... > struct i2c_adapter *i2c; > + struct i2c_of_probe_data probe_data = {0}; Reversed xmas tree order? '0' is not needed. ... > + /* Grab resources */ > + for_each_child_of_node_scoped(i2c_node, node) { > + u32 addr; > + > + if (!of_node_name_prefix(node, type)) > + continue; Is it third or fourth copy of this code? At some point you probably want #define for_each_child_of_node_with_prefix_scoped() for_each_if(...) (or equivalent) > + if (of_property_read_u32(node, "reg", &addr)) > + continue; > + > + dev_dbg(dev, "Requesting resources for %pOF\n", node); > + ret = i2c_of_probe_get_res(dev, node, &probe_data); > + if (ret) > + return ret; > + }
On Thu, Aug 22, 2024 at 10:09 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Aug 22, 2024 at 05:20:00PM +0800, Chen-Yu Tsai wrote: > > This adds regulator management to the I2C OF component prober. > > Components that the prober intends to probe likely require their > > regulator supplies be enabled, and GPIOs be toggled to enable them or > > bring them out of reset before they will respond to probe attempts. > > GPIOs will be handled in the next patch. > > > > Without specific knowledge of each component's resource names or > > power sequencing requirements, the prober can only enable the > > regulator supplies all at once, and toggle the GPIOs all at once. > > Luckily, reset pins tend to be active low, while enable pins tend to > > be active high, so setting the raw status of all GPIO pins to high > > should work. The wait time before and after resources are enabled > > are collected from existing drivers and device trees. > > > > The prober collects resources from all possible components and enables > > them together, instead of enabling resources and probing each component > > one by one. The latter approach does not provide any boot time benefits > > over simply enabling each component and letting each driver probe > > sequentially. > > > > The prober will also deduplicate the resources, since on a component > > swap out or co-layout design, the resources are always the same. > > While duplicate regulator supplies won't cause much issue, shared > > GPIOs don't work reliably, especially with other drivers. For the > > same reason, the prober will release the GPIOs before the successfully > > probed component is actually enabled. > > ... > > > /* > > > * address responds. > > * > > * TODO: > > - * - Support handling common regulators and GPIOs. > > + * - Support handling common GPIOs. > > You can split this to two lines in the first place and have less churn in this > patch and the other one. Ack. > > * - Support I2C muxes > > */ > > .. > > > +/* Returns number of regulator supplies found for node, or error. */ > > +static int i2c_of_probe_get_regulator(struct device *dev, struct device_node *node, > > + struct i2c_of_probe_data *data) > > +{ > > + struct regulator_bulk_data *tmp, *new_regulators; > > + int ret; > > + > > + ret = of_regulator_bulk_get_all(dev, node, &tmp); > > + if (ret <= 0) > > + return ret; > > I would split this and explain 0 case. Ack. > > + if (!data->regulators) { > > + data->regulators = tmp; > > + data->regulators_num = ret; > > + return ret; > > + }; > > + > > + new_regulators = krealloc(data->regulators, > > + sizeof(*tmp) * (data->regulators_num + ret), > > krealloc_array() Ack. Somehow I didn't find this function while I was rewriting the code. > > + GFP_KERNEL); > > + if (!new_regulators) { > > + regulator_bulk_free(ret, tmp); > > + return -ENOMEM; > > + } > > + > > + data->regulators = new_regulators; > > > + for (unsigned int i = 0; i < ret; i++) > > + memcpy(&data->regulators[data->regulators_num++], &tmp[i], sizeof(*tmp)); > > Seems like copying array to array, no? If so, can't be done in a single memcpy() call? Ack. > > + return ret; > > +} > > ... > > > +static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, > > + struct i2c_of_probe_data *data) > > +{ > > + struct property *prop; > > + int ret; > > + > > + ret = i2c_of_probe_get_regulator(dev, node, data); > > + if (ret < 0) { > > + dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node); > > + goto err_cleanup; > > + } > > + > > + return 0; > > + > > +err_cleanup: > > + i2c_of_probe_free_res(data); > > + return ret; > > +} > > Hmm... why not > > static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, > struct i2c_of_probe_data *data) > { > struct property *prop; > int ret; > > ret = i2c_of_probe_get_regulator(dev, node, data); > if (ret < 0) { > i2c_of_probe_free_res(data); > return dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node); > } > > return 0; > } > > ... That would be more churn in the next patch, which introduces another error condition requiring the same cleanup. > > +static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data) > > +{ > > + int ret = 0; > > Redundant assignment. Ack. > > + dev_dbg(dev, "Enabling regulator supplies\n"); > > + > > + ret = regulator_bulk_enable(data->regulators_num, data->regulators); > > + if (ret) > > + return ret; > > + > > + /* largest post-power-on pre-reset-deassert delay seen among drivers */ > > + msleep(500); > > How would we monitor if any [new] driver wants to use bigger timeout? The assumption is that the person doing the integration should test for this. This prober doesn't get called everywhere. It needs a driver to call it, and that driver is written by someone for some specific platform. Maybe I should explicitly spell that out in the function description? Or even make it a parameter? Also, having an arbitrarily large number here doesn't help platforms that want to minimize boot time. On that front I'm also thinking about whether it is possible to do a handover to the actual driver so that the latter doesn't have to go through the whole power sequence again. > > + return 0; > > +} > > ... > > > struct i2c_adapter *i2c; > > + struct i2c_of_probe_data probe_data = {0}; > > Reversed xmas tree order? OK... > '0' is not needed. Ack. > ... > > > + /* Grab resources */ > > + for_each_child_of_node_scoped(i2c_node, node) { > > + u32 addr; > > + > > + if (!of_node_name_prefix(node, type)) > > + continue; > > Is it third or fourth copy of this code? At some point you probably want > > #define for_each_child_of_node_with_prefix_scoped() > for_each_if(...) > > (or equivalent) Ack. Thank you for the review. ChenYu > > + if (of_property_read_u32(node, "reg", &addr)) > > + continue; > > + > > + dev_dbg(dev, "Requesting resources for %pOF\n", node); > > + ret = i2c_of_probe_get_res(dev, node, &probe_data); > > + if (ret) > > + return ret; > > + } > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Aug 23, 2024 at 05:35:59PM +0800, Chen-Yu Tsai wrote: > On Thu, Aug 22, 2024 at 10:09 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Aug 22, 2024 at 05:20:00PM +0800, Chen-Yu Tsai wrote: ... > > Hmm... why not > > > > static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, > > struct i2c_of_probe_data *data) > > { > > struct property *prop; > > int ret; > > > > ret = i2c_of_probe_get_regulator(dev, node, data); > > if (ret < 0) { > > i2c_of_probe_free_res(data); > > return dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node); > > } > > > > return 0; > > } > > That would be more churn in the next patch, which introduces another > error condition requiring the same cleanup. OK! ... > > > + /* largest post-power-on pre-reset-deassert delay seen among drivers */ > > > + msleep(500); > > > > How would we monitor if any [new] driver wants to use bigger timeout? > > The assumption is that the person doing the integration should test for > this. This prober doesn't get called everywhere. It needs a driver to > call it, and that driver is written by someone for some specific platform. > Maybe I should explicitly spell that out in the function description? > Or even make it a parameter? > > Also, having an arbitrarily large number here doesn't help platforms that > want to minimize boot time. On that front I'm also thinking about whether > it is possible to do a handover to the actual driver so that the latter > doesn't have to go through the whole power sequence again. Yeah, I think the best effort is to have a parameter.
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c index bb7b231201b0..32184cfd10f6 100644 --- a/drivers/i2c/i2c-core-of-prober.c +++ b/drivers/i2c/i2c-core-of-prober.c @@ -6,12 +6,14 @@ */ #include <linux/cleanup.h> +#include <linux/delay.h> #include <linux/device.h> #include <linux/dev_printk.h> #include <linux/err.h> #include <linux/i2c.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> /* @@ -27,10 +29,93 @@ * address responds. * * TODO: - * - Support handling common regulators and GPIOs. + * - Support handling common GPIOs. * - Support I2C muxes */ +struct i2c_of_probe_data { + struct regulator_bulk_data *regulators; + unsigned int regulators_num; +}; + +/* Returns number of regulator supplies found for node, or error. */ +static int i2c_of_probe_get_regulator(struct device *dev, struct device_node *node, + struct i2c_of_probe_data *data) +{ + struct regulator_bulk_data *tmp, *new_regulators; + int ret; + + ret = of_regulator_bulk_get_all(dev, node, &tmp); + if (ret <= 0) + return ret; + + if (!data->regulators) { + data->regulators = tmp; + data->regulators_num = ret; + return ret; + }; + + new_regulators = krealloc(data->regulators, + sizeof(*tmp) * (data->regulators_num + ret), + GFP_KERNEL); + if (!new_regulators) { + regulator_bulk_free(ret, tmp); + return -ENOMEM; + } + + data->regulators = new_regulators; + + for (unsigned int i = 0; i < ret; i++) + memcpy(&data->regulators[data->regulators_num++], &tmp[i], sizeof(*tmp)); + + return ret; +} + +static void i2c_of_probe_free_res(struct i2c_of_probe_data *data) +{ + regulator_bulk_free(data->regulators_num, data->regulators); +} + +static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, + struct i2c_of_probe_data *data) +{ + struct property *prop; + int ret; + + ret = i2c_of_probe_get_regulator(dev, node, data); + if (ret < 0) { + dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node); + goto err_cleanup; + } + + return 0; + +err_cleanup: + i2c_of_probe_free_res(data); + return ret; +} + +static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data) +{ + int ret = 0; + + dev_dbg(dev, "Enabling regulator supplies\n"); + + ret = regulator_bulk_enable(data->regulators_num, data->regulators); + if (ret) + return ret; + + /* largest post-power-on pre-reset-deassert delay seen among drivers */ + msleep(500); + + return 0; +} + +static void i2c_of_probe_disable_regulators(struct i2c_of_probe_data *data) +{ + regulator_bulk_disable(data->regulators_num, data->regulators); +} + static struct device_node *i2c_of_probe_get_i2c_node(struct device *dev, const char *type) { struct device_node *node __free(device_node) = of_find_node_by_name(NULL, type); @@ -110,6 +195,7 @@ static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node int i2c_of_probe_component(struct device *dev, const char *type) { struct i2c_adapter *i2c; + struct i2c_of_probe_data probe_data = {0}; int ret; struct device_node *i2c_node __free(device_node) = i2c_of_probe_get_i2c_node(dev, type); @@ -133,6 +219,30 @@ int i2c_of_probe_component(struct device *dev, const char *type) if (!i2c) return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n"); + /* Grab resources */ + for_each_child_of_node_scoped(i2c_node, node) { + u32 addr; + + if (!of_node_name_prefix(node, type)) + continue; + if (of_property_read_u32(node, "reg", &addr)) + continue; + + dev_dbg(dev, "Requesting resources for %pOF\n", node); + ret = i2c_of_probe_get_res(dev, node, &probe_data); + if (ret) + return ret; + } + + dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num); + + /* Enable resources */ + ret = i2c_of_probe_enable_res(dev, &probe_data); + if (ret) { + i2c_of_probe_free_res(&probe_data); + return dev_err_probe(dev, ret, "Failed to enable resources\n"); + } + ret = 0; for_each_child_of_node_scoped(i2c_node, node) { union i2c_smbus_data data; @@ -150,6 +260,8 @@ int i2c_of_probe_component(struct device *dev, const char *type) break; } + i2c_of_probe_disable_regulators(&probe_data); + i2c_of_probe_free_res(&probe_data); i2c_put_adapter(i2c); return ret;
This adds regulator management to the I2C OF component prober. Components that the prober intends to probe likely require their regulator supplies be enabled, and GPIOs be toggled to enable them or bring them out of reset before they will respond to probe attempts. GPIOs will be handled in the next patch. Without specific knowledge of each component's resource names or power sequencing requirements, the prober can only enable the regulator supplies all at once, and toggle the GPIOs all at once. Luckily, reset pins tend to be active low, while enable pins tend to be active high, so setting the raw status of all GPIO pins to high should work. The wait time before and after resources are enabled are collected from existing drivers and device trees. The prober collects resources from all possible components and enables them together, instead of enabling resources and probing each component one by one. The latter approach does not provide any boot time benefits over simply enabling each component and letting each driver probe sequentially. The prober will also deduplicate the resources, since on a component swap out or co-layout design, the resources are always the same. While duplicate regulator supplies won't cause much issue, shared GPIOs don't work reliably, especially with other drivers. For the same reason, the prober will release the GPIOs before the successfully probed component is actually enabled. Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- Changes since v4: - Split out GPIO handling to separate patch - Rewrote using of_regulator_bulk_get_all() - Replaced "regulators" with "regulator supplies" in debug messages Changes since v3: - New patch This change is kept as a separate patch for now since the changes are quite numerous. --- drivers/i2c/i2c-core-of-prober.c | 114 ++++++++++++++++++++++++++++++- 1 file changed, 113 insertions(+), 1 deletion(-)