[PATCH 0/4] Qualcomm REFGEN regulator

[PATCH 0/4] Qualcomm REFGEN regulator

[Konrad Dybcio]

Recent Qualcomm SoCs have a REFGEN (reference voltage generator) regulator
responsible for providing a reference voltage to some on-SoC IPs (like DSI
or PHYs). It can be turned off when unused to save power.

This series introduces the driver for it and lets the DSI driver
consume it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (4):
      dt-bindings: regulator: Describe Qualcomm REFGEN regulator
      regulator: Introduce Qualcomm REFGEN regulator driver
      dt-bindings: display/msm: dsi-controller-main: Allow refgen-supply
      drm/msm/dsi: Hook up refgen regulator

 .../bindings/display/msm/dsi-controller-main.yaml  |   4 +
 .../regulator/qcom,sdm845-refgen-regulator.yaml    |  56 ++++++
 drivers/gpu/drm/msm/dsi/dsi_cfg.c                  |   2 +
 drivers/regulator/Kconfig                          |  10 ++
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-refgen-regulator.c          | 187 +++++++++++++++++++++
 6 files changed, 260 insertions(+)
---
base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2
change-id: 20230628-topic-refgen-14fb0b762115

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/4] dt-bindings: regulator: Describe Qualcomm REFGEN regulator

[Konrad Dybcio]

Modern Qualcomm SoCs have a REFGEN (reference voltage generator)
regulator, providing reference voltage to on-chip IP, like PHYs.
It's controlled through MMIO and we can toggle it or read its state back.

Describe it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../regulator/qcom,sdm845-refgen-regulator.yaml    | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,sdm845-refgen-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,sdm845-refgen-regulator.yaml
new file mode 100644
index 000000000000..19d3eb9db98f
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,sdm845-refgen-regulator.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/qcom,sdm845-refgen-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. REFGEN Regulator
+
+maintainers:
+  - Konrad Dybcio <konradybcio@kernel.org>
+
+description: |
+    The REFGEN (reference voltage renegator) regulator provides reference
+    voltage for on-chip IPs (like PHYs) on some Qualcomm SoCs.
+
+allOf:
+  - $ref: regulator.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - qcom,sc7180-refgen-regulator
+              - qcom,sc8180x-refgen-regulator
+              - qcom,sm8150-refgen-regulator
+          - const: qcom,sdm845-refgen-regulator
+
+      - items:
+          - enum:
+              - qcom,sc7280-refgen-regulator
+              - qcom,sc8280xp-refgen-regulator
+              - qcom,sm6350-refgen-regulator
+              - qcom,sm6375-refgen-regulator
+              - qcom,sm8350-refgen-regulator
+          - const: qcom,sm8250-refgen-regulator
+
+      - enum:
+          - qcom,sdm845-refgen-regulator
+          - qcom,sm8250-refgen-regulator
+
+  reg: true
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    regulator@162f000 {
+      compatible = "qcom,sm8250-refgen-regulator";
+      reg = <0 0x0162f000 0 0x84>;
+    };
+...

-- 
2.41.0

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

[Konrad Dybcio]

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(+)

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");

-- 
2.41.0

[PATCH 3/4] dt-bindings: display/msm: dsi-controller-main: Allow refgen-supply

[Konrad Dybcio]

DSI host needs REFGEN to be enabled (if it's present on a given platform).
Allow consuming it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../devicetree/bindings/display/msm/dsi-controller-main.yaml          | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 01848bdd5873..76270992305a 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -166,6 +166,10 @@ properties:
     description:
       Phandle to vdd regulator device node
 
+  refgen-supply:
+    description:
+      Phandle to REFGEN regulator device node
+
   vcca-supply:
     description:
       Phandle to vdd regulator device node

-- 
2.41.0

[PATCH 4/4] drm/msm/dsi: Hook up refgen regulator

[Konrad Dybcio]

Consume the refgen supply on configurations that may use it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 8a5fb6df7210..1f98ff74ceb0 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -160,6 +160,7 @@ static const char * const dsi_v2_4_clk_names[] = {
 
 static const struct regulator_bulk_data dsi_v2_4_regulators[] = {
 	{ .supply = "vdda", .init_load_uA = 21800 },	/* 1.2 V */
+	{ .supply = "refgen" },
 };
 
 static const struct msm_dsi_config sdm845_dsi_cfg = {
@@ -191,6 +192,7 @@ static const struct msm_dsi_config sm8550_dsi_cfg = {
 
 static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
 	{ .supply = "vdda", .init_load_uA = 8350 },	/* 1.2 V */
+	{ .supply = "refgen" },
 };
 
 static const struct msm_dsi_config sc7280_dsi_cfg = {

-- 
2.41.0

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Konrad Dybcio]

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

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

[Konrad Dybcio]

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

Re: [PATCH 1/4] dt-bindings: regulator: Describe Qualcomm REFGEN regulator

[Rob Herring]

On Wed, Jun 28, 2023 at 06:29:45PM +0200, Konrad Dybcio wrote:
> Modern Qualcomm SoCs have a REFGEN (reference voltage generator)
> regulator, providing reference voltage to on-chip IP, like PHYs.
> It's controlled through MMIO and we can toggle it or read its state back.
> 
> Describe it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../regulator/qcom,sdm845-refgen-regulator.yaml    | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,sdm845-refgen-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,sdm845-refgen-regulator.yaml
> new file mode 100644
> index 000000000000..19d3eb9db98f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom,sdm845-refgen-regulator.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/qcom,sdm845-refgen-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. REFGEN Regulator
> +
> +maintainers:
> +  - Konrad Dybcio <konradybcio@kernel.org>
> +
> +description: |

Don't need '|'.

> +    The REFGEN (reference voltage renegator) regulator provides reference

renegator?

> +    voltage for on-chip IPs (like PHYs) on some Qualcomm SoCs.
> +
> +allOf:
> +  - $ref: regulator.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - qcom,sc7180-refgen-regulator
> +              - qcom,sc8180x-refgen-regulator
> +              - qcom,sm8150-refgen-regulator
> +          - const: qcom,sdm845-refgen-regulator
> +
> +      - items:
> +          - enum:
> +              - qcom,sc7280-refgen-regulator
> +              - qcom,sc8280xp-refgen-regulator
> +              - qcom,sm6350-refgen-regulator
> +              - qcom,sm6375-refgen-regulator
> +              - qcom,sm8350-refgen-regulator
> +          - const: qcom,sm8250-refgen-regulator
> +
> +      - enum:
> +          - qcom,sdm845-refgen-regulator
> +          - qcom,sm8250-refgen-regulator
> +
> +  reg: true

Need to define how many.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    regulator@162f000 {
> +      compatible = "qcom,sm8250-refgen-regulator";
> +      reg = <0 0x0162f000 0 0x84>;

Default cell size is 1.

> +    };
> +...
> 
> -- 
> 2.41.0
> 

Re: [PATCH 3/4] dt-bindings: display/msm: dsi-controller-main: Allow refgen-supply

[Rob Herring]

On Wed, 28 Jun 2023 18:29:47 +0200, Konrad Dybcio wrote:
> DSI host needs REFGEN to be enabled (if it's present on a given platform).
> Allow consuming it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/display/msm/dsi-controller-main.yaml          | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>