diff mbox series

[2/4] regulator: Introduce Qualcomm REFGEN regulator driver

Message ID 20230628-topic-refgen-v1-2-126e59573eeb@linaro.org (mailing list archive)
State New, archived
Headers show
Series Qualcomm REFGEN regulator | expand

Commit Message

Konrad Dybcio June 28, 2023, 4:29 p.m. UTC 
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

Mark Brown June 28, 2023, 7:28 p.m. UTC | #1 
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.
kernel test robot June 28, 2023, 9:25 p.m. UTC | #2 
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
Konrad Dybcio June 29, 2023, 8:43 a.m. UTC | #3 
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
Konrad Dybcio June 29, 2023, 10:44 a.m. UTC | #4 
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
diff mbox series

Patch

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 2c2405024ace..ea5549d62825 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -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
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index ebfa75379c20..a044ad20e202 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -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
diff --git a/drivers/regulator/qcom-refgen-regulator.c b/drivers/regulator/qcom-refgen-regulator.c
new file mode 100644
index 000000000000..854ab91f649c
--- /dev/null
+++ b/drivers/regulator/qcom-refgen-regulator.c
@@ -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");