| Message ID | 71fab82672524b95632cdb588c16edfc9711866a.1521246069.git.collinsd@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi David,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/David-Collins/regulator-add-QCOM-RPMh-regulator-driver/20180319-011147
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> drivers/regulator/qcom_rpmh-regulator.c:18:10: fatal error: soc/qcom/cmd-db.h: No such file or directory
#include <soc/qcom/cmd-db.h>
^~~~~~~~~~~~~~~~~~~
compilation terminated.
coccinelle warnings: (new ones prefixed by >>)
>> drivers/regulator/qcom_rpmh-regulator.c:1104:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
vim +18 drivers/regulator/qcom_rpmh-regulator.c
5
6 #include <linux/bitops.h>
7 #include <linux/err.h>
8 #include <linux/kernel.h>
9 #include <linux/module.h>
10 #include <linux/of.h>
11 #include <linux/of_device.h>
12 #include <linux/platform_device.h>
13 #include <linux/slab.h>
14 #include <linux/string.h>
15 #include <linux/regulator/driver.h>
16 #include <linux/regulator/machine.h>
17 #include <linux/regulator/of_regulator.h>
> 18 #include <soc/qcom/cmd-db.h>
19 #include <soc/qcom/rpmh.h>
20 #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
21
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, On Fri, Mar 16, 2018 at 6:09 PM, David Collins <collinsd@codeaurora.org> wrote: > +/** > + * struct rpmh_regulator_mode - RPMh VRM mode attributes > + * @pmic_mode: Raw PMIC mode value written into VRM mode voting > + * register (i.e. RPMH_REGULATOR_MODE_*) > + * @framework_mode: Regulator framework mode value > + * (i.e. REGULATOR_MODE_*) > + * @min_load_ua: The minimum load current in microamps which > + * would utilize this mode Thinking of this as "min load" seems backward to me. Shouldn't we be keeping track of "max load". AKA: Use "idle" if the load is less than 1000 mA Use "normal" if the load is more than 1000 mA but less than 5000 mA Use "fast" if the load is more than 5000 mA. I'd think of "1000 mA as the max load that "idle" can handle". The reason I don't think of it as "min" is that you can't say "1000 mA is the min load the "normal" can handle". Normal can handle smaller loads just fine, it's just not ideal. Thinking of it in terms of "max" would also get rid of that weird "needs to be 0" entry in the device tree too. You could either leave the number off the top one (and set to INT_MAX in software?) or you could use that to put in some sort of sane maximum current. I'd bet there's something in the datasheet for this. If someone in software got mixed up and kept requesting more and more current, this could catch their error. > +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + > + return vreg->enabled; You can't read hardware? The regulator framework expects you to read hardware. If it was common not to be able to read hardware then the regulator framework would just keep track of the enabled state for you. Right now if a regulator was left on by the BIOS (presumably some have to be since otherwise you're running on a computer that takes no power), you'll still return false at bootup? Seems non-ideal. > +} > + > +static int rpmh_regulator_enable(struct regulator_dev *rdev) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + struct tcs_cmd cmd = { > + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, > + .data = RPMH_REGULATOR_ENABLE, > + }; > + int ret; > + > + if (vreg->enabled) > + return 0; Does the "if" test above ever hit? I'd think the regulator framework would handle this. > +static int rpmh_regulator_disable(struct regulator_dev *rdev) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + struct tcs_cmd cmd = { > + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, > + .data = RPMH_REGULATOR_DISABLE, > + }; > + int ret; > + > + if (!vreg->enabled) > + return 0; Does the "if" test above ever hit? I'd think the regulator framework would handle this. > +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev, > + int min_uv, int max_uv, unsigned int *selector) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + struct tcs_cmd cmd = { > + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE, > + }; > + const struct regulator_linear_range *range; > + int mv, uv, ret; > + bool wait_for_ack; > + > + mv = DIV_ROUND_UP(min_uv, 1000); > + uv = mv * 1000; > + if (uv > max_uv) { > + vreg_err(vreg, "no set points available in range %d-%d uV\n", > + min_uv, max_uv); > + return -EINVAL; > + } > + > + range = vreg->hw_data->voltage_range; > + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step); It seems like you should remove the existing check you have for "if (uv > max_uv)" and replace with a check here. Specifically, it seems like the DIV_ROUND_UP for the selector could also bump you over the max. AKA: ... bool wait_for_ack; range = vreg->hw_data->voltage_range; *selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step); uv = *selector * range->uV_step + range->min_uV if (uv > max_uv) { ... } Hold up, though. Why don't you implement set_voltage_sel() instead of set_voltage()? That's what literally everyone else does, well except PWM regulators. Using that will get rid of most of this code, won't it? Even the check to see if perhaps the voltage isn't changing. > + > + if (uv == vreg->voltage) > + return 0; > + > + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage; Do you often see "wait_for_ack = false" in reality? Most regulator usage I've seen requests a fairly tight range. AKA: I don't often see: set_voltage(min=3000mV, max=3300mV) set_voltage(min=1800mV, max=3300mV) Instead, I see: set_voltage(min=3000mV, max=3300mV) set_voltage(min=1800mV, max=1900mV) So you'll always have wait_for_ack = true in the cases I've seen. ...but are you certain it's useful to wait for an ack anyway when the voltage is falling? Most regulators won't guarantee that the voltage has actually fallen even after they ack you. Specifically if a regulator is under light load and it doesn't have an active discharge circuit then the regulator might fall very slowly over time. As a specific example, see commit 7c5209c315ea ("mmc: core: Increase delay for voltage to stabilize from 3.3V to 1.8V"). That was a lot of words, so to sum it all up: * If you have no actual examples where you see "wait_for_ack = false" then remove this code and always wait. * If you have evidence that the time spent waiting for the ack is slowing you down, consider always setting wait_for_ack to false when you're lowering the voltage. Anyone who truly cares could just set something like the device tree property regulator-settling-time-down-us. ...or, assuming Mark doesn't hate it, they could set the always-wait-for-ack property in the device tree. NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy ARC things for this?), we're probably talking about a small handful of voltage transitions per boot, right? > +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + > + return vreg->voltage; I guess there's no way to read the voltage at bootup? So this will return 0 until someone sets it? ...maybe less of a big deal due to the "qcom,regulator-initial-voltage" property? > +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev, > + unsigned int mode) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + struct tcs_cmd cmd = { > + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE, > + }; > + int i, ret; > + > + if (mode == vreg->mode) > + return 0; > + > + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) > + if (vreg->hw_data->mode_map[i].framework_mode == mode) > + break; > + if (i >= RPMH_REGULATOR_MODE_COUNT) { > + vreg_err(vreg, "invalid mode=%u\n", mode); > + return -EINVAL; > + } > + > + cmd.data = vreg->hw_data->mode_map[i].pmic_mode; > + > + ret = rpmh_regulator_send_request(vreg, &cmd, 1, > + mode < vreg->mode || !vreg->mode); Please explain the "mode < vreg->mode || !vreg->mode" test in words. > +static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + > + return vreg->mode; You'll probably guess that I'd expect you to read this from hardware. > +/** > + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations > + * for a VRM RPMh resource from device tree > + * vreg: Pointer to the rpmh regulator resource > + * > + * This function initializes the mode[] array of vreg based upon the values > + * of optional device tree properties. > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg) > +{ > + struct device_node *node = vreg->of_node; > + const struct rpmh_regulator_mode *map; > + const char *prop; > + int i, len, ret; > + u32 *buf; > + > + map = vreg->hw_data->mode_map; > + if (!map) > + return 0; > + > + /* qcom,allowed-modes is optional */ > + prop = "qcom,allowed-modes"; > + len = of_property_count_elems_of_size(node, prop, sizeof(u32)); > + if (len < 0) > + return 0; Seems like it might be worth it to count "qcom,mode-threshold-currents" too and confirm the count is the same? If someone left in an extra threshold current you won't notice otherwise (right?) > + > + vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len, > + sizeof(*vreg->mode_map), GFP_KERNEL); I keep getting myself confused because you have two things called "mode_map" and they work differently: vreg->mode_map - contains 1 element per allowed mode. Need to iterate through this to map "framework mode" to "pmic mode". Note: because of the need to iterate this isn't really a "map" in my mind. vreg->hw_data->mode_map - contains 1 element for each possible "device tree mode". Index into this using "device tree mode" and you get both a "framework mode" and "pmic mode". IMHO it would be better to have a table like "dt_to_linux_mode" that was just a simple list: static const int dt_to_linux_mode_bob[] = [ [RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_RET] = -EINVAL, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST ]; static const int dt_to_linux_mode_ldo_smps[] = [RPMH_REGULATOR_MODE_PASS] = -EINVAL, [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST ]; You'd only use that in the "map_mode" functions and when parsing qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could actually just call the map_mode function. This would be especially a good way to do this if you moved "allowed-modes" into the regulator core, which seems like a good idea. The nice thing about this is that only place you need to conceptually keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing code. Otherwise you just think of the Linux version. --- Once you do the above, then your other list could just be named "allowed_modes". This would make it obvious that this isn't a map itself but that you could iterate over it to accomplish a mapping. > +/** > + * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated > + * with the PMIC and initialize important pointers for each > + * regulator > + * @pmic: Pointer to the RPMh regulator PMIC > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic) > +{ > + struct device_node *node; > + int i; > + > + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node); > + if (pmic->vreg_count == 0) { > + dev_err(pmic->dev, "could not find any regulator subnodes\n"); > + return -ENODEV; > + } > + > + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count, > + sizeof(*pmic->vreg), GFP_KERNEL); > + if (!pmic->vreg) > + return -ENOMEM; > + > + i = 0; > + for_each_available_child_of_node(pmic->dev->of_node, node) { > + pmic->vreg[i].of_node = node; > + pmic->vreg[i].pmic = pmic; > + > + i++; > + } While I can believe that things don't crash, you're not quite using for_each_available_child_of_node() correctly. You need to reorganize your code structure to fix. Specifically the "node" that's provided to each iteration only has its refcount held for each iteration. By the end of your function the refcount of all the "of_node"s that you stored wil be 0. Doh. You could try to fix this by adding a of_node_get() before storing the node and that would work, but that would complicate your error handling. You'll need to do this in a "cleanup" error path of probe and in remove. :( A better solution is to get rid of the rpmh_regulator_allocate_vreg() function and move the functionality into probe. There, instead of iterating over pmic->vreg_count, just use the for_each_available_child_of_node() function. If rpmh_regulator_init_vreg() you'll have to manually of_node_put() but that should be OK. Why will that work? You'll call rpmh_regulator_init_vreg() while the reference count is still held. In that function you'll call devm_regulator_register(), which will grab the refcount if it needs it. Since that's a devm_ function you can be sure that it will properly drop the refcount at the right times. NOTE: with this you presumably should remove the "of_node" from the "struct rpmh_vreg". It seems like it shouldn't be needed anymore and it's good not to keep the pointer around if you didn't call of_node_get() yourself. > +/** > + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource > + * request for this regulator based on optional device tree > + * properties > + * @vreg: Pointer to the RPMh regulator > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg) > +{ > + struct tcs_cmd cmd[2] = { }; > + const char *prop; > + int cmd_count = 0; > + int ret; > + u32 temp; > + > + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) { > + prop = "qcom,headroom-voltage"; > + ret = of_property_read_u32(vreg->of_node, prop, &temp); > + if (!ret) { > + if (temp < RPMH_VRM_HEADROOM_MIN_UV || > + temp > RPMH_VRM_HEADROOM_MAX_UV) { > + vreg_err(vreg, "%s=%u is invalid\n", > + prop, temp); > + return -EINVAL; > + } > + vreg->headroom_voltage = temp; > + > + cmd[cmd_count].addr > + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM; > + cmd[cmd_count++].data > + = DIV_ROUND_UP(vreg->headroom_voltage, 1000); > + } > + > + prop = "qcom,regulator-initial-voltage"; > + ret = of_property_read_u32(vreg->of_node, prop, &temp); > + if (!ret) { > + if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) { > + vreg_err(vreg, "%s=%u is invalid\n", > + prop, temp); > + return -EINVAL; > + } > + vreg->voltage = temp; > + > + cmd[cmd_count].addr > + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE; > + cmd[cmd_count++].data > + = DIV_ROUND_UP(vreg->voltage, 1000); It seems like you should set vreg->voltage to the actual result of the division * 1000. AKA: if the user said the initial voltage was 123,456 then vreg->voltage should become 124,000. Actually, shouldn't you somehow resolve this with "struct rpmh_vreg_hw_data". It seems like some regulators have 128 steps, some have 256 steps, etc. You should have a function that reconciles the requested voltage with the one that hardware will actually provide. ...and, actually, you should share code for this reconciliation with rpmh_regulator_vrm_set_voltage(). > +/** > + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator > + * @vreg: Pointer to the RPMh regulator > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg) > +{ > + struct device *dev = vreg->pmic->dev; > + struct regulator_config reg_config = {}; > + const struct rpmh_vreg_init_data *rpmh_data = NULL; > + const char *type_name = NULL; > + enum rpmh_regulator_type type; > + struct regulator_init_data *init_data; > + int ret, i; > + > + for (i = 0; i < vreg->pmic->init_data->count; i++) { > + if (!strcmp(vreg->pmic->init_data->vreg_data[i].name, > + vreg->of_node->name)) { > + rpmh_data = &vreg->pmic->init_data->vreg_data[i]; > + break; > + } > + } > + > + if (!rpmh_data) { > + dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n", > + vreg->of_node->name, vreg->pmic->init_data->name); > + return -EINVAL; > + } > + > + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d", > + rpmh_data->resource_name_base, vreg->pmic->pmic_id, > + rpmh_data->id); > + if (!vreg->resource_name) > + return -ENOMEM; > + > + vreg->addr = cmd_db_read_addr(vreg->resource_name); > + if (!vreg->addr) { > + vreg_err(vreg, "could not find RPMh address for resource %s\n", > + vreg->resource_name); > + return -ENODEV; > + } > + > + vreg->rdesc.name = rpmh_data->name; > + vreg->rdesc.supply_name = rpmh_data->supply_name; > + vreg->regulator_type = rpmh_data->regulator_type; > + vreg->hw_data = rpmh_data->hw_data; > + > + if (rpmh_data->hw_data->voltage_range) { > + vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range; > + vreg->rdesc.n_linear_ranges = 1; > + vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages; > + } > + > + /* Optional override for the default RPMh accelerator type */ > + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type", > + &type_name); > + if (!ret) { > + if (!strcmp("vrm", type_name)) { > + vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM; > + } else if (!strcmp("xob", type_name)) { > + vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB; > + } else { > + vreg_err(vreg, "Unknown RPMh accelerator type %s\n", > + type_name); > + return -EINVAL; > + } As per comment in device tree patch, it seems really weird that you can override this. Are you sure? > +static const struct rpmh_regulator_mode > +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = { > + [RPMH_REGULATOR_MODE_PASS] = { > + .pmic_mode = 0, > + .framework_mode = REGULATOR_MODE_STANDBY, Is "PASS" truly the same concept as the Linux concept of STANDBY. If so, why do you need a separate define for it? If it truly is the same, it seems like you can simplify everything by just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and "RPMH_REGULATOR_MODE_PASS" and just call it "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that "standby" maps to "retention" for some regulators and maps to "pass" for other regulators if you want to map PMIC documentation. ...but getting rid of this distinction simply means less error checking and fewer tables in Linux. If "pass" really shouldn't map to "standby" then this seems like a hack and you should add the concept of "pass" to the core regulator framework. > +static const struct rpmh_vreg_hw_data pmic4_pldo_hw_data = { > + .voltage_range = &(const struct regulator_linear_range) > + REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000), This seems pretty iffy to me. You're relying on the compiler to make an anonymous chunk of memory with a "struct regulator_linear_range" in it and then storing a pointer to said anonymous chunk of memory? Nobody else using REGULATOR_LINEAR_RANGE() does this. Why not just get rid of the pointer and put the structure right inside "struct rpmh_vreg_hw_data"? It'll get rid of the weird cast / strange anonymous chunk, save an indirection, and save 4 bytes for a pointer. > +/** > + * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each > + * of the regulator nodes associated with it > + * @pdev: Pointer to the platform device of the RPMh PMIC > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + const struct of_device_id *match; > + struct rpmh_pmic *pmic; > + struct device_node *node; > + int ret, i; > + > + node = dev->of_node; > + > + if (!node) { > + dev_err(dev, "Device tree node is missing\n"); > + return -EINVAL; > + } > + > + ret = cmd_db_ready(); > + if (ret < 0) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Command DB not available, ret=%d\n", ret); > + return ret; > + } > + > + pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL); > + if (!pmic) > + return -ENOMEM; > + > + pmic->dev = dev; > + platform_set_drvdata(pdev, pmic); > + > + pmic->rpmh_client = rpmh_get_client(pdev); It seems like you'd do yourself (and the other clients of rpmh) a favor if you added a devm_rpmh_get_client() in a patch before this one. Adding a devm version of a calls is pretty easy and you'll be able to completely get rid of your "remove" function. ...and get rid of the "cleanup" exit here. > +static struct platform_driver rpmh_regulator_driver = { > + .driver = { > + .name = "qcom-rpmh-regulator", > + .of_match_table = rpmh_regulator_match_table, > + .owner = THIS_MODULE, As per the robot, no need to set .owner here. The core will do it. > + }, > + .probe = rpmh_regulator_probe, > + .remove = rpmh_regulator_remove, > +}; > + > +static int rpmh_regulator_init(void) > +{ > + return platform_driver_register(&rpmh_regulator_driver); > +} > + > +static void rpmh_regulator_exit(void) > +{ > + platform_driver_unregister(&rpmh_regulator_driver); > +} > + > +arch_initcall(rpmh_regulator_init); I always get yelled at when I try to use arch_initcall() for stuff like this. You should do what everyone else does and use module_platform_driver() to declare your driver. Yeah, regulators are important and (as I remember) they get probed slightly early anyway, but everything else in the system just gotta deal with the fact that they'll sometimes get deferred probes. > +module_exit(rpmh_regulator_exit); > + > +MODULE_DESCRIPTION("Qualcomm RPMh regulator driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h > new file mode 100644 > index 0000000..f854e0e > --- /dev/null > +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h Device tree guys will yell at you here. The "include/dt-bindings" bits are supposed to be together with the bindings. Different maintainers have different beliefs here, but I think the way that's least likely to get you yelled at by the most people is: Patch #1: bindings (.txt and include file) Patch #2: the driver ...with the idea being that if another operating system wanted just the bindings they could get it all in one patch. > @@ -0,0 +1,40 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ > + > +#ifndef __QCOM_RPMH_REGULATOR_H > +#define __QCOM_RPMH_REGULATOR_H > + > +/* > + * These mode constants may be used for qcom,allowed-modes and qcom,init-mode Not "qcom,init-mode". This is actually "regulator-initial-mode" now. > + * properties of an RPMh resource. Each type of regulator supports a subset of > + * the possible modes. > + * > + * %RPMH_REGULATOR_MODE_PASS: Pass-through mode in which output is directly > + * tied to input. This mode is only supported by > + * BOB type regulators. > + * %RPMH_REGULATOR_MODE_RET: Retention mode in which only an extremely small > + * load current is allowed. This mode is supported > + * by LDO and SMPS type regulators. > + * %RPMH_REGULATOR_MODE_LPM: Low power mode in which a small load current is > + * allowed. This mode corresponds to PFM for SMPS > + * and BOB type regulators. This mode is supported > + * by LDO, HFSMPS, BOB, and PMIC4 FTSMPS type > + * regulators. > + * %RPMH_REGULATOR_MODE_AUTO: Auto mode in which the regulator hardware > + * automatically switches between LPM and HPM based > + * upon the real-time load current. This mode is > + * supported by HFSMPS, BOB, and PMIC4 FTSMPS type > + * regulators. > + * %RPMH_REGULATOR_MODE_HPM: High power mode in which the full rated current > + * of the regulator is allowed. This mode > + * corresponds to PWM for SMPS and BOB type > + * regulators. This mode is supported by all types > + * of regulators. > + */ > +#define RPMH_REGULATOR_MODE_PASS 0 > +#define RPMH_REGULATOR_MODE_RET 1 > +#define RPMH_REGULATOR_MODE_LPM 2 > +#define RPMH_REGULATOR_MODE_AUTO 3 > +#define RPMH_REGULATOR_MODE_HPM 4 > + > +#endif OK, I think that's enough for one review session. Looking forward to seeing responses / the next version. Let me know if anything I said looks wrong or if you have questions! :) -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting David Collins (2018-03-16 18:09:10) > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index 097f617..e0ecd0a 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM > Qualcomm RPM as a module. The module will be named > "qcom_rpm-regulator". > > +config REGULATOR_QCOM_RPMH > + tristate "Qualcomm Technologies, Inc. RPMh regulator driver" > + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST What's the build dependency on OF? > + help > + This driver supports control of PMIC regulators via the RPMh hardware > + block found on Qualcomm Technologies Inc. SoCs. RPMh regulator > + control allows for voting on regulator state between multiple > + processors within the SoC. > + > config REGULATOR_QCOM_SMD_RPM > tristate "Qualcomm SMD based RPM regulator driver" > depends on QCOM_SMD_RPM > diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c > new file mode 100644 > index 0000000..808f949 > --- /dev/null > +++ b/drivers/regulator/qcom_rpmh-regulator.c > @@ -0,0 +1,1124 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ > + > +#define pr_fmt(fmt) "%s: " fmt, __func__ > + > +#include <linux/bitops.h> > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> Including machine is usually a red flag in regulator drivers. > +#include <linux/regulator/of_regulator.h> > +#include <soc/qcom/cmd-db.h> > +#include <soc/qcom/rpmh.h> > +#include <dt-bindings/regulator/qcom,rpmh-regulator.h> > + > +/** > + * enum rpmh_regulator_type - supported RPMh accelerator types > + * %RPMH_REGULATOR_TYPE_VRM: RPMh VRM accelerator which supports voting on > + * enable, voltage, mode, and headroom voltage of > + * LDO, SMPS, VS, and BOB type PMIC regulators. > + * %RPMH_REGULATOR_TYPE_XOB: RPMh XOB accelerator which supports voting on > + * the enable state of PMIC regulators. > + */ > +enum rpmh_regulator_type { > + RPMH_REGULATOR_TYPE_VRM, > + RPMH_REGULATOR_TYPE_XOB, > +}; > + > +/* Min and max limits of VRM resource request parameters */ > +#define RPMH_VRM_MIN_UV 0 > +#define RPMH_VRM_MAX_UV 8191000 > + > +#define RPMH_VRM_HEADROOM_MIN_UV 0 > +#define RPMH_VRM_HEADROOM_MAX_UV 511000 > + > +#define RPMH_VRM_MODE_MIN 0 > +#define RPMH_VRM_MODE_MAX 7 > + > +/* Register offsets: */ Why the colon? ^ Why even have the comment? > +#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0 > +#define RPMH_REGULATOR_REG_ENABLE 0x4 > +#define RPMH_REGULATOR_REG_VRM_MODE 0x8 > +#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC > + > +/* Enable register values: */ > +#define RPMH_REGULATOR_DISABLE 0x0 > +#define RPMH_REGULATOR_ENABLE 0x1 > + > +/* Number of unique hardware modes supported: */ Both above also look useless. > +#define RPMH_REGULATOR_MODE_COUNT 5 > + > +/** > + * struct rpmh_regulator_mode - RPMh VRM mode attributes > + * @pmic_mode: Raw PMIC mode value written into VRM mode voting > + * register (i.e. RPMH_REGULATOR_MODE_*) > + * @framework_mode: Regulator framework mode value > + * (i.e. REGULATOR_MODE_*) > + * @min_load_ua: The minimum load current in microamps which > + * would utilize this mode > + * > + * Software selects the lowest mode for which aggr_load_ua >= min_load_ua. > + */ > +struct rpmh_regulator_mode { > + u32 pmic_mode; > + u32 framework_mode; > + int min_load_ua; > +}; > + > +/** > + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations > + * @mode_map: Array of size RPMH_REGULATOR_MODE_COUNT which > + * maps RPMH_REGULATOR_MODE_* indices into PMIC > + * mode and regulator framework mode that are > + * supported by this PMIC regulator type > + * @voltage_range: The single range of voltages supported by this > + * PMIC regulator type > + * @n_voltages: The number of unique voltage set points defined > + * by voltage_range > + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined > + * in device tree to a regulator framework mode > + */ > +struct rpmh_vreg_hw_data { > + const struct rpmh_regulator_mode *mode_map; > + const struct regulator_linear_range *voltage_range; > + int n_voltages; > + unsigned int (*of_map_mode)(unsigned int mode); > +}; > + > +struct rpmh_pmic; > + > +/** > + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a > + * single regulator device > + * @of_node: Device tree node pointer of the regulator > + * @pmic: Pointer to the PMIC containing the regulator > + * @resource_name: Name of the RPMh regulator resource which is > + * mapped to an RPMh accelerator address via > + * command DB. This name must match to one that is > + * defined by the bootloader. > + * @addr: Base address of the regulator resource within > + * an RPMh accelerator > + * @rdesc: Regulator descriptor > + * @rdev: Regulator device pointer returned by > + * devm_regulator_register() Is this used? Why save it around? > + * @hw_data: PMIC regulator configuration data for this RPMh > + * regulator > + * @regulator_type: RPMh accelerator type for this regulator > + * resource > + * @always_wait_for_ack: Boolean flag indicating if a request must always > + * wait for an ACK from RPMh before continuing even > + * if it corresponds to a strictly lower power > + * state (e.g. enabled --> disabled). > + * @mode_map: An array of modes which may be configured at > + * runtime by setting the load current > + * @mode_count: The number of entries in the mode_map array. > + * @enabled: Boolean indicating if the regulator is enabled > + * or not > + * @voltage: RPMh VRM regulator voltage in microvolts So call it uV? > + * @mode: RPMh VRM regulator current framework mode > + * @headroom_voltage: RPMh VRM regulator minimum headroom voltage > + * required headroom_uV? > + */ > +struct rpmh_vreg { > + struct device_node *of_node; > + struct rpmh_pmic *pmic; > + const char *resource_name; > + u32 addr; > + struct regulator_desc rdesc; > + struct regulator_dev *rdev; > + const struct rpmh_vreg_hw_data *hw_data; > + enum rpmh_regulator_type regulator_type; > + bool always_wait_for_ack; > + struct rpmh_regulator_mode *mode_map; > + int mode_count; size_t? > + > + bool enabled; > + int voltage; > + unsigned int mode; > + int headroom_voltage; Please try to limit the things that are assigned into these structs and then never used outside of init. It adds complexity to the code when a local variable in the function would work just as well. > +}; > + > +/** > + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator > + * @name: Name for the regulator which also corresponds > + * to the device tree subnode name of the regulator > + * @resource_name_base: RPMh regulator resource name prefix. E.g. > + * "ldo" for RPMh resource "ldoa1". Maybe it should be "ldo%c1"? Then we could kasprintf the name with the pmic_id and drop the 'id' member entirely. > + * @supply_name: Parent supply regulator name > + * @id: Regulator number within the PMIC > + * @regulator_type: RPMh accelerator type used to manage this > + * regulator > + * @hw_data: Configuration data for this PMIC regulator type > + */ > +struct rpmh_vreg_init_data { > + const char *name; > + const char *resource_name_base; > + const char *supply_name; > + int id; > + enum rpmh_regulator_type regulator_type; > + const struct rpmh_vreg_hw_data *hw_data; > +}; > + > +/** > + * struct rpmh_pmic_init_data - initialization data for a PMIC > + * @name: PMIC name > + * @vreg_data: Array of data for each regulator in the PMIC > + * @count: Number of entries in vreg_data > + */ > +struct rpmh_pmic_init_data { > + const char *name; > + const struct rpmh_vreg_init_data *vreg_data; > + int count; > +}; > + > +/** > + * struct rpmh_pmic - top level data structure of all regulators found on a PMIC > + * @dev: Device pointer of the PMIC device for the > + * regulators > + * @rpmh_client: Handle used for rpmh communications > + * @vreg: Array of rpmh regulator structs representing the > + * individual regulators found on this PMIC chip > + * which are configured via device tree. > + * @vreg_count: The number of entries in the vreg array. > + * @pmic_id: Letter used to identify this PMIC within the > + * system. This is dictated by boot loader > + * specifications on a given target. > + * @init_data: Pointer to the matched PMIC initialization data > + */ > +struct rpmh_pmic { > + struct device *dev; > + struct rpmh_client *rpmh_client; > + struct rpmh_vreg *vreg; It's a circle! Life is a circle! It's a circle! > + int vreg_count; > + const char *pmic_id; > + const struct rpmh_pmic_init_data *init_data; Hopefully we don't really need this entire struct and we can just use local variables instead. > +}; > + > +#define vreg_err(vreg, message, ...) \ > + pr_err("%s %s: " message, (vreg)->pmic->init_data->name, \ > + (vreg)->rdesc.name, ##__VA_ARGS__) > +#define vreg_info(vreg, message, ...) \ > + pr_info("%s %s: " message, (vreg)->pmic->init_data->name, \ > + (vreg)->rdesc.name, ##__VA_ARGS__) > +#define vreg_debug(vreg, message, ...) \ > + pr_debug("%s %s: " message, (vreg)->pmic->init_data->name, \ > + (vreg)->rdesc.name, ##__VA_ARGS__) > + > +/** > + * rpmh_regulator_send_request() - send the request to RPMh > + * @vreg: Pointer to the RPMh regulator > + * @cmd: RPMh commands to send > + * @count: Size of cmd array > + * @wait_for_ack: Boolean indicating if execution must wait until the > + * request has been acknowledged as complete > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_send_request(struct rpmh_vreg *vreg, > + struct tcs_cmd *cmd, int count, bool wait_for_ack) > +{ > + int ret; > + > + if (wait_for_ack || vreg->always_wait_for_ack) > + ret = rpmh_write(vreg->pmic->rpmh_client, > + RPMH_ACTIVE_ONLY_STATE, cmd, count); > + else > + ret = rpmh_write_async(vreg->pmic->rpmh_client, > + RPMH_ACTIVE_ONLY_STATE, cmd, count); > + if (ret < 0) > + vreg_err(vreg, "rpmh_write() failed, ret=%d\n", ret); > + > + return ret; > +} > + [...] > +static int rpmh_regulator_enable(struct regulator_dev *rdev) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + struct tcs_cmd cmd = { > + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, > + .data = RPMH_REGULATOR_ENABLE, > + }; > + int ret; > + > + if (vreg->enabled) > + return 0; > + > + ret = rpmh_regulator_send_request(vreg, &cmd, 1, true); > + if (ret < 0) { > + vreg_err(vreg, "enable failed, ret=%d\n", ret); Do we really need all these error messages deep down in the regulator drivers? If consumers care, they'll print some error message themselves anyway. > + return ret; > + } > + > + vreg->enabled = true; > + > + return 0; > +} [...] > + > +/** > + * rpmh_regulator_vrm_set_load() - set the PMIC mode based upon the maximum load > + * required from the VRM rpmh-regulator > + * @rdev: Regulator device pointer for the rpmh-regulator > + * @load_ua: Maximum current required from all consumers in microamps > + * > + * This function is passed as a callback function into the regulator ops that > + * are registered for each VRM rpmh-regulator device. > + * > + * This function sets the mode of the regulator to that which has the highest > + * min support load less than or equal to load_ua. Example: s/support/supported/ > + * mode_count = 3 > + * mode_map[].min_load_ua = 0, 100000, 6000000 > + * > + * load_ua = 10000 --> i = 0 > + * load_ua = 250000 --> i = 1 > + * load_ua = 7000000 --> i = 2 > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_ua) > +{ > + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > + int i; > + > + /* No need to check element 0 as it will be the default. */ > + for (i = vreg->mode_count - 1; i > 0; i--) > + if (vreg->mode_map[i].min_load_ua <= load_ua) > + break; > + > + return rpmh_regulator_vrm_set_mode(rdev, > + vreg->mode_map[i].framework_mode); > +} > + > +static const struct regulator_ops rpmh_regulator_vrm_ops = { > + .enable = rpmh_regulator_enable, > + .disable = rpmh_regulator_disable, > + .is_enabled = rpmh_regulator_is_enabled, > + .set_voltage = rpmh_regulator_vrm_set_voltage, > + .get_voltage = rpmh_regulator_vrm_get_voltage, > + .list_voltage = regulator_list_voltage_linear_range, > + .set_mode = rpmh_regulator_vrm_set_mode, > + .get_mode = rpmh_regulator_vrm_get_mode, > + .set_load = rpmh_regulator_vrm_set_load, > +}; > + > +static const struct regulator_ops rpmh_regulator_xob_ops = { > + .enable = rpmh_regulator_enable, > + .disable = rpmh_regulator_disable, > + .is_enabled = rpmh_regulator_is_enabled, > +}; > + > +static const struct regulator_ops *rpmh_regulator_ops[] = { > + [RPMH_REGULATOR_TYPE_VRM] = &rpmh_regulator_vrm_ops, > + [RPMH_REGULATOR_TYPE_XOB] = &rpmh_regulator_xob_ops, > +}; > + > +/** > + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations > + * for a VRM RPMh resource from device tree > + * vreg: Pointer to the rpmh regulator resource > + * > + * This function initializes the mode[] array of vreg based upon the values > + * of optional device tree properties. > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg) > +{ I have a feeling this should all come from the driver data, not DT. Doubtful this really changes for each board. > + struct device_node *node = vreg->of_node; > + const struct rpmh_regulator_mode *map; > + const char *prop; > + int i, len, ret; > + u32 *buf; > + > + map = vreg->hw_data->mode_map; > + if (!map) > + return 0; > + > + /* qcom,allowed-modes is optional */ > + prop = "qcom,allowed-modes"; > + len = of_property_count_elems_of_size(node, prop, sizeof(u32)); > + if (len < 0) > + return 0; > + > + vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len, > + sizeof(*vreg->mode_map), GFP_KERNEL); > + if (!vreg->mode_map) > + return -ENOMEM; > + vreg->mode_count = len; > + > + buf = kcalloc(len, sizeof(*buf), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + ret = of_property_read_u32_array(node, prop, buf, len); > + if (ret < 0) { > + vreg_err(vreg, "unable to read %s, ret=%d\n", > + prop, ret); > + goto done; > + } > + > + for (i = 0; i < len; i++) { > + if (buf[i] >= RPMH_REGULATOR_MODE_COUNT > + || !map[buf[i]].framework_mode) { > + vreg_err(vreg, "element %d of %s = %u is invalid for this regulator\n", > + i, prop, buf[i]); > + ret = -EINVAL; > + goto done; > + } > + > + vreg->mode_map[i].pmic_mode = map[buf[i]].pmic_mode; > + vreg->mode_map[i].framework_mode = map[buf[i]].framework_mode; > + > + if (i > 0 && vreg->mode_map[i].pmic_mode > + <= vreg->mode_map[i - 1].pmic_mode) { > + vreg_err(vreg, "%s elements are not in ascending order\n", > + prop); > + ret = -EINVAL; > + goto done; > + } > + } > + > + prop = "qcom,mode-threshold-currents"; > + ret = of_property_read_u32_array(node, prop, buf, len); > + if (ret < 0) { > + vreg_err(vreg, "unable to read %s, ret=%d\n", > + prop, ret); > + goto done; > + } > + > + for (i = 0; i < len; i++) { > + vreg->mode_map[i].min_load_ua = buf[i]; > + > + if (i > 0 && vreg->mode_map[i].min_load_ua > + <= vreg->mode_map[i - 1].min_load_ua) { > + vreg_err(vreg, "%s elements are not in ascending order\n", > + prop); > + ret = -EINVAL; > + goto done; > + } > + } > + > +done: > + kfree(buf); > + return ret; > +} > + > +/** > + * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated > + * with the PMIC and initialize important pointers for each > + * regulator > + * @pmic: Pointer to the RPMh regulator PMIC > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic) > +{ > + struct device_node *node; > + int i; > + > + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node); > + if (pmic->vreg_count == 0) { > + dev_err(pmic->dev, "could not find any regulator subnodes\n"); > + return -ENODEV; > + } > + > + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count, > + sizeof(*pmic->vreg), GFP_KERNEL); > + if (!pmic->vreg) > + return -ENOMEM; Please just allocate one at a time. It's not like we have thousands of these things to worry about. > + > + i = 0; > + for_each_available_child_of_node(pmic->dev->of_node, node) { > + pmic->vreg[i].of_node = node; > + pmic->vreg[i].pmic = pmic; > + > + i++; > + } > + > + return 0; > +} > + > +/** > + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource > + * request for this regulator based on optional device tree > + * properties > + * @vreg: Pointer to the RPMh regulator > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg) > +{ > + struct tcs_cmd cmd[2] = { }; > + const char *prop; > + int cmd_count = 0; > + int ret; > + u32 temp; > + > + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) { > + prop = "qcom,headroom-voltage"; Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh registers. This probably needs to be pushed into the framework and come down through a 'set_headroom' op in the regulator ops via a regulator-headroom-microvolt property that's parsed in of_regulator.c. > + ret = of_property_read_u32(vreg->of_node, prop, &temp); > + if (!ret) { > + if (temp < RPMH_VRM_HEADROOM_MIN_UV || > + temp > RPMH_VRM_HEADROOM_MAX_UV) { > + vreg_err(vreg, "%s=%u is invalid\n", > + prop, temp); > + return -EINVAL; > + } > + vreg->headroom_voltage = temp; > + > + cmd[cmd_count].addr > + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM; > + cmd[cmd_count++].data > + = DIV_ROUND_UP(vreg->headroom_voltage, 1000); > + } > + > + prop = "qcom,regulator-initial-voltage"; DT constraints should take care of this by setting voltages on all regulators that need them? > + ret = of_property_read_u32(vreg->of_node, prop, &temp); > + if (!ret) { > + if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) { > + vreg_err(vreg, "%s=%u is invalid\n", > + prop, temp); > + return -EINVAL; > + } > + vreg->voltage = temp; > + > + cmd[cmd_count].addr > + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE; > + cmd[cmd_count++].data > + = DIV_ROUND_UP(vreg->voltage, 1000); > + } > + } > + > + if (cmd_count) { > + ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true); > + if (ret < 0) { > + vreg_err(vreg, "could not send default config, ret=%d\n", > + ret); > + return ret; > + } > + } > + > + return 0; > +} > + > +/** > + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator Heh, abbributes. > + * @vreg: Pointer to the RPMh regulator > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg) > +{ > + struct device *dev = vreg->pmic->dev; > + struct regulator_config reg_config = {}; > + const struct rpmh_vreg_init_data *rpmh_data = NULL; > + const char *type_name = NULL; > + enum rpmh_regulator_type type; > + struct regulator_init_data *init_data; > + int ret, i; > + > + for (i = 0; i < vreg->pmic->init_data->count; i++) { > + if (!strcmp(vreg->pmic->init_data->vreg_data[i].name, > + vreg->of_node->name)) { > + rpmh_data = &vreg->pmic->init_data->vreg_data[i]; > + break; > + } > + } > + > + if (!rpmh_data) { > + dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n", > + vreg->of_node->name, vreg->pmic->init_data->name); > + return -EINVAL; > + } > + > + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d", > + rpmh_data->resource_name_base, vreg->pmic->pmic_id, > + rpmh_data->id); > + if (!vreg->resource_name) > + return -ENOMEM; This isn't used outside of this function, so remove the vreg::resource_name member and use a local variable that gets freed on exit please. > + > + vreg->addr = cmd_db_read_addr(vreg->resource_name); > + if (!vreg->addr) { > + vreg_err(vreg, "could not find RPMh address for resource %s\n", > + vreg->resource_name); > + return -ENODEV; > + } > + > + vreg->rdesc.name = rpmh_data->name; > + vreg->rdesc.supply_name = rpmh_data->supply_name; > + vreg->regulator_type = rpmh_data->regulator_type; > + vreg->hw_data = rpmh_data->hw_data; > + > + if (rpmh_data->hw_data->voltage_range) { > + vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range; > + vreg->rdesc.n_linear_ranges = 1; > + vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages; > + } > + > + /* Optional override for the default RPMh accelerator type */ > + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type", Can this property have override in the name? And then because it is called override, perhaps it should come from the driver instead of DT because DT may need an override itself. Also, is this currently being used? If not I'd prefer we drop this until we need it. > + &type_name); > + if (!ret) { > + if (!strcmp("vrm", type_name)) { > + vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM; > + } else if (!strcmp("xob", type_name)) { > + vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB; > + } else { > + vreg_err(vreg, "Unknown RPMh accelerator type %s\n", > + type_name); > + return -EINVAL; > + } > + } > + > + type = vreg->regulator_type; > + > + if (type == RPMH_REGULATOR_TYPE_VRM) { > + ret = rpmh_regulator_parse_vrm_modes(vreg); > + if (ret < 0) { > + vreg_err(vreg, "could not parse vrm mode mapping, ret=%d\n", > + ret); > + return ret; > + } > + } > + > + vreg->always_wait_for_ack = of_property_read_bool(vreg->of_node, > + "qcom,always-wait-for-ack"); > + > + vreg->rdesc.owner = THIS_MODULE; > + vreg->rdesc.type = REGULATOR_VOLTAGE; > + vreg->rdesc.ops = rpmh_regulator_ops[type]; > + vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode; > + > + init_data = of_get_regulator_init_data(dev, vreg->of_node, > + &vreg->rdesc); > + if (!init_data) > + return -ENOMEM; > + > + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) { > + vreg->rdesc.fixed_uV = init_data->constraints.min_uV; > + init_data->constraints.apply_uV = 0; > + vreg->rdesc.n_voltages = 1; > + } What is this doing? Usually constraints aren't touched by the driver. > + > + if (vreg->hw_data->mode_map) { > + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE; Huh, I thought this was assigned by the framework. > + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) > + init_data->constraints.valid_modes_mask > + |= vreg->hw_data->mode_map[i].framework_mode; > + } > + > + reg_config.dev = dev; > + reg_config.init_data = init_data; > + reg_config.of_node = vreg->of_node; > + reg_config.driver_data = vreg; > + > + ret = rpmh_regulator_load_default_parameters(vreg); > + if (ret < 0) { > + vreg_err(vreg, "unable to load default parameters, ret=%d\n", > + ret); > + return ret; > + } > + > + vreg->rdev = devm_regulator_register(dev, &vreg->rdesc, ®_config); > + if (IS_ERR(vreg->rdev)) { > + ret = PTR_ERR(vreg->rdev); > + vreg->rdev = NULL; > + vreg_err(vreg, "devm_regulator_register() failed, ret=%d\n", > + ret); > + return ret; > + } > + > + vreg_debug(vreg, "registered RPMh resource %s @ 0x%05X\n", > + vreg->resource_name, vreg->addr); > + > + return ret; > +} > + [...] > + > +static const struct rpmh_pmic_init_data pm8005_pmic_data = { > + .name = "PM8005", > + .vreg_data = pm8005_vreg_data, > + .count = ARRAY_SIZE(pm8005_vreg_data), > +}; Kill 'name' please, and then get rid of 'count' and NUL terminate the array instead. This follows previous rpm regulator driver styles. Also, drop the macro that does stringification. We should end up with the match_table pointing to static arrays of structs that look like: { "s1", VRM, "smp", 1, &pmic4_ftsmps42, "vdd_s1", }, And yes, drop the RPMH_REGULATOR_TYPE_ prefix and _hw_data postfix. > + > +static const struct of_device_id rpmh_regulator_match_table[] = { > + { > + .compatible = "qcom,pm8998-rpmh-regulators", > + .data = &pm8998_pmic_data, > + }, > + { > + .compatible = "qcom,pmi8998-rpmh-regulators", > + .data = &pmi8998_pmic_data, > + }, > + { > + .compatible = "qcom,pm8005-rpmh-regulators", > + .data = &pm8005_pmic_data, > + }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table); Please move this array next to the driver structure. > + > +/** > + * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each > + * of the regulator nodes associated with it > + * @pdev: Pointer to the platform device of the RPMh PMIC > + * > + * Return: 0 on success, errno on failure > + */ > +static int rpmh_regulator_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + const struct of_device_id *match; > + struct rpmh_pmic *pmic; > + struct device_node *node; > + int ret, i; > + > + node = dev->of_node; > + > + if (!node) { > + dev_err(dev, "Device tree node is missing\n"); > + return -EINVAL; > + } This should never happen. Please remove. > + > + ret = cmd_db_ready(); > + if (ret < 0) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Command DB not available, ret=%d\n", ret); > + return ret; > + } We should just make rpmh parent device call cmd_db_ready() so that these devices aren't even populated until then and so that cmd_db_ready() is only in one place. Lina? > + > + pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL); > + if (!pmic) > + return -ENOMEM; > + > + pmic->dev = dev; > + platform_set_drvdata(pdev, pmic); > + > + pmic->rpmh_client = rpmh_get_client(pdev); > + if (IS_ERR(pmic->rpmh_client)) { > + ret = PTR_ERR(pmic->rpmh_client); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to request RPMh client, ret=%d\n", > + ret); I fail to see how this happens given that rpmh creates this device. > + return ret; > + } > + > + match = of_match_node(rpmh_regulator_match_table, node); > + if (match) { > + pmic->init_data = match->data; > + } else { > + dev_err(dev, "could not find compatible string match\n"); > + ret = -ENODEV; > + goto cleanup; > + } Use of_device_get_match_data() instead and print nothing on error. > + > + ret = of_property_read_string(node, "qcom,pmic-id", > + &pmic->pmic_id); > + if (ret < 0) { > + dev_err(dev, "qcom,pmic-id missing in DT node\n"); > + goto cleanup; > + } > + > + ret = rpmh_regulator_allocate_vreg(pmic); _allocate_vregs (plural)? > + if (ret < 0) { > + dev_err(dev, "failed to allocate regulator subnode array, ret=%d\n", Please no allocation error messages, kmalloc already prints a bunch of info. > + ret); > + goto cleanup; > + } > + > + for (i = 0; i < pmic->vreg_count; i++) { > + ret = rpmh_regulator_init_vreg(&pmic->vreg[i]); > + if (ret < 0) { > + dev_err(dev, "unable to initialize rpmh-regulator vreg %s, ret=%d\n", > + pmic->vreg[i].of_node->name, ret); > + goto cleanup; > + } > + } > + > + dev_dbg(dev, "successfully probed %d %s regulators\n", > + pmic->vreg_count, pmic->init_data->name); Doesn't the regulator framework already print a bunch of constraint stuff when regulators are registered? Seems sort of spammy even for debug mode. Plus it's the only reason for pmic::name right now. > + > + return ret; > + > +cleanup: > + rpmh_release(pmic->rpmh_client); > + > + return ret; > +} > + > +static int rpmh_regulator_remove(struct platform_device *pdev) > +{ > + struct rpmh_pmic *pmic = platform_get_drvdata(pdev); > + > + rpmh_release(pmic->rpmh_client); I'm still lost on what rpmh_client is giving us besides more code we don't need. I'll ping the rpmh thread again. > + > + return 0; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Doug, Thank you for the very detailed review feedback. On 03/20/2018 07:16 PM, Doug Anderson wrote: > On Fri, Mar 16, 2018 at 6:09 PM, David Collins <collinsd@codeaurora.org> wrote: >> +/** >> + * struct rpmh_regulator_mode - RPMh VRM mode attributes >> + * @pmic_mode: Raw PMIC mode value written into VRM mode voting >> + * register (i.e. RPMH_REGULATOR_MODE_*) >> + * @framework_mode: Regulator framework mode value >> + * (i.e. REGULATOR_MODE_*) >> + * @min_load_ua: The minimum load current in microamps which >> + * would utilize this mode > > Thinking of this as "min load" seems backward to me. Shouldn't we be > keeping track of "max load". AKA: > > Use "idle" if the load is less than 1000 mA > Use "normal" if the load is more than 1000 mA but less than 5000 mA > Use "fast" if the load is more than 5000 mA. > > I'd think of "1000 mA as the max load that "idle" can handle". The > reason I don't think of it as "min" is that you can't say "1000 mA is > the min load the "normal" can handle". Normal can handle smaller > loads just fine, it's just not ideal. > > Thinking of it in terms of "max" would also get rid of that weird > "needs to be 0" entry in the device tree too. You could either leave > the number off the top one (and set to INT_MAX in software?) or you > could use that to put in some sort of sane maximum current. I'd bet > there's something in the datasheet for this. If someone in software > got mixed up and kept requesting more and more current, this could > catch their error. Sure, I'll change the logic to use max instead of min threshold currents in the driver and device tree. >> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + >> + return vreg->enabled; > > You can't read hardware? The regulator framework expects you to read > hardware. If it was common not to be able to read hardware then the > regulator framework would just keep track of the enabled state for > you. Right now if a regulator was left on by the BIOS (presumably > some have to be since otherwise you're running on a computer that > takes no power), you'll still return false at bootup? Seems > non-ideal. Correct, the RPMh interface provides no mechanism to poll the physical PMIC regulator hardware state. However, that isn't entirely a bad thing due to the nature of RPMh regulator control. Several processors within the SoC besides the application processor are able to vote on the state of PMIC regulators via RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across votes from different processors for each votable quantity for a given regulator (enable, voltage, mode, and headroom voltage for VRM; enable for XOB). The aggregated values are then configured in the PMIC regulator via SPMI transactions. If Linux could read the physical PMIC regulator state and report it back via is_enabled(), get_voltage() and get_mode() regulator ops, then we could get into situations where the applications processor fails to vote on a regulator which is requested by a Linux consumer. For example, say that the modem processor has requested a given regulator to be enabled via RPMh. Later, a Linux consumer driver on the application processor calls regulator_enable() for the regulator. The regulator framework will call the is_enabled() callback which will return true. This would then short-circuit the regulator_enable() call and skip calling the rpmh-regulator enable() callback. This means that the application processor would have no enable vote present within RPMh for the regulator. If the modem processor then voted to disable the regulator, it would physically be disabled even though a consumer on the application processor expects it to stay enabled. >> +} >> + >> +static int rpmh_regulator_enable(struct regulator_dev *rdev) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + struct tcs_cmd cmd = { >> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, >> + .data = RPMH_REGULATOR_ENABLE, >> + }; >> + int ret; >> + >> + if (vreg->enabled) >> + return 0; > > Does the "if" test above ever hit? I'd think the regulator framework > would handle this. I'm not sure if it has ever been hit. I can remove it. >> +static int rpmh_regulator_disable(struct regulator_dev *rdev) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + struct tcs_cmd cmd = { >> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, >> + .data = RPMH_REGULATOR_DISABLE, >> + }; >> + int ret; >> + >> + if (!vreg->enabled) >> + return 0; > > Does the "if" test above ever hit? I'd think the regulator framework > would handle this. I'm not sure if it has ever been hit. I can remove it. >> +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev, >> + int min_uv, int max_uv, unsigned int *selector) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + struct tcs_cmd cmd = { >> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE, >> + }; >> + const struct regulator_linear_range *range; >> + int mv, uv, ret; >> + bool wait_for_ack; >> + >> + mv = DIV_ROUND_UP(min_uv, 1000); >> + uv = mv * 1000; >> + if (uv > max_uv) { >> + vreg_err(vreg, "no set points available in range %d-%d uV\n", >> + min_uv, max_uv); >> + return -EINVAL; >> + } >> + >> + range = vreg->hw_data->voltage_range; >> + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step); > > It seems like you should remove the existing check you have for "if > (uv > max_uv)" and replace with a check here. Specifically, it seems > like the DIV_ROUND_UP for the selector could also bump you over the > max. AKA: > > ... > bool wait_for_ack; > > range = vreg->hw_data->voltage_range; > *selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step); > uv = *selector * range->uV_step + range->min_uV > if (uv > max_uv) { > ... > } The VRM voltage vote register has units of mV (as does the voltage control register for the PMIC regulators). The PMIC hardware automatically rounds up the mV requests to the next physically available set point. This was designed to simplify the life for software so that there isn't a need to worry about voltage step size which varies between regulator types. The intention of the mv check was to verify that what is being programmed into VRM is within the requested min_uv-max_uv range. However, since this driver is forced to be aware of the step size, I can modify this as you suggested with the stricter step-size bounding. > Hold up, though. Why don't you implement set_voltage_sel() instead of > set_voltage()? That's what literally everyone else does, well except > PWM regulators. Using that will get rid of most of this code, won't > it? Even the check to see if perhaps the voltage isn't changing. There are two cases that I can think of: 1. Having a set_voltage() callback allows for delaying for an RPMh request ACK during certain voltage set point decreasing scenarios (to be elaborated below). 2. Having a get_voltage() as opposed to get_voltage_sel() callback allows an uninitialized voltage of 0 to be returned in the case that no initial voltage is specified in device tree. This eliminates the possibility of the regulator framework short-circuiting a regulator_set_voltage() call that happens to match the voltage corresponding to selector 0. >> + >> + if (uv == vreg->voltage) >> + return 0; >> + >> + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage; > > Do you often see "wait_for_ack = false" in reality? Most regulator > usage I've seen requests a fairly tight range. AKA: I don't often > see: > > set_voltage(min=3000mV, max=3300mV) > set_voltage(min=1800mV, max=3300mV) I can't think of a good example off the top of my head. However, this situation is certainly valid and can arise any time that there is a shared rail and the hardware connected to it has minimum voltage requirements per use case but large maximum voltage allowed. > Instead, I see: > > set_voltage(min=3000mV, max=3300mV) > set_voltage(min=1800mV, max=1900mV) > > So you'll always have wait_for_ack = true in the cases I've seen. > > ...but are you certain it's useful to wait for an ack anyway when the > voltage is falling? Most regulators won't guarantee that the voltage > has actually fallen even after they ack you. Specifically if a > regulator is under light load and it doesn't have an active discharge > circuit then the regulator might fall very slowly over time. As a > specific example, see commit 7c5209c315ea ("mmc: core: Increase delay > for voltage to stabilize from 3.3V to 1.8V"). I'm aware of one important instance in which decreasing voltage needs a delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case that I had in mind with the 'max_uv < vreg->voltage' check. However, you are correct that hardware will report completion of the voltage slewing early in this case since the comparator is checking for output >= set point. I can remove this special case check. > That was a lot of words, so to sum it all up: > > * If you have no actual examples where you see "wait_for_ack = false" > then remove this code and always wait. > > * If you have evidence that the time spent waiting for the ack is > slowing you down, consider always setting wait_for_ack to false when > you're lowering the voltage. Anyone who truly cares could just set > something like the device tree property > regulator-settling-time-down-us. ...or, assuming Mark doesn't hate > it, they could set the always-wait-for-ack property in the device > tree. I'm ok with changing the logic so that it waits for an ACK when increasing voltage (required for correctness) and does not wait for an ACK when decreasing voltage. > NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy > ARC things for this?), we're probably talking about a small handful of > voltage transitions per boot, right? VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh regulator requests made during different use cases. >> +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + >> + return vreg->voltage; > > I guess there's no way to read the voltage at bootup? So this will > return 0 until someone sets it? ...maybe less of a big deal due to > the "qcom,regulator-initial-voltage" property? Correct. As mentioned above, there is no way to read the physical PMIC regulator voltage from RPMh. Yes, this will return 0 unless qcom,regulator-initial-voltage is specified or the set_voltage() regulator op is called. Returning an invalid voltage of 0 uV here is convenient as it ensures that there is no way to accidentally skip setting the voltage due to returning the voltage of selector 0. >> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev, >> + unsigned int mode) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + struct tcs_cmd cmd = { >> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE, >> + }; >> + int i, ret; >> + >> + if (mode == vreg->mode) >> + return 0; >> + >> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) >> + if (vreg->hw_data->mode_map[i].framework_mode == mode) >> + break; >> + if (i >= RPMH_REGULATOR_MODE_COUNT) { >> + vreg_err(vreg, "invalid mode=%u\n", mode); >> + return -EINVAL; >> + } >> + >> + cmd.data = vreg->hw_data->mode_map[i].pmic_mode; >> + >> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, >> + mode < vreg->mode || !vreg->mode); > > Please explain the "mode < vreg->mode || !vreg->mode" test in words. This waits for an ACK from RPMh of successfully mode transition when switching to a higher power mode (regulator framework modes are ordered with highest power == lowest numerical value) or when setting the first mode. >> +static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + >> + return vreg->mode; > > You'll probably guess that I'd expect you to read this from hardware. As mentioned above, there is no way to read the physical PMIC regulator mode from RPMh. >> +/** >> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations >> + * for a VRM RPMh resource from device tree >> + * vreg: Pointer to the rpmh regulator resource >> + * >> + * This function initializes the mode[] array of vreg based upon the values >> + * of optional device tree properties. >> + * >> + * Return: 0 on success, errno on failure >> + */ >> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg) >> +{ >> + struct device_node *node = vreg->of_node; >> + const struct rpmh_regulator_mode *map; >> + const char *prop; >> + int i, len, ret; >> + u32 *buf; >> + >> + map = vreg->hw_data->mode_map; >> + if (!map) >> + return 0; >> + >> + /* qcom,allowed-modes is optional */ >> + prop = "qcom,allowed-modes"; >> + len = of_property_count_elems_of_size(node, prop, sizeof(u32)); >> + if (len < 0) >> + return 0; > > Seems like it might be worth it to count > "qcom,mode-threshold-currents" too and confirm the count is the same? > If someone left in an extra threshold current you won't notice > otherwise (right?) Sure, I'll add a check for that. >> + >> + vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len, >> + sizeof(*vreg->mode_map), GFP_KERNEL); > > I keep getting myself confused because you have two things called > "mode_map" and they work differently: > > vreg->mode_map - contains 1 element per allowed mode. Need to iterate > through this to map "framework mode" to "pmic mode". Note: because of > the need to iterate this isn't really a "map" in my mind. > > vreg->hw_data->mode_map - contains 1 element for each possible "device > tree mode". Index into this using "device tree mode" and you get both > a "framework mode" and "pmic mode". I agree that this is confusing. I reused struct rpmh_regulator_mode for both tables out of convenience. I'll change this. > IMHO it would be better to have a table like "dt_to_linux_mode" that > was just a simple list: > > static const int dt_to_linux_mode_bob[] = [ > [RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY, > [RPMH_REGULATOR_MODE_RET] = -EINVAL, > [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, > [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, > [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST > ]; > > static const int dt_to_linux_mode_ldo_smps[] = > [RPMH_REGULATOR_MODE_PASS] = -EINVAL, > [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, > [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, > [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, > [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST > ]; > > You'd only use that in the "map_mode" functions and when parsing > qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could > actually just call the map_mode function. This would be especially a > good way to do this if you moved "allowed-modes" into the regulator > core, which seems like a good idea. > > The nice thing about this is that only place you need to conceptually > keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing > code. Otherwise you just think of the Linux version. I would be ok with defining arrays like these to handle the mapping of PMIC regulator hardware agnostic "device tree mode" to "framework mode". However, a second set of arrays would then be needed to map from "framework mode" to hardware specific "pmic mode". I suppose that having the second array indexed by framework mode would make the code a little simpler since iteration would not be needed. Here is an explanation for why the "device tree mode" abstraction is present in the first place. Between different Qualcomm Technologies, Inc. PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, LPM/PFM, Retention, and pass-through). However, the register values for the same modes vary between different regulator types and different PMIC families. This patch is adding support for several PMIC4 family PMICs. The values needed for to-be-released PMIC5 PMIC regulators are different. As an example, here are the different values for LPM/PFM across PMIC families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" ensures that it is not possible to inadvertently specify a PMIC specific mode in device tree which corresponds to the wrong type or family but which aliases a value that would be accepted as correct. > Once you do the above, then your other list could just be named > "allowed_modes". This would make it obvious that this isn't a map > itself but that you could iterate over it to accomplish a mapping. I'm concerned with using the name "allowed_modes" as it tends to imply the wrong idea. Perhaps something along the lines of "drms_modes" would be better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes utilized by the set_load() callback along with corresponding threshold load currents. >> +/** >> + * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated >> + * with the PMIC and initialize important pointers for each >> + * regulator >> + * @pmic: Pointer to the RPMh regulator PMIC >> + * >> + * Return: 0 on success, errno on failure >> + */ >> +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic) >> +{ >> + struct device_node *node; >> + int i; >> + >> + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node); >> + if (pmic->vreg_count == 0) { >> + dev_err(pmic->dev, "could not find any regulator subnodes\n"); >> + return -ENODEV; >> + } >> + >> + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count, >> + sizeof(*pmic->vreg), GFP_KERNEL); >> + if (!pmic->vreg) >> + return -ENOMEM; >> + >> + i = 0; >> + for_each_available_child_of_node(pmic->dev->of_node, node) { >> + pmic->vreg[i].of_node = node; >> + pmic->vreg[i].pmic = pmic; >> + >> + i++; >> + } > > While I can believe that things don't crash, you're not quite using > for_each_available_child_of_node() correctly. You need to reorganize > your code structure to fix. Specifically the "node" that's provided > to each iteration only has its refcount held for each iteration. By > the end of your function the refcount of all the "of_node"s that you > stored wil be 0. Doh. > > You could try to fix this by adding a of_node_get() before storing the > node and that would work, but that would complicate your error > handling. You'll need to do this in a "cleanup" error path of probe > and in remove. :( > > A better solution is to get rid of the rpmh_regulator_allocate_vreg() > function and move the functionality into probe. There, instead of > iterating over pmic->vreg_count, just use the > for_each_available_child_of_node() function. If > rpmh_regulator_init_vreg() you'll have to manually of_node_put() but > that should be OK. > > Why will that work? You'll call rpmh_regulator_init_vreg() while the > reference count is still held. In that function you'll call > devm_regulator_register(), which will grab the refcount if it needs > it. Since that's a devm_ function you can be sure that it will > properly drop the refcount at the right times. > > NOTE: with this you presumably should remove the "of_node" from the > "struct rpmh_vreg". It seems like it shouldn't be needed anymore and > it's good not to keep the pointer around if you didn't call > of_node_get() yourself. I'll make this change. >> +/** >> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource >> + * request for this regulator based on optional device tree >> + * properties >> + * @vreg: Pointer to the RPMh regulator >> + * >> + * Return: 0 on success, errno on failure >> + */ >> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg) >> +{ >> + struct tcs_cmd cmd[2] = { }; >> + const char *prop; >> + int cmd_count = 0; >> + int ret; >> + u32 temp; >> + >> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) { >> + prop = "qcom,headroom-voltage"; >> + ret = of_property_read_u32(vreg->of_node, prop, &temp); >> + if (!ret) { >> + if (temp < RPMH_VRM_HEADROOM_MIN_UV || >> + temp > RPMH_VRM_HEADROOM_MAX_UV) { >> + vreg_err(vreg, "%s=%u is invalid\n", >> + prop, temp); >> + return -EINVAL; >> + } >> + vreg->headroom_voltage = temp; >> + >> + cmd[cmd_count].addr >> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM; >> + cmd[cmd_count++].data >> + = DIV_ROUND_UP(vreg->headroom_voltage, 1000); >> + } >> + >> + prop = "qcom,regulator-initial-voltage"; >> + ret = of_property_read_u32(vreg->of_node, prop, &temp); >> + if (!ret) { >> + if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) { >> + vreg_err(vreg, "%s=%u is invalid\n", >> + prop, temp); >> + return -EINVAL; >> + } >> + vreg->voltage = temp; >> + >> + cmd[cmd_count].addr >> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE; >> + cmd[cmd_count++].data >> + = DIV_ROUND_UP(vreg->voltage, 1000); > > It seems like you should set vreg->voltage to the actual result of the > division * 1000. AKA: if the user said the initial voltage was > 123,456 then vreg->voltage should become 124,000. Sure, I'll change this. > Actually, shouldn't you somehow resolve this with "struct > rpmh_vreg_hw_data". It seems like some regulators have 128 steps, > some have 256 steps, etc. You should have a function that reconciles > the requested voltage with the one that hardware will actually > provide. > > ...and, actually, you should share code for this reconciliation with > rpmh_regulator_vrm_set_voltage(). I'll see what I can do about this. >> +/** >> + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator >> + * @vreg: Pointer to the RPMh regulator >> + * >> + * Return: 0 on success, errno on failure >> + */ >> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg) >> +{ >> + struct device *dev = vreg->pmic->dev; >> + struct regulator_config reg_config = {}; >> + const struct rpmh_vreg_init_data *rpmh_data = NULL; >> + const char *type_name = NULL; >> + enum rpmh_regulator_type type; >> + struct regulator_init_data *init_data; >> + int ret, i; >> + >> + for (i = 0; i < vreg->pmic->init_data->count; i++) { >> + if (!strcmp(vreg->pmic->init_data->vreg_data[i].name, >> + vreg->of_node->name)) { >> + rpmh_data = &vreg->pmic->init_data->vreg_data[i]; >> + break; >> + } >> + } >> + >> + if (!rpmh_data) { >> + dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n", >> + vreg->of_node->name, vreg->pmic->init_data->name); >> + return -EINVAL; >> + } >> + >> + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d", >> + rpmh_data->resource_name_base, vreg->pmic->pmic_id, >> + rpmh_data->id); >> + if (!vreg->resource_name) >> + return -ENOMEM; >> + >> + vreg->addr = cmd_db_read_addr(vreg->resource_name); >> + if (!vreg->addr) { >> + vreg_err(vreg, "could not find RPMh address for resource %s\n", >> + vreg->resource_name); >> + return -ENODEV; >> + } >> + >> + vreg->rdesc.name = rpmh_data->name; >> + vreg->rdesc.supply_name = rpmh_data->supply_name; >> + vreg->regulator_type = rpmh_data->regulator_type; >> + vreg->hw_data = rpmh_data->hw_data; >> + >> + if (rpmh_data->hw_data->voltage_range) { >> + vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range; >> + vreg->rdesc.n_linear_ranges = 1; >> + vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages; >> + } >> + >> + /* Optional override for the default RPMh accelerator type */ >> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type", >> + &type_name); >> + if (!ret) { >> + if (!strcmp("vrm", type_name)) { >> + vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM; >> + } else if (!strcmp("xob", type_name)) { >> + vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB; >> + } else { >> + vreg_err(vreg, "Unknown RPMh accelerator type %s\n", >> + type_name); >> + return -EINVAL; >> + } > > As per comment in device tree patch, it seems really weird that you > can override this. Are you sure? Yes, we definitely need to have a board-specific mechanism to specify that a regulator which could be handled via VRM must be handled via XOB on a given board. I explained this in more detail in [1]. >> +static const struct rpmh_regulator_mode >> +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = { >> + [RPMH_REGULATOR_MODE_PASS] = { >> + .pmic_mode = 0, >> + .framework_mode = REGULATOR_MODE_STANDBY, > > Is "PASS" truly the same concept as the Linux concept of STANDBY. If > so, why do you need a separate define for it? > > If it truly is the same, it seems like you can simplify everything by > just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and > "RPMH_REGULATOR_MODE_PASS" and just call it > "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that > "standby" maps to "retention" for some regulators and maps to "pass" > for other regulators if you want to map PMIC documentation. ...but > getting rid of this distinction simply means less error checking and > fewer tables in Linux. > > If "pass" really shouldn't map to "standby" then this seems like a > hack and you should add the concept of "pass" to the core regulator > framework. For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a very low power mode which offers better power effeciency (i.e. lower ground current) than low power mode (LPM) for low load scenarios with the trade off of worse load regulator. It is typically applied when the system is asleep for regulators that must stay on but which have very low and stable load. Retention maps very well to REGULATOR_MODE_STANDBY. Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, the BOB output is directly connected to the BOB input (typically Vbat). This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is somewhat hacky to use it in this way. However, doing so makes qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode could be handled via get_bypass() and set_bypass() regulator ops. Doing this would require more complicated ops selections in the driver since it could no longer be determined simply by VRM vs XOB, it would instead need to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass() callbacks would be complicated because both would be writing to the same VRM address (mode control) with bypass==true taking precedence. Additionally, there is currently no way to specify default bypass from DT. A BOB-specific property would be needed to get this information. >> +static const struct rpmh_vreg_hw_data pmic4_pldo_hw_data = { >> + .voltage_range = &(const struct regulator_linear_range) >> + REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000), > > This seems pretty iffy to me. You're relying on the compiler to make > an anonymous chunk of memory with a "struct regulator_linear_range" in > it and then storing a pointer to said anonymous chunk of memory? > Nobody else using REGULATOR_LINEAR_RANGE() does this. A similiar approach is used in qcom_smd-regulator.c [2]: static const struct regulator_desc pm8941_nldo = { .linear_ranges = (struct regulator_linear_range[]) { REGULATOR_LINEAR_RANGE(750000, 0, 63, 12500), }, .n_linear_ranges = 1, .n_voltages = 64, .ops = &rpm_smps_ldo_ops, }; > Why not just get rid of the pointer and put the structure right inside > "struct rpmh_vreg_hw_data"? It'll get rid of the weird cast / strange > anonymous chunk, save an indirection, and save 4 bytes for a pointer. I'll make this change. >> +/** >> + * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each >> + * of the regulator nodes associated with it >> + * @pdev: Pointer to the platform device of the RPMh PMIC >> + * >> + * Return: 0 on success, errno on failure >> + */ >> +static int rpmh_regulator_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + const struct of_device_id *match; >> + struct rpmh_pmic *pmic; >> + struct device_node *node; >> + int ret, i; >> + >> + node = dev->of_node; >> + >> + if (!node) { >> + dev_err(dev, "Device tree node is missing\n"); >> + return -EINVAL; >> + } >> + >> + ret = cmd_db_ready(); >> + if (ret < 0) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "Command DB not available, ret=%d\n", ret); >> + return ret; >> + } >> + >> + pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL); >> + if (!pmic) >> + return -ENOMEM; >> + >> + pmic->dev = dev; >> + platform_set_drvdata(pdev, pmic); >> + >> + pmic->rpmh_client = rpmh_get_client(pdev); > > It seems like you'd do yourself (and the other clients of rpmh) a > favor if you added a devm_rpmh_get_client() in a patch before this > one. Adding a devm version of a calls is pretty easy and you'll be > able to completely get rid of your "remove" function. ...and get rid > of the "cleanup" exit here. Lina, could you please add this to your rpmh patch series since it is still undergoing review? [3] >> +static struct platform_driver rpmh_regulator_driver = { >> + .driver = { >> + .name = "qcom-rpmh-regulator", >> + .of_match_table = rpmh_regulator_match_table, >> + .owner = THIS_MODULE, > > As per the robot, no need to set .owner here. The core will do it. I'll remove this. >> + }, >> + .probe = rpmh_regulator_probe, >> + .remove = rpmh_regulator_remove, >> +}; >> + >> +static int rpmh_regulator_init(void) >> +{ >> + return platform_driver_register(&rpmh_regulator_driver); >> +} >> + >> +static void rpmh_regulator_exit(void) >> +{ >> + platform_driver_unregister(&rpmh_regulator_driver); >> +} >> + >> +arch_initcall(rpmh_regulator_init); > > I always get yelled at when I try to use arch_initcall() for stuff > like this. You should do what everyone else does and use > module_platform_driver() to declare your driver. Yeah, regulators are > important and (as I remember) they get probed slightly early anyway, > but everything else in the system just gotta deal with the fact that > they'll sometimes get deferred probes. I agree that consumers should handle probe deferral. Unfortunately, reality isn't always so nice. Also, probe deferrals increase boot-up time which is particularly problematic in the mobile space. I suppose that I can change this to module_platform_driver() for now. If any major issues arise it could be changed back to arch_initcall(). Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using subsys_initcall() right now in place of module_platform_driver(). >> +module_exit(rpmh_regulator_exit); >> + >> +MODULE_DESCRIPTION("Qualcomm RPMh regulator driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h >> new file mode 100644 >> index 0000000..f854e0e >> --- /dev/null >> +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h > > Device tree guys will yell at you here. The "include/dt-bindings" > bits are supposed to be together with the bindings. Different > maintainers have different beliefs here, but I think the way that's > least likely to get you yelled at by the most people is: > > Patch #1: bindings (.txt and include file) > Patch #2: the driver > > ...with the idea being that if another operating system wanted just > the bindings they could get it all in one patch. I'll move the header into the DT patch of my series. >> @@ -0,0 +1,40 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ >> + >> +#ifndef __QCOM_RPMH_REGULATOR_H >> +#define __QCOM_RPMH_REGULATOR_H >> + >> +/* >> + * These mode constants may be used for qcom,allowed-modes and qcom,init-mode > > Not "qcom,init-mode". This is actually "regulator-initial-mode" now. I'll correct this. Take care, David [1]: https://lkml.org/lkml/2018/3/21/877 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/qcom_smd-regulator.c?h=v4.16-rc6#n252 [3]: https://lkml.org/lkml/2018/3/9/979
Hello Stephen, Thank you for the very detailed review feedback. On 03/21/2018 12:07 PM, Stephen Boyd wrote: > Quoting David Collins (2018-03-16 18:09:10) >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >> index 097f617..e0ecd0a 100644 >> --- a/drivers/regulator/Kconfig >> +++ b/drivers/regulator/Kconfig >> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM >> Qualcomm RPM as a module. The module will be named >> "qcom_rpm-regulator". >> >> +config REGULATOR_QCOM_RPMH >> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver" >> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST > > What's the build dependency on OF? The qcom_rpmh-regulator driver cannot function without device tree support enabled. I suppose that it might be able to compile, but it wouldn't be useful. >> diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c ... >> +#include <linux/string.h> >> +#include <linux/regulator/driver.h> >> +#include <linux/regulator/machine.h> > > Including machine is usually a red flag in regulator drivers. Many of the regulator drivers include machine.h. It is done here in order to allow manipulation of constraints.valid_ops_mask (to set REGULATOR_CHANGE_MODE) and constraints.valid_mode_mask. There is no DT method to configure these fields. >> +/* Register offsets: */ > > Why the colon? ^ > Why even have the comment? I can remove such comments if they are not preferred in upstream code. >> +/* Enable register values: */ >> +#define RPMH_REGULATOR_DISABLE 0x0 >> +#define RPMH_REGULATOR_ENABLE 0x1 >> + >> +/* Number of unique hardware modes supported: */ > > Both above also look useless. I'll remove them too. >> + * @rdev: Regulator device pointer returned by >> + * devm_regulator_register() > > Is this used? Why save it around? I'll remove it. >> + * @voltage: RPMh VRM regulator voltage in microvolts > > So call it uV? Ok. >> + * @mode: RPMh VRM regulator current framework mode >> + * @headroom_voltage: RPMh VRM regulator minimum headroom voltage >> + * required > > headroom_uV? Ok. >> +struct rpmh_vreg { >> + struct device_node *of_node; >> + struct rpmh_pmic *pmic; >> + const char *resource_name; >> + u32 addr; >> + struct regulator_desc rdesc; >> + struct regulator_dev *rdev; >> + const struct rpmh_vreg_hw_data *hw_data; >> + enum rpmh_regulator_type regulator_type; >> + bool always_wait_for_ack; >> + struct rpmh_regulator_mode *mode_map; >> + int mode_count; > > size_t? Ok. >> + >> + bool enabled; >> + int voltage; >> + unsigned int mode; >> + int headroom_voltage; > > Please try to limit the things that are assigned into these structs and > then never used outside of init. It adds complexity to the code when a > local variable in the function would work just as well. I'll remove headroom_voltage from this struct. >> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator >> + * @name: Name for the regulator which also corresponds >> + * to the device tree subnode name of the regulator >> + * @resource_name_base: RPMh regulator resource name prefix. E.g. >> + * "ldo" for RPMh resource "ldoa1". > > Maybe it should be "ldo%c1"? Then we could kasprintf the name with the > pmic_id and drop the 'id' member entirely. I can make this modification (though with %s instead of %c for simplicity with DT string parsing). Hopefully having a variable format string doesn't trigger any static analysis tools. >> +struct rpmh_pmic { >> + struct device *dev; >> + struct rpmh_client *rpmh_client; >> + struct rpmh_vreg *vreg; > > It's a circle! Life is a circle! It's a circle! I can get rid of the vreg array. >> + int vreg_count; >> + const char *pmic_id; >> + const struct rpmh_pmic_init_data *init_data; > > Hopefully we don't really need this entire struct and we can just use > local variables instead. Outside of probe-time, this is used by struct rpmh_vreg in order to access rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug and error messages). I suppose that rpmh_client could be specified in struct rpmh_vreg directly. >> +static int rpmh_regulator_enable(struct regulator_dev *rdev) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + struct tcs_cmd cmd = { >> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, >> + .data = RPMH_REGULATOR_ENABLE, >> + }; >> + int ret; >> + >> + if (vreg->enabled) >> + return 0; >> + >> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, true); >> + if (ret < 0) { >> + vreg_err(vreg, "enable failed, ret=%d\n", ret); > > Do we really need all these error messages deep down in the regulator > drivers? If consumers care, they'll print some error message themselves > anyway. I'd prefer to keep error messages in place unless you have a strong objection. They make debugging simpler. >> +/** >> + * rpmh_regulator_vrm_set_load() - set the PMIC mode based upon the maximum load >> + * required from the VRM rpmh-regulator >> + * @rdev: Regulator device pointer for the rpmh-regulator >> + * @load_ua: Maximum current required from all consumers in microamps >> + * >> + * This function is passed as a callback function into the regulator ops that >> + * are registered for each VRM rpmh-regulator device. >> + * >> + * This function sets the mode of the regulator to that which has the highest >> + * min support load less than or equal to load_ua. Example: > > s/support/supported/ ACK >> +/** >> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations >> + * for a VRM RPMh resource from device tree >> + * vreg: Pointer to the rpmh regulator resource >> + * >> + * This function initializes the mode[] array of vreg based upon the values >> + * of optional device tree properties. >> + * >> + * Return: 0 on success, errno on failure >> + */ >> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg) >> +{ > > I have a feeling this should all come from the driver data, not DT. > Doubtful this really changes for each board. This needs to be determined at a board level instead of hard-coded per regulator type. For LDOs switching between LPM and HPM typically happens at 10 mA or 30 mA per hardware documentation. Unfortunately, sharing control of regulators with other processors adds some subtlety. Consider the case of a regulator with 10 mA LPM/HPM threshold physically. Say that modem and application processors each have a load on the regulator that draws 9 mA. If they each respect the 10 mA threshold, then they'd each vote for LPM. VRM will aggregate these requests together which will result in the regulator being set to LPM even though the total load is 18 mA (which would require HPM). To get around this corner case, a threshold of 1 uA can be used for all supplies that have non-application processor consumers. Thus, any non-zero current request will result in setting the regulator to HPM (which is always safe). Another example is SMPS regulators which should theoretically always be able to operate in AUTO mode. However, there may be board/system issues that require switching to PWM mode (HPM) for certain use cases as it has better load regulation (even though it can source the same amount of current as AUTO mode). If there is more than one consumer that requires this ability for a given regulator, then regulator_set_load() is the only option since it provides aggregation, where as regulator_set_mode() does not. >> +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic) >> +{ >> + struct device_node *node; >> + int i; >> + >> + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node); >> + if (pmic->vreg_count == 0) { >> + dev_err(pmic->dev, "could not find any regulator subnodes\n"); >> + return -ENODEV; >> + } >> + >> + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count, >> + sizeof(*pmic->vreg), GFP_KERNEL); >> + if (!pmic->vreg) >> + return -ENOMEM; > > Please just allocate one at a time. It's not like we have thousands of > these things to worry about. Sure, I'll change this. >> +/** >> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource >> + * request for this regulator based on optional device tree >> + * properties >> + * @vreg: Pointer to the RPMh regulator >> + * >> + * Return: 0 on success, errno on failure >> + */ >> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg) >> +{ >> + struct tcs_cmd cmd[2] = { }; >> + const char *prop; >> + int cmd_count = 0; >> + int ret; >> + u32 temp; >> + >> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) { >> + prop = "qcom,headroom-voltage"; > > Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh > registers. This probably needs to be pushed into the framework and come > down through a 'set_headroom' op in the regulator ops via a > regulator-headroom-microvolt property that's parsed in of_regulator.c. The qcom,headroom-voltage property is equivalent to struct regulator_desc.min_dropout_uV, but handled in hardware. I don't see the need to make a new regulator op to configure this value dynamically. Headroom typically does not need to change. Also, we don't really want this particular value plumbed into min_dropout_uV since we need to pass it directly to hardware and not have the regulator framework attempt to use it for a parent. >> + prop = "qcom,regulator-initial-voltage"; > > DT constraints should take care of this by setting voltages on all > regulators that need them? Unfortunately not. At regulator registration time, the regulator framework machine_constraints_voltage() function will call get_voltage() op and check if the voltage is in the min_uV to max_uV constraint range. If it is, then nothing happens. If it's not, then it will call the set_voltage() call back to set the voltage to min_uV if current_uV < min_uV or max_uV if current_uV > max_uV. Since the rpmh regulator driver doesn't know the initial voltage, get_voltage() will return 0 initially. As a result, machine_constraints_voltage() will always set the regulator to the minimum constraint voltage specified in DT. That behavior may not be acceptable for some regulators depending upon the hardware state at kernel initialization. Therefore, we need a DT mechanism to specify a single voltage to configure by default. >> + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator > > Heh, abbributes. I'll fix this. >> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg) ... >> + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d", >> + rpmh_data->resource_name_base, vreg->pmic->pmic_id, >> + rpmh_data->id); >> + if (!vreg->resource_name) >> + return -ENOMEM; > > This isn't used outside of this function, so remove the > vreg::resource_name member and use a local variable that gets freed > on exit please. Ok. >> + /* Optional override for the default RPMh accelerator type */ >> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type", > > Can this property have override in the name? And then because it is > called override, perhaps it should come from the driver instead of DT > because DT may need an override itself. Yes, I can add 'override' to the name of the property. I'm not following your second sentence. We require the ability to specify that a given regulator is using XOB instead of VRM at a board level. This means that the override needs to happen in DT instead of the driver. See [1] for more details. > Also, is this currently being used? If not I'd prefer we drop this until we > need it. It will be needed on a to-be-released Qualcomm Technologies, Inc. SoC that is currently under development. I suppose that I can leave this property out for the initial patch. However, we would still need to ensure that the driver architecture allows for this property to be read in a later patch and determine the regulator ops and whether or not to perform VRM specific initialization. >> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) { >> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV; >> + init_data->constraints.apply_uV = 0; >> + vreg->rdesc.n_voltages = 1; >> + } > > What is this doing? Usually constraints aren't touched by the driver. For XOB managed regulators, we need to set fixed_uV to match the DT constraint voltage and n_voltages = 1. This allows consumers regulator_set_voltage() calls to succeed for such regulators. It works the same as a fixed regulator. I think that apply_uV = 0 could be left out. >> + >> + if (vreg->hw_data->mode_map) { >> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE; > > Huh, I thought this was assigned by the framework. No, this is not set anywhere in the regulator framework. There isn't a DT method to configure it. It seems that it could only be handled before with board files. Other regulator drivers also configure it. >> +static const struct rpmh_pmic_init_data pm8005_pmic_data = { >> + .name = "PM8005", >> + .vreg_data = pm8005_vreg_data, >> + .count = ARRAY_SIZE(pm8005_vreg_data), >> +}; > > Kill 'name' please, and then get rid of 'count' and NUL terminate the > array instead. This follows previous rpm regulator driver styles. Also, > drop the macro that does stringification. We should end up with the > match_table pointing to static arrays of structs that look like: > > { "s1", VRM, "smp", 1, &pmic4_ftsmps42, "vdd_s1", }, > > And yes, drop the RPMH_REGULATOR_TYPE_ prefix and _hw_data postfix. Ok. >> +static const struct of_device_id rpmh_regulator_match_table[] = { >> + { >> + .compatible = "qcom,pm8998-rpmh-regulators", >> + .data = &pm8998_pmic_data, >> + }, >> + { >> + .compatible = "qcom,pmi8998-rpmh-regulators", >> + .data = &pmi8998_pmic_data, >> + }, >> + { >> + .compatible = "qcom,pm8005-rpmh-regulators", >> + .data = &pm8005_pmic_data, >> + }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table); > > Please move this array next to the driver structure. Ok. >> +static int rpmh_regulator_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + const struct of_device_id *match; >> + struct rpmh_pmic *pmic; >> + struct device_node *node; >> + int ret, i; >> + >> + node = dev->of_node; >> + >> + if (!node) { >> + dev_err(dev, "Device tree node is missing\n"); >> + return -EINVAL; >> + } > > This should never happen. Please remove. Ok. >> + >> + ret = cmd_db_ready(); >> + if (ret < 0) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "Command DB not available, ret=%d\n", ret); >> + return ret; >> + } > > We should just make rpmh parent device call cmd_db_ready() so that these > devices aren't even populated until then and so that cmd_db_ready() is > only in one place. Lina? Let's see if Lina has qualms about this plan. >> + pmic->rpmh_client = rpmh_get_client(pdev); >> + if (IS_ERR(pmic->rpmh_client)) { >> + ret = PTR_ERR(pmic->rpmh_client); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to request RPMh client, ret=%d\n", >> + ret); > > I fail to see how this happens given that rpmh creates this device. I'll remove the EPROBE_DEFER check. >> + match = of_match_node(rpmh_regulator_match_table, node); >> + if (match) { >> + pmic->init_data = match->data; >> + } else { >> + dev_err(dev, "could not find compatible string match\n"); >> + ret = -ENODEV; >> + goto cleanup; >> + } > > Use of_device_get_match_data() instead and print nothing on error. Ok. >> + ret = rpmh_regulator_allocate_vreg(pmic); > > _allocate_vregs (plural)? This function is going to be removed per Doug's suggestions. >> + if (ret < 0) { >> + dev_err(dev, "failed to allocate regulator subnode array, ret=%d\n", > > Please no allocation error messages, kmalloc already prints a bunch of > info. Ok. >> + dev_dbg(dev, "successfully probed %d %s regulators\n", >> + pmic->vreg_count, pmic->init_data->name); > > Doesn't the regulator framework already print a bunch of constraint > stuff when regulators are registered? Seems sort of spammy even for > debug mode. Plus it's the only reason for pmic::name right now. I think that the regulator framework will only print something if it has to set the voltage in order to bring the regulator voltage into the constraint range. I suppose that I can remove this message. >> +static int rpmh_regulator_remove(struct platform_device *pdev) >> +{ >> + struct rpmh_pmic *pmic = platform_get_drvdata(pdev); >> + >> + rpmh_release(pmic->rpmh_client); > > I'm still lost on what rpmh_client is giving us besides more code we > don't need. I'll ping the rpmh thread again. Let's see if Lina is willing to add some devm_* calls so that no cleanup is required. Take care, David [1]: https://lkml.org/lkml/2018/3/21/877
Hi, On Thu, Mar 22, 2018 at 3:31 PM, David Collins <collinsd@codeaurora.org> wrote: >>> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev) >>> +{ >>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>> + >>> + return vreg->enabled; >> >> You can't read hardware? The regulator framework expects you to read >> hardware. If it was common not to be able to read hardware then the >> regulator framework would just keep track of the enabled state for >> you. Right now if a regulator was left on by the BIOS (presumably >> some have to be since otherwise you're running on a computer that >> takes no power), you'll still return false at bootup? Seems >> non-ideal. > > Correct, the RPMh interface provides no mechanism to poll the physical > PMIC regulator hardware state. > > However, that isn't entirely a bad thing due to the nature of RPMh > regulator control. Several processors within the SoC besides the > application processor are able to vote on the state of PMIC regulators via > RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across > votes from different processors for each votable quantity for a given > regulator (enable, voltage, mode, and headroom voltage for VRM; enable for > XOB). The aggregated values are then configured in the PMIC regulator via > SPMI transactions. > > If Linux could read the physical PMIC regulator state and report it back > via is_enabled(), get_voltage() and get_mode() regulator ops, then we > could get into situations where the applications processor fails to vote > on a regulator which is requested by a Linux consumer. For example, say > that the modem processor has requested a given regulator to be enabled via > RPMh. Later, a Linux consumer driver on the application processor calls > regulator_enable() for the regulator. The regulator framework will call > the is_enabled() callback which will return true. This would then > short-circuit the regulator_enable() call and skip calling the > rpmh-regulator enable() callback. This means that the application > processor would have no enable vote present within RPMh for the regulator. > If the modem processor then voted to disable the regulator, it would > physically be disabled even though a consumer on the application processor > expects it to stay enabled. How about if I say it another way: can you read the vote that the boot code (pre-Linux) might have left for this regulator? ...if the boot code didn't init it, then I guess you could return 0, but if it did it would be nice to report it. AKA: I understand your concerns about other (non-AP) users. I just want to read the state that the BIOS left us in. >>> +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev, >>> + int min_uv, int max_uv, unsigned int *selector) >>> +{ >>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>> + struct tcs_cmd cmd = { >>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE, >>> + }; >>> + const struct regulator_linear_range *range; >>> + int mv, uv, ret; >>> + bool wait_for_ack; >>> + >>> + mv = DIV_ROUND_UP(min_uv, 1000); >>> + uv = mv * 1000; >>> + if (uv > max_uv) { >>> + vreg_err(vreg, "no set points available in range %d-%d uV\n", >>> + min_uv, max_uv); >>> + return -EINVAL; >>> + } >>> + >>> + range = vreg->hw_data->voltage_range; >>> + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step); >> >> It seems like you should remove the existing check you have for "if >> (uv > max_uv)" and replace with a check here. Specifically, it seems >> like the DIV_ROUND_UP for the selector could also bump you over the >> max. AKA: >> >> ... >> bool wait_for_ack; >> >> range = vreg->hw_data->voltage_range; >> *selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step); >> uv = *selector * range->uV_step + range->min_uV >> if (uv > max_uv) { >> ... >> } > > The VRM voltage vote register has units of mV (as does the voltage control > register for the PMIC regulators). The PMIC hardware automatically rounds > up the mV requests to the next physically available set point. This was > designed to simplify the life for software so that there isn't a need to > worry about voltage step size which varies between regulator types. > > The intention of the mv check was to verify that what is being programmed > into VRM is within the requested min_uv-max_uv range. However, since this > driver is forced to be aware of the step size, I can modify this as you > suggested with the stricter step-size bounding. > > >> Hold up, though. Why don't you implement set_voltage_sel() instead of >> set_voltage()? That's what literally everyone else does, well except >> PWM regulators. Using that will get rid of most of this code, won't >> it? Even the check to see if perhaps the voltage isn't changing. > > There are two cases that I can think of: 1. Having a set_voltage() > callback allows for delaying for an RPMh request ACK during certain > voltage set point decreasing scenarios (to be elaborated below). Can't you still have a delay in set_voltage_sel()? > 2. > Having a get_voltage() as opposed to get_voltage_sel() callback allows an > uninitialized voltage of 0 to be returned in the case that no initial > voltage is specified in device tree. This eliminates the possibility of > the regulator framework short-circuiting a regulator_set_voltage() call > that happens to match the voltage corresponding to selector 0. Interesting. I suppose you could mix them (have set_voltage_sel() and get_voltage()) as long as you documented why you were doing it. Then we'd have to see if Mark was happy with that... >>> + >>> + if (uv == vreg->voltage) >>> + return 0; >>> + >>> + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage; >> >> Do you often see "wait_for_ack = false" in reality? Most regulator >> usage I've seen requests a fairly tight range. AKA: I don't often >> see: >> >> set_voltage(min=3000mV, max=3300mV) >> set_voltage(min=1800mV, max=3300mV) > > I can't think of a good example off the top of my head. However, this > situation is certainly valid and can arise any time that there is a shared > rail and the hardware connected to it has minimum voltage requirements per > use case but large maximum voltage allowed. Yup. I'm not saying it's not allowed, it's just not something I've come across so far. ...but (assuming I read the code correctly) this is the only case your "wait_for_ack" was optimizing for. I don't mind having the optimization if it's needed, I just don't want it if there are no known users. >> Instead, I see: >> >> set_voltage(min=3000mV, max=3300mV) >> set_voltage(min=1800mV, max=1900mV) >> >> So you'll always have wait_for_ack = true in the cases I've seen. >> >> ...but are you certain it's useful to wait for an ack anyway when the >> voltage is falling? Most regulators won't guarantee that the voltage >> has actually fallen even after they ack you. Specifically if a >> regulator is under light load and it doesn't have an active discharge >> circuit then the regulator might fall very slowly over time. As a >> specific example, see commit 7c5209c315ea ("mmc: core: Increase delay >> for voltage to stabilize from 3.3V to 1.8V"). > > I'm aware of one important instance in which decreasing voltage needs a > delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case > that I had in mind with the 'max_uv < vreg->voltage' check. However, you > are correct that hardware will report completion of the voltage slewing > early in this case since the comparator is checking for output >= set > point. I can remove this special case check. > > >> That was a lot of words, so to sum it all up: >> >> * If you have no actual examples where you see "wait_for_ack = false" >> then remove this code and always wait. >> >> * If you have evidence that the time spent waiting for the ack is >> slowing you down, consider always setting wait_for_ack to false when >> you're lowering the voltage. Anyone who truly cares could just set >> something like the device tree property >> regulator-settling-time-down-us. ...or, assuming Mark doesn't hate >> it, they could set the always-wait-for-ack property in the device >> tree. > > I'm ok with changing the logic so that it waits for an ACK when increasing > voltage (required for correctness) and does not wait for an ACK when > decreasing voltage. Sounds fine with me. You could even add a comment saying that waiting for an Ack when falling isn't so useful unless the line has some sort of active discharge. You've already got a device tree feature for this, so it would be a good to document it there too. We'll also have to see if Mark wants this to be more generic in some way. It seems that other regulators could benefit from this type of knowledge and it would be nice if drivers could specify this and have it work on all PMICs. >> NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy >> ARC things for this?), we're probably talking about a small handful of >> voltage transitions per boot, right? > > VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh > regulator requests made during different use cases. > > >>> +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev) >>> +{ >>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>> + >>> + return vreg->voltage; >> >> I guess there's no way to read the voltage at bootup? So this will >> return 0 until someone sets it? ...maybe less of a big deal due to >> the "qcom,regulator-initial-voltage" property? > > Correct. As mentioned above, there is no way to read the physical PMIC > regulator voltage from RPMh. Yes, this will return 0 unless > qcom,regulator-initial-voltage is specified or the set_voltage() regulator > op is called. Returning an invalid voltage of 0 uV here is convenient as > it ensures that there is no way to accidentally skip setting the voltage > due to returning the voltage of selector 0. I actually wonder if it would be wiser to return an error code if you try to read a regulator that has never been set. >>> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev, >>> + unsigned int mode) >>> +{ >>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>> + struct tcs_cmd cmd = { >>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE, >>> + }; >>> + int i, ret; >>> + >>> + if (mode == vreg->mode) >>> + return 0; >>> + >>> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) >>> + if (vreg->hw_data->mode_map[i].framework_mode == mode) >>> + break; >>> + if (i >= RPMH_REGULATOR_MODE_COUNT) { >>> + vreg_err(vreg, "invalid mode=%u\n", mode); >>> + return -EINVAL; >>> + } >>> + >>> + cmd.data = vreg->hw_data->mode_map[i].pmic_mode; >>> + >>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, >>> + mode < vreg->mode || !vreg->mode); >> >> Please explain the "mode < vreg->mode || !vreg->mode" test in words. > > This waits for an ACK from RPMh of successfully mode transition when > switching to a higher power mode (regulator framework modes are ordered > with highest power == lowest numerical value) or when setting the first mode. Sorry, I meant: please add this explanation to comments in the function. >> IMHO it would be better to have a table like "dt_to_linux_mode" that >> was just a simple list: >> >> static const int dt_to_linux_mode_bob[] = [ >> [RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY, >> [RPMH_REGULATOR_MODE_RET] = -EINVAL, >> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, >> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, >> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST >> ]; >> >> static const int dt_to_linux_mode_ldo_smps[] = >> [RPMH_REGULATOR_MODE_PASS] = -EINVAL, >> [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, >> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, >> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, >> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST >> ]; >> >> You'd only use that in the "map_mode" functions and when parsing >> qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could >> actually just call the map_mode function. This would be especially a >> good way to do this if you moved "allowed-modes" into the regulator >> core, which seems like a good idea. >> >> The nice thing about this is that only place you need to conceptually >> keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing >> code. Otherwise you just think of the Linux version. > > I would be ok with defining arrays like these to handle the mapping of > PMIC regulator hardware agnostic "device tree mode" to "framework mode". > However, a second set of arrays would then be needed to map from > "framework mode" to hardware specific "pmic mode". I suppose that having > the second array indexed by framework mode would make the code a little > simpler since iteration would not be needed. Yeah, I was thinking that with 2 arrays the code would be more obvious. > Here is an explanation for why the "device tree mode" abstraction is > present in the first place. Between different Qualcomm Technologies, Inc. > PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, > LPM/PFM, Retention, and pass-through). However, the register values for > the same modes vary between different regulator types and different PMIC > families. This patch is adding support for several PMIC4 family PMICs. > The values needed for to-be-released PMIC5 PMIC regulators are different. > As an example, here are the different values for LPM/PFM across PMIC > families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 > LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" > ensures that it is not possible to inadvertently specify a PMIC specific > mode in device tree which corresponds to the wrong type or family but > which aliases a value that would be accepted as correct. I'm OK with having the "device tree mode" abstraction, and in fact the current regulator framework seems to want you to have this anyway. If I read the code correctly, you're required to have the conversion function and there's no default. >> Once you do the above, then your other list could just be named >> "allowed_modes". This would make it obvious that this isn't a map >> itself but that you could iterate over it to accomplish a mapping. > > I'm concerned with using the name "allowed_modes" as it tends to imply the > wrong idea. Perhaps something along the lines of "drms_modes" would be > better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes > utilized by the set_load() callback along with corresponding threshold > load currents. Yeah, that name is OK by me. At least I can lookup in the header file to find out that DRMS means "Dynamic Regulator Mode Switching". :) >>> +static const struct rpmh_regulator_mode >>> +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = { >>> + [RPMH_REGULATOR_MODE_PASS] = { >>> + .pmic_mode = 0, >>> + .framework_mode = REGULATOR_MODE_STANDBY, >> >> Is "PASS" truly the same concept as the Linux concept of STANDBY. If >> so, why do you need a separate define for it? >> >> If it truly is the same, it seems like you can simplify everything by >> just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and >> "RPMH_REGULATOR_MODE_PASS" and just call it >> "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that >> "standby" maps to "retention" for some regulators and maps to "pass" >> for other regulators if you want to map PMIC documentation. ...but >> getting rid of this distinction simply means less error checking and >> fewer tables in Linux. >> >> If "pass" really shouldn't map to "standby" then this seems like a >> hack and you should add the concept of "pass" to the core regulator >> framework. > > For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a > very low power mode which offers better power effeciency (i.e. lower > ground current) than low power mode (LPM) for low load scenarios with the > trade off of worse load regulator. It is typically applied when the > system is asleep for regulators that must stay on but which have very low > and stable load. Retention maps very well to REGULATOR_MODE_STANDBY. Yup, this seems like a perfect mapping between the Qualcomm mode and the regulator framework. > Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm > Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, > the BOB output is directly connected to the BOB input (typically Vbat). > This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is > somewhat hacky to use it in this way. However, doing so makes > qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode > could be handled via get_bypass() and set_bypass() regulator ops. Doing > this would require more complicated ops selections in the driver since it > could no longer be determined simply by VRM vs XOB, it would instead need > to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass() > callbacks would be complicated because both would be writing to the same > VRM address (mode control) with bypass==true taking precedence. > Additionally, there is currently no way to specify default bypass from DT. > A BOB-specific property would be needed to get this information. I've never poked at the get_bypass() / set_bypass(), but it sounds better to me to use them. I'm not a fan of the current hack. Even aside from the bit of hackiness, I'm slightly concerned that some of your logic that generally assumes lower integers = lower power modes would break. For instance in rpmh_regulator_vrm_set_mode() switching to "PASS" would look like you're switching to a low power mode so you'd skip the "wait for ack", right? I could sorta imagine things getting confused when trying to specify the mA load and having it switch modes automatically. I'd also notice that "regulator/qcom_spmi-regulator.c" seems to be using get_bypass() / set_bypass(), so maybe qcom-related clients will expect this? >>> + }, >>> + .probe = rpmh_regulator_probe, >>> + .remove = rpmh_regulator_remove, >>> +}; >>> + >>> +static int rpmh_regulator_init(void) >>> +{ >>> + return platform_driver_register(&rpmh_regulator_driver); >>> +} >>> + >>> +static void rpmh_regulator_exit(void) >>> +{ >>> + platform_driver_unregister(&rpmh_regulator_driver); >>> +} >>> + >>> +arch_initcall(rpmh_regulator_init); >> >> I always get yelled at when I try to use arch_initcall() for stuff >> like this. You should do what everyone else does and use >> module_platform_driver() to declare your driver. Yeah, regulators are >> important and (as I remember) they get probed slightly early anyway, >> but everything else in the system just gotta deal with the fact that >> they'll sometimes get deferred probes. > > I agree that consumers should handle probe deferral. Unfortunately, > reality isn't always so nice. Also, probe deferrals increase boot-up time > which is particularly problematic in the mobile space. Sigh, yeah. I'm not a fan either. If you can convince Mark that you should use arch_initcall() or subsys_initcall() I won't yell. ...but in the past I've seen others get yelled at. Note: in actuality it doesn't always increase boot time a whole lot. In theory as long as the CPU is running full bore and you're not killing parallelism too much then the only "waste" is the bit of time to run the start of the probe. It's not much. Every time someone claims "but my boot is slow because of deferrals" others say "where is the evidence?" and I don't remember a whole lot of evidence being presented. Maybe you have evidence? ...but deferrals _do_ for sure increase the time for certain peripherals to come up, and if those peripherals are things like the LCD displays then it sucks. > I suppose that I > can change this to module_platform_driver() for now. If any major issues > arise it could be changed back to arch_initcall(). Probably wise. > Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using > subsys_initcall() right now in place of module_platform_driver(). Yeah, I've made the argument before and been told "they are grandfathered in." -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 22 2018 at 19:30 -0600, David Collins wrote: >Hello Stephen, > >Thank you for the very detailed review feedback. > >On 03/21/2018 12:07 PM, Stephen Boyd wrote: >> Quoting David Collins (2018-03-16 18:09:10) >>> +static int rpmh_regulator_remove(struct platform_device *pdev) >>> +{ >>> + struct rpmh_pmic *pmic = platform_get_drvdata(pdev); >>> + >>> + rpmh_release(pmic->rpmh_client); >> >> I'm still lost on what rpmh_client is giving us besides more code we >> don't need. I'll ping the rpmh thread again. > >Let's see if Lina is willing to add some devm_* calls so that no cleanup >is required. > I will look into this. -- Lina -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 23, 2018 at 01:00:47PM -0700, Doug Anderson wrote: > On Thu, Mar 22, 2018 at 3:31 PM, David Collins <collinsd@codeaurora.org> wrote: > > There are two cases that I can think of: 1. Having a set_voltage() > > callback allows for delaying for an RPMh request ACK during certain > > voltage set point decreasing scenarios (to be elaborated below). > Can't you still have a delay in set_voltage_sel()? We have specific support for adding ramp delays, no need to open code it in operations. > > 2. > > Having a get_voltage() as opposed to get_voltage_sel() callback allows an > > uninitialized voltage of 0 to be returned in the case that no initial > > voltage is specified in device tree. This eliminates the possibility of > > the regulator framework short-circuiting a regulator_set_voltage() call > > that happens to match the voltage corresponding to selector 0. > Interesting. I suppose you could mix them (have set_voltage_sel() and > get_voltage()) as long as you documented why you were doing it. Then > we'd have to see if Mark was happy with that... This is a *terrible* idea which will almost certainly break. If the driver can't read values it should return an appropriate error code. > > I'm aware of one important instance in which decreasing voltage needs a > > delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case > > that I had in mind with the 'max_uv < vreg->voltage' check. However, you > > are correct that hardware will report completion of the voltage slewing > > early in this case since the comparator is checking for output >= set > > point. I can remove this special case check. You can't usefully wait for voltages to fall, you can never guarantee what the loading on the device is. It's something the user has to manage if they care. > > Here is an explanation for why the "device tree mode" abstraction is > > present in the first place. Between different Qualcomm Technologies, Inc. > > PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, > > LPM/PFM, Retention, and pass-through). However, the register values for > > the same modes vary between different regulator types and different PMIC > > families. This patch is adding support for several PMIC4 family PMICs. > > The values needed for to-be-released PMIC5 PMIC regulators are different. > > As an example, here are the different values for LPM/PFM across PMIC > > families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 > > LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" > > ensures that it is not possible to inadvertently specify a PMIC specific > > mode in device tree which corresponds to the wrong type or family but > > which aliases a value that would be accepted as correct. > I'm OK with having the "device tree mode" abstraction, and in fact the > current regulator framework seems to want you to have this anyway. If > I read the code correctly, you're required to have the conversion > function and there's no default. I didn't spot this in the code but something called "device tree mode" sounds like it's going to be awfully confusing... > > Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm > > Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, > > the BOB output is directly connected to the BOB input (typically Vbat). ... > > qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode > > could be handled via get_bypass() and set_bypass() regulator ops. Doing > > this would require more complicated ops selections in the driver since it This is exactly the functionality supported by the bypass operations. Any complexity due to the hardware design is unfortunate but honestly the way the QC regulator stuff is designed they seem like a bit of a lost cause on that front - they look very different to any other hardware we've seen. > I've never poked at the get_bypass() / set_bypass(), but it sounds > better to me to use them. I'm not a fan of the current hack. Even > aside from the bit of hackiness, I'm slightly concerned that some of > your logic that generally assumes lower integers = lower power modes > would break. Yes, abusing the framework is just going to make things even worse. > >>> +arch_initcall(rpmh_regulator_init); > >> I always get yelled at when I try to use arch_initcall() for stuff > >> like this. You should do what everyone else does and use > > I agree that consumers should handle probe deferral. Unfortunately, > > reality isn't always so nice. Also, probe deferrals increase boot-up time > > which is particularly problematic in the mobile space. > Sigh, yeah. I'm not a fan either. If you can convince Mark that you > should use arch_initcall() or subsys_initcall() I won't yell. ...but > in the past I've seen others get yelled at. Do you have concrete consumers that have a good reason for doing this? > Note: in actuality it doesn't always increase boot time a whole lot. Note also that we now have the device dependency mechanism that Raphael implemented with the explicit idea that that it'd be used to avoid unneeded deferrals. > ...but deferrals _do_ for sure increase the time for certain > peripherals to come up, and if those peripherals are things like the > LCD displays then it sucks. There's been some discussion of allowing the user to specific certain devices to target as priorities for probing which should deal with that.
Hi, On Tue, Mar 27, 2018 at 4:56 AM, Mark Brown <broonie@kernel.org> wrote: >> > Here is an explanation for why the "device tree mode" abstraction is >> > present in the first place. Between different Qualcomm Technologies, Inc. >> > PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, >> > LPM/PFM, Retention, and pass-through). However, the register values for >> > the same modes vary between different regulator types and different PMIC >> > families. This patch is adding support for several PMIC4 family PMICs. >> > The values needed for to-be-released PMIC5 PMIC regulators are different. >> > As an example, here are the different values for LPM/PFM across PMIC >> > families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 >> > LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" >> > ensures that it is not possible to inadvertently specify a PMIC specific >> > mode in device tree which corresponds to the wrong type or family but >> > which aliases a value that would be accepted as correct. > >> I'm OK with having the "device tree mode" abstraction, and in fact the >> current regulator framework seems to want you to have this anyway. If >> I read the code correctly, you're required to have the conversion >> function and there's no default. > > I didn't spot this in the code but something called "device tree mode" > sounds like it's going to be awfully confusing... Just to clarify this bit: The regulator framework allows for a callback function of_map_mode() in regulator drivers. My reading of the code shows that if you wish to use the property "regulator-initial-mode" (like this regulator does) that it is _required_ to provide an of_map_mode() function. Assuming I didn't mess up my analysis, the entire job of of_map_mode() is to convert from one integer to another. It should take the number that was specified in the device tree and convert it to a REGULATOR_MODE_XXX. That means that the regulator framework is enforcing a distinct and per-regulator numbering system for the mode (I called this "device tree mode"). Just for kicks, right now these are the REGULATOR_MODEs: #define REGULATOR_MODE_FAST 0x1 #define REGULATOR_MODE_NORMAL 0x2 #define REGULATOR_MODE_IDLE 0x4 #define REGULATOR_MODE_STANDBY 0x8 -- In cpcap_map_mode(): 0 (CPCAP_BIT_AUDIO_NORMAL_MODE) => 0x2 (normal) 0x40 (CPCAP_BIT_AUDIO_LOW_PWR) ==> 0x8 (standby) else => error -- In max77802_map_mode(): 3 (MAX77802_OPMODE_NORMAL) ==> 0x2 (normal) else (intends 1 or MAX77802_OPMODE_LP) ==> 0x8 (standby) -- In spmi_regulator_of_map_mode(): 1 => 0x2 (normal) 2 => 0x1 (fast) else => 0x4 (idle) -- In twl4030reg_map_mode(): 0xe (RES_STATE_ACTIVE) => 0x2 (normal) 0x8 (RES_STATE_SLEEP) => 0x8 (standby) else => error -- So basically it sounds like everyone makes up some arbitrary numbering system that is only used in their device tree files and needs to be mapped into the standard numbering system... Perhaps the right answer is to improve the regulator core to make of_map_mode() optional? We'd have to figure out where to place the #defines for the default of_map_mode() so they could be used in .dts files, but that should be possible. Presumably it would be wise to add a new property that was a bitmask of valid modes... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/23/2018 01:00 PM, Doug Anderson wrote: > On Thu, Mar 22, 2018 at 3:31 PM, David Collins <collinsd@codeaurora.org> wrote: >>>> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev) >>>> +{ >>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>>> + >>>> + return vreg->enabled; >>> >>> You can't read hardware? The regulator framework expects you to read >>> hardware. If it was common not to be able to read hardware then the >>> regulator framework would just keep track of the enabled state for >>> you. Right now if a regulator was left on by the BIOS (presumably >>> some have to be since otherwise you're running on a computer that >>> takes no power), you'll still return false at bootup? Seems >>> non-ideal. >> >> Correct, the RPMh interface provides no mechanism to poll the physical >> PMIC regulator hardware state. >> >> However, that isn't entirely a bad thing due to the nature of RPMh >> regulator control. Several processors within the SoC besides the >> application processor are able to vote on the state of PMIC regulators via >> RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across >> votes from different processors for each votable quantity for a given >> regulator (enable, voltage, mode, and headroom voltage for VRM; enable for >> XOB). The aggregated values are then configured in the PMIC regulator via >> SPMI transactions. >> >> If Linux could read the physical PMIC regulator state and report it back >> via is_enabled(), get_voltage() and get_mode() regulator ops, then we >> could get into situations where the applications processor fails to vote >> on a regulator which is requested by a Linux consumer. For example, say >> that the modem processor has requested a given regulator to be enabled via >> RPMh. Later, a Linux consumer driver on the application processor calls >> regulator_enable() for the regulator. The regulator framework will call >> the is_enabled() callback which will return true. This would then >> short-circuit the regulator_enable() call and skip calling the >> rpmh-regulator enable() callback. This means that the application >> processor would have no enable vote present within RPMh for the regulator. >> If the modem processor then voted to disable the regulator, it would >> physically be disabled even though a consumer on the application processor >> expects it to stay enabled. > > How about if I say it another way: can you read the vote that the boot > code (pre-Linux) might have left for this regulator? ...if the boot > code didn't init it, then I guess you could return 0, but if it did it > would be nice to report it. > > AKA: I understand your concerns about other (non-AP) users. I just > want to read the state that the BIOS left us in. Unfortunately, this is not possible either. The RPMh interface is write-only. >>>> +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev, >>>> + int min_uv, int max_uv, unsigned int *selector) >>>> +{ >>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>>> + struct tcs_cmd cmd = { >>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE, >>>> + }; >>>> + const struct regulator_linear_range *range; >>>> + int mv, uv, ret; >>>> + bool wait_for_ack; >>>> + >>>> + mv = DIV_ROUND_UP(min_uv, 1000); >>>> + uv = mv * 1000; >>>> + if (uv > max_uv) { >>>> + vreg_err(vreg, "no set points available in range %d-%d uV\n", >>>> + min_uv, max_uv); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + range = vreg->hw_data->voltage_range; >>>> + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step); >>> >>> It seems like you should remove the existing check you have for "if >>> (uv > max_uv)" and replace with a check here. Specifically, it seems >>> like the DIV_ROUND_UP for the selector could also bump you over the >>> max. AKA: >>> >>> ... >>> bool wait_for_ack; >>> >>> range = vreg->hw_data->voltage_range; >>> *selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step); >>> uv = *selector * range->uV_step + range->min_uV >>> if (uv > max_uv) { >>> ... >>> } >> >> The VRM voltage vote register has units of mV (as does the voltage control >> register for the PMIC regulators). The PMIC hardware automatically rounds >> up the mV requests to the next physically available set point. This was >> designed to simplify the life for software so that there isn't a need to >> worry about voltage step size which varies between regulator types. >> >> The intention of the mv check was to verify that what is being programmed >> into VRM is within the requested min_uv-max_uv range. However, since this >> driver is forced to be aware of the step size, I can modify this as you >> suggested with the stricter step-size bounding. >> >> >>> Hold up, though. Why don't you implement set_voltage_sel() instead of >>> set_voltage()? That's what literally everyone else does, well except >>> PWM regulators. Using that will get rid of most of this code, won't >>> it? Even the check to see if perhaps the voltage isn't changing. >> >> There are two cases that I can think of: 1. Having a set_voltage() >> callback allows for delaying for an RPMh request ACK during certain >> voltage set point decreasing scenarios (to be elaborated below). > > Can't you still have a delay in set_voltage_sel()? The same behavior cannot be achieved with set_voltage_sel as it is only aware of a single selector value, not a min to max allowed consumer range. However, since the driver can't guarantee that the voltage will have actually slewed down due to potentially high output capacitance and low load, this case can be safely ignored. I'll switch to a set_voltage_sel() callback. >> 2. >> Having a get_voltage() as opposed to get_voltage_sel() callback allows an >> uninitialized voltage of 0 to be returned in the case that no initial >> voltage is specified in device tree. This eliminates the possibility of >> the regulator framework short-circuiting a regulator_set_voltage() call >> that happens to match the voltage corresponding to selector 0. > > Interesting. I suppose you could mix them (have set_voltage_sel() and > get_voltage()) as long as you documented why you were doing it. Then > we'd have to see if Mark was happy with that... I'll switch to using get_voltage_sel(). >>>> + >>>> + if (uv == vreg->voltage) >>>> + return 0; >>>> + >>>> + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage; >>> >>> Do you often see "wait_for_ack = false" in reality? Most regulator >>> usage I've seen requests a fairly tight range. AKA: I don't often >>> see: >>> >>> set_voltage(min=3000mV, max=3300mV) >>> set_voltage(min=1800mV, max=3300mV) >> >> I can't think of a good example off the top of my head. However, this >> situation is certainly valid and can arise any time that there is a shared >> rail and the hardware connected to it has minimum voltage requirements per >> use case but large maximum voltage allowed. > > Yup. I'm not saying it's not allowed, it's just not something I've > come across so far. ...but (assuming I read the code correctly) this > is the only case your "wait_for_ack" was optimizing for. I don't mind > having the optimization if it's needed, I just don't want it if there > are no known users. I'll change it to be simply: increasing voltage: wait for ACK (required), decreasing voltage: don't wait for ACK (potential performance optimization). >>> Instead, I see: >>> >>> set_voltage(min=3000mV, max=3300mV) >>> set_voltage(min=1800mV, max=1900mV) >>> >>> So you'll always have wait_for_ack = true in the cases I've seen. >>> >>> ...but are you certain it's useful to wait for an ack anyway when the >>> voltage is falling? Most regulators won't guarantee that the voltage >>> has actually fallen even after they ack you. Specifically if a >>> regulator is under light load and it doesn't have an active discharge >>> circuit then the regulator might fall very slowly over time. As a >>> specific example, see commit 7c5209c315ea ("mmc: core: Increase delay >>> for voltage to stabilize from 3.3V to 1.8V"). >> >> I'm aware of one important instance in which decreasing voltage needs a >> delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case >> that I had in mind with the 'max_uv < vreg->voltage' check. However, you >> are correct that hardware will report completion of the voltage slewing >> early in this case since the comparator is checking for output >= set >> point. I can remove this special case check. >> >> >>> That was a lot of words, so to sum it all up: >>> >>> * If you have no actual examples where you see "wait_for_ack = false" >>> then remove this code and always wait. >>> >>> * If you have evidence that the time spent waiting for the ack is >>> slowing you down, consider always setting wait_for_ack to false when >>> you're lowering the voltage. Anyone who truly cares could just set >>> something like the device tree property >>> regulator-settling-time-down-us. ...or, assuming Mark doesn't hate >>> it, they could set the always-wait-for-ack property in the device >>> tree. >> >> I'm ok with changing the logic so that it waits for an ACK when increasing >> voltage (required for correctness) and does not wait for an ACK when >> decreasing voltage. > > Sounds fine with me. You could even add a comment saying that waiting > for an Ack when falling isn't so useful unless the line has some sort > of active discharge. You've already got a device tree feature for > this, so it would be a good to document it there too. > > We'll also have to see if Mark wants this to be more generic in some > way. It seems that other regulators could benefit from this type of > knowledge and it would be nice if drivers could specify this and have > it work on all PMICs. As Mark pointed out, this case can't really be handled by the driver since the regulator could be lightly loaded. Therefore, it is safe to ignore and simply use set_voltage_sel(). >>> NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy >>> ARC things for this?), we're probably talking about a small handful of >>> voltage transitions per boot, right? >> >> VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh >> regulator requests made during different use cases. >> >> >>>> +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev) >>>> +{ >>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>>> + >>>> + return vreg->voltage; >>> >>> I guess there's no way to read the voltage at bootup? So this will >>> return 0 until someone sets it? ...maybe less of a big deal due to >>> the "qcom,regulator-initial-voltage" property? >> >> Correct. As mentioned above, there is no way to read the physical PMIC >> regulator voltage from RPMh. Yes, this will return 0 unless >> qcom,regulator-initial-voltage is specified or the set_voltage() regulator >> op is called. Returning an invalid voltage of 0 uV here is convenient as >> it ensures that there is no way to accidentally skip setting the voltage >> due to returning the voltage of selector 0. > > I actually wonder if it would be wiser to return an error code if you > try to read a regulator that has never been set. I'll try this out and see if the regulator framework complains during regulator registration. >>>> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev, >>>> + unsigned int mode) >>>> +{ >>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>>> + struct tcs_cmd cmd = { >>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE, >>>> + }; >>>> + int i, ret; >>>> + >>>> + if (mode == vreg->mode) >>>> + return 0; >>>> + >>>> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) >>>> + if (vreg->hw_data->mode_map[i].framework_mode == mode) >>>> + break; >>>> + if (i >= RPMH_REGULATOR_MODE_COUNT) { >>>> + vreg_err(vreg, "invalid mode=%u\n", mode); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + cmd.data = vreg->hw_data->mode_map[i].pmic_mode; >>>> + >>>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, >>>> + mode < vreg->mode || !vreg->mode); >>> >>> Please explain the "mode < vreg->mode || !vreg->mode" test in words. >> >> This waits for an ACK from RPMh of successfully mode transition when >> switching to a higher power mode (regulator framework modes are ordered >> with highest power == lowest numerical value) or when setting the first mode. > > Sorry, I meant: please add this explanation to comments in the function. Sure, I'll add a comment for it. >>> IMHO it would be better to have a table like "dt_to_linux_mode" that >>> was just a simple list: >>> >>> static const int dt_to_linux_mode_bob[] = [ >>> [RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY, >>> [RPMH_REGULATOR_MODE_RET] = -EINVAL, >>> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, >>> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, >>> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST >>> ]; >>> >>> static const int dt_to_linux_mode_ldo_smps[] = >>> [RPMH_REGULATOR_MODE_PASS] = -EINVAL, >>> [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, >>> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, >>> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, >>> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST >>> ]; >>> >>> You'd only use that in the "map_mode" functions and when parsing >>> qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could >>> actually just call the map_mode function. This would be especially a >>> good way to do this if you moved "allowed-modes" into the regulator >>> core, which seems like a good idea. >>> >>> The nice thing about this is that only place you need to conceptually >>> keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing >>> code. Otherwise you just think of the Linux version. >> >> I would be ok with defining arrays like these to handle the mapping of >> PMIC regulator hardware agnostic "device tree mode" to "framework mode". >> However, a second set of arrays would then be needed to map from >> "framework mode" to hardware specific "pmic mode". I suppose that having >> the second array indexed by framework mode would make the code a little >> simpler since iteration would not be needed. > > Yeah, I was thinking that with 2 arrays the code would be more obvious. I'll make this modification. >> Here is an explanation for why the "device tree mode" abstraction is >> present in the first place. Between different Qualcomm Technologies, Inc. >> PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, >> LPM/PFM, Retention, and pass-through). However, the register values for >> the same modes vary between different regulator types and different PMIC >> families. This patch is adding support for several PMIC4 family PMICs. >> The values needed for to-be-released PMIC5 PMIC regulators are different. >> As an example, here are the different values for LPM/PFM across PMIC >> families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 >> LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" >> ensures that it is not possible to inadvertently specify a PMIC specific >> mode in device tree which corresponds to the wrong type or family but >> which aliases a value that would be accepted as correct. > > I'm OK with having the "device tree mode" abstraction, and in fact the > current regulator framework seems to want you to have this anyway. If > I read the code correctly, you're required to have the conversion > function and there's no default. I will continue to use this abstraction. >>> Once you do the above, then your other list could just be named >>> "allowed_modes". This would make it obvious that this isn't a map >>> itself but that you could iterate over it to accomplish a mapping. >> >> I'm concerned with using the name "allowed_modes" as it tends to imply the >> wrong idea. Perhaps something along the lines of "drms_modes" would be >> better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes >> utilized by the set_load() callback along with corresponding threshold >> load currents. > > Yeah, that name is OK by me. At least I can lookup in the header file > to find out that DRMS means "Dynamic Regulator Mode Switching". :) I'll make this modification. >>>> +static const struct rpmh_regulator_mode >>>> +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = { >>>> + [RPMH_REGULATOR_MODE_PASS] = { >>>> + .pmic_mode = 0, >>>> + .framework_mode = REGULATOR_MODE_STANDBY, >>> >>> Is "PASS" truly the same concept as the Linux concept of STANDBY. If >>> so, why do you need a separate define for it? >>> >>> If it truly is the same, it seems like you can simplify everything by >>> just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and >>> "RPMH_REGULATOR_MODE_PASS" and just call it >>> "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that >>> "standby" maps to "retention" for some regulators and maps to "pass" >>> for other regulators if you want to map PMIC documentation. ...but >>> getting rid of this distinction simply means less error checking and >>> fewer tables in Linux. >>> >>> If "pass" really shouldn't map to "standby" then this seems like a >>> hack and you should add the concept of "pass" to the core regulator >>> framework. >> >> For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a >> very low power mode which offers better power effeciency (i.e. lower >> ground current) than low power mode (LPM) for low load scenarios with the >> trade off of worse load regulator. It is typically applied when the >> system is asleep for regulators that must stay on but which have very low >> and stable load. Retention maps very well to REGULATOR_MODE_STANDBY. > > Yup, this seems like a perfect mapping between the Qualcomm mode and > the regulator framework. > > >> Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm >> Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, >> the BOB output is directly connected to the BOB input (typically Vbat). >> This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is >> somewhat hacky to use it in this way. However, doing so makes >> qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode >> could be handled via get_bypass() and set_bypass() regulator ops. Doing >> this would require more complicated ops selections in the driver since it >> could no longer be determined simply by VRM vs XOB, it would instead need >> to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass() >> callbacks would be complicated because both would be writing to the same >> VRM address (mode control) with bypass==true taking precedence. >> Additionally, there is currently no way to specify default bypass from DT. >> A BOB-specific property would be needed to get this information. > > I've never poked at the get_bypass() / set_bypass(), but it sounds > better to me to use them. I'm not a fan of the current hack. Even > aside from the bit of hackiness, I'm slightly concerned that some of > your logic that generally assumes lower integers = lower power modes > would break. > > For instance in rpmh_regulator_vrm_set_mode() switching to "PASS" > would look like you're switching to a low power mode so you'd skip the > "wait for ack", right? I could sorta imagine things getting confused > when trying to specify the mA load and having it switch modes > automatically. > > I'd also notice that "regulator/qcom_spmi-regulator.c" seems to be > using get_bypass() / set_bypass(), so maybe qcom-related clients will > expect this? I'll implement BOB PASS mode via get_bypass() and set_bypass() callbacks. >>>> + }, >>>> + .probe = rpmh_regulator_probe, >>>> + .remove = rpmh_regulator_remove, >>>> +}; >>>> + >>>> +static int rpmh_regulator_init(void) >>>> +{ >>>> + return platform_driver_register(&rpmh_regulator_driver); >>>> +} >>>> + >>>> +static void rpmh_regulator_exit(void) >>>> +{ >>>> + platform_driver_unregister(&rpmh_regulator_driver); >>>> +} >>>> + >>>> +arch_initcall(rpmh_regulator_init); >>> >>> I always get yelled at when I try to use arch_initcall() for stuff >>> like this. You should do what everyone else does and use >>> module_platform_driver() to declare your driver. Yeah, regulators are >>> important and (as I remember) they get probed slightly early anyway, >>> but everything else in the system just gotta deal with the fact that >>> they'll sometimes get deferred probes. >> >> I agree that consumers should handle probe deferral. Unfortunately, >> reality isn't always so nice. Also, probe deferrals increase boot-up time >> which is particularly problematic in the mobile space. > > Sigh, yeah. I'm not a fan either. If you can convince Mark that you > should use arch_initcall() or subsys_initcall() I won't yell. ...but > in the past I've seen others get yelled at. > > Note: in actuality it doesn't always increase boot time a whole lot. > In theory as long as the CPU is running full bore and you're not > killing parallelism too much then the only "waste" is the bit of time > to run the start of the probe. It's not much. Every time someone > claims "but my boot is slow because of deferrals" others say "where is > the evidence?" and I don't remember a whole lot of evidence being > presented. Maybe you have evidence? > > ...but deferrals _do_ for sure increase the time for certain > peripherals to come up, and if those peripherals are things like the > LCD displays then it sucks. > > >> I suppose that I >> can change this to module_platform_driver() for now. If any major issues >> arise it could be changed back to arch_initcall(). > > Probably wise. I'll switch to using module_platform_driver(). >> Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using >> subsys_initcall() right now in place of module_platform_driver(). > > Yeah, I've made the argument before and been told "they are grandfathered in." Ah, I see. Take care, David
On 03/27/2018 04:56 AM, Mark Brown wrote: > On Fri, Mar 23, 2018 at 01:00:47PM -0700, Doug Anderson wrote: >> On Thu, Mar 22, 2018 at 3:31 PM, David Collins <collinsd@codeaurora.org> wrote: > >>> There are two cases that I can think of: 1. Having a set_voltage() >>> callback allows for delaying for an RPMh request ACK during certain >>> voltage set point decreasing scenarios (to be elaborated below). > >> Can't you still have a delay in set_voltage_sel()? > > We have specific support for adding ramp delays, no need to open code it > in operations. I'll switch to using set_voltage_sel(). No ramp delay is need in Linux. RPMh hardware sends an ACK back to Linux after the voltage reaches the desired set point (i.e. is greater than or equal to the set point). The qcom_rpmh-regulator driver just needs to decide if it is going to wait for the ACK from RPMh or not. >>> 2. >>> Having a get_voltage() as opposed to get_voltage_sel() callback allows an >>> uninitialized voltage of 0 to be returned in the case that no initial >>> voltage is specified in device tree. This eliminates the possibility of >>> the regulator framework short-circuiting a regulator_set_voltage() call >>> that happens to match the voltage corresponding to selector 0. > >> Interesting. I suppose you could mix them (have set_voltage_sel() and >> get_voltage()) as long as you documented why you were doing it. Then >> we'd have to see if Mark was happy with that... > > This is a *terrible* idea which will almost certainly break. If the > driver can't read values it should return an appropriate error code. I'll switch to using get_voltage_sel(). I'll test out returning an error value in the case of an uninitialized voltage. Hopefully the regulator framework won't halt regulator registration because of it. >>> I'm aware of one important instance in which decreasing voltage needs a >>> delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case >>> that I had in mind with the 'max_uv < vreg->voltage' check. However, you >>> are correct that hardware will report completion of the voltage slewing >>> early in this case since the comparator is checking for output >= set >>> point. I can remove this special case check. > > You can't usefully wait for voltages to fall, you can never guarantee > what the loading on the device is. It's something the user has to > manage if they care. I agree. This case can't be handled in the regulator driver. >>> Here is an explanation for why the "device tree mode" abstraction is >>> present in the first place. Between different Qualcomm Technologies, Inc. >>> PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, >>> LPM/PFM, Retention, and pass-through). However, the register values for >>> the same modes vary between different regulator types and different PMIC >>> families. This patch is adding support for several PMIC4 family PMICs. >>> The values needed for to-be-released PMIC5 PMIC regulators are different. >>> As an example, here are the different values for LPM/PFM across PMIC >>> families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 >>> LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" >>> ensures that it is not possible to inadvertently specify a PMIC specific >>> mode in device tree which corresponds to the wrong type or family but >>> which aliases a value that would be accepted as correct. > >> I'm OK with having the "device tree mode" abstraction, and in fact the >> current regulator framework seems to want you to have this anyway. If >> I read the code correctly, you're required to have the conversion >> function and there's no default. > > I didn't spot this in the code but something called "device tree mode" > sounds like it's going to be awfully confusing... As I explained in the earlier email, it makes the device tree configurations much simpler and less confusing/error prone. I'd like to keep this concept around unless their are strong objections. >>> Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm >>> Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, >>> the BOB output is directly connected to the BOB input (typically Vbat). > > ... > >>> qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode >>> could be handled via get_bypass() and set_bypass() regulator ops. Doing >>> this would require more complicated ops selections in the driver since it > > This is exactly the functionality supported by the bypass operations. > Any complexity due to the hardware design is unfortunate but honestly > the way the QC regulator stuff is designed they seem like a bit of a > lost cause on that front - they look very different to any other > hardware we've seen. > >> I've never poked at the get_bypass() / set_bypass(), but it sounds >> better to me to use them. I'm not a fan of the current hack. Even >> aside from the bit of hackiness, I'm slightly concerned that some of >> your logic that generally assumes lower integers = lower power modes >> would break. > > Yes, abusing the framework is just going to make things even worse. I'll implement get_bypass() and set_bypass() callbacks for BOB. >>>>> +arch_initcall(rpmh_regulator_init); > >>>> I always get yelled at when I try to use arch_initcall() for stuff >>>> like this. You should do what everyone else does and use > >>> I agree that consumers should handle probe deferral. Unfortunately, >>> reality isn't always so nice. Also, probe deferrals increase boot-up time >>> which is particularly problematic in the mobile space. > >> Sigh, yeah. I'm not a fan either. If you can convince Mark that you >> should use arch_initcall() or subsys_initcall() I won't yell. ...but >> in the past I've seen others get yelled at. > > Do you have concrete consumers that have a good reason for doing this? I don't have any examples off the top of my head. >> Note: in actuality it doesn't always increase boot time a whole lot. > > Note also that we now have the device dependency mechanism that Raphael > implemented with the explicit idea that that it'd be used to avoid > unneeded deferrals. > >> ...but deferrals _do_ for sure increase the time for certain >> peripherals to come up, and if those peripherals are things like the >> LCD displays then it sucks. > > There's been some discussion of allowing the user to specific certain > devices to target as priorities for probing which should deal with that. I'll look into these features. Take care, David
On Tue, Mar 27, 2018 at 04:38:07PM -0700, David Collins wrote: > On 03/27/2018 04:56 AM, Mark Brown wrote: > > I didn't spot this in the code but something called "device tree mode" > > sounds like it's going to be awfully confusing... > As I explained in the earlier email, it makes the device tree > configurations much simpler and less confusing/error prone. I'd like to > keep this concept around unless their are strong objections. Like I say I didn't spot this in the code, I did give it a brief once over but as Doug had spotted such extensive problems I was expecting a resend. It *is* setting off big alarm bells though, in general if your individual driver is doing something weird to make life easier that's a sign that it's doing things at the wrong level.
On Tue, Mar 27, 2018 at 01:51:56PM -0700, Doug Anderson wrote: > Assuming I didn't mess up my analysis, the entire job of of_map_mode() > is to convert from one integer to another. It should take the number > that was specified in the device tree and convert it to a > REGULATOR_MODE_XXX. That means that the regulator framework is > enforcing a distinct and per-regulator numbering system for the mode > (I called this "device tree mode"). OK, the confusion comes from your terminology invention rather than the driver then :( > So basically it sounds like everyone makes up some arbitrary numbering > system that is only used in their device tree files and needs to be > mapped into the standard numbering system... It's not just that. The very modes themselves are not defined at all consistently between regulators - the set of options varies wildly as does the range of applications they are suitable for. We want people to not only define numbers here but also names that make sense in the context of the regulator documentation that can then be mapped into the concept of modes we've inherited in the regulator subsystem.
Hi, On Wed, Mar 21, 2018 at 12:07 PM, Stephen Boyd <sboyd@kernel.org> wrote: >> +static int rpmh_regulator_remove(struct platform_device *pdev) >> +{ >> + struct rpmh_pmic *pmic = platform_get_drvdata(pdev); >> + >> + rpmh_release(pmic->rpmh_client); > > I'm still lost on what rpmh_client is giving us besides more code we > don't need. I'll ping the rpmh thread again. It's not completely obvious to me what you are asking here and I think you didn't actually get back to pinging the RPMH thread, did you? In his response, David seems to have taken your comment as "I agree with Doug, please add devm_rpmh_get_client", but I'm not sure that's what you meant. It sounds a lot like you're suggesting totally gutting the concept of the "rpmh_client". I certainly haven't reviewed RPMH a whole lot, but perhaps you can make it more obvious how this would work. In any case, I guess this is a bit off topic for the regulator series. Perhaps you can respond to the RPMH series with more details about what you're looking for? Thanks! -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting David Collins (2018-03-22 18:30:06) > On 03/21/2018 12:07 PM, Stephen Boyd wrote: > > Quoting David Collins (2018-03-16 18:09:10) > >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > >> index 097f617..e0ecd0a 100644 > >> --- a/drivers/regulator/Kconfig > >> +++ b/drivers/regulator/Kconfig > >> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM > >> Qualcomm RPM as a module. The module will be named > >> "qcom_rpm-regulator". > >> > >> +config REGULATOR_QCOM_RPMH > >> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver" > >> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST > > > > What's the build dependency on OF? > > The qcom_rpmh-regulator driver cannot function without device tree support > enabled. I suppose that it might be able to compile, but it wouldn't be > useful. If it isn't a build dependency then we usually leave it out because it's not always useful to express runtime constraints in Kconfig. Probably all we need is depends on QCOM_RPMH || COMPILE_TEST and then stubs take care of it from there. > > >> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator > >> + * @name: Name for the regulator which also corresponds > >> + * to the device tree subnode name of the regulator > >> + * @resource_name_base: RPMh regulator resource name prefix. E.g. > >> + * "ldo" for RPMh resource "ldoa1". > > > > Maybe it should be "ldo%c1"? Then we could kasprintf the name with the > > pmic_id and drop the 'id' member entirely. > > I can make this modification (though with %s instead of %c for simplicity > with DT string parsing). Hopefully having a variable format string > doesn't trigger any static analysis tools. Oh it's variable how many digits in the 'id' number there are? I'll look at v2. > > > >> + int vreg_count; > >> + const char *pmic_id; > >> + const struct rpmh_pmic_init_data *init_data; > > > > Hopefully we don't really need this entire struct and we can just use > > local variables instead. > > Outside of probe-time, this is used by struct rpmh_vreg in order to access > rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug > and error messages). I suppose that rpmh_client could be specified in > struct rpmh_vreg directly. > Hopefully rpm_client goes away. Maybe it would just be dev pointer to hold onto then and possibly rip the name of each regulator out of the framework name for it? > > > >> +/** > >> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations > >> + * for a VRM RPMh resource from device tree > >> + * vreg: Pointer to the rpmh regulator resource > >> + * > >> + * This function initializes the mode[] array of vreg based upon the values > >> + * of optional device tree properties. > >> + * > >> + * Return: 0 on success, errno on failure > >> + */ > >> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg) > >> +{ > > > > I have a feeling this should all come from the driver data, not DT. > > Doubtful this really changes for each board. > > This needs to be determined at a board level instead of hard-coded per > regulator type. For LDOs switching between LPM and HPM typically happens > at 10 mA or 30 mA per hardware documentation. Unfortunately, sharing > control of regulators with other processors adds some subtlety. > > Consider the case of a regulator with 10 mA LPM/HPM threshold physically. > Say that modem and application processors each have a load on the > regulator that draws 9 mA. If they each respect the 10 mA threshold, then > they'd each vote for LPM. VRM will aggregate these requests together > which will result in the regulator being set to LPM even though the total > load is 18 mA (which would require HPM). To get around this corner case, > a threshold of 1 uA can be used for all supplies that have non-application > processor consumers. Thus, any non-zero current request will result in > setting the regulator to HPM (which is always safe). Sad. Sounds like the rpmh interface is broken and should be expressing the load instead of the mode so that aggregation on the rpm side can pick the right mode. So now we have a workaround by specifying some default minimum value that we add in? > > Another example is SMPS regulators which should theoretically always be > able to operate in AUTO mode. However, there may be board/system issues > that require switching to PWM mode (HPM) for certain use cases as it has > better load regulation (even though it can source the same amount of > current as AUTO mode). If there is more than one consumer that requires > this ability for a given regulator, then regulator_set_load() is the only > option since it provides aggregation, where as regulator_set_mode() does not. I lost the code context, but my general comment was that the modes that the hardware supports should come from the driver. If there's some sort of constraint on what those modes end up being because some board has an issue with some mode then we would want a binding that indicates such a mode needs to be avoided, instead of listing the ones that are supported. So default assume what registers support is there and then knock out things when board designs constrain it. That's all I'm saying. Maybe it's just "supported" that threw me off. > > >> +/** > >> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource > >> + * request for this regulator based on optional device tree > >> + * properties > >> + * @vreg: Pointer to the RPMh regulator > >> + * > >> + * Return: 0 on success, errno on failure > >> + */ > >> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg) > >> +{ > >> + struct tcs_cmd cmd[2] = { }; > >> + const char *prop; > >> + int cmd_count = 0; > >> + int ret; > >> + u32 temp; > >> + > >> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) { > >> + prop = "qcom,headroom-voltage"; > > > > Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh > > registers. This probably needs to be pushed into the framework and come > > down through a 'set_headroom' op in the regulator ops via a > > regulator-headroom-microvolt property that's parsed in of_regulator.c. > > The qcom,headroom-voltage property is equivalent to struct > regulator_desc.min_dropout_uV, but handled in hardware. I don't see the > need to make a new regulator op to configure this value dynamically. > Headroom typically does not need to change. Also, we don't really want > this particular value plumbed into min_dropout_uV since we need to pass it > directly to hardware and not have the regulator framework attempt to use > it for a parent. Ok? We have other regulator ops just to configure boot time things. The goal is to come up with generic regulator properties that can be applied from the framework perspective and be used by other regulator drivers in the future. > > > >> + prop = "qcom,regulator-initial-voltage"; > > > > DT constraints should take care of this by setting voltages on all > > regulators that need them? > > Unfortunately not. At regulator registration time, the regulator > framework machine_constraints_voltage() function will call get_voltage() > op and check if the voltage is in the min_uV to max_uV constraint range. > If it is, then nothing happens. If it's not, then it will call the > set_voltage() call back to set the voltage to min_uV if current_uV < > min_uV or max_uV if current_uV > max_uV. Since the rpmh regulator driver > doesn't know the initial voltage, get_voltage() will return 0 initially. > As a result, machine_constraints_voltage() will always set the regulator > to the minimum constraint voltage specified in DT. > > That behavior may not be acceptable for some regulators depending upon the > hardware state at kernel initialization. Therefore, we need a DT > mechanism to specify a single voltage to configure by default. Cool. This should be a generic regulator DT property that all regulators can use. We have the same problem on other RPM based regulator drivers where boot sends a bunch of voltages because we don't know what it is by default out of boot and we can't read it. > > > >> + /* Optional override for the default RPMh accelerator type */ > >> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type", > > > > Can this property have override in the name? And then because it is > > called override, perhaps it should come from the driver instead of DT > > because DT may need an override itself. > > Yes, I can add 'override' to the name of the property. I'm not following > your second sentence. We require the ability to specify that a given > regulator is using XOB instead of VRM at a board level. This means that > the override needs to happen in DT instead of the driver. See [1] for > more details. Ah ok. Can this be "discovered" through cmd-db by probing for VRM and then for XOB if there isn't a VRM one? Just wondering why we need to describe it in DT at all if cmd-db will have one or the other. > > > Also, is this currently being used? If not I'd prefer we drop this until we > > need it. > > It will be needed on a to-be-released Qualcomm Technologies, Inc. SoC that > is currently under development. I suppose that I can leave this property > out for the initial patch. However, we would still need to ensure that > the driver architecture allows for this property to be read in a later > patch and determine the regulator ops and whether or not to perform VRM > specific initialization. OK. > > > >> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) { > >> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV; > >> + init_data->constraints.apply_uV = 0; > >> + vreg->rdesc.n_voltages = 1; > >> + } > > > > What is this doing? Usually constraints aren't touched by the driver. > > For XOB managed regulators, we need to set fixed_uV to match the DT > constraint voltage and n_voltages = 1. This allows consumers > regulator_set_voltage() calls to succeed for such regulators. It works > the same as a fixed regulator. I think that apply_uV = 0 could be left out. > Wouldn't XOB regulators only have one voltage specified for min/max in DT already though? I seem to recall that's how we make set_voltage() in those cases work. Or it could be that drivers aren't supposed to call set_voltage() on single or fixed voltage regulators anyway, because constraints take care of it for them. > > >> + > >> + if (vreg->hw_data->mode_map) { > >> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE; > > > > Huh, I thought this was assigned by the framework. > > No, this is not set anywhere in the regulator framework. There isn't a DT > method to configure it. It seems that it could only be handled before > with board files. Other regulator drivers also configure it. Hmm ok. Would something be bad if we did support it through DT? I can't seem to recall the history here. > >> + > >> + ret = cmd_db_ready(); > >> + if (ret < 0) { > >> + if (ret != -EPROBE_DEFER) > >> + dev_err(dev, "Command DB not available, ret=%d\n", ret); > >> + return ret; > >> + } > > > > We should just make rpmh parent device call cmd_db_ready() so that these > > devices aren't even populated until then and so that cmd_db_ready() is > > only in one place. Lina? > > Let's see if Lina has qualms about this plan. Sounds like you're ok with it. > > >> + dev_dbg(dev, "successfully probed %d %s regulators\n", > >> + pmic->vreg_count, pmic->init_data->name); > > > > Doesn't the regulator framework already print a bunch of constraint > > stuff when regulators are registered? Seems sort of spammy even for > > debug mode. Plus it's the only reason for pmic::name right now. > > I think that the regulator framework will only print something if it has > to set the voltage in order to bring the regulator voltage into the > constraint range. I suppose that I can remove this message. Ah you're right. Still seems like it would be better to put some sort of debug print into the core on regulator registration if anything. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 18, 2018 at 10:55:49PM -0700, Stephen Boyd wrote: > I lost the code context, but my general comment was that the modes that > the hardware supports should come from the driver. If there's some sort > of constraint on what those modes end up being because some board has an > issue with some mode then we would want a binding that indicates such a No, that's *never* how regulator constraints work. We only change the hardware configuration if we have explicit permission from constraints to do so, otherwise everything is left as we found it. That way if your board catches fire or whatever it wasn't something that Linux initiated just from default behaviour. > > > Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh > > > registers. This probably needs to be pushed into the framework and come > > > down through a 'set_headroom' op in the regulator ops via a > > > regulator-headroom-microvolt property that's parsed in of_regulator.c. > > The qcom,headroom-voltage property is equivalent to struct > > regulator_desc.min_dropout_uV, but handled in hardware. I don't see the > > need to make a new regulator op to configure this value dynamically. > Ok? We have other regulator ops just to configure boot time things. The > goal is to come up with generic regulator properties that can be applied > from the framework perspective and be used by other regulator drivers in > the future. This doesn't sound like what the min_dropout_uV constraint is intended to handle - that's there for the regulator driver (not constraints) to indicate how much headroom the regulator needs in the supply voltage in order to provide regulation. It's not something the regulator uses, it's something that gets fed into voltage requests made on the supply of the regulator which I can't see that the hardware is going to be able to handle unaided. > Cool. This should be a generic regulator DT property that all regulators > can use. We have the same problem on other RPM based regulator drivers > where boot sends a bunch of voltages because we don't know what it is by > default out of boot and we can't read it. Ideally future versions of the RPM will have improved interfaces, there's a bunch of problems like this :( > > > > >> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) { > > >> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV; > > >> + init_data->constraints.apply_uV = 0; > > >> + vreg->rdesc.n_voltages = 1; > > >> + } > > > What is this doing? Usually constraints aren't touched by the driver. > > For XOB managed regulators, we need to set fixed_uV to match the DT > > constraint voltage and n_voltages = 1. This allows consumers > > regulator_set_voltage() calls to succeed for such regulators. It works > > the same as a fixed regulator. I think that apply_uV = 0 could be left out. > Wouldn't XOB regulators only have one voltage specified for min/max in > DT already though? I seem to recall that's how we make set_voltage() in > those cases work. Or it could be that drivers aren't supposed to call > set_voltage() on single or fixed voltage regulators anyway, because > constraints take care of it for them. Yes, constraints that specify a single voltage are done by setting min and max to the same value. fixed_uV is *only* for regulators that have a physically fixed voltage. > > >> + if (vreg->hw_data->mode_map) { > > >> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE; A driver must *NEVER* modify any constraints. > > > Huh, I thought this was assigned by the framework. > > No, this is not set anywhere in the regulator framework. There isn't a DT > > method to configure it. It seems that it could only be handled before > > with board files. Other regulator drivers also configure it. > Hmm ok. Would something be bad if we did support it through DT? I can't > seem to recall the history here. Yes, if someone wants to configure this through DT they should add support for configuring it using DT. The mode support in most regulators is not practically useful so nobody did that yet. Mostly the hardware does a much better job of adjusting modes on the fly for anything that's going on at runtime than software is ever likely to do so it's not worth it.
On 04/18/2018 10:55 PM, Stephen Boyd wrote: > Quoting David Collins (2018-03-22 18:30:06) >> On 03/21/2018 12:07 PM, Stephen Boyd wrote: >>> Quoting David Collins (2018-03-16 18:09:10) >>>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >>>> index 097f617..e0ecd0a 100644 >>>> --- a/drivers/regulator/Kconfig >>>> +++ b/drivers/regulator/Kconfig >>>> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM >>>> Qualcomm RPM as a module. The module will be named >>>> "qcom_rpm-regulator". >>>> >>>> +config REGULATOR_QCOM_RPMH >>>> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver" >>>> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST >>> >>> What's the build dependency on OF? >> >> The qcom_rpmh-regulator driver cannot function without device tree support >> enabled. I suppose that it might be able to compile, but it wouldn't be >> useful. > > If it isn't a build dependency then we usually leave it out because it's > not always useful to express runtime constraints in Kconfig. Probably > all we need is depends on QCOM_RPMH || COMPILE_TEST and then stubs take > care of it from there. Ok. I'll change this to: depends on QCOM_RPMH || COMPILE_TEST >>>> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator >>>> + * @name: Name for the regulator which also corresponds >>>> + * to the device tree subnode name of the regulator >>>> + * @resource_name_base: RPMh regulator resource name prefix. E.g. >>>> + * "ldo" for RPMh resource "ldoa1". >>> >>> Maybe it should be "ldo%c1"? Then we could kasprintf the name with the >>> pmic_id and drop the 'id' member entirely. >> >> I can make this modification (though with %s instead of %c for simplicity >> with DT string parsing). Hopefully having a variable format string >> doesn't trigger any static analysis tools. > > Oh it's variable how many digits in the 'id' number there are? I'll look > at v2. At the moment, the id field has only ever been a single character. However, that isn't set in stone. I was mainly interested in using %s instead of %c since it doesn't require any special handling of the string read directly from DT. >>>> + int vreg_count; >>>> + const char *pmic_id; >>>> + const struct rpmh_pmic_init_data *init_data; >>> >>> Hopefully we don't really need this entire struct and we can just use >>> local variables instead. >> >> Outside of probe-time, this is used by struct rpmh_vreg in order to access >> rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug >> and error messages). I suppose that rpmh_client could be specified in >> struct rpmh_vreg directly. >> > > Hopefully rpm_client goes away. Maybe it would just be dev pointer to > hold onto then and possibly rip the name of each regulator out of the > framework name for it? In the v2 patch set, I removed this top-level struct and moved rpmh_client into struct rpmh_vreg. Now that v6 of the rpmh series is out with rpmh_client removed and dev pointer in its place, I'll add the dev pointer to struct rpmh_vreg in my v3 patch set. >>>> +/** >>>> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations >>>> + * for a VRM RPMh resource from device tree >>>> + * vreg: Pointer to the rpmh regulator resource >>>> + * >>>> + * This function initializes the mode[] array of vreg based upon the values >>>> + * of optional device tree properties. >>>> + * >>>> + * Return: 0 on success, errno on failure >>>> + */ >>>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg) >>>> +{ >>> >>> I have a feeling this should all come from the driver data, not DT. >>> Doubtful this really changes for each board. >> >> This needs to be determined at a board level instead of hard-coded per >> regulator type. For LDOs switching between LPM and HPM typically happens >> at 10 mA or 30 mA per hardware documentation. Unfortunately, sharing >> control of regulators with other processors adds some subtlety. >> >> Consider the case of a regulator with 10 mA LPM/HPM threshold physically. >> Say that modem and application processors each have a load on the >> regulator that draws 9 mA. If they each respect the 10 mA threshold, then >> they'd each vote for LPM. VRM will aggregate these requests together >> which will result in the regulator being set to LPM even though the total >> load is 18 mA (which would require HPM). To get around this corner case, >> a threshold of 1 uA can be used for all supplies that have non-application >> processor consumers. Thus, any non-zero current request will result in >> setting the regulator to HPM (which is always safe). > > Sad. Sounds like the rpmh interface is broken and should be expressing > the load instead of the mode so that aggregation on the rpm side can > pick the right mode. So now we have a workaround by specifying some > default minimum value that we add in? RPMh hardware is not going to support load current aggregation. The aggregation method in hardware is to pick the highest mode control register value requested by all processors and send that to the PMIC. We are avoiding the LPM + LPM request issue on downstream targets by configuring the LPM vs HPM threshold current such that any load_uA total greater than 0 from Linux consumers results in a HPM request. >> Another example is SMPS regulators which should theoretically always be >> able to operate in AUTO mode. However, there may be board/system issues >> that require switching to PWM mode (HPM) for certain use cases as it has >> better load regulation (even though it can source the same amount of >> current as AUTO mode). If there is more than one consumer that requires >> this ability for a given regulator, then regulator_set_load() is the only >> option since it provides aggregation, where as regulator_set_mode() does not. > > I lost the code context, but my general comment was that the modes that > the hardware supports should come from the driver. If there's some sort > of constraint on what those modes end up being because some board has an > issue with some mode then we would want a binding that indicates such a > mode needs to be avoided, instead of listing the ones that are > supported. So default assume what registers support is there and then > knock out things when board designs constrain it. That's all I'm saying. > Maybe it's just "supported" that threw me off. As Mark explained, modes need to be explicitly allowed per board-specific configurations. >>>> +/** >>>> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource >>>> + * request for this regulator based on optional device tree >>>> + * properties >>>> + * @vreg: Pointer to the RPMh regulator >>>> + * >>>> + * Return: 0 on success, errno on failure >>>> + */ >>>> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg) >>>> +{ >>>> + struct tcs_cmd cmd[2] = { }; >>>> + const char *prop; >>>> + int cmd_count = 0; >>>> + int ret; >>>> + u32 temp; >>>> + >>>> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) { >>>> + prop = "qcom,headroom-voltage"; >>> >>> Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh >>> registers. This probably needs to be pushed into the framework and come >>> down through a 'set_headroom' op in the regulator ops via a >>> regulator-headroom-microvolt property that's parsed in of_regulator.c. >> >> The qcom,headroom-voltage property is equivalent to struct >> regulator_desc.min_dropout_uV, but handled in hardware. I don't see the >> need to make a new regulator op to configure this value dynamically. >> Headroom typically does not need to change. Also, we don't really want >> this particular value plumbed into min_dropout_uV since we need to pass it >> directly to hardware and not have the regulator framework attempt to use >> it for a parent. > > Ok? We have other regulator ops just to configure boot time things. The > goal is to come up with generic regulator properties that can be applied > from the framework perspective and be used by other regulator drivers in > the future. Hardware enforced headroom voltage is a feature very specific to RPMh. It is not generic. I don't see any need to add general support for this in the regulator framework. Software managed headroom voltage is already present in the framework. >>>> + prop = "qcom,regulator-initial-voltage"; >>> >>> DT constraints should take care of this by setting voltages on all >>> regulators that need them? >> >> Unfortunately not. At regulator registration time, the regulator >> framework machine_constraints_voltage() function will call get_voltage() >> op and check if the voltage is in the min_uV to max_uV constraint range. >> If it is, then nothing happens. If it's not, then it will call the >> set_voltage() call back to set the voltage to min_uV if current_uV < >> min_uV or max_uV if current_uV > max_uV. Since the rpmh regulator driver >> doesn't know the initial voltage, get_voltage() will return 0 initially. >> As a result, machine_constraints_voltage() will always set the regulator >> to the minimum constraint voltage specified in DT. >> >> That behavior may not be acceptable for some regulators depending upon the >> hardware state at kernel initialization. Therefore, we need a DT >> mechanism to specify a single voltage to configure by default. > > Cool. This should be a generic regulator DT property that all regulators > can use. We have the same problem on other RPM based regulator drivers > where boot sends a bunch of voltages because we don't know what it is by > default out of boot and we can't read it. I suppose that I can look into adding a generic regulator-initial-microvolt property if that is strongly preferred over an rpmh-regulator specific one. >>>> + /* Optional override for the default RPMh accelerator type */ >>>> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type", >>> >>> Can this property have override in the name? And then because it is >>> called override, perhaps it should come from the driver instead of DT >>> because DT may need an override itself. >> >> Yes, I can add 'override' to the name of the property. I'm not following >> your second sentence. We require the ability to specify that a given >> regulator is using XOB instead of VRM at a board level. This means that >> the override needs to happen in DT instead of the driver. See [1] for >> more details. > > Ah ok. Can this be "discovered" through cmd-db by probing for VRM and > then for XOB if there isn't a VRM one? Just wondering why we need to > describe it in DT at all if cmd-db will have one or the other. Unfortunately, this cannot be determined via command DB. Command DB will only show us the mapping from resource name string to RPMh address. The address does not distinguish VRM vs XOB. There are plans for adding command DB AUX data to specify VRM vs XOB in future chips, but this is not present for SDM845. >>>> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) { >>>> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV; >>>> + init_data->constraints.apply_uV = 0; >>>> + vreg->rdesc.n_voltages = 1; >>>> + } >>> >>> What is this doing? Usually constraints aren't touched by the driver. >> >> For XOB managed regulators, we need to set fixed_uV to match the DT >> constraint voltage and n_voltages = 1. This allows consumers >> regulator_set_voltage() calls to succeed for such regulators. It works >> the same as a fixed regulator. I think that apply_uV = 0 could be left out. >> > > Wouldn't XOB regulators only have one voltage specified for min/max in > DT already though? I seem to recall that's how we make set_voltage() in > those cases work. Or it could be that drivers aren't supposed to call > set_voltage() on single or fixed voltage regulators anyway, because > constraints take care of it for them. The XOB regulators will indeed have regulator-min-microvolt == regulator-max-microvolt in DT. However, this is not sufficient to ensure that consumer regulator_set_voltage() calls intersecting the constraint voltage return successfully. I have not specified any set_voltage() or get_voltage() callbacks for XOB regulators because it is physically not possible to configure their voltage. As a result, consumer regulator_set_voltage calls can only succeed if desc.fixed_uV is set and desc.n_voltages == 1. See [1] and [2]. Note that I removed init_data->constraints.apply_uV manipulation in the qcom_rpmh-regulator v2 patch set. Using the existing fixed_uV feature of the regulator framework seemed like a better option than open-coding a get_voltage() callback for XOB which returns a new struct rpmh_vreg field that stores the fixed XOB voltage. Do you agree with this approach? I'll add init_data->constraints.min_uV == init_data->constraints.max_uV to the above check as well. >>>> + >>>> + if (vreg->hw_data->mode_map) { >>>> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE; >>> >>> Huh, I thought this was assigned by the framework. >> >> No, this is not set anywhere in the regulator framework. There isn't a DT >> method to configure it. It seems that it could only be handled before >> with board files. Other regulator drivers also configure it. > > Hmm ok. Would something be bad if we did support it through DT? I can't > seem to recall the history here. I'll send out a patch to support this in of_get_regulation_constraints(). >>>> + ret = cmd_db_ready(); >>>> + if (ret < 0) { >>>> + if (ret != -EPROBE_DEFER) >>>> + dev_err(dev, "Command DB not available, ret=%d\n", ret); >>>> + return ret; >>>> + } >>> >>> We should just make rpmh parent device call cmd_db_ready() so that these >>> devices aren't even populated until then and so that cmd_db_ready() is >>> only in one place. Lina? >> >> Let's see if Lina has qualms about this plan. > > Sounds like you're ok with it. Sure, I'll remove this check if Lina agrees to add it in the rpmh driver. Thanks, David [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c?h=v4.17-rc1#n2940 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c?h=v4.17-rc1#n3319
On 04/19/2018 05:08 AM, Mark Brown wrote: > On Wed, Apr 18, 2018 at 10:55:49PM -0700, Stephen Boyd wrote: >>>> Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh >>>> registers. This probably needs to be pushed into the framework and come >>>> down through a 'set_headroom' op in the regulator ops via a >>>> regulator-headroom-microvolt property that's parsed in of_regulator.c. > >>> The qcom,headroom-voltage property is equivalent to struct >>> regulator_desc.min_dropout_uV, but handled in hardware. I don't see the >>> need to make a new regulator op to configure this value dynamically. > >> Ok? We have other regulator ops just to configure boot time things. The >> goal is to come up with generic regulator properties that can be applied >> from the framework perspective and be used by other regulator drivers in >> the future. > > This doesn't sound like what the min_dropout_uV constraint is intended > to handle - that's there for the regulator driver (not constraints) to > indicate how much headroom the regulator needs in the supply voltage in > order to provide regulation. It's not something the regulator uses, > it's something that gets fed into voltage requests made on the supply of > the regulator which I can't see that the hardware is going to be able to > handle unaided. RPMh hardware enforces the requested minimum headroom voltage for all regulators with a parent. It has full knowledge of the parent-child connections of regulators on the board (as programmed by the bootloader). It automatically reconfigures the parent voltage when needed as a result of requests changing the voltage of any of its child regulators. >> Cool. This should be a generic regulator DT property that all regulators >> can use. We have the same problem on other RPM based regulator drivers >> where boot sends a bunch of voltages because we don't know what it is by >> default out of boot and we can't read it. > > Ideally future versions of the RPM will have improved interfaces, > there's a bunch of problems like this :( Do you have a preference for qcom,regulator-initial-microvolt vs a generic framework supported regulator-initial-microvolt property for configuring a specific voltage at registration time? We'll need to have support for one or the other in order for the qcom_rpmh-regulator driver to be functional. >>>>> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) { >>>>> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV; >>>>> + init_data->constraints.apply_uV = 0; >>>>> + vreg->rdesc.n_voltages = 1; >>>>> + } > >>>> What is this doing? Usually constraints aren't touched by the driver. > >>> For XOB managed regulators, we need to set fixed_uV to match the DT >>> constraint voltage and n_voltages = 1. This allows consumers >>> regulator_set_voltage() calls to succeed for such regulators. It works >>> the same as a fixed regulator. I think that apply_uV = 0 could be left out. > >> Wouldn't XOB regulators only have one voltage specified for min/max in >> DT already though? I seem to recall that's how we make set_voltage() in >> those cases work. Or it could be that drivers aren't supposed to call >> set_voltage() on single or fixed voltage regulators anyway, because >> constraints take care of it for them. > > Yes, constraints that specify a single voltage are done by setting min > and max to the same value. fixed_uV is *only* for regulators that have > a physically fixed voltage. XOB managed regulators physically cannot change voltage. Therefore, do you agree that it is reasonable to use fixed_uV for them? Note that I removed init_data->constraints.apply_uV manipulation in version 2 of this patch. >>>>> + if (vreg->hw_data->mode_map) { >>>>> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE; > > A driver must *NEVER* modify any constraints. > >>>> Huh, I thought this was assigned by the framework. > >>> No, this is not set anywhere in the regulator framework. There isn't a DT >>> method to configure it. It seems that it could only be handled before >>> with board files. Other regulator drivers also configure it. > >> Hmm ok. Would something be bad if we did support it through DT? I can't >> seem to recall the history here. > > Yes, if someone wants to configure this through DT they should add > support for configuring it using DT. The mode support in most > regulators is not practically useful so nobody did that yet. Mostly the > hardware does a much better job of adjusting modes on the fly for > anything that's going on at runtime than software is ever likely to do > so it's not worth it. I'll send out a patch to add generic support for this configuration via device tree in the of_get_regulation_constraints() function. Thanks, David
On Fri, Apr 20 2018 at 13:07 -0600, David Collins wrote: >On 04/18/2018 10:55 PM, Stephen Boyd wrote: >> Quoting David Collins (2018-03-22 18:30:06) >>> On 03/21/2018 12:07 PM, Stephen Boyd wrote: >>>> Quoting David Collins (2018-03-16 18:09:10) >>>>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >>>>> index 097f617..e0ecd0a 100644 >>>>> + ret = cmd_db_ready(); >>>>> + if (ret < 0) { >>>>> + if (ret != -EPROBE_DEFER) >>>>> + dev_err(dev, "Command DB not available, ret=%d\n", ret); >>>>> + return ret; >>>>> + } >>>> >>>> We should just make rpmh parent device call cmd_db_ready() so that these >>>> devices aren't even populated until then and so that cmd_db_ready() is >>>> only in one place. Lina? >>> >>> Let's see if Lina has qualms about this plan. >> >> Sounds like you're ok with it. > >Sure, I'll remove this check if Lina agrees to add it in the rpmh driver. > We want to make the RSC nodes child of Command DB? That way we probe the controllers only if the command DB is ready? I could do that. Just so you know, there is are no strict directives to use Command DB. If a driver knows the information it needs to pass to the accelerator, it may choose to skip command DB completely. Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 20, 2018 at 12:28:21PM -0700, David Collins wrote: > On 04/19/2018 05:08 AM, Mark Brown wrote: > > This doesn't sound like what the min_dropout_uV constraint is intended > > to handle - that's there for the regulator driver (not constraints) to > > indicate how much headroom the regulator needs in the supply voltage in > > order to provide regulation. It's not something the regulator uses, > > it's something that gets fed into voltage requests made on the supply of > > the regulator which I can't see that the hardware is going to be able to > > handle unaided. > RPMh hardware enforces the requested minimum headroom voltage for all > regulators with a parent. It has full knowledge of the parent-child > connections of regulators on the board (as programmed by the bootloader). > It automatically reconfigures the parent voltage when needed as a result > of requests changing the voltage of any of its child regulators. If the hardware has full knowledge of all these constraints and enforces them transparently then why does the kernel care that it's doing that? Doesn't it defeat the point of it doing all this stuff if we have to know about it? > > Ideally future versions of the RPM will have improved interfaces, > > there's a bunch of problems like this :( > Do you have a preference for qcom,regulator-initial-microvolt vs a generic > framework supported regulator-initial-microvolt property for configuring a > specific voltage at registration time? We'll need to have support for one > or the other in order for the qcom_rpmh-regulator driver to be functional. This is basically specific to Qualcomm, I can't off hand think of any other devices with similar issues. > > Yes, constraints that specify a single voltage are done by setting min > > and max to the same value. fixed_uV is *only* for regulators that have > > a physically fixed voltage. > XOB managed regulators physically cannot change voltage. Therefore, do > you agree that it is reasonable to use fixed_uV for them? Note that I > removed init_data->constraints.apply_uV manipulation in version 2 of this > patch. If these regulators can't change voltage then surely we know what voltage they have without needing it to be specified in DT?
>>>>>> + ret = cmd_db_ready(); >>>>>> + if (ret < 0) { >>>>>> + if (ret != -EPROBE_DEFER) >>>>>> + dev_err(dev, "Command DB not available, >>>>>> ret=%d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>> >>>>> We should just make rpmh parent device call cmd_db_ready() so that these >>>>> devices aren't even populated until then and so that cmd_db_ready() is >>>>> only in one place. Lina? >>>> >>>> Let's see if Lina has qualms about this plan. >>> >>> Sounds like you're ok with it. >> >> Sure, I'll remove this check if Lina agrees to add it in the rpmh driver. >> > We want to make the RSC nodes child of Command DB? That way we probe the > controllers only if the command DB is ready? > > I could do that. Just so you know, there is are no strict directives to > use Command DB. If a driver knows the information it needs to pass to > the accelerator, it may choose to skip command DB completely. The ask here is for the rpmh driver to call cmd_db_ready() in its probe function and return any error encountered (e.g. -EPROBE_DEFER). I suppose that specifying the rpmh rsc device tree node as a child inside of the command DB node could achieve the same result. However, that seems a little hacky as the rsc device is describing a hardware block physically found on the SoC as opposed to a subcomponent of the command DB SMEM data structure. I defer to device tree maintainers to comment on the acceptability of this approach. While technically it is possible for an rpmh consumer to intrinsically know the addresses for its resources without polling command DB, do you have any examples where this is the case? Thanks, David
On 04/24/2018 10:41 AM, Mark Brown wrote: > On Fri, Apr 20, 2018 at 12:28:21PM -0700, David Collins wrote: >> RPMh hardware enforces the requested minimum headroom voltage for all >> regulators with a parent. It has full knowledge of the parent-child >> connections of regulators on the board (as programmed by the bootloader). >> It automatically reconfigures the parent voltage when needed as a result >> of requests changing the voltage of any of its child regulators. > > If the hardware has full knowledge of all these constraints and enforces > them transparently then why does the kernel care that it's doing that? > Doesn't it defeat the point of it doing all this stuff if we have to > know about it? The RPMh hardware is aware of the parent-child connections between regulators as well as minimum headroom to ensure stable LDO voltage output for subregulated LDOs. The intention of having the headroom be a configurable property for processors is to support usecases in which subregulated LDO loads are particularly sensitive to noise and require additional headroom. Such usecases are board dependent and beyond the baseline configurations set in RPMh hardware. >>> Ideally future versions of the RPM will have improved interfaces, >>> there's a bunch of problems like this :( > >> Do you have a preference for qcom,regulator-initial-microvolt vs a generic >> framework supported regulator-initial-microvolt property for configuring a >> specific voltage at registration time? We'll need to have support for one >> or the other in order for the qcom_rpmh-regulator driver to be functional. > > This is basically specific to Qualcomm, I can't off hand think of any > other devices with similar issues. I will go with qcom,regulator-initial-microvolt then. >>> Yes, constraints that specify a single voltage are done by setting min >>> and max to the same value. fixed_uV is *only* for regulators that have >>> a physically fixed voltage. > >> XOB managed regulators physically cannot change voltage. Therefore, do >> you agree that it is reasonable to use fixed_uV for them? Note that I >> removed init_data->constraints.apply_uV manipulation in version 2 of this >> patch. > > If these regulators can't change voltage then surely we know what > voltage they have without needing it to be specified in DT? In the case of XOB managed LDO regulators, the LDOs physically can be configured to different voltages by the bootloader. However, the RPMh interface provides no mechanism for the application processor to read or change that voltage. Therefore, we need a way to specify such voltages in a board specific (as opposed to driver specific) manner (i.e. device tree). Take care, David
On Tue, Apr 24, 2018 at 01:46:21PM -0700, David Collins wrote: > On 04/24/2018 10:41 AM, Mark Brown wrote: > > If the hardware has full knowledge of all these constraints and enforces > > them transparently then why does the kernel care that it's doing that? > > Doesn't it defeat the point of it doing all this stuff if we have to > > know about it? > The RPMh hardware is aware of the parent-child connections between > regulators as well as minimum headroom to ensure stable LDO voltage output > for subregulated LDOs. The intention of having the headroom be a > configurable property for processors is to support usecases in which > subregulated LDO loads are particularly sensitive to noise and require > additional headroom. Such usecases are board dependent and beyond the > baseline configurations set in RPMh hardware. So the hardware implementation is some hard coding stuff that doesn't really adequately reflect reality? This seems unfortunate. However do we really need to tell the hardware about the fact that we're adding extra headroom - are there actual interactions with non-Linux things here? > >> XOB managed regulators physically cannot change voltage. Therefore, do > >> you agree that it is reasonable to use fixed_uV for them? Note that I > >> removed init_data->constraints.apply_uV manipulation in version 2 of this > >> patch. > > If these regulators can't change voltage then surely we know what > > voltage they have without needing it to be specified in DT? > In the case of XOB managed LDO regulators, the LDOs physically can be > configured to different voltages by the bootloader. However, the RPMh > interface provides no mechanism for the application processor to read or > change that voltage. Therefore, we need a way to specify such voltages in > a board specific (as opposed to driver specific) manner (i.e. device tree). Is the kernel somehow prevented from varying these voltages?
On 05/16/2018 11:09 PM, Mark Brown wrote: > On Tue, Apr 24, 2018 at 01:46:21PM -0700, David Collins wrote: >> The RPMh hardware is aware of the parent-child connections between >> regulators as well as minimum headroom to ensure stable LDO voltage output >> for subregulated LDOs. The intention of having the headroom be a >> configurable property for processors is to support usecases in which >> subregulated LDO loads are particularly sensitive to noise and require >> additional headroom. Such usecases are board dependent and beyond the >> baseline configurations set in RPMh hardware. > > So the hardware implementation is some hard coding stuff that doesn't > really adequately reflect reality? This seems unfortunate. However do > we really need to tell the hardware about the fact that we're adding > extra headroom - are there actual interactions with non-Linux things > here? The RPMh hardware is configured by the boot loader. The configuration does reflect reality; however, it cannot handle all configurations at initialization time. Specific headroom management typically comes up in modem usecases for RF supplies that are sensitive to noise. This feature allows RPMh masters (application processor, modem processor, etc) to make requests only for the regulators that they directly care about without having to worry about power grid parent-child details and setting the voltage of parent regulators in order to ensure sufficient headroom. If you really don't like having this feature present in the Linux RPMh regulator driver, then I'd be ok removing it. It is not required for SDM845 which the driver is initially targeting. >>>> XOB managed regulators physically cannot change voltage. Therefore, do >>>> you agree that it is reasonable to use fixed_uV for them? Note that I >>>> removed init_data->constraints.apply_uV manipulation in version 2 of this >>>> patch. > >>> If these regulators can't change voltage then surely we know what >>> voltage they have without needing it to be specified in DT? > >> In the case of XOB managed LDO regulators, the LDOs physically can be >> configured to different voltages by the bootloader. However, the RPMh >> interface provides no mechanism for the application processor to read or >> change that voltage. Therefore, we need a way to specify such voltages in >> a board specific (as opposed to driver specific) manner (i.e. device tree). > > Is the kernel somehow prevented from varying these voltages? Yes. Physically, there exists no RPMh register to read or write the voltage of LDOs managed via XOB. Additionally, the kernel running on the application processor is blocked from configuring the voltage via a direct SPMI writes by access permissions that crash the system when violated. Take care, David
On Thu, May 17, 2018 at 01:48:41PM -0700, David Collins wrote: > The RPMh hardware is configured by the boot loader. The configuration > does reflect reality; however, it cannot handle all configurations at > initialization time. Specific headroom management typically comes up in > modem usecases for RF supplies that are sensitive to noise. This feature ... > If you really don't like having this feature present in the Linux RPMh > regulator driver, then I'd be ok removing it. It is not required for > SDM845 which the driver is initially targeting. It's certainly going to be easier to review it separately. > >> In the case of XOB managed LDO regulators, the LDOs physically can be > >> configured to different voltages by the bootloader. However, the RPMh > >> interface provides no mechanism for the application processor to read or > >> change that voltage. Therefore, we need a way to specify such voltages in > >> a board specific (as opposed to driver specific) manner (i.e. device tree). > > Is the kernel somehow prevented from varying these voltages? > Yes. Physically, there exists no RPMh register to read or write the > voltage of LDOs managed via XOB. Additionally, the kernel running on the > application processor is blocked from configuring the voltage via a direct > SPMI writes by access permissions that crash the system when violated. *sigh* Please provide feedback on the problems this (and everything else) is causing to the team working on the firmware. The number of issues with this interface compared to anything else we've seen is really noticable, I see what it's trying to do with providing something like the regulator API is doing but there's quite a lot of missing steps in it which cause no end of problems for general purpose software written on top of it.
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 097f617..e0ecd0a 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM Qualcomm RPM as a module. The module will be named "qcom_rpm-regulator". +config REGULATOR_QCOM_RPMH + tristate "Qualcomm Technologies, Inc. RPMh regulator driver" + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST + help + This driver supports control of PMIC regulators via the RPMh hardware + block found on Qualcomm Technologies Inc. SoCs. RPMh regulator + control allows for voting on regulator state between multiple + processors within the SoC. + config REGULATOR_QCOM_SMD_RPM tristate "Qualcomm SMD based RPM regulator driver" depends on QCOM_SMD_RPM diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 590674f..c2274dd 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -77,6 +77,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o +obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom_rpmh-regulator.o obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c new file mode 100644 index 0000000..808f949 --- /dev/null +++ b/drivers/regulator/qcom_rpmh-regulator.c @@ -0,0 +1,1124 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ + +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> +#include <linux/regulator/of_regulator.h> +#include <soc/qcom/cmd-db.h> +#include <soc/qcom/rpmh.h> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h> + +/** + * enum rpmh_regulator_type - supported RPMh accelerator types + * %RPMH_REGULATOR_TYPE_VRM: RPMh VRM accelerator which supports voting on + * enable, voltage, mode, and headroom voltage of + * LDO, SMPS, VS, and BOB type PMIC regulators. + * %RPMH_REGULATOR_TYPE_XOB: RPMh XOB accelerator which supports voting on + * the enable state of PMIC regulators. + */ +enum rpmh_regulator_type { + RPMH_REGULATOR_TYPE_VRM, + RPMH_REGULATOR_TYPE_XOB, +}; + +/* Min and max limits of VRM resource request parameters */ +#define RPMH_VRM_MIN_UV 0 +#define RPMH_VRM_MAX_UV 8191000 + +#define RPMH_VRM_HEADROOM_MIN_UV 0 +#define RPMH_VRM_HEADROOM_MAX_UV 511000 + +#define RPMH_VRM_MODE_MIN 0 +#define RPMH_VRM_MODE_MAX 7 + +/* Register offsets: */ +#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0 +#define RPMH_REGULATOR_REG_ENABLE 0x4 +#define RPMH_REGULATOR_REG_VRM_MODE 0x8 +#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC + +/* Enable register values: */ +#define RPMH_REGULATOR_DISABLE 0x0 +#define RPMH_REGULATOR_ENABLE 0x1 + +/* Number of unique hardware modes supported: */ +#define RPMH_REGULATOR_MODE_COUNT 5 + +/** + * struct rpmh_regulator_mode - RPMh VRM mode attributes + * @pmic_mode: Raw PMIC mode value written into VRM mode voting + * register (i.e. RPMH_REGULATOR_MODE_*) + * @framework_mode: Regulator framework mode value + * (i.e. REGULATOR_MODE_*) + * @min_load_ua: The minimum load current in microamps which + * would utilize this mode + * + * Software selects the lowest mode for which aggr_load_ua >= min_load_ua. + */ +struct rpmh_regulator_mode { + u32 pmic_mode; + u32 framework_mode; + int min_load_ua; +}; + +/** + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations + * @mode_map: Array of size RPMH_REGULATOR_MODE_COUNT which + * maps RPMH_REGULATOR_MODE_* indices into PMIC + * mode and regulator framework mode that are + * supported by this PMIC regulator type + * @voltage_range: The single range of voltages supported by this + * PMIC regulator type + * @n_voltages: The number of unique voltage set points defined + * by voltage_range + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined + * in device tree to a regulator framework mode + */ +struct rpmh_vreg_hw_data { + const struct rpmh_regulator_mode *mode_map; + const struct regulator_linear_range *voltage_range; + int n_voltages; + unsigned int (*of_map_mode)(unsigned int mode); +}; + +struct rpmh_pmic; + +/** + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a + * single regulator device + * @of_node: Device tree node pointer of the regulator + * @pmic: Pointer to the PMIC containing the regulator + * @resource_name: Name of the RPMh regulator resource which is + * mapped to an RPMh accelerator address via + * command DB. This name must match to one that is + * defined by the bootloader. + * @addr: Base address of the regulator resource within + * an RPMh accelerator + * @rdesc: Regulator descriptor + * @rdev: Regulator device pointer returned by + * devm_regulator_register() + * @hw_data: PMIC regulator configuration data for this RPMh + * regulator + * @regulator_type: RPMh accelerator type for this regulator + * resource + * @always_wait_for_ack: Boolean flag indicating if a request must always + * wait for an ACK from RPMh before continuing even + * if it corresponds to a strictly lower power + * state (e.g. enabled --> disabled). + * @mode_map: An array of modes which may be configured at + * runtime by setting the load current + * @mode_count: The number of entries in the mode_map array. + * @enabled: Boolean indicating if the regulator is enabled + * or not + * @voltage: RPMh VRM regulator voltage in microvolts + * @mode: RPMh VRM regulator current framework mode + * @headroom_voltage: RPMh VRM regulator minimum headroom voltage + * required + */ +struct rpmh_vreg { + struct device_node *of_node; + struct rpmh_pmic *pmic; + const char *resource_name; + u32 addr; + struct regulator_desc rdesc; + struct regulator_dev *rdev; + const struct rpmh_vreg_hw_data *hw_data; + enum rpmh_regulator_type regulator_type; + bool always_wait_for_ack; + struct rpmh_regulator_mode *mode_map; + int mode_count; + + bool enabled; + int voltage; + unsigned int mode; + int headroom_voltage; +}; + +/** + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator + * @name: Name for the regulator which also corresponds + * to the device tree subnode name of the regulator + * @resource_name_base: RPMh regulator resource name prefix. E.g. + * "ldo" for RPMh resource "ldoa1". + * @supply_name: Parent supply regulator name + * @id: Regulator number within the PMIC + * @regulator_type: RPMh accelerator type used to manage this + * regulator + * @hw_data: Configuration data for this PMIC regulator type + */ +struct rpmh_vreg_init_data { + const char *name; + const char *resource_name_base; + const char *supply_name; + int id; + enum rpmh_regulator_type regulator_type; + const struct rpmh_vreg_hw_data *hw_data; +}; + +/** + * struct rpmh_pmic_init_data - initialization data for a PMIC + * @name: PMIC name + * @vreg_data: Array of data for each regulator in the PMIC + * @count: Number of entries in vreg_data + */ +struct rpmh_pmic_init_data { + const char *name; + const struct rpmh_vreg_init_data *vreg_data; + int count; +}; + +/** + * struct rpmh_pmic - top level data structure of all regulators found on a PMIC + * @dev: Device pointer of the PMIC device for the + * regulators + * @rpmh_client: Handle used for rpmh communications + * @vreg: Array of rpmh regulator structs representing the + * individual regulators found on this PMIC chip + * which are configured via device tree. + * @vreg_count: The number of entries in the vreg array. + * @pmic_id: Letter used to identify this PMIC within the + * system. This is dictated by boot loader + * specifications on a given target. + * @init_data: Pointer to the matched PMIC initialization data + */ +struct rpmh_pmic { + struct device *dev; + struct rpmh_client *rpmh_client; + struct rpmh_vreg *vreg; + int vreg_count; + const char *pmic_id; + const struct rpmh_pmic_init_data *init_data; +}; + +#define vreg_err(vreg, message, ...) \ + pr_err("%s %s: " message, (vreg)->pmic->init_data->name, \ + (vreg)->rdesc.name, ##__VA_ARGS__) +#define vreg_info(vreg, message, ...) \ + pr_info("%s %s: " message, (vreg)->pmic->init_data->name, \ + (vreg)->rdesc.name, ##__VA_ARGS__) +#define vreg_debug(vreg, message, ...) \ + pr_debug("%s %s: " message, (vreg)->pmic->init_data->name, \ + (vreg)->rdesc.name, ##__VA_ARGS__) + +/** + * rpmh_regulator_send_request() - send the request to RPMh + * @vreg: Pointer to the RPMh regulator + * @cmd: RPMh commands to send + * @count: Size of cmd array + * @wait_for_ack: Boolean indicating if execution must wait until the + * request has been acknowledged as complete + * + * Return: 0 on success, errno on failure + */ +static int rpmh_regulator_send_request(struct rpmh_vreg *vreg, + struct tcs_cmd *cmd, int count, bool wait_for_ack) +{ + int ret; + + if (wait_for_ack || vreg->always_wait_for_ack) + ret = rpmh_write(vreg->pmic->rpmh_client, + RPMH_ACTIVE_ONLY_STATE, cmd, count); + else + ret = rpmh_write_async(vreg->pmic->rpmh_client, + RPMH_ACTIVE_ONLY_STATE, cmd, count); + if (ret < 0) + vreg_err(vreg, "rpmh_write() failed, ret=%d\n", ret); + + return ret; +} + +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + + return vreg->enabled; +} + +static int rpmh_regulator_enable(struct regulator_dev *rdev) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + struct tcs_cmd cmd = { + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, + .data = RPMH_REGULATOR_ENABLE, + }; + int ret; + + if (vreg->enabled) + return 0; + + ret = rpmh_regulator_send_request(vreg, &cmd, 1, true); + if (ret < 0) { + vreg_err(vreg, "enable failed, ret=%d\n", ret); + return ret; + } + + vreg->enabled = true; + + return 0; +} + +static int rpmh_regulator_disable(struct regulator_dev *rdev) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + struct tcs_cmd cmd = { + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE, + .data = RPMH_REGULATOR_DISABLE, + }; + int ret; + + if (!vreg->enabled) + return 0; + + ret = rpmh_regulator_send_request(vreg, &cmd, 1, false); + if (ret < 0) { + vreg_err(vreg, "disable failed, ret=%d\n", ret); + return ret; + } + + vreg->enabled = false; + + return 0; +} + +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev, + int min_uv, int max_uv, unsigned int *selector) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + struct tcs_cmd cmd = { + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE, + }; + const struct regulator_linear_range *range; + int mv, uv, ret; + bool wait_for_ack; + + mv = DIV_ROUND_UP(min_uv, 1000); + uv = mv * 1000; + if (uv > max_uv) { + vreg_err(vreg, "no set points available in range %d-%d uV\n", + min_uv, max_uv); + return -EINVAL; + } + + range = vreg->hw_data->voltage_range; + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step); + + if (uv == vreg->voltage) + return 0; + + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage; + cmd.data = mv; + + ret = rpmh_regulator_send_request(vreg, &cmd, 1, wait_for_ack); + if (ret < 0) { + vreg_err(vreg, "set voltage=%d uV failed, ret=%d\n", uv, ret); + return ret; + } + + vreg->voltage = uv; + + return 0; +} + +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + + return vreg->voltage; +} + +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev, + unsigned int mode) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + struct tcs_cmd cmd = { + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE, + }; + int i, ret; + + if (mode == vreg->mode) + return 0; + + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) + if (vreg->hw_data->mode_map[i].framework_mode == mode) + break; + if (i >= RPMH_REGULATOR_MODE_COUNT) { + vreg_err(vreg, "invalid mode=%u\n", mode); + return -EINVAL; + } + + cmd.data = vreg->hw_data->mode_map[i].pmic_mode; + + ret = rpmh_regulator_send_request(vreg, &cmd, 1, + mode < vreg->mode || !vreg->mode); + if (ret < 0) { + vreg_err(vreg, "set mode=%u failed, ret=%d\n", cmd.data, ret); + return ret; + } + + vreg->mode = mode; + + return ret; +} + +static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + + return vreg->mode; +} + +/** + * rpmh_regulator_vrm_set_load() - set the PMIC mode based upon the maximum load + * required from the VRM rpmh-regulator + * @rdev: Regulator device pointer for the rpmh-regulator + * @load_ua: Maximum current required from all consumers in microamps + * + * This function is passed as a callback function into the regulator ops that + * are registered for each VRM rpmh-regulator device. + * + * This function sets the mode of the regulator to that which has the highest + * min support load less than or equal to load_ua. Example: + * mode_count = 3 + * mode_map[].min_load_ua = 0, 100000, 6000000 + * + * load_ua = 10000 --> i = 0 + * load_ua = 250000 --> i = 1 + * load_ua = 7000000 --> i = 2 + * + * Return: 0 on success, errno on failure + */ +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_ua) +{ + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); + int i; + + /* No need to check element 0 as it will be the default. */ + for (i = vreg->mode_count - 1; i > 0; i--) + if (vreg->mode_map[i].min_load_ua <= load_ua) + break; + + return rpmh_regulator_vrm_set_mode(rdev, + vreg->mode_map[i].framework_mode); +} + +static const struct regulator_ops rpmh_regulator_vrm_ops = { + .enable = rpmh_regulator_enable, + .disable = rpmh_regulator_disable, + .is_enabled = rpmh_regulator_is_enabled, + .set_voltage = rpmh_regulator_vrm_set_voltage, + .get_voltage = rpmh_regulator_vrm_get_voltage, + .list_voltage = regulator_list_voltage_linear_range, + .set_mode = rpmh_regulator_vrm_set_mode, + .get_mode = rpmh_regulator_vrm_get_mode, + .set_load = rpmh_regulator_vrm_set_load, +}; + +static const struct regulator_ops rpmh_regulator_xob_ops = { + .enable = rpmh_regulator_enable, + .disable = rpmh_regulator_disable, + .is_enabled = rpmh_regulator_is_enabled, +}; + +static const struct regulator_ops *rpmh_regulator_ops[] = { + [RPMH_REGULATOR_TYPE_VRM] = &rpmh_regulator_vrm_ops, + [RPMH_REGULATOR_TYPE_XOB] = &rpmh_regulator_xob_ops, +}; + +/** + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations + * for a VRM RPMh resource from device tree + * vreg: Pointer to the rpmh regulator resource + * + * This function initializes the mode[] array of vreg based upon the values + * of optional device tree properties. + * + * Return: 0 on success, errno on failure + */ +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg) +{ + struct device_node *node = vreg->of_node; + const struct rpmh_regulator_mode *map; + const char *prop; + int i, len, ret; + u32 *buf; + + map = vreg->hw_data->mode_map; + if (!map) + return 0; + + /* qcom,allowed-modes is optional */ + prop = "qcom,allowed-modes"; + len = of_property_count_elems_of_size(node, prop, sizeof(u32)); + if (len < 0) + return 0; + + vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len, + sizeof(*vreg->mode_map), GFP_KERNEL); + if (!vreg->mode_map) + return -ENOMEM; + vreg->mode_count = len; + + buf = kcalloc(len, sizeof(*buf), GFP_KERNEL); + if (!buf) + return -ENOMEM; + + ret = of_property_read_u32_array(node, prop, buf, len); + if (ret < 0) { + vreg_err(vreg, "unable to read %s, ret=%d\n", + prop, ret); + goto done; + } + + for (i = 0; i < len; i++) { + if (buf[i] >= RPMH_REGULATOR_MODE_COUNT + || !map[buf[i]].framework_mode) { + vreg_err(vreg, "element %d of %s = %u is invalid for this regulator\n", + i, prop, buf[i]); + ret = -EINVAL; + goto done; + } + + vreg->mode_map[i].pmic_mode = map[buf[i]].pmic_mode; + vreg->mode_map[i].framework_mode = map[buf[i]].framework_mode; + + if (i > 0 && vreg->mode_map[i].pmic_mode + <= vreg->mode_map[i - 1].pmic_mode) { + vreg_err(vreg, "%s elements are not in ascending order\n", + prop); + ret = -EINVAL; + goto done; + } + } + + prop = "qcom,mode-threshold-currents"; + ret = of_property_read_u32_array(node, prop, buf, len); + if (ret < 0) { + vreg_err(vreg, "unable to read %s, ret=%d\n", + prop, ret); + goto done; + } + + for (i = 0; i < len; i++) { + vreg->mode_map[i].min_load_ua = buf[i]; + + if (i > 0 && vreg->mode_map[i].min_load_ua + <= vreg->mode_map[i - 1].min_load_ua) { + vreg_err(vreg, "%s elements are not in ascending order\n", + prop); + ret = -EINVAL; + goto done; + } + } + +done: + kfree(buf); + return ret; +} + +/** + * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated + * with the PMIC and initialize important pointers for each + * regulator + * @pmic: Pointer to the RPMh regulator PMIC + * + * Return: 0 on success, errno on failure + */ +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic) +{ + struct device_node *node; + int i; + + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node); + if (pmic->vreg_count == 0) { + dev_err(pmic->dev, "could not find any regulator subnodes\n"); + return -ENODEV; + } + + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count, + sizeof(*pmic->vreg), GFP_KERNEL); + if (!pmic->vreg) + return -ENOMEM; + + i = 0; + for_each_available_child_of_node(pmic->dev->of_node, node) { + pmic->vreg[i].of_node = node; + pmic->vreg[i].pmic = pmic; + + i++; + } + + return 0; +} + +/** + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource + * request for this regulator based on optional device tree + * properties + * @vreg: Pointer to the RPMh regulator + * + * Return: 0 on success, errno on failure + */ +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg) +{ + struct tcs_cmd cmd[2] = { }; + const char *prop; + int cmd_count = 0; + int ret; + u32 temp; + + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) { + prop = "qcom,headroom-voltage"; + ret = of_property_read_u32(vreg->of_node, prop, &temp); + if (!ret) { + if (temp < RPMH_VRM_HEADROOM_MIN_UV || + temp > RPMH_VRM_HEADROOM_MAX_UV) { + vreg_err(vreg, "%s=%u is invalid\n", + prop, temp); + return -EINVAL; + } + vreg->headroom_voltage = temp; + + cmd[cmd_count].addr + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM; + cmd[cmd_count++].data + = DIV_ROUND_UP(vreg->headroom_voltage, 1000); + } + + prop = "qcom,regulator-initial-voltage"; + ret = of_property_read_u32(vreg->of_node, prop, &temp); + if (!ret) { + if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) { + vreg_err(vreg, "%s=%u is invalid\n", + prop, temp); + return -EINVAL; + } + vreg->voltage = temp; + + cmd[cmd_count].addr + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE; + cmd[cmd_count++].data + = DIV_ROUND_UP(vreg->voltage, 1000); + } + } + + if (cmd_count) { + ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true); + if (ret < 0) { + vreg_err(vreg, "could not send default config, ret=%d\n", + ret); + return ret; + } + } + + return 0; +} + +/** + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator + * @vreg: Pointer to the RPMh regulator + * + * Return: 0 on success, errno on failure + */ +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg) +{ + struct device *dev = vreg->pmic->dev; + struct regulator_config reg_config = {}; + const struct rpmh_vreg_init_data *rpmh_data = NULL; + const char *type_name = NULL; + enum rpmh_regulator_type type; + struct regulator_init_data *init_data; + int ret, i; + + for (i = 0; i < vreg->pmic->init_data->count; i++) { + if (!strcmp(vreg->pmic->init_data->vreg_data[i].name, + vreg->of_node->name)) { + rpmh_data = &vreg->pmic->init_data->vreg_data[i]; + break; + } + } + + if (!rpmh_data) { + dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n", + vreg->of_node->name, vreg->pmic->init_data->name); + return -EINVAL; + } + + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d", + rpmh_data->resource_name_base, vreg->pmic->pmic_id, + rpmh_data->id); + if (!vreg->resource_name) + return -ENOMEM; + + vreg->addr = cmd_db_read_addr(vreg->resource_name); + if (!vreg->addr) { + vreg_err(vreg, "could not find RPMh address for resource %s\n", + vreg->resource_name); + return -ENODEV; + } + + vreg->rdesc.name = rpmh_data->name; + vreg->rdesc.supply_name = rpmh_data->supply_name; + vreg->regulator_type = rpmh_data->regulator_type; + vreg->hw_data = rpmh_data->hw_data; + + if (rpmh_data->hw_data->voltage_range) { + vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range; + vreg->rdesc.n_linear_ranges = 1; + vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages; + } + + /* Optional override for the default RPMh accelerator type */ + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type", + &type_name); + if (!ret) { + if (!strcmp("vrm", type_name)) { + vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM; + } else if (!strcmp("xob", type_name)) { + vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB; + } else { + vreg_err(vreg, "Unknown RPMh accelerator type %s\n", + type_name); + return -EINVAL; + } + } + + type = vreg->regulator_type; + + if (type == RPMH_REGULATOR_TYPE_VRM) { + ret = rpmh_regulator_parse_vrm_modes(vreg); + if (ret < 0) { + vreg_err(vreg, "could not parse vrm mode mapping, ret=%d\n", + ret); + return ret; + } + } + + vreg->always_wait_for_ack = of_property_read_bool(vreg->of_node, + "qcom,always-wait-for-ack"); + + vreg->rdesc.owner = THIS_MODULE; + vreg->rdesc.type = REGULATOR_VOLTAGE; + vreg->rdesc.ops = rpmh_regulator_ops[type]; + vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode; + + init_data = of_get_regulator_init_data(dev, vreg->of_node, + &vreg->rdesc); + if (!init_data) + return -ENOMEM; + + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) { + vreg->rdesc.fixed_uV = init_data->constraints.min_uV; + init_data->constraints.apply_uV = 0; + vreg->rdesc.n_voltages = 1; + } + + if (vreg->hw_data->mode_map) { + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE; + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++) + init_data->constraints.valid_modes_mask + |= vreg->hw_data->mode_map[i].framework_mode; + } + + reg_config.dev = dev; + reg_config.init_data = init_data; + reg_config.of_node = vreg->of_node; + reg_config.driver_data = vreg; + + ret = rpmh_regulator_load_default_parameters(vreg); + if (ret < 0) { + vreg_err(vreg, "unable to load default parameters, ret=%d\n", + ret); + return ret; + } + + vreg->rdev = devm_regulator_register(dev, &vreg->rdesc, ®_config); + if (IS_ERR(vreg->rdev)) { + ret = PTR_ERR(vreg->rdev); + vreg->rdev = NULL; + vreg_err(vreg, "devm_regulator_register() failed, ret=%d\n", + ret); + return ret; + } + + vreg_debug(vreg, "registered RPMh resource %s @ 0x%05X\n", + vreg->resource_name, vreg->addr); + + return ret; +} + +/* + * Mappings from RPMh generic modes to VRM accelerator modes and regulator + * framework modes for each regulator type. + */ +static const struct rpmh_regulator_mode +rpmh_regulator_mode_map_pmic4_ldo[RPMH_REGULATOR_MODE_COUNT] = { + [RPMH_REGULATOR_MODE_RET] = { + .pmic_mode = 4, + .framework_mode = REGULATOR_MODE_STANDBY, + }, + [RPMH_REGULATOR_MODE_LPM] = { + .pmic_mode = 5, + .framework_mode = REGULATOR_MODE_IDLE, + }, + [RPMH_REGULATOR_MODE_HPM] = { + .pmic_mode = 7, + .framework_mode = REGULATOR_MODE_FAST, + }, +}; + +static const struct rpmh_regulator_mode +rpmh_regulator_mode_map_pmic4_smps[RPMH_REGULATOR_MODE_COUNT] = { + [RPMH_REGULATOR_MODE_RET] = { + .pmic_mode = 4, + .framework_mode = REGULATOR_MODE_STANDBY, + }, + [RPMH_REGULATOR_MODE_LPM] = { + .pmic_mode = 5, + .framework_mode = REGULATOR_MODE_IDLE, + }, + [RPMH_REGULATOR_MODE_AUTO] = { + .pmic_mode = 6, + .framework_mode = REGULATOR_MODE_NORMAL, + }, + [RPMH_REGULATOR_MODE_HPM] = { + .pmic_mode = 7, + .framework_mode = REGULATOR_MODE_FAST, + }, +}; + +static const struct rpmh_regulator_mode +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = { + [RPMH_REGULATOR_MODE_PASS] = { + .pmic_mode = 0, + .framework_mode = REGULATOR_MODE_STANDBY, + }, + [RPMH_REGULATOR_MODE_LPM] = { + .pmic_mode = 1, + .framework_mode = REGULATOR_MODE_IDLE, + }, + [RPMH_REGULATOR_MODE_AUTO] = { + .pmic_mode = 2, + .framework_mode = REGULATOR_MODE_NORMAL, + }, + [RPMH_REGULATOR_MODE_HPM] = { + .pmic_mode = 3, + .framework_mode = REGULATOR_MODE_FAST, + }, +}; + +static unsigned int rpmh_regulator_vrm_of_map_mode(unsigned int mode, + const struct rpmh_regulator_mode *mode_map) +{ + if (mode > RPMH_REGULATOR_MODE_COUNT) + return -EINVAL; + else if (mode_map[mode].framework_mode == 0) + return -EINVAL; + + return mode_map[mode].framework_mode; +} + +static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode) +{ + return rpmh_regulator_vrm_of_map_mode(mode, + rpmh_regulator_mode_map_pmic4_ldo); +} + +static unsigned int rpmh_regulator_pmic4_smps_of_map_mode(unsigned int mode) +{ + return rpmh_regulator_vrm_of_map_mode(mode, + rpmh_regulator_mode_map_pmic4_smps); +} + +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode) +{ + return rpmh_regulator_vrm_of_map_mode(mode, + rpmh_regulator_mode_map_pmic4_bob); +} + +static const struct rpmh_vreg_hw_data pmic4_pldo_hw_data = { + .voltage_range = &(const struct regulator_linear_range) + REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000), + .n_voltages = 256, + .mode_map = rpmh_regulator_mode_map_pmic4_ldo, + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode, +}; + +static const struct rpmh_vreg_hw_data pmic4_pldo_lv_hw_data = { + .voltage_range = &(const struct regulator_linear_range) + REGULATOR_LINEAR_RANGE(1256000, 0, 127, 8000), + .n_voltages = 128, + .mode_map = rpmh_regulator_mode_map_pmic4_ldo, + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode, +}; + +static const struct rpmh_vreg_hw_data pmic4_nldo_hw_data = { + .voltage_range = &(const struct regulator_linear_range) + REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000), + .n_voltages = 128, + .mode_map = rpmh_regulator_mode_map_pmic4_ldo, + .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode, +}; + +static const struct rpmh_vreg_hw_data pmic4_hfsmps3_hw_data = { + .voltage_range = &(const struct regulator_linear_range) + REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000), + .n_voltages = 216, + .mode_map = rpmh_regulator_mode_map_pmic4_smps, + .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode, +}; + +static const struct rpmh_vreg_hw_data pmic4_ftsmps426_hw_data = { + .voltage_range = &(const struct regulator_linear_range) + REGULATOR_LINEAR_RANGE(320000, 0, 258, 4000), + .n_voltages = 259, + .mode_map = rpmh_regulator_mode_map_pmic4_smps, + .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode, +}; + +static const struct rpmh_vreg_hw_data pmic4_bob_hw_data = { + .voltage_range = &(const struct regulator_linear_range) + REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000), + .n_voltages = 84, + .mode_map = rpmh_regulator_mode_map_pmic4_bob, + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode, +}; + +static const struct rpmh_vreg_hw_data pmic4_lvs_hw_data = { + /* LVS hardware does not support voltage or mode configuration. */ +}; + +#define RPMH_VREG(_name, _hw_type, _type, _base_name, _id, _supply_name) \ +{ \ + .name = #_name, \ + .hw_data = &_hw_type##_hw_data, \ + .regulator_type = RPMH_REGULATOR_TYPE_##_type, \ + .resource_name_base = #_base_name, \ + .id = _id, \ + .supply_name = #_supply_name, \ +} + +static const struct rpmh_vreg_init_data pm8998_vreg_data[] = { + RPMH_VREG(smps1, pmic4_ftsmps426, VRM, smp, 1, vdd_s1), + RPMH_VREG(smps2, pmic4_ftsmps426, VRM, smp, 2, vdd_s2), + RPMH_VREG(smps3, pmic4_hfsmps3, VRM, smp, 3, vdd_s3), + RPMH_VREG(smps4, pmic4_hfsmps3, VRM, smp, 4, vdd_s4), + RPMH_VREG(smps5, pmic4_hfsmps3, VRM, smp, 5, vdd_s5), + RPMH_VREG(smps6, pmic4_ftsmps426, VRM, smp, 6, vdd_s6), + RPMH_VREG(smps7, pmic4_ftsmps426, VRM, smp, 7, vdd_s7), + RPMH_VREG(smps8, pmic4_ftsmps426, VRM, smp, 8, vdd_s8), + RPMH_VREG(smps9, pmic4_ftsmps426, VRM, smp, 9, vdd_s9), + RPMH_VREG(smps10, pmic4_ftsmps426, VRM, smp, 10, vdd_s10), + RPMH_VREG(smps11, pmic4_ftsmps426, VRM, smp, 11, vdd_s11), + RPMH_VREG(smps12, pmic4_ftsmps426, VRM, smp, 12, vdd_s12), + RPMH_VREG(smps13, pmic4_ftsmps426, VRM, smp, 13, vdd_s13), + RPMH_VREG(ldo1, pmic4_nldo, VRM, ldo, 1, vdd_l1_l27), + RPMH_VREG(ldo2, pmic4_nldo, VRM, ldo, 2, vdd_l2_l8_l17), + RPMH_VREG(ldo3, pmic4_nldo, VRM, ldo, 3, vdd_l3_l11), + RPMH_VREG(ldo4, pmic4_nldo, VRM, ldo, 4, vdd_l4_l5), + RPMH_VREG(ldo5, pmic4_nldo, VRM, ldo, 5, vdd_l4_l5), + RPMH_VREG(ldo6, pmic4_pldo, VRM, ldo, 6, vdd_l6), + RPMH_VREG(ldo7, pmic4_pldo_lv, VRM, ldo, 7, vdd_l7_l12_l14_l15), + RPMH_VREG(ldo8, pmic4_nldo, VRM, ldo, 8, vdd_l2_l8_l17), + RPMH_VREG(ldo9, pmic4_pldo, VRM, ldo, 9, vdd_l9), + RPMH_VREG(ldo10, pmic4_pldo, VRM, ldo, 10, vdd_l10_l23_l25), + RPMH_VREG(ldo11, pmic4_nldo, VRM, ldo, 11, vdd_l3_l11), + RPMH_VREG(ldo12, pmic4_pldo_lv, VRM, ldo, 12, vdd_l7_l12_l14_l15), + RPMH_VREG(ldo13, pmic4_pldo, VRM, ldo, 13, vdd_l13_l19_l21), + RPMH_VREG(ldo14, pmic4_pldo_lv, VRM, ldo, 14, vdd_l7_l12_l14_l15), + RPMH_VREG(ldo15, pmic4_pldo_lv, VRM, ldo, 15, vdd_l7_l12_l14_l15), + RPMH_VREG(ldo16, pmic4_pldo, VRM, ldo, 16, vdd_l16_l28), + RPMH_VREG(ldo17, pmic4_nldo, VRM, ldo, 17, vdd_l2_l8_l17), + RPMH_VREG(ldo18, pmic4_pldo, VRM, ldo, 18, vdd_l18_l22), + RPMH_VREG(ldo19, pmic4_pldo, VRM, ldo, 19, vdd_l13_l19_l21), + RPMH_VREG(ldo20, pmic4_pldo, VRM, ldo, 20, vdd_l20_l24), + RPMH_VREG(ldo21, pmic4_pldo, VRM, ldo, 21, vdd_l13_l19_l21), + RPMH_VREG(ldo22, pmic4_pldo, VRM, ldo, 22, vdd_l18_l22), + RPMH_VREG(ldo23, pmic4_pldo, VRM, ldo, 23, vdd_l10_l23_l25), + RPMH_VREG(ldo24, pmic4_pldo, VRM, ldo, 24, vdd_l20_l24), + RPMH_VREG(ldo25, pmic4_pldo, VRM, ldo, 25, vdd_l10_l23_l25), + RPMH_VREG(ldo26, pmic4_nldo, VRM, ldo, 26, vdd_l26), + RPMH_VREG(ldo27, pmic4_nldo, VRM, ldo, 27, vdd_l1_l27), + RPMH_VREG(ldo28, pmic4_pldo, VRM, ldo, 28, vdd_l16_l28), + RPMH_VREG(lvs1, pmic4_lvs, XOB, vs, 1, vdd_lvs1_lvs2), + RPMH_VREG(lvs2, pmic4_lvs, XOB, vs, 2, vdd_lvs1_lvs2), +}; + +static const struct rpmh_vreg_init_data pmi8998_vreg_data[] = { + RPMH_VREG(bob, pmic4_bob, VRM, bob, 1, vdd_bob), +}; + +static const struct rpmh_vreg_init_data pm8005_vreg_data[] = { + RPMH_VREG(smps1, pmic4_ftsmps426, VRM, smp, 1, vdd_s1), + RPMH_VREG(smps2, pmic4_ftsmps426, VRM, smp, 2, vdd_s2), + RPMH_VREG(smps3, pmic4_ftsmps426, VRM, smp, 3, vdd_s3), + RPMH_VREG(smps4, pmic4_ftsmps426, VRM, smp, 4, vdd_s4), +}; + +static const struct rpmh_pmic_init_data pm8998_pmic_data = { + .name = "PM8998", + .vreg_data = pm8998_vreg_data, + .count = ARRAY_SIZE(pm8998_vreg_data), +}; + +static const struct rpmh_pmic_init_data pmi8998_pmic_data = { + .name = "PMI8998", + .vreg_data = pmi8998_vreg_data, + .count = ARRAY_SIZE(pmi8998_vreg_data), +}; + +static const struct rpmh_pmic_init_data pm8005_pmic_data = { + .name = "PM8005", + .vreg_data = pm8005_vreg_data, + .count = ARRAY_SIZE(pm8005_vreg_data), +}; + +static const struct of_device_id rpmh_regulator_match_table[] = { + { + .compatible = "qcom,pm8998-rpmh-regulators", + .data = &pm8998_pmic_data, + }, + { + .compatible = "qcom,pmi8998-rpmh-regulators", + .data = &pmi8998_pmic_data, + }, + { + .compatible = "qcom,pm8005-rpmh-regulators", + .data = &pm8005_pmic_data, + }, + {} +}; +MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table); + +/** + * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each + * of the regulator nodes associated with it + * @pdev: Pointer to the platform device of the RPMh PMIC + * + * Return: 0 on success, errno on failure + */ +static int rpmh_regulator_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + const struct of_device_id *match; + struct rpmh_pmic *pmic; + struct device_node *node; + int ret, i; + + node = dev->of_node; + + if (!node) { + dev_err(dev, "Device tree node is missing\n"); + return -EINVAL; + } + + ret = cmd_db_ready(); + if (ret < 0) { + if (ret != -EPROBE_DEFER) + dev_err(dev, "Command DB not available, ret=%d\n", ret); + return ret; + } + + pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL); + if (!pmic) + return -ENOMEM; + + pmic->dev = dev; + platform_set_drvdata(pdev, pmic); + + pmic->rpmh_client = rpmh_get_client(pdev); + if (IS_ERR(pmic->rpmh_client)) { + ret = PTR_ERR(pmic->rpmh_client); + if (ret != -EPROBE_DEFER) + dev_err(dev, "failed to request RPMh client, ret=%d\n", + ret); + return ret; + } + + match = of_match_node(rpmh_regulator_match_table, node); + if (match) { + pmic->init_data = match->data; + } else { + dev_err(dev, "could not find compatible string match\n"); + ret = -ENODEV; + goto cleanup; + } + + ret = of_property_read_string(node, "qcom,pmic-id", + &pmic->pmic_id); + if (ret < 0) { + dev_err(dev, "qcom,pmic-id missing in DT node\n"); + goto cleanup; + } + + ret = rpmh_regulator_allocate_vreg(pmic); + if (ret < 0) { + dev_err(dev, "failed to allocate regulator subnode array, ret=%d\n", + ret); + goto cleanup; + } + + for (i = 0; i < pmic->vreg_count; i++) { + ret = rpmh_regulator_init_vreg(&pmic->vreg[i]); + if (ret < 0) { + dev_err(dev, "unable to initialize rpmh-regulator vreg %s, ret=%d\n", + pmic->vreg[i].of_node->name, ret); + goto cleanup; + } + } + + dev_dbg(dev, "successfully probed %d %s regulators\n", + pmic->vreg_count, pmic->init_data->name); + + return ret; + +cleanup: + rpmh_release(pmic->rpmh_client); + + return ret; +} + +static int rpmh_regulator_remove(struct platform_device *pdev) +{ + struct rpmh_pmic *pmic = platform_get_drvdata(pdev); + + rpmh_release(pmic->rpmh_client); + + return 0; +} + +static struct platform_driver rpmh_regulator_driver = { + .driver = { + .name = "qcom-rpmh-regulator", + .of_match_table = rpmh_regulator_match_table, + .owner = THIS_MODULE, + }, + .probe = rpmh_regulator_probe, + .remove = rpmh_regulator_remove, +}; + +static int rpmh_regulator_init(void) +{ + return platform_driver_register(&rpmh_regulator_driver); +} + +static void rpmh_regulator_exit(void) +{ + platform_driver_unregister(&rpmh_regulator_driver); +} + +arch_initcall(rpmh_regulator_init); +module_exit(rpmh_regulator_exit); + +MODULE_DESCRIPTION("Qualcomm RPMh regulator driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h new file mode 100644 index 0000000..f854e0e --- /dev/null +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h @@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ + +#ifndef __QCOM_RPMH_REGULATOR_H +#define __QCOM_RPMH_REGULATOR_H + +/* + * These mode constants may be used for qcom,allowed-modes and qcom,init-mode + * properties of an RPMh resource. Each type of regulator supports a subset of + * the possible modes. + * + * %RPMH_REGULATOR_MODE_PASS: Pass-through mode in which output is directly + * tied to input. This mode is only supported by + * BOB type regulators. + * %RPMH_REGULATOR_MODE_RET: Retention mode in which only an extremely small + * load current is allowed. This mode is supported + * by LDO and SMPS type regulators. + * %RPMH_REGULATOR_MODE_LPM: Low power mode in which a small load current is + * allowed. This mode corresponds to PFM for SMPS + * and BOB type regulators. This mode is supported + * by LDO, HFSMPS, BOB, and PMIC4 FTSMPS type + * regulators. + * %RPMH_REGULATOR_MODE_AUTO: Auto mode in which the regulator hardware + * automatically switches between LPM and HPM based + * upon the real-time load current. This mode is + * supported by HFSMPS, BOB, and PMIC4 FTSMPS type + * regulators. + * %RPMH_REGULATOR_MODE_HPM: High power mode in which the full rated current + * of the regulator is allowed. This mode + * corresponds to PWM for SMPS and BOB type + * regulators. This mode is supported by all types + * of regulators. + */ +#define RPMH_REGULATOR_MODE_PASS 0 +#define RPMH_REGULATOR_MODE_RET 1 +#define RPMH_REGULATOR_MODE_LPM 2 +#define RPMH_REGULATOR_MODE_AUTO 3 +#define RPMH_REGULATOR_MODE_HPM 4 + +#endif
Add the QCOM RPMh regulator driver to manage PMIC regulators which are controlled via RPMh on some Qualcomm Technologies, Inc. SoCs. RPMh is a hardware block which contains several accelerators which are used to manage various hardware resources that are shared between the processors of the SoC. The final hardware state of a regulator is determined within RPMh by performing max aggregation of the requests made by all of the processors. Add support for PMIC regulator control via the voltage regulator manager (VRM) and oscillator buffer (XOB) RPMh accelerators. VRM supports manipulation of enable state, voltage, mode, and headroom voltage. XOB supports manipulation of enable state. Signed-off-by: David Collins <collinsd@codeaurora.org> --- drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/qcom_rpmh-regulator.c | 1124 ++++++++++++++++++++ .../dt-bindings/regulator/qcom,rpmh-regulator.h | 40 + 4 files changed, 1174 insertions(+) create mode 100644 drivers/regulator/qcom_rpmh-regulator.c create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h