[2/4] regulator: Introduce Qualcomm REFGEN regulator driver
Commit Message
Modern Qualcomm SoCs have a REFGEN (reference voltage generator)
regulator, providing reference voltage to on-chip IP, like PHYs.
Add a driver to support toggling that regulator.
This driver is based on the driver available in the downstream msm-5.4
kernel. It's been cleaned up and partly remade to match upstream
standards.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
drivers/regulator/Kconfig | 10 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-refgen-regulator.c | 187 ++++++++++++++++++++++++++++++
3 files changed, 198 insertions(+)
Comments
On Wed, Jun 28, 2023 at 06:29:46PM +0200, Konrad Dybcio wrote:
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2017, 2019-2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023, Linaro Limited
> + */
Please use a C++ comment for the whole thing for consistency.
> +static int qcom_sdm845_refgen_enable(struct regulator_dev *rdev)
> +{
> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
> +
> + regmap_update_bits(vreg->base, REFGEN_REG_BG_CTRL,
> + REFGEN_BG_CTRL_MASK, REFGEN_BG_CTRL_ENABLE);
> + regmap_write(vreg->base, REFGEN_REG_BIAS_EN, REFGEN_BIAS_EN_ENABLE);
For the enable and disable operations we use a mix of _update_bits() and
absolute writes with no FIELD_PREP()...
> +static int qcom_sdm845_refgen_is_enabled(struct regulator_dev *rdev)
> +{
> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
> + u32 val;
> +
> + regmap_read(vreg->base, REFGEN_REG_BG_CTRL, &val);
> + if (FIELD_GET(REFGEN_BG_CTRL_MASK, val) != REFGEN_BG_CTRL_ENABLE)
> + return 0;
> +
> + regmap_read(vreg->base, REFGEN_REG_BIAS_EN, &val);
> + if (FIELD_GET(REFGEN_BIAS_EN_MASK, val) != REFGEN_BIAS_EN_ENABLE)
> + return 0;
...but when we read back the status we use FIELD_GET(). This looks like
a bug, and given that one of the fields starts at bit 1 it presumably is
one - FIELD_GET() will do shifting.
> +static int qcom_sm8250_refgen_enable(struct regulator_dev *rdev)
> +{
> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
> +
> + regmap_update_bits(vreg->base, REFGEN_REG_PWRDWN_CTRL5,
> + REFGEN_PWRDWN_CTRL5_MASK, REFGEN_PWRDWN_CTRL5_ENABLE);
This is a single bit in a single register so could use the standard
helpers rather than open coding, the sdm845 does need custom operations
due to having two fields to manage.
Hi Konrad,
kernel test robot noticed the following build errors:
[auto build test ERROR on 5c875096d59010cee4e00da1f9c7bdb07a025dc2]
url: https://github.com/intel-lab-lkp/linux/commits/Konrad-Dybcio/dt-bindings-regulator-Describe-Qualcomm-REFGEN-regulator/20230629-003148
base: 5c875096d59010cee4e00da1f9c7bdb07a025dc2
patch link: https://lore.kernel.org/r/20230628-topic-refgen-v1-2-126e59573eeb%40linaro.org
patch subject: [PATCH 2/4] regulator: Introduce Qualcomm REFGEN regulator driver
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20230629/202306290533.nqqGHj1w-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230629/202306290533.nqqGHj1w-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306290533.nqqGHj1w-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/irqflags.h:17,
from include/linux/spinlock.h:59,
from include/linux/kref.h:16,
from include/linux/mm_types.h:8,
from include/linux/buildid.h:5,
from include/linux/module.h:14,
from drivers/regulator/qcom-refgen-regulator.c:7:
arch/loongarch/include/asm/percpu.h:20:4: error: #error compiler support for the model attribute is necessary when a recent assembler is used
20 | # error compiler support for the model attribute is necessary when a recent assembler is used
| ^~~~~
drivers/regulator/qcom-refgen-regulator.c: In function 'qcom_sdm845_refgen_is_enabled':
>> drivers/regulator/qcom-refgen-regulator.c:64:13: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
64 | if (FIELD_GET(REFGEN_BG_CTRL_MASK, val) != REFGEN_BG_CTRL_ENABLE)
| ^~~~~~~~~
cc1: some warnings being treated as errors
vim +/FIELD_GET +64 drivers/regulator/qcom-refgen-regulator.c
57
58 static int qcom_sdm845_refgen_is_enabled(struct regulator_dev *rdev)
59 {
60 struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
61 u32 val;
62
63 regmap_read(vreg->base, REFGEN_REG_BG_CTRL, &val);
> 64 if (FIELD_GET(REFGEN_BG_CTRL_MASK, val) != REFGEN_BG_CTRL_ENABLE)
65 return 0;
66
67 regmap_read(vreg->base, REFGEN_REG_BIAS_EN, &val);
68 if (FIELD_GET(REFGEN_BIAS_EN_MASK, val) != REFGEN_BIAS_EN_ENABLE)
69 return 0;
70
71 return 1;
72 }
73
On 28.06.2023 21:28, Mark Brown wrote:
> On Wed, Jun 28, 2023 at 06:29:46PM +0200, Konrad Dybcio wrote:
>
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2017, 2019-2020, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023, Linaro Limited
>> + */
>
> Please use a C++ comment for the whole thing for consistency.
Oh that's new!
>
>> +static int qcom_sdm845_refgen_enable(struct regulator_dev *rdev)
>> +{
>> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
>> +
>> + regmap_update_bits(vreg->base, REFGEN_REG_BG_CTRL,
>> + REFGEN_BG_CTRL_MASK, REFGEN_BG_CTRL_ENABLE);
>> + regmap_write(vreg->base, REFGEN_REG_BIAS_EN, REFGEN_BIAS_EN_ENABLE);
>
> For the enable and disable operations we use a mix of _update_bits() and
> absolute writes with no FIELD_PREP()...
This absolute write was accidentally fine as the mask began at bit0...
>
>> +static int qcom_sdm845_refgen_is_enabled(struct regulator_dev *rdev)
>> +{
>> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
>> + u32 val;
>> +
>> + regmap_read(vreg->base, REFGEN_REG_BG_CTRL, &val);
>> + if (FIELD_GET(REFGEN_BG_CTRL_MASK, val) != REFGEN_BG_CTRL_ENABLE)
>> + return 0;
>> +
>> + regmap_read(vreg->base, REFGEN_REG_BIAS_EN, &val);
>> + if (FIELD_GET(REFGEN_BIAS_EN_MASK, val) != REFGEN_BIAS_EN_ENABLE)
>> + return 0;
>
> ...but when we read back the status we use FIELD_GET(). This looks like
> a bug, and given that one of the fields starts at bit 1 it presumably is
> one - FIELD_GET() will do shifting.
...but a 2-bit-wide field will never equal 6.
Looks like I put unshifted values in the defines for REFGEN_BG_CTRL..
Thanks for spotting that!
>
>> +static int qcom_sm8250_refgen_enable(struct regulator_dev *rdev)
>> +{
>> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
>> +
>> + regmap_update_bits(vreg->base, REFGEN_REG_PWRDWN_CTRL5,
>> + REFGEN_PWRDWN_CTRL5_MASK, REFGEN_PWRDWN_CTRL5_ENABLE);
>
> This is a single bit in a single register so could use the standard
> helpers rather than open coding, the sdm845 does need custom operations
> due to having two fields to manage.
Forgot that's a thing!
Konrad
On 29.06.2023 10:43, Konrad Dybcio wrote:
> On 28.06.2023 21:28, Mark Brown wrote:
>> On Wed, Jun 28, 2023 at 06:29:46PM +0200, Konrad Dybcio wrote:
>>
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2017, 2019-2020, The Linux Foundation. All rights reserved.
>>> + * Copyright (c) 2023, Linaro Limited
>>> + */
>>
>> Please use a C++ comment for the whole thing for consistency.
> Oh that's new!
>
>>
>>> +static int qcom_sdm845_refgen_enable(struct regulator_dev *rdev)
>>> +{
>>> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
>>> +
>>> + regmap_update_bits(vreg->base, REFGEN_REG_BG_CTRL,
>>> + REFGEN_BG_CTRL_MASK, REFGEN_BG_CTRL_ENABLE);
>>> + regmap_write(vreg->base, REFGEN_REG_BIAS_EN, REFGEN_BIAS_EN_ENABLE);
>>
>> For the enable and disable operations we use a mix of _update_bits() and
>> absolute writes with no FIELD_PREP()...
> This absolute write was accidentally fine as the mask began at bit0...
>
>>
>>> +static int qcom_sdm845_refgen_is_enabled(struct regulator_dev *rdev)
>>> +{
>>> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
>>> + u32 val;
>>> +
>>> + regmap_read(vreg->base, REFGEN_REG_BG_CTRL, &val);
>>> + if (FIELD_GET(REFGEN_BG_CTRL_MASK, val) != REFGEN_BG_CTRL_ENABLE)
>>> + return 0;
>>> +
>>> + regmap_read(vreg->base, REFGEN_REG_BIAS_EN, &val);
>>> + if (FIELD_GET(REFGEN_BIAS_EN_MASK, val) != REFGEN_BIAS_EN_ENABLE)
>>> + return 0;
>>
>> ...but when we read back the status we use FIELD_GET(). This looks like
>> a bug, and given that one of the fields starts at bit 1 it presumably is
>> one - FIELD_GET() will do shifting.
> ...but a 2-bit-wide field will never equal 6.
> Looks like I put unshifted values in the defines for REFGEN_BG_CTRL..
>
> Thanks for spotting that!
Even worse, I noticed I've been feeding a raw address into regmap
functions.. :)
Konrad
>
>>
>>> +static int qcom_sm8250_refgen_enable(struct regulator_dev *rdev)
>>> +{
>>> + struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
>>> +
>>> + regmap_update_bits(vreg->base, REFGEN_REG_PWRDWN_CTRL5,
>>> + REFGEN_PWRDWN_CTRL5_MASK, REFGEN_PWRDWN_CTRL5_ENABLE);
>>
>> This is a single bit in a single register so could use the standard
>> helpers rather than open coding, the sdm845 does need custom operations
>> due to having two fields to manage.
> Forgot that's a thing!
>
> Konrad
@@ -978,6 +978,16 @@ config REGULATOR_PWM
This driver supports PWM controlled voltage regulators. PWM
duty cycle can increase or decrease the voltage.
+config REGULATOR_QCOM_REFGEN
+ tristate "Qualcomm REFGEN regulator driver"
+ depends on REGMAP
+ help
+ This driver supports the MMIO-mapped reference voltage regulator,
+ used internally by some PHYs on many Qualcomm SoCs.
+
+ Say M here if you want to include support for this regulator as
+ a module. The module will be named "qcom-refgen-regulator".
+
config REGULATOR_QCOM_RPM
tristate "Qualcomm RPM regulator driver"
depends on MFD_QCOM_RPM
@@ -107,6 +107,7 @@ obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_REFGEN) += qcom-refgen-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
new file mode 100644
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2017, 2019-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+#define REFGEN_REG_BIAS_EN 0x08
+#define REFGEN_BIAS_EN_MASK GENMASK(2, 0)
+ #define REFGEN_BIAS_EN_ENABLE 0x7
+ #define REFGEN_BIAS_EN_DISABLE 0x6
+
+#define REFGEN_REG_BG_CTRL 0x14
+#define REFGEN_BG_CTRL_MASK GENMASK(2, 1)
+ #define REFGEN_BG_CTRL_ENABLE 0x6
+ #define REFGEN_BG_CTRL_DISABLE 0x4
+
+#define REFGEN_REG_PWRDWN_CTRL5 0x80
+#define REFGEN_PWRDWN_CTRL5_MASK BIT(0)
+ #define REFGEN_PWRDWN_CTRL5_ENABLE 0x1
+ #define REFGEN_PWRDWN_CTRL5_DISABLE 0x0
+
+struct qcom_refgen {
+ struct regulator_desc rdesc;
+ struct regulator_dev *rdev;
+ void __iomem *base;
+};
+
+static int qcom_sdm845_refgen_enable(struct regulator_dev *rdev)
+{
+ struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
+
+ regmap_update_bits(vreg->base, REFGEN_REG_BG_CTRL,
+ REFGEN_BG_CTRL_MASK, REFGEN_BG_CTRL_ENABLE);
+ regmap_write(vreg->base, REFGEN_REG_BIAS_EN, REFGEN_BIAS_EN_ENABLE);
+
+ return 0;
+}
+
+static int qcom_sdm845_refgen_disable(struct regulator_dev *rdev)
+{
+ struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
+
+ regmap_write(vreg->base, REFGEN_REG_BIAS_EN, REFGEN_BIAS_EN_DISABLE);
+ regmap_update_bits(vreg->base, REFGEN_REG_BG_CTRL,
+ REFGEN_BG_CTRL_MASK, REFGEN_BG_CTRL_DISABLE);
+
+ return 0;
+}
+
+static int qcom_sdm845_refgen_is_enabled(struct regulator_dev *rdev)
+{
+ struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
+ u32 val;
+
+ regmap_read(vreg->base, REFGEN_REG_BG_CTRL, &val);
+ if (FIELD_GET(REFGEN_BG_CTRL_MASK, val) != REFGEN_BG_CTRL_ENABLE)
+ return 0;
+
+ regmap_read(vreg->base, REFGEN_REG_BIAS_EN, &val);
+ if (FIELD_GET(REFGEN_BIAS_EN_MASK, val) != REFGEN_BIAS_EN_ENABLE)
+ return 0;
+
+ return 1;
+}
+
+static const struct regulator_ops sdm845_refgen_ops = {
+ .enable = qcom_sdm845_refgen_enable,
+ .disable = qcom_sdm845_refgen_disable,
+ .is_enabled = qcom_sdm845_refgen_is_enabled,
+};
+
+static int qcom_sm8250_refgen_enable(struct regulator_dev *rdev)
+{
+ struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
+
+ regmap_update_bits(vreg->base, REFGEN_REG_PWRDWN_CTRL5,
+ REFGEN_PWRDWN_CTRL5_MASK, REFGEN_PWRDWN_CTRL5_ENABLE);
+
+ return 0;
+}
+
+static int qcom_sm8250_refgen_disable(struct regulator_dev *rdev)
+{
+ struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
+
+ regmap_update_bits(vreg->base, REFGEN_REG_PWRDWN_CTRL5,
+ REFGEN_PWRDWN_CTRL5_MASK, REFGEN_PWRDWN_CTRL5_DISABLE);
+
+ return 0;
+}
+
+static int qcom_sm8250_refgen_is_enabled(struct regulator_dev *rdev)
+{
+ struct qcom_refgen *vreg = rdev_get_drvdata(rdev);
+ u32 val;
+
+ regmap_read(vreg->base, REFGEN_REG_PWRDWN_CTRL5, &val);
+
+ return FIELD_GET(REFGEN_PWRDWN_CTRL5_MASK, val) == REFGEN_PWRDWN_CTRL5_ENABLE;
+}
+
+static const struct regulator_ops sm8250_refgen_ops = {
+ .enable = qcom_sm8250_refgen_enable,
+ .disable = qcom_sm8250_refgen_disable,
+ .is_enabled = qcom_sm8250_refgen_is_enabled,
+};
+
+static const struct regmap_config qcom_refgen_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+};
+
+static int qcom_refgen_probe(struct platform_device *pdev)
+{
+ struct regulator_init_data *init_data;
+ struct regulator_config config = {};
+ struct device *dev = &pdev->dev;
+ struct regulator_desc *rdesc;
+ struct qcom_refgen *vreg;
+ void __iomem *base;
+
+ vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
+ if (!vreg)
+ return -ENOMEM;
+
+ vreg->rdesc.ops = of_device_get_match_data(dev);
+ if (!vreg->rdesc.ops)
+ return -ENODATA;
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ vreg->base = devm_regmap_init_mmio(dev, base, &qcom_refgen_regmap_config);
+ if (IS_ERR(vreg->base))
+ return PTR_ERR(vreg->base);
+
+ init_data = of_get_regulator_init_data(dev, dev->of_node, &vreg->rdesc);
+ if (!init_data)
+ return -ENOMEM;
+
+ rdesc = &vreg->rdesc;
+
+ rdesc->name = "refgen";
+ rdesc->owner = THIS_MODULE;
+ rdesc->type = REGULATOR_VOLTAGE;
+ rdesc->enable_time = 5; /* us */
+
+ config.dev = dev;
+ config.init_data = init_data;
+ config.driver_data = vreg;
+ config.of_node = dev->of_node;
+
+ vreg->rdev = devm_regulator_register(dev, rdesc, &config);
+ if (IS_ERR(vreg->rdev))
+ return PTR_ERR(vreg->rdev);
+
+ return 0;
+}
+
+static const struct of_device_id qcom_refgen_match_table[] = {
+ { .compatible = "qcom,sdm845-refgen-regulator", .data = &sdm845_refgen_ops },
+ { .compatible = "qcom,sm8250-refgen-regulator", .data = &sm8250_refgen_ops },
+ { }
+};
+
+static struct platform_driver qcom_refgen_driver = {
+ .probe = qcom_refgen_probe,
+ .driver = {
+ .name = "qcom-refgen-regulator",
+ .of_match_table = qcom_refgen_match_table,
+ },
+};
+module_platform_driver(qcom_refgen_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Qualcomm REFGEN regulator driver");