[PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support

[PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support

[Sam Protsenko]

Power Domains in Exynos850 are not really different from other Exynos
platforms. Enabling Exynos850 support in the PD driver is really just a
matter of adding:

    static const struct exynos_pm_domain_config exynos850_cfg = {
        .local_pwr_cfg = 0x1,
    };

to the driver. But in the face of recent developments, e.g. this patch:

    arm64: dts: exynos: move MIPI phy to PMU node in Exynos5433

it looked logical to rework the PD driver a bit to support its nesting
under the PMU node, while adding Exynos850 support to it. Initially I
only wanted to add syscon regmap support via some dedicated property,
but pulling PD nodes under the PMU syscon looks like more correct way.

This patch series provides next changes:

  1. Make it possible for PD nodes to be children of PMU
  2. Add Exynos850 support to PD driver
  3. A bit of refactoring in PD driver
  4. Corresponding changes to dt-bindings

Dependencies inside of the series:

  - patch #2 depends on patch #1
  - patch #6 depends on patch #1
  - patches 3,4,5,6 should be applied in the same order as in the series

Sam Protsenko (6):
  dt-bindings: power: pd-samsung: Add Exynos850 support
  dt-bindings: power: pd-samsung: Allow pd nodes to be children of PMU
  soc: samsung: pm_domains: Extract DT handling into a separate function
  soc: samsung: pm_domains: Implement proper I/O operations
  soc: samsung: pm_domains: Allow PD to be a child of PMU syscon
  soc: samsung: pm_domains: Add Exynos850 support

 .../devicetree/bindings/power/pd-samsung.yaml |  12 +-
 MAINTAINERS                                   |   1 +
 drivers/soc/samsung/Kconfig                   |   1 +
 drivers/soc/samsung/pm_domains.c              | 132 +++++++++++++++---
 .../power/samsung,exynos850-power.h           |  17 +++
 5 files changed, 142 insertions(+), 21 deletions(-)
 create mode 100644 include/dt-bindings/power/samsung,exynos850-power.h

-- 
2.39.2

[PATCH 1/6] dt-bindings: power: pd-samsung: Add Exynos850 support

[Sam Protsenko]

Document the compatible string for Exynos850 power domains controller.
Also add power domain indices which can be used in "samsung,pd-index"
property to specify a particular power domain in the device tree.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 .../devicetree/bindings/power/pd-samsung.yaml   |  1 +
 MAINTAINERS                                     |  1 +
 .../dt-bindings/power/samsung,exynos850-power.h | 17 +++++++++++++++++
 3 files changed, 19 insertions(+)
 create mode 100644 include/dt-bindings/power/samsung,exynos850-power.h

diff --git a/Documentation/devicetree/bindings/power/pd-samsung.yaml b/Documentation/devicetree/bindings/power/pd-samsung.yaml
index 9c2c51133457..a353a705292c 100644
--- a/Documentation/devicetree/bindings/power/pd-samsung.yaml
+++ b/Documentation/devicetree/bindings/power/pd-samsung.yaml
@@ -21,6 +21,7 @@ properties:
     enum:
       - samsung,exynos4210-pd
       - samsung,exynos5433-pd
+      - samsung,exynos850-pd
 
   reg:
     maxItems: 1
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d5bc223f305..53e11e48639c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2720,6 +2720,7 @@ F:	drivers/pwm/pwm-samsung.c
 F:	drivers/soc/samsung/
 F:	drivers/tty/serial/samsung*
 F:	include/clocksource/samsung_pwm.h
+F:	include/dt-bindings/power/samsung,*
 F:	include/linux/platform_data/*s3c*
 F:	include/linux/serial_s3c.h
 F:	include/linux/soc/samsung/
diff --git a/include/dt-bindings/power/samsung,exynos850-power.h b/include/dt-bindings/power/samsung,exynos850-power.h
new file mode 100644
index 000000000000..a8d877b5515a
--- /dev/null
+++ b/include/dt-bindings/power/samsung,exynos850-power.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2023 Linaro Ltd.
+ * Author: Sam Protsenko <semen.protsenko@linaro.org>
+ */
+
+#ifndef _DT_BINDINGS_POWER_EXYNOS850_POWER_H
+#define _DT_BINDINGS_POWER_EXYNOS850_POWER_H
+
+#define EXYNOS850_PD_HSI		0
+#define EXYNOS850_PD_G3D		1
+#define EXYNOS850_PD_MFCMSCL		2
+#define EXYNOS850_PD_DPU		3
+#define EXYNOS850_PD_AUD		4
+#define EXYNOS850_PD_IS			5
+
+#endif /* _DT_BINDINGS_POWER_EXYNOS850_POWER_H */
-- 
2.39.2

[PATCH 2/6] dt-bindings: power: pd-samsung: Allow pd nodes to be children of PMU

[Sam Protsenko]

Introduce a new "samsung,pd-index" property to choose a specific power
domain. This way it would be possible to avoid specifying any addresses
in power domain nodes, relying solely on syscon regmap from the parent
node (which should be a PMU system controller). Therefore the "reg"
property is deprecated now, as it's more logical to describe power
domains as children of PMU node, because PD registers reside in the PMU
area.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 .../devicetree/bindings/power/pd-samsung.yaml         | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/power/pd-samsung.yaml b/Documentation/devicetree/bindings/power/pd-samsung.yaml
index a353a705292c..73178b1a56ea 100644
--- a/Documentation/devicetree/bindings/power/pd-samsung.yaml
+++ b/Documentation/devicetree/bindings/power/pd-samsung.yaml
@@ -25,6 +25,10 @@ properties:
 
   reg:
     maxItems: 1
+    deprecated: true
+    description:
+      Physical base address and length of Power Domains area (if not a child of
+      PMU).
 
   clocks:
     deprecated: true
@@ -45,10 +49,15 @@ properties:
   power-domains:
     maxItems: 1
 
+  samsung,pd-index:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Power domain index (if a child of PMU). Valid values are defined in::
+        "include/dt-bindings/power/samsung,exynos850-power.h" - for Exynos850
+
 required:
   - compatible
   - "#power-domain-cells"
-  - reg
 
 unevaluatedProperties: false
 
-- 
2.39.2

[PATCH 3/6] soc: samsung: pm_domains: Extract DT handling into a separate function

[Sam Protsenko]

As DT parsing code tends to grow with time, make it a separate routine.
While at it, replace kstrdup_const() with devm_kstrdup_const() in order
to avoid manual memory management and simplify the error path.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/soc/samsung/pm_domains.c | 39 +++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/soc/samsung/pm_domains.c b/drivers/soc/samsung/pm_domains.c
index d07f3c9d6903..522a43005a5a 100644
--- a/drivers/soc/samsung/pm_domains.c
+++ b/drivers/soc/samsung/pm_domains.c
@@ -27,6 +27,7 @@ struct exynos_pm_domain_config {
  * Exynos specific wrapper around the generic power domain
  */
 struct exynos_pm_domain {
+	struct device *dev;
 	void __iomem *base;
 	struct generic_pm_domain pd;
 	u32 local_pwr_cfg;
@@ -91,42 +92,48 @@ static const struct of_device_id exynos_pm_domain_of_match[] = {
 	{ },
 };
 
-static const char *exynos_get_domain_name(struct device_node *node)
+static int exynos_pd_parse_dt(struct exynos_pm_domain *pd)
 {
+	const struct exynos_pm_domain_config *variant;
+	struct device *dev = pd->dev;
+	struct device_node *np = dev->of_node;
 	const char *name;
 
-	if (of_property_read_string(node, "label", &name) < 0)
-		name = kbasename(node->full_name);
-	return kstrdup_const(name, GFP_KERNEL);
+	variant = of_device_get_match_data(dev);
+	pd->local_pwr_cfg = variant->local_pwr_cfg;
+
+	if (of_property_read_string(np, "label", &name) < 0)
+		name = kbasename(np->full_name);
+	pd->pd.name = devm_kstrdup_const(dev, name, GFP_KERNEL);
+	if (!pd->pd.name)
+		return -ENOMEM;
+
+	pd->base = of_iomap(np, 0);
+	if (!pd->base)
+		return -ENODEV;
+
+	return 0;
 }
 
 static int exynos_pd_probe(struct platform_device *pdev)
 {
-	const struct exynos_pm_domain_config *pm_domain_cfg;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args child, parent;
 	struct exynos_pm_domain *pd;
 	int on, ret;
 
-	pm_domain_cfg = of_device_get_match_data(dev);
 	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
 	if (!pd)
 		return -ENOMEM;
 
-	pd->pd.name = exynos_get_domain_name(np);
-	if (!pd->pd.name)
-		return -ENOMEM;
-
-	pd->base = of_iomap(np, 0);
-	if (!pd->base) {
-		kfree_const(pd->pd.name);
-		return -ENODEV;
-	}
+	pd->dev = dev;
+	ret = exynos_pd_parse_dt(pd);
+	if (ret)
+		return ret;
 
 	pd->pd.power_off = exynos_pd_power_off;
 	pd->pd.power_on = exynos_pd_power_on;
-	pd->local_pwr_cfg = pm_domain_cfg->local_pwr_cfg;
 
 	on = readl_relaxed(pd->base + 0x4) & pd->local_pwr_cfg;
 
-- 
2.39.2

[PATCH 4/6] soc: samsung: pm_domains: Implement proper I/O operations

[Sam Protsenko]

Instead of doing in-place readl()/writel() calls fed with magic numbers,
provide dedicated read/write functions which implement proper register
accesses:
  - Get rid of magic numbers by introducing actual constants for PD
    registers
  - Rework the write function to perform a RMW operation, as PD
    registers have some bits markes as "Reserved" in TRM, which
    shouldn't be changed
  - Add helper functions for reading the STATUS reg and writing
    CONFIGURATION reg, to make user code more neat and clean

New functions are designed in such a way that it's easy to rework those
further on top of regmap API.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/soc/samsung/pm_domains.c | 42 +++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/samsung/pm_domains.c b/drivers/soc/samsung/pm_domains.c
index 522a43005a5a..dd1ec3541e11 100644
--- a/drivers/soc/samsung/pm_domains.c
+++ b/drivers/soc/samsung/pm_domains.c
@@ -18,6 +18,10 @@
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
 
+/* Register offsets inside Power Domain area in PMU */
+#define EXYNOS_PD_CONF		0x0
+#define EXYNOS_PD_STATUS	0x4
+
 struct exynos_pm_domain_config {
 	/* Value for LOCAL_PWR_CFG and STATUS fields for each domain */
 	u32 local_pwr_cfg;
@@ -33,6 +37,37 @@ struct exynos_pm_domain {
 	u32 local_pwr_cfg;
 };
 
+static void exynos_pd_write(struct exynos_pm_domain *pd, unsigned int reg,
+			    unsigned int mask, unsigned int val)
+{
+	u32 v;
+
+	v = readl_relaxed(pd->base + reg);
+	v = (v & ~mask) | val;
+	writel_relaxed(v, pd->base + reg);
+}
+
+static void exynos_pd_read(struct exynos_pm_domain *pd, unsigned int reg,
+			   unsigned int *val)
+{
+	*val = readl_relaxed(pd->base + reg);
+}
+
+static unsigned int exynos_pd_read_status(struct exynos_pm_domain *pd)
+{
+	unsigned int val;
+
+	exynos_pd_read(pd, EXYNOS_PD_STATUS, &val);
+	val &= pd->local_pwr_cfg;
+
+	return val;
+}
+
+static void exynos_pd_write_conf(struct exynos_pm_domain *pd, u32 val)
+{
+	exynos_pd_write(pd, EXYNOS_PD_CONF, pd->local_pwr_cfg, val);
+}
+
 static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
 {
 	struct exynos_pm_domain *pd;
@@ -44,12 +79,12 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
 	base = pd->base;
 
 	pwr = power_on ? pd->local_pwr_cfg : 0;
-	writel_relaxed(pwr, base);
+	exynos_pd_write_conf(pd, pwr);
 
 	/* Wait max 1ms */
 	timeout = 10;
 
-	while ((readl_relaxed(base + 0x4) & pd->local_pwr_cfg) != pwr) {
+	while (exynos_pd_read_status(pd) != pwr) {
 		if (!timeout) {
 			op = (power_on) ? "enable" : "disable";
 			pr_err("Power domain %s %s failed\n", domain->name, op);
@@ -135,8 +170,7 @@ static int exynos_pd_probe(struct platform_device *pdev)
 	pd->pd.power_off = exynos_pd_power_off;
 	pd->pd.power_on = exynos_pd_power_on;
 
-	on = readl_relaxed(pd->base + 0x4) & pd->local_pwr_cfg;
-
+	on = exynos_pd_read_status(pd);
 	pm_genpd_init(&pd->pd, NULL, !on);
 	ret = of_genpd_add_provider_simple(np, &pd->pd);
 
-- 
2.39.2

[PATCH 5/6] soc: samsung: pm_domains: Allow PD to be a child of PMU syscon

[Sam Protsenko]

Power Domains registers are a part of the PMU area in Exynos SoCs. The
PMU area is shared between multiple users (like WDT driver, reset
driver, PD driver, etc), and it's usually already implemented as a
system controller in the SoC device tree. Make it possible for a PD node
to be a child of that PMU syscon and utilize its shared regmap instance
in PD driver to access the PMU area registers.

When a PD node is a child of PMU, the "samsung,pd-index" DT property is
used to specify the particular power domain (instead of providing base
address in "reg" property). Implement the support for that index
property, so that the driver can look up corresponding register offsets
by that index, if the property is present. But also keep the
compatibility with existing device trees where the index property is not
defined in PD nodes and which rely on raw read/write access to the PMU
registers.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/soc/samsung/Kconfig      |  1 +
 drivers/soc/samsung/pm_domains.c | 49 ++++++++++++++++++++++++++------
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
index 7a8f291e7704..dfe7a973b272 100644
--- a/drivers/soc/samsung/Kconfig
+++ b/drivers/soc/samsung/Kconfig
@@ -51,6 +51,7 @@ config EXYNOS_PMU_ARM_DRIVERS
 config EXYNOS_PM_DOMAINS
 	bool "Exynos PM domains" if COMPILE_TEST
 	depends on (ARCH_EXYNOS && PM_GENERIC_DOMAINS) || COMPILE_TEST
+	select MFD_SYSCON
 
 config SAMSUNG_PM_CHECK
 	bool "S3C2410 PM Suspend Memory CRC"
diff --git a/drivers/soc/samsung/pm_domains.c b/drivers/soc/samsung/pm_domains.c
index dd1ec3541e11..ec630a151247 100644
--- a/drivers/soc/samsung/pm_domains.c
+++ b/drivers/soc/samsung/pm_domains.c
@@ -17,6 +17,8 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 /* Register offsets inside Power Domain area in PMU */
 #define EXYNOS_PD_CONF		0x0
@@ -25,6 +27,10 @@
 struct exynos_pm_domain_config {
 	/* Value for LOCAL_PWR_CFG and STATUS fields for each domain */
 	u32 local_pwr_cfg;
+
+	/* Power domain offsets in PMU area, for each power domain index */
+	const unsigned int *pd_offsets;
+	size_t pd_offsets_num;
 };
 
 /*
@@ -35,22 +41,32 @@ struct exynos_pm_domain {
 	void __iomem *base;
 	struct generic_pm_domain pd;
 	u32 local_pwr_cfg;
+
+	unsigned int offset;
+	struct regmap *pmureg;
 };
 
 static void exynos_pd_write(struct exynos_pm_domain *pd, unsigned int reg,
 			    unsigned int mask, unsigned int val)
 {
-	u32 v;
-
-	v = readl_relaxed(pd->base + reg);
-	v = (v & ~mask) | val;
-	writel_relaxed(v, pd->base + reg);
+	if (pd->pmureg) {
+		regmap_update_bits(pd->pmureg, pd->offset + reg, mask, val);
+	} else {
+		u32 v;
+
+		v = readl_relaxed(pd->base + reg);
+		v = (v & ~mask) | val;
+		writel_relaxed(v, pd->base + reg);
+	}
 }
 
 static void exynos_pd_read(struct exynos_pm_domain *pd, unsigned int reg,
 			   unsigned int *val)
 {
-	*val = readl_relaxed(pd->base + reg);
+	if (pd->pmureg)
+		regmap_read(pd->pmureg, pd->offset + reg, val);
+	else
+		*val = readl_relaxed(pd->base + reg);
 }
 
 static unsigned int exynos_pd_read_status(struct exynos_pm_domain *pd)
@@ -133,6 +149,8 @@ static int exynos_pd_parse_dt(struct exynos_pm_domain *pd)
 	struct device *dev = pd->dev;
 	struct device_node *np = dev->of_node;
 	const char *name;
+	u32 index;
+	int ret;
 
 	variant = of_device_get_match_data(dev);
 	pd->local_pwr_cfg = variant->local_pwr_cfg;
@@ -143,9 +161,22 @@ static int exynos_pd_parse_dt(struct exynos_pm_domain *pd)
 	if (!pd->pd.name)
 		return -ENOMEM;
 
-	pd->base = of_iomap(np, 0);
-	if (!pd->base)
-		return -ENODEV;
+	ret = of_property_read_u32(np, "samsung,pd-index", &index);
+	if (!ret) {
+		if (index >= variant->pd_offsets_num)
+			return -EINVAL;
+		if (!dev->parent)
+			return -ENODEV;
+
+		pd->offset = variant->pd_offsets[index];
+		pd->pmureg = syscon_node_to_regmap(dev->parent->of_node);
+		if (IS_ERR(pd->pmureg))
+			return PTR_ERR(pd->pmureg);
+	} else {
+		pd->base = of_iomap(np, 0);
+		if (!pd->base)
+			return -ENODEV;
+	}
 
 	return 0;
 }
-- 
2.39.2

[PATCH 6/6] soc: samsung: pm_domains: Add Exynos850 support

[Sam Protsenko]

Add a new compatible string for Exynos850, providing all HW specific
data, which enables Exynos PD support for this chip.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/soc/samsung/pm_domains.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/soc/samsung/pm_domains.c b/drivers/soc/samsung/pm_domains.c
index ec630a151247..795d8a9cd4b5 100644
--- a/drivers/soc/samsung/pm_domains.c
+++ b/drivers/soc/samsung/pm_domains.c
@@ -20,6 +20,8 @@
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 
+#include <dt-bindings/power/samsung,exynos850-power.h>
+
 /* Register offsets inside Power Domain area in PMU */
 #define EXYNOS_PD_CONF		0x0
 #define EXYNOS_PD_STATUS	0x4
@@ -132,6 +134,21 @@ static const struct exynos_pm_domain_config exynos5433_cfg = {
 	.local_pwr_cfg		= 0xf,
 };
 
+static const unsigned int exynos850_pd_offsets[] = {
+	[EXYNOS850_PD_HSI]	= 0x1c80,
+	[EXYNOS850_PD_G3D]	= 0x1d00,
+	[EXYNOS850_PD_MFCMSCL]	= 0x1d80,
+	[EXYNOS850_PD_DPU]	= 0x2000,
+	[EXYNOS850_PD_AUD]	= 0x2080,
+	[EXYNOS850_PD_IS]	= 0x2100,
+};
+
+static const struct exynos_pm_domain_config exynos850_cfg = {
+	.local_pwr_cfg		= 0x1,
+	.pd_offsets		= exynos850_pd_offsets,
+	.pd_offsets_num		= ARRAY_SIZE(exynos850_pd_offsets),
+};
+
 static const struct of_device_id exynos_pm_domain_of_match[] = {
 	{
 		.compatible = "samsung,exynos4210-pd",
@@ -139,6 +156,9 @@ static const struct of_device_id exynos_pm_domain_of_match[] = {
 	}, {
 		.compatible = "samsung,exynos5433-pd",
 		.data = &exynos5433_cfg,
+	}, {
+		.compatible = "samsung,exynos850-pd",
+		.data = &exynos850_cfg,
 	},
 	{ },
 };
-- 
2.39.2

Re: [PATCH 4/6] soc: samsung: pm_domains: Implement proper I/O operations

[kernel test robot]

Hi Sam,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on krzk/for-next]
[also build test WARNING on robh/for-next krzk-dt/for-next linus/master v6.3-rc1 next-20230308]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sam-Protsenko/dt-bindings-power-pd-samsung-Add-Exynos850-support/20230309-071202
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git for-next
patch link:    https://lore.kernel.org/r/20230308230931.27261-5-semen.protsenko%40linaro.org
patch subject: [PATCH 4/6] soc: samsung: pm_domains: Implement proper I/O operations
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230309/202303090824.TjEOnQJ8-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2dabd70f9264ef6cd044de6c3cbd3d83bfef8442
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sam-Protsenko/dt-bindings-power-pd-samsung-Add-Exynos850-support/20230309-071202
        git checkout 2dabd70f9264ef6cd044de6c3cbd3d83bfef8442
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/soc/samsung/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303090824.TjEOnQJ8-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/soc/samsung/pm_domains.c: In function 'exynos_pd_power':
>> drivers/soc/samsung/pm_domains.c:74:23: warning: variable 'base' set but not used [-Wunused-but-set-variable]
      74 |         void __iomem *base;
         |                       ^~~~


vim +/base +74 drivers/soc/samsung/pm_domains.c

2dabd70f9264ef drivers/soc/samsung/pm_domains.c  Sam Protsenko       2023-03-08   70  
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   71  static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   72  {
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   73  	struct exynos_pm_domain *pd;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27  @74  	void __iomem *base;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   75  	u32 timeout, pwr;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   76  	char *op;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   77  
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   78  	pd = container_of(domain, struct exynos_pm_domain, pd);
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   79  	base = pd->base;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   80  
c028e175713698 drivers/soc/samsung/pm_domains.c  Krzysztof Kozlowski 2016-05-10   81  	pwr = power_on ? pd->local_pwr_cfg : 0;
2dabd70f9264ef drivers/soc/samsung/pm_domains.c  Sam Protsenko       2023-03-08   82  	exynos_pd_write_conf(pd, pwr);
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   83  
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   84  	/* Wait max 1ms */
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   85  	timeout = 10;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   86  
2dabd70f9264ef drivers/soc/samsung/pm_domains.c  Sam Protsenko       2023-03-08   87  	while (exynos_pd_read_status(pd) != pwr) {
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   88  		if (!timeout) {
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   89  			op = (power_on) ? "enable" : "disable";
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   90  			pr_err("Power domain %s %s failed\n", domain->name, op);
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   91  			return -ETIMEDOUT;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   92  		}
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   93  		timeout--;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   94  		cpu_relax();
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   95  		usleep_range(80, 100);
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   96  	}
c760569d0e9c06 arch/arm/mach-exynos/pm_domains.c Prathyush K         2014-07-11   97  
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   98  	return 0;
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27   99  }
91cfbd4ee0875f arch/arm/mach-exynos/pm_domains.c Thomas Abraham      2012-01-27  100  

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

Re: [PATCH 1/6] dt-bindings: power: pd-samsung: Add Exynos850 support

[Krzysztof Kozlowski]

On 09/03/2023 00:09, Sam Protsenko wrote:
> Document the compatible string for Exynos850 power domains controller.
> Also add power domain indices which can be used in "samsung,pd-index"

There is no such property...

> property to specify a particular power domain in the device tree.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  .../devicetree/bindings/power/pd-samsung.yaml   |  1 +
>  MAINTAINERS                                     |  1 +
>  .../dt-bindings/power/samsung,exynos850-power.h | 17 +++++++++++++++++
>  3 files changed, 19 insertions(+)
>  create mode 100644 include/dt-bindings/power/samsung,exynos850-power.h
> 
> diff --git a/Documentation/devicetree/bindings/power/pd-samsung.yaml b/Documentation/devicetree/bindings/power/pd-samsung.yaml
> index 9c2c51133457..a353a705292c 100644
> --- a/Documentation/devicetree/bindings/power/pd-samsung.yaml
> +++ b/Documentation/devicetree/bindings/power/pd-samsung.yaml
> @@ -21,6 +21,7 @@ properties:
>      enum:
>        - samsung,exynos4210-pd
>        - samsung,exynos5433-pd
> +      - samsung,exynos850-pd
>  
>    reg:
>      maxItems: 1
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d5bc223f305..53e11e48639c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2720,6 +2720,7 @@ F:	drivers/pwm/pwm-samsung.c
>  F:	drivers/soc/samsung/
>  F:	drivers/tty/serial/samsung*
>  F:	include/clocksource/samsung_pwm.h
> +F:	include/dt-bindings/power/samsung,*
>  F:	include/linux/platform_data/*s3c*
>  F:	include/linux/serial_s3c.h
>  F:	include/linux/soc/samsung/
> diff --git a/include/dt-bindings/power/samsung,exynos850-power.h b/include/dt-bindings/power/samsung,exynos850-power.h
> new file mode 100644
> index 000000000000..a8d877b5515a
> --- /dev/null
> +++ b/include/dt-bindings/power/samsung,exynos850-power.h

Filename matching compatible, so samsung,exynos850-pd.h... but I
actually wonder why do we need it. How power domains are organized in
Exynos850?

Best regards,
Krzysztof

Re: [PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support

[Marek Szyprowski]

Hi Sam,

On 09.03.2023 00:09, Sam Protsenko wrote:
> Power Domains in Exynos850 are not really different from other Exynos
> platforms. Enabling Exynos850 support in the PD driver is really just a
> matter of adding:
>
>      static const struct exynos_pm_domain_config exynos850_cfg = {
>          .local_pwr_cfg = 0x1,
>      };
>
> to the driver. But in the face of recent developments, e.g. this patch:
>
>      arm64: dts: exynos: move MIPI phy to PMU node in Exynos5433
>
> it looked logical to rework the PD driver a bit to support its nesting
> under the PMU node, while adding Exynos850 support to it. Initially I
> only wanted to add syscon regmap support via some dedicated property,
> but pulling PD nodes under the PMU syscon looks like more correct way.

Frankly speaking if you are changing this, you can go even further. 
Simply make PMU node a PM domain provider and specify the power domain 
as a phandle parameter. This is how it should have been done from the 
beginning, but for some unknown reasons wasn't. There is really no need 
to have a separate node for each power domain. This will also move 
implementation details to the PMU / power domain drivers and it will 
make it much easier to extend/modify it in the future. IMHO same applies 
for PHY nodes.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support

[Krzysztof Kozlowski]

On 09/03/2023 11:12, Marek Szyprowski wrote:
> Hi Sam,
> 
> On 09.03.2023 00:09, Sam Protsenko wrote:
>> Power Domains in Exynos850 are not really different from other Exynos
>> platforms. Enabling Exynos850 support in the PD driver is really just a
>> matter of adding:
>>
>>      static const struct exynos_pm_domain_config exynos850_cfg = {
>>          .local_pwr_cfg = 0x1,
>>      };
>>
>> to the driver. But in the face of recent developments, e.g. this patch:
>>
>>      arm64: dts: exynos: move MIPI phy to PMU node in Exynos5433
>>
>> it looked logical to rework the PD driver a bit to support its nesting
>> under the PMU node, while adding Exynos850 support to it. Initially I
>> only wanted to add syscon regmap support via some dedicated property,
>> but pulling PD nodes under the PMU syscon looks like more correct way.
> 
> Frankly speaking if you are changing this, you can go even further. 
> Simply make PMU node a PM domain provider and specify the power domain 
> as a phandle parameter. This is how it should have been done from the 
> beginning, but for some unknown reasons wasn't. There is really no need 
> to have a separate node for each power domain. This will also move 
> implementation details to the PMU / power domain drivers and it will 
> make it much easier to extend/modify it in the future. IMHO same applies 
> for PHY nodes.

I agree. The "samsung,pd-index" property is not a correct approach.
Either you use address space or not. If not, then this should be part of
power domain provider, which is also matching most of other SoC
architectures.


Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: power: pd-samsung: Allow pd nodes to be children of PMU

[Krzysztof Kozlowski]

On 09/03/2023 00:09, Sam Protsenko wrote:
> Introduce a new "samsung,pd-index" property to choose a specific power
> domain. This way it would be possible to avoid specifying any addresses
> in power domain nodes, relying solely on syscon regmap from the parent
> node (which should be a PMU system controller). Therefore the "reg"
> property is deprecated now, as it's more logical to describe power
> domains as children of PMU node, because PD registers reside in the PMU
> area.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  .../devicetree/bindings/power/pd-samsung.yaml         | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/pd-samsung.yaml b/Documentation/devicetree/bindings/power/pd-samsung.yaml
> index a353a705292c..73178b1a56ea 100644
> --- a/Documentation/devicetree/bindings/power/pd-samsung.yaml
> +++ b/Documentation/devicetree/bindings/power/pd-samsung.yaml
> @@ -25,6 +25,10 @@ properties:
>  
>    reg:
>      maxItems: 1
> +    deprecated: true
> +    description:
> +      Physical base address and length of Power Domains area (if not a child of
> +      PMU).
>  
>    clocks:
>      deprecated: true
> @@ -45,10 +49,15 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  samsung,pd-index:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Power domain index (if a child of PMU). Valid values are defined in::
> +        "include/dt-bindings/power/samsung,exynos850-power.h" - for Exynos850

DT nodes should not have any IDs, except what is in 'reg'. Thus please
go with Marek's proposal of merging power domains into PMU driver and
using proper xlate.

Best regards,
Krzysztof

Re: [PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support

[Sam Protsenko]

On Fri, 10 Mar 2023 at 08:41, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 09/03/2023 11:12, Marek Szyprowski wrote:
> > Hi Sam,
> >
> > On 09.03.2023 00:09, Sam Protsenko wrote:
> >> Power Domains in Exynos850 are not really different from other Exynos
> >> platforms. Enabling Exynos850 support in the PD driver is really just a
> >> matter of adding:
> >>
> >>      static const struct exynos_pm_domain_config exynos850_cfg = {
> >>          .local_pwr_cfg = 0x1,
> >>      };
> >>
> >> to the driver. But in the face of recent developments, e.g. this patch:
> >>
> >>      arm64: dts: exynos: move MIPI phy to PMU node in Exynos5433
> >>
> >> it looked logical to rework the PD driver a bit to support its nesting
> >> under the PMU node, while adding Exynos850 support to it. Initially I
> >> only wanted to add syscon regmap support via some dedicated property,
> >> but pulling PD nodes under the PMU syscon looks like more correct way.
> >
> > Frankly speaking if you are changing this, you can go even further.
> > Simply make PMU node a PM domain provider and specify the power domain
> > as a phandle parameter. This is how it should have been done from the
> > beginning, but for some unknown reasons wasn't. There is really no need
> > to have a separate node for each power domain. This will also move
> > implementation details to the PMU / power domain drivers and it will
> > make it much easier to extend/modify it in the future. IMHO same applies
> > for PHY nodes.
>
> I agree. The "samsung,pd-index" property is not a correct approach.
> Either you use address space or not. If not, then this should be part of
> power domain provider, which is also matching most of other SoC
> architectures.
>

Did a bit of research, looked at how it's implemented on other
platforms. Before I start reworking it, want to check with you on a
couple of decisions, to avoid unnecessary resubmissions later, if it's
ok:

1. Instead of actually merging PD driver into PMU driver, guess it
might be better to create a new power-controller driver (e.g.
drivers/soc/samsung/exynos-power.c). This is how it's implemented for
the most of platforms, and this way we can neatly separate it from
what we already have in the PMU driver (not really power controller
related things). This way, in device tree we'll have a
power-controller node under PMU node, and this node can be referenced
further as a phandle in power-domains properties of users.

2. After moving PD implementation into a new power-controller driver
(with new compatibility string), the old one (pm_domains.c) should be
probably removed. Is it reasonable, e.g. from point of view of
compatibility with out-of-tree (downstream) dts's? Also, if I remove
the PD driver, probably all existing (upstream) Exynos dts's should be
reworked to use the new power-controller compatibility string?

3. Where to keep offsets for each power domain (inside of PMU register
area). Can be done in dts (a separate child node of power-controller
for each power domain), or in the power-controller driver. I saw both
of those ways for different platforms actually. But I guess offsets
are more like internal details, and should be kept inside the driver,
for each supported SoC.

4. Specifying particular power domain in power-domains property. Guess
the best way would be to have some indexes defined in dt-bindings
header, and use those like this:

        power-domains = <&power_controller EXYNOS850_PD_G3D>;

   Those constants can be also used then in the driver, to keep PD
offsets in the array, etc. Another way would be to use reg offsets,
but indices look better: can provide more flexibility in the driver in
future, e.g. if we'd need to add some more details other that offsets
later.

Please let me know what you think. At the moment I have to switch to
another task temporarily. When I get back to this one, discussing the
above items would help me a great deal.

Thanks!

>
> Best regards,
> Krzysztof
>

Re: [PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support

[Krzysztof Kozlowski]

On 20/03/2023 18:57, Sam Protsenko wrote:
> 
> Did a bit of research, looked at how it's implemented on other
> platforms. Before I start reworking it, want to check with you on a
> couple of decisions, to avoid unnecessary resubmissions later, if it's
> ok:
> 
> 1. Instead of actually merging PD driver into PMU driver, guess it
> might be better to create a new power-controller driver (e.g.
> drivers/soc/samsung/exynos-power.c). This is how it's implemented for
> the most of platforms, and this way we can neatly separate it from
> what we already have in the PMU driver (not really power controller
> related things). This way, in device tree we'll have a
> power-controller node under PMU node, and this node can be referenced
> further as a phandle in power-domains properties of users.

Whether you want separate driver is different from wanting separate
device node in DT. About the Devicetree, the PMU node is the power
management unit responsible for:
1. power domains,
2. system power.

Thus separate device node rather does not fit here. This is one bigger
device which can have sub-blocks (e.g. system-reboot), but power domain
handling does not look like separate from it. It is its core.

Now about driver - whatever creates readable and maintainable code :)

> 
> 2. After moving PD implementation into a new power-controller driver
> (with new compatibility string), 
> the old one (pm_domains.c) should be
> probably removed. Is it reasonable, e.g. from point of view of
> compatibility with out-of-tree (downstream) dts's? 

You must not break the ABI, so existing bindings and driver must stay.
The power domain cells in PMU node will be the trigger for using new code.

> Also, if I remove
> the PD driver, probably all existing (upstream) Exynos dts's should be
> reworked to use the new power-controller compatibility string?
> 
> 3. Where to keep offsets for each power domain (inside of PMU register
> area). Can be done in dts (a separate child node of power-controller
> for each power domain), or in the power-controller driver. 

Just like in clock or reset drivers, this should be in the driver.

> I saw both
> of those ways for different platforms actually. 

Then share examples, because maybe I think about something else...

> But I guess offsets
> are more like internal details, and should be kept inside the driver,
> for each supported SoC.
> 
> 4. Specifying particular power domain in power-domains property. Guess
> the best way would be to have some indexes defined in dt-bindings
> header, and use those like this:
> 
>         power-domains = <&power_controller EXYNOS850_PD_G3D>;

Yes.

> 
>    Those constants can be also used then in the driver, to keep PD
> offsets in the array, etc.

Yes.

> Another way would be to use reg offsets,
> but indices look better: can provide more flexibility in the driver in
> future, e.g. if we'd need to add some more details other that offsets
> later.

Right, use logical IDs and driver will do the translation.

> 
> Please let me know what you think. At the moment I have to switch to
> another task temporarily. When I get back to this one, discussing the
> above items would help me a great deal.


Sure, thanks for the work, keep safe and strong!

Best regards,
Krzysztof