[PATCH v2 1/3] regmap: irq: add support for chips who have separate unmask registers

[PATCH v2 1/3] regmap: irq: add support for chips who have separate unmask registers

[Barry Song]

From: Guo Zeng <Guo.Zeng@csr.com>

Some chips have separate unmask registers from mask registers for
some consideration of concurrency SMP write performance. And this
patch adds a flag for it.

An user will be CSR SiRFSoC ARM chips.

Signed-off-by: Guo Zeng <Guo.Zeng@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 drivers/base/regmap/regmap-irq.c | 28 ++++++++++++++++++++++++++--
 include/linux/regmap.h           |  3 +++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 38d1f72..1821c3b 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -63,6 +63,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	struct regmap *map = d->map;
 	int i, ret;
 	u32 reg;
+	u32 unmask_offset;
 
 	if (d->chip->runtime_pm) {
 		ret = pm_runtime_get_sync(map->dev);
@@ -82,7 +83,22 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		if (d->chip->mask_invert)
 			ret = regmap_update_bits(d->map, reg,
 					 d->mask_buf_def[i], ~d->mask_buf[i]);
-		else
+		else if (d->chip->unmask_base) {
+			/* set mask with mask_base register */
+			ret = regmap_update_bits(d->map, reg,
+					d->mask_buf_def[i], ~d->mask_buf[i]);
+			if (ret < 0)
+				dev_err(d->map->dev,
+					"Failed to sync unmasks in %x\n",
+					reg);
+			unmask_offset = d->chip->unmask_base -
+							d->chip->mask_base;
+			/* clear mask with unmask_base register */
+			ret = regmap_update_bits(d->map,
+					reg + unmask_offset,
+					d->mask_buf_def[i],
+					d->mask_buf[i]);
+		} else
 			ret = regmap_update_bits(d->map, reg,
 					 d->mask_buf_def[i], d->mask_buf[i]);
 		if (ret != 0)
@@ -339,6 +355,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	int i;
 	int ret = -ENOMEM;
 	u32 reg;
+	u32 unmask_offset;
 
 	if (chip->num_regs <= 0)
 		return -EINVAL;
@@ -420,7 +437,14 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 		if (chip->mask_invert)
 			ret = regmap_update_bits(map, reg,
 					 d->mask_buf[i], ~d->mask_buf[i]);
-		else
+		else if (d->chip->unmask_base) {
+			unmask_offset = d->chip->unmask_base -
+					d->chip->mask_base;
+			ret = regmap_update_bits(d->map,
+					reg + unmask_offset,
+					d->mask_buf[i],
+					d->mask_buf[i]);
+		} else
 			ret = regmap_update_bits(map, reg,
 					 d->mask_buf[i], d->mask_buf[i]);
 		if (ret != 0) {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 8fc0bfd..f98fe9f 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -800,6 +800,8 @@ struct regmap_irq {
  *
  * @status_base: Base status register address.
  * @mask_base:   Base mask register address.
+ * @unmask_base:  Base unmask register address. for chips who have
+ *                separate mask and unmask registers
  * @ack_base:    Base ack address. If zero then the chip is clear on read.
  *               Using zero value is possible with @use_ack bit.
  * @wake_base:   Base address for wake enables.  If zero unsupported.
@@ -820,6 +822,7 @@ struct regmap_irq_chip {
 
 	unsigned int status_base;
 	unsigned int mask_base;
+	unsigned int unmask_base;
 	unsigned int ack_base;
 	unsigned int wake_base;
 	unsigned int irq_reg_stride;
-- 
1.9.1

[PATCH v2 2/3] regmap: irq: add ack_invert flag for chips using cleared bits as ack

[Barry Song]

From: Guo Zeng <Guo.Zeng@csr.com>

An user will be CSR SiRFSoC ARM chips.

Signed-off-by: Guo Zeng <Guo.Zeng@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 drivers/base/regmap/regmap-irq.c | 12 ++++++++++--
 include/linux/regmap.h           |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 1821c3b..14af2c6 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -132,7 +132,11 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 		if (d->mask_buf[i] && (d->chip->ack_base || d->chip->use_ack)) {
 			reg = d->chip->ack_base +
 				(i * map->reg_stride * d->irq_reg_stride);
-			ret = regmap_write(map, reg, d->mask_buf[i]);
+			/* some chips ack by write 0 */
+			if (d->chip->ack_invert)
+				ret = regmap_write(map, reg, ~d->mask_buf[i]);
+			else
+				ret = regmap_write(map, reg, d->mask_buf[i]);
 			if (ret != 0)
 				dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
 					reg, ret);
@@ -469,7 +473,11 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 		if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
 			reg = chip->ack_base +
 				(i * map->reg_stride * d->irq_reg_stride);
-			ret = regmap_write(map, reg,
+			if (chip->ack_invert)
+				ret = regmap_write(map, reg,
+					~(d->status_buf[i] & d->mask_buf[i]));
+			else
+				ret = regmap_write(map, reg,
 					d->status_buf[i] & d->mask_buf[i]);
 			if (ret != 0) {
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f98fe9f..f36c9f9 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -809,6 +809,7 @@ struct regmap_irq {
  * @init_ack_masked: Ack all masked interrupts once during initalization.
  * @mask_invert: Inverted mask register: cleared bits are masked out.
  * @use_ack:     Use @ack register even if it is zero.
+ * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
  *
@@ -829,6 +830,7 @@ struct regmap_irq_chip {
 	bool init_ack_masked:1;
 	bool mask_invert:1;
 	bool use_ack:1;
+	bool ack_invert:1;
 	bool wake_invert:1;
 	bool runtime_pm:1;
 
-- 
1.9.1

[PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Barry Song]

From: Guo Zeng <Guo.Zeng@csr.com>

CSR SiRFSoC Power Control Module includes power on or power
off for sysctl power and gnss, it also includes onkey, RTC
domain clock controllers and interrupt controller for power
events.

Signed-off-by: Guo Zeng <Guo.Zeng@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 .../devicetree/bindings/mfd/sirf-pwrc.txt          |  37 ++++
 drivers/mfd/Kconfig                                |  12 ++
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/sirfsoc_pwrc.c                         | 238 +++++++++++++++++++++
 include/linux/mfd/sirfsoc_pwrc.h                   |  97 +++++++++
 5 files changed, 386 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
 create mode 100644 drivers/mfd/sirfsoc_pwrc.c
 create mode 100644 include/linux/mfd/sirfsoc_pwrc.h

diff --git a/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
new file mode 100644
index 0000000..b919cdd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
@@ -0,0 +1,37 @@
+SiRFSoC Power Controller (PWRC) module
+
+Power Controller is used to control the whole SoC power process.
+The power controller controls the process of Power-On sequence,
+Power-Off sequence, different power modes switching and power
+status configuration. the pwrc is access by io bridge, so the
+node's parent should be io bridge.
+
+Required properties:
+- compatible: "sirf,prima2-pwrc", or "sirf,atlas7-pwrc"
+- reg: Address range of pwrc register set
+- sysctl:mfd cell device of pwrc
+- rtcm-clk:mfd cell device of pwrc
+- onkey:mfd cell device of pwrc
+
+Example:
+
+pwrc@3000 {
+	compatible = "sirf,atlas7-pwrc";
+	reg = <0x3000 0x100>;
+	sysctl {
+		compatible = "sirf,sirf-sysctl";
+	};
+
+	rtcm-clk {
+		compatible = "sirf,atlas7-rtcmclk";
+	};
+
+	onkey {
+		compatible = "sirf,prima2-onkey";
+	};
+};
+
+pwrc@3000 {
+	compatible = "sirf,prima2-pwrc";
+	reg = <0x3000 0x100>;
+};
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 99d6367..5b67bee 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1515,5 +1515,17 @@ config MFD_VEXPRESS_SYSREG
 	  System Registers are the platform configuration block
 	  on the ARM Ltd. Versatile Express board.
 
+config MFD_SIRFSOC_PWRC
+        bool "CSR SiRFSoC on-chip Power Control Module"
+	depends on ARCH_SIRF
+	default y
+        select MFD_CORE
+        select REGMAP_IRQ
+        help
+         CSR SiRFSoC Power Control Module includes power on or power
+	 off for sysctl power and gnss, it also includes onkey, RTC
+	 domain clock controllers and interrupt controller for power
+	 events.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a59e3fc..255fb80 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -192,3 +192,5 @@ obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
 obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
+
+obj-$(CONFIG_MFD_SIRFSOC_PWRC)	+= sirfsoc_pwrc.o
diff --git a/drivers/mfd/sirfsoc_pwrc.c b/drivers/mfd/sirfsoc_pwrc.c
new file mode 100644
index 0000000..b43f8b4
--- /dev/null
+++ b/drivers/mfd/sirfsoc_pwrc.c
@@ -0,0 +1,238 @@
+/*
+ * power management mfd for CSR SiRFSoC chips
+ *
+ * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/regmap.h>
+#include <linux/module.h>
+#include <linux/rtc/sirfsoc_rtciobrg.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/sirfsoc_pwrc.h>
+
+struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc = {
+	.pwrc_pdn_ctrl_set = 0x0,
+	.pwrc_pdn_ctrl_clr = 0x4,
+	.pwrc_pon_status = 0x8,
+	.pwrc_trigger_en_set = 0xc,
+	.pwrc_trigger_en_clr = 0x10,
+	.pwrc_int_mask_set = 0x14,
+	.pwrc_int_mask_clr = 0x18,
+	.pwrc_int_status = 0x1c,
+	.pwrc_pin_status = 0x20,
+	.pwrc_rtc_pll_ctrl = 0x28,
+	.pwrc_gpio3_debug = 0x34,
+	.pwrc_rtc_noc_pwrctl_set = 0x38,
+	.pwrc_rtc_noc_pwrctl_clr = 0x3c,
+	.pwrc_rtc_can_ctrl = 0x48,
+	.pwrc_rtc_can_status = 0x4c,
+	.pwrc_fsm_m3_ctrl = 0x50,
+	.pwrc_fsm_state = 0x54,
+	.pwrc_rtcldo_reg = 0x58,
+	.pwrc_gnss_ctrl = 0x5c,
+	.pwrc_gnss_status = 0x60,
+	.pwrc_xtal_reg = 0x64,
+	.pwrc_xtal_ldo_mux_sel = 0x68,
+	.pwrc_rtc_sw_rstc_set = 0x6c,
+	.pwrc_rtc_sw_rstc_clr = 0x70,
+	.pwrc_power_sw_ctrl_set = 0x74,
+	.pwrc_power_sw_ctrl_clr = 0x78,
+	.pwrc_rtc_dcog = 0x7c,
+	.pwrc_m3_memories = 0x80,
+	.pwrc_can0_memory = 0x84,
+	.pwrc_rtc_gnss_memory = 0x88,
+	.pwrc_m3_clk_en = 0x8c,
+	.pwrc_can0_clk_en = 0x90,
+	.pwrc_spi0_clk_en = 0x94,
+	.pwrc_rtc_sec_clk_en = 0x98,
+	.pwrc_rtc_noc_clk_en = 0x9c,
+};
+
+struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc = {
+	.pwrc_pdn_ctrl_set = 0x0,
+	.pwrc_pon_status = 0x4,
+	.pwrc_trigger_en_set = 0x8,
+	.pwrc_int_status = 0xc,
+	.pwrc_int_mask_set = 0x10,
+	.pwrc_pin_status = 0x14,
+	.pwrc_scratch_pad1 = 0x18,
+	.pwrc_scratch_pad2 = 0x1c,
+	.pwrc_scratch_pad3 = 0x20,
+	.pwrc_scratch_pad4 = 0x24,
+	.pwrc_scratch_pad5 = 0x28,
+	.pwrc_scratch_pad6 = 0x2c,
+	.pwrc_scratch_pad7 = 0x30,
+	.pwrc_scratch_pad8 = 0x34,
+	.pwrc_scratch_pad9 = 0x38,
+	.pwrc_scratch_pad10 = 0x3c,
+	.pwrc_scratch_pad11 = 0x40,
+	.pwrc_scratch_pad12 = 0x44,
+	.pwrc_gpio3_clk = 0x54,
+	.pwrc_gpio_ds = 0x78,
+};
+
+static const struct regmap_irq pwrc_irqs[] = {
+	/* INT0 */
+	[PWRC_IRQ_ONKEY] = {
+		.mask = BIT(PWRC_IRQ_ONKEY),
+	},
+	[PWRC_IRQ_EXT_ONKEY] = {
+		.mask = BIT(PWRC_IRQ_EXT_ONKEY),
+	},
+};
+
+static struct regmap_irq_chip pwrc_irq_chip = {
+	.name = "pwrc_irq",
+	.irqs = pwrc_irqs,
+	.num_irqs = ARRAY_SIZE(pwrc_irqs),
+	.num_regs = 1,
+	.ack_invert = 1,
+	.init_ack_masked = true,
+};
+
+static const struct of_device_id pwrc_ids[] = {
+	{ .compatible = "sirf,prima2-pwrc", .data = &sirfsoc_prima2_pwrc},
+	{ .compatible = "sirf,atlas7-pwrc", .data = &sirfsoc_a7da_pwrc},
+	{}
+};
+
+static const struct mfd_cell pwrc_devs[] = {
+	{
+		.name = "rtcmclk",
+		.of_compatible = "sirf,atlas7-rtcmclk",
+	}, {
+		.name = "sirf-sysctl",
+		.of_compatible = "sirf,sirf-sysctl",
+	}, {
+		.name = "gps-power",
+		.of_compatible = "sirf,gps-power",
+	}, {
+		.name = "onkey",
+		.of_compatible = "sirf,prima2-onkey",
+	},
+};
+
+static const struct regmap_config pwrc_regmap_config = {
+	.fast_io = true,
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static int sirfsoc_pwrc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct sirfsoc_pwrc_info *pwrcinfo;
+	struct regmap_irq_chip *regmap_irq_chip;
+	struct sirfsoc_pwrc_register *pwrc_reg;
+	struct regmap *map;
+	int ret;
+	u32 base;
+
+	if (of_property_read_u32(np, "reg", &base))
+		panic("unable to find base address of pwrc node in dtb\n");
+
+	pwrcinfo = devm_kzalloc(&pdev->dev,
+			sizeof(struct sirfsoc_pwrc_info), GFP_KERNEL);
+	if (!pwrcinfo)
+		return -ENOMEM;
+	pwrcinfo->base = base;
+
+	/*
+	 * pwrc behind rtciobrg offset is diff between prima2 and atlas7
+	 * here match to each ids data for it.
+	 */
+	match = of_match_node(pwrc_ids, np);
+	pwrcinfo->pwrc_reg = (struct sirfsoc_pwrc_register *)match->data;
+
+	if (of_device_is_compatible(np, "sirf,atlas7-pwrc"))
+		pwrcinfo->ver = PWRC_ATLAS7_VER;
+	else if (of_device_is_compatible(np, "sirf,prima2-pwrc"))
+		pwrcinfo->ver = PWRC_PRIMA2_VER;
+	else
+		return -EINVAL;
+
+	of_node_put(np);
+
+	map = (struct regmap *)devm_regmap_init_iobg(&pdev->dev,
+			&pwrc_regmap_config);
+	if (IS_ERR(map)) {
+		ret = PTR_ERR(map);
+		dev_err(&pdev->dev, "Failed to allocate register map: %d\n",
+			ret);
+		goto err;
+	}
+
+	pwrcinfo->regmap = map;
+	pwrcinfo->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pwrcinfo);
+
+	ret = mfd_add_devices(pwrcinfo->dev, 0, pwrc_devs,
+		ARRAY_SIZE(pwrc_devs), NULL, 0, NULL);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add all pwrc subdev\n");
+		goto err;
+	}
+
+	ret = of_irq_get(pdev->dev.of_node, 0);
+	if (ret <= 0) {
+		dev_info(&pdev->dev,
+			"Unable to find IRQ for pwrc. ret=%d\n", ret);
+		goto err_irq;
+	}
+
+	pwrcinfo->irq = ret;
+	regmap_irq_chip = &pwrc_irq_chip;
+	pwrcinfo->regmap_irq_chip = regmap_irq_chip;
+
+	pwrc_reg = pwrcinfo->pwrc_reg;
+	regmap_irq_chip->mask_base = pwrcinfo->base +
+						pwrc_reg->pwrc_int_mask_set;
+	regmap_irq_chip->unmask_base = pwrcinfo->base +
+				pwrc_reg->pwrc_int_mask_clr;
+	regmap_irq_chip->status_base = pwrcinfo->base +
+						pwrc_reg->pwrc_int_status;
+	regmap_irq_chip->ack_base = pwrcinfo->base +
+						pwrc_reg->pwrc_int_status;
+
+	/* enable onkey trigger interrupt controller */
+	ret = regmap_update_bits(map,
+			pwrcinfo->base +
+			pwrc_reg->pwrc_trigger_en_set,
+			BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY),
+			BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY));
+	if (ret < 0)
+		goto err_irq;
+
+	/* add irq controller for pwrc */
+	ret = regmap_add_irq_chip(map, pwrcinfo->irq, IRQF_ONESHOT,
+				-1, pwrcinfo->regmap_irq_chip,
+				&pwrcinfo->irq_data);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add regmap irq controller for pwrc\n");
+		goto err;
+	}
+
+	return 0;
+err_irq:
+	mfd_remove_devices(pwrcinfo->dev);
+err:
+	return ret;
+}
+
+static struct platform_driver sirfsoc_pwrc_driver = {
+	.probe	= sirfsoc_pwrc_probe,
+	.driver	= {
+		.name = "sirfsoc_pwrc",
+		.of_match_table = pwrc_ids,
+	},
+};
+module_platform_driver(sirfsoc_pwrc_driver);
diff --git a/include/linux/mfd/sirfsoc_pwrc.h b/include/linux/mfd/sirfsoc_pwrc.h
new file mode 100644
index 0000000..ee2b3d5
--- /dev/null
+++ b/include/linux/mfd/sirfsoc_pwrc.h
@@ -0,0 +1,97 @@
+/*
+ * CSR SiRFSoC power control module MFD interface
+ *
+ * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
+ *
+ * Licensed under GPLv2 or later.
+ */
+#ifndef _SIRFSOC_PWRC_H_
+#define _SIRFSOC_PWRC_H_
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+
+#define PWRC_PDN_CTRL_OFFSET	0
+#define AUDIO_POWER_EN_BIT	14
+
+struct sirfsoc_pwrc_register {
+	/* hardware pwrc specific */
+	u32 pwrc_pdn_ctrl_set;
+	u32 pwrc_pdn_ctrl_clr;
+	u32 pwrc_pon_status;
+	u32 pwrc_trigger_en_set;
+	u32 pwrc_trigger_en_clr;
+	u32 pwrc_int_mask_set;
+	u32 pwrc_int_mask_clr;
+	u32 pwrc_int_status;
+	u32 pwrc_pin_status;
+	u32 pwrc_rtc_pll_ctrl;
+	u32 pwrc_gpio3_debug;
+	u32 pwrc_rtc_noc_pwrctl_set;
+	u32 pwrc_rtc_noc_pwrctl_clr;
+	u32 pwrc_rtc_can_ctrl;
+	u32 pwrc_rtc_can_status;
+	u32 pwrc_fsm_m3_ctrl;
+	u32 pwrc_fsm_state;
+	u32 pwrc_rtcldo_reg;
+	u32 pwrc_gnss_ctrl;
+	u32 pwrc_gnss_status;
+	u32 pwrc_xtal_reg;
+	u32 pwrc_xtal_ldo_mux_sel;
+	u32 pwrc_rtc_sw_rstc_set;
+	u32 pwrc_rtc_sw_rstc_clr;
+	u32 pwrc_power_sw_ctrl_set;
+	u32 pwrc_power_sw_ctrl_clr;
+	u32 pwrc_rtc_dcog;
+	u32 pwrc_m3_memories;
+	u32 pwrc_can0_memory;
+	u32 pwrc_rtc_gnss_memory;
+	u32 pwrc_m3_clk_en;
+	u32 pwrc_can0_clk_en;
+	u32 pwrc_spi0_clk_en;
+	u32 pwrc_rtc_sec_clk_en;
+	u32 pwrc_rtc_noc_clk_en;
+
+	/*only for prima2*/
+	u32 pwrc_scratch_pad1;
+	u32 pwrc_scratch_pad2;
+	u32 pwrc_scratch_pad3;
+	u32 pwrc_scratch_pad4;
+	u32 pwrc_scratch_pad5;
+	u32 pwrc_scratch_pad6;
+	u32 pwrc_scratch_pad7;
+	u32 pwrc_scratch_pad8;
+	u32 pwrc_scratch_pad9;
+	u32 pwrc_scratch_pad10;
+	u32 pwrc_scratch_pad11;
+	u32 pwrc_scratch_pad12;
+	u32 pwrc_gpio3_clk;
+	u32 pwrc_gpio_ds;
+
+};
+
+enum pwrc_version {
+	PWRC_MARCO_VER,
+	PWRC_PRIMA2_VER,
+	PWRC_ATLAS7_VER,
+};
+
+struct sirfsoc_pwrc_info {
+	struct device *dev;
+	struct regmap *regmap;
+	struct sirfsoc_pwrc_register *pwrc_reg;
+	struct regmap_irq_chip *regmap_irq_chip;
+	struct regmap_irq_chip_data *irq_data;
+	u32 ver;
+	u32 base;
+	int irq;
+};
+
+enum {
+	PWRC_IRQ_ONKEY = 0,
+	PWRC_IRQ_EXT_ONKEY,
+	PWRC_MAX_IRQ,
+};
+
+extern struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc;
+extern struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc;
+#endif
-- 
1.9.1

Re: [PATCH v2 1/3] regmap: irq: add support for chips who have separate unmask registers

[Mark Brown]

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

On Thu, Sep 17, 2015 at 05:23:20AM +0000, Barry Song wrote:
> From: Guo Zeng <Guo.Zeng@csr.com>
> 
> Some chips have separate unmask registers from mask registers for
> some consideration of concurrency SMP write performance. And this
> patch adds a flag for it.

I've pushed a tag regmap-irq-unmask so these can be pulled into another
tree if needed (like for the MFD).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 1/3] regmap: irq: add support for chips who have separate unmask registers

[Barry Song]

2015-09-17 18:53 GMT+08:00 Mark Brown <broonie@kernel.org>:
> On Thu, Sep 17, 2015 at 05:23:20AM +0000, Barry Song wrote:
>> From: Guo Zeng <Guo.Zeng@csr.com>
>>
>> Some chips have separate unmask registers from mask registers for
>> some consideration of concurrency SMP write performance. And this
>> patch adds a flag for it.
>
> I've pushed a tag regmap-irq-unmask so these can be pulled into another
> tree if needed (like for the MFD).

thanks, Mark, found it in your tree
https://git.kernel.org/cgit/linux/kernel/git/broonie/regmap.git/

-barry

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Lee Jones]

On Thu, 17 Sep 2015, Barry Song wrote:

> From: Guo Zeng <Guo.Zeng@csr.com>
> 
> CSR SiRFSoC Power Control Module includes power on or power
> off for sysctl power and gnss, it also includes onkey, RTC
> domain clock controllers and interrupt controller for power
> events.

There are lots of Three (and four) Letter Abbreviations (TLAs) here,
which mean nothing to the average reader.  Please break them out in
the commit log as I have done i.e. "Long Abbreviated Word (LAW)", so
us normies can see what they mean.

> Signed-off-by: Guo Zeng <Guo.Zeng@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
>  .../devicetree/bindings/mfd/sirf-pwrc.txt          |  37 ++++

This should be in a separate patch.

>  drivers/mfd/Kconfig                                |  12 ++
>  drivers/mfd/Makefile                               |   2 +
>  drivers/mfd/sirfsoc_pwrc.c                         | 238 +++++++++++++++++++++
>  include/linux/mfd/sirfsoc_pwrc.h                   |  97 +++++++++
>  5 files changed, 386 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
>  create mode 100644 drivers/mfd/sirfsoc_pwrc.c
>  create mode 100644 include/linux/mfd/sirfsoc_pwrc.h
> 
> diff --git a/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
> new file mode 100644
> index 0000000..b919cdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
> @@ -0,0 +1,37 @@
> +SiRFSoC Power Controller (PWRC) module
> +
> +Power Controller is used to control the whole SoC power process.
> +The power controller controls the process of Power-On sequence,

s/power controller/Power Controller/

> +Power-Off sequence, different power modes switching and power
> +status configuration. the pwrc is access by io bridge, so the

s/the pwrc/The PWRC/

s/access/accessed/

s/io/IO/

> +node's parent should be io bridge.

s/io/IO/

Not quite sure what an IO bridge is though.

> +Required properties:
> +- compatible: "sirf,prima2-pwrc", or "sirf,atlas7-pwrc"
> +- reg: Address range of pwrc register set

Address start and size.

> +- sysctl:mfd cell device of pwrc
> +- rtcm-clk:mfd cell device of pwrc
> +- onkey:mfd cell device of pwrc

MFD is a Linuxisum.  Please find another way to document them.

I always find documentation easier to read then it's tabbed out
nicely.  Like:

Required properties:
- compatible	: "sirf,prima2-pwrc", or "sirf,atlas7-pwrc"
- reg		: Address range of pwrc register set
- sysctl	: mfd cell device of pwrc
- rtcm-clk	: mfd cell device of pwrc
- onkey		: mfd cell device of pwrc

> +Example:
> +
> +pwrc@3000 {
> +	compatible = "sirf,atlas7-pwrc";
> +	reg = <0x3000 0x100>;

This doesn't look like a real address.  It looks like an offset.
Please provide a full example, complete with the parent node (which I
assume has ranges set-up).

> +	sysctl {
> +		compatible = "sirf,sirf-sysctl";
> +	};
> +
> +	rtcm-clk {
> +		compatible = "sirf,atlas7-rtcmclk";
> +	};
> +
> +	onkey {
> +		compatible = "sirf,prima2-onkey";
> +	};

Why do these require their own cells?

Do they have their own properties?  If so, where are they documented?

> +};
> +
> +pwrc@3000 {
> +	compatible = "sirf,prima2-pwrc";
> +	reg = <0x3000 0x100>;
> +};
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 99d6367..5b67bee 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1515,5 +1515,17 @@ config MFD_VEXPRESS_SYSREG
>  	  System Registers are the platform configuration block
>  	  on the ARM Ltd. Versatile Express board.
>  
> +config MFD_SIRFSOC_PWRC
> +        bool "CSR SiRFSoC on-chip Power Control Module"
> +	depends on ARCH_SIRF
> +	default y
> +        select MFD_CORE
> +        select REGMAP_IRQ
> +        help
> +         CSR SiRFSoC Power Control Module includes power on or power
> +	 off for sysctl power and gnss, it also includes onkey, RTC
> +	 domain clock controllers and interrupt controller for power
> +	 events.

Break out the TLAs.

Your tabbing is all over the place here.  Please match existing
entries.

>  endmenu
>  endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a59e3fc..255fb80 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -192,3 +192,5 @@ obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
>  obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
> +
> +obj-$(CONFIG_MFD_SIRFSOC_PWRC)	+= sirfsoc_pwrc.o
> diff --git a/drivers/mfd/sirfsoc_pwrc.c b/drivers/mfd/sirfsoc_pwrc.c
> new file mode 100644
> index 0000000..b43f8b4
> --- /dev/null
> +++ b/drivers/mfd/sirfsoc_pwrc.c
> @@ -0,0 +1,238 @@
> +/*
> + * power management mfd for CSR SiRFSoC chips

"Power Management MFD for CSR SiRFSoC chips"

> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.

This is out of date.

> + * Licensed under GPLv2 or later.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +#include <linux/regmap.h>
> +#include <linux/module.h>

This isn't a module.

> +#include <linux/rtc/sirfsoc_rtciobrg.h>

What's this for?

> +#include <linux/mfd/core.h>
> +#include <linux/mfd/sirfsoc_pwrc.h>

Header files should be in alphabetical order.

> +struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc = {
> +	.pwrc_pdn_ctrl_set = 0x0,
> +	.pwrc_pdn_ctrl_clr = 0x4,
> +	.pwrc_pon_status = 0x8,
> +	.pwrc_trigger_en_set = 0xc,
> +	.pwrc_trigger_en_clr = 0x10,
> +	.pwrc_int_mask_set = 0x14,
> +	.pwrc_int_mask_clr = 0x18,
> +	.pwrc_int_status = 0x1c,
> +	.pwrc_pin_status = 0x20,
> +	.pwrc_rtc_pll_ctrl = 0x28,
> +	.pwrc_gpio3_debug = 0x34,
> +	.pwrc_rtc_noc_pwrctl_set = 0x38,
> +	.pwrc_rtc_noc_pwrctl_clr = 0x3c,
> +	.pwrc_rtc_can_ctrl = 0x48,
> +	.pwrc_rtc_can_status = 0x4c,
> +	.pwrc_fsm_m3_ctrl = 0x50,
> +	.pwrc_fsm_state = 0x54,
> +	.pwrc_rtcldo_reg = 0x58,
> +	.pwrc_gnss_ctrl = 0x5c,
> +	.pwrc_gnss_status = 0x60,
> +	.pwrc_xtal_reg = 0x64,
> +	.pwrc_xtal_ldo_mux_sel = 0x68,
> +	.pwrc_rtc_sw_rstc_set = 0x6c,
> +	.pwrc_rtc_sw_rstc_clr = 0x70,
> +	.pwrc_power_sw_ctrl_set = 0x74,
> +	.pwrc_power_sw_ctrl_clr = 0x78,
> +	.pwrc_rtc_dcog = 0x7c,
> +	.pwrc_m3_memories = 0x80,
> +	.pwrc_can0_memory = 0x84,
> +	.pwrc_rtc_gnss_memory = 0x88,
> +	.pwrc_m3_clk_en = 0x8c,
> +	.pwrc_can0_clk_en = 0x90,
> +	.pwrc_spi0_clk_en = 0x94,
> +	.pwrc_rtc_sec_clk_en = 0x98,
> +	.pwrc_rtc_noc_clk_en = 0x9c,
> +};
> +
> +struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc = {
> +	.pwrc_pdn_ctrl_set = 0x0,
> +	.pwrc_pon_status = 0x4,
> +	.pwrc_trigger_en_set = 0x8,
> +	.pwrc_int_status = 0xc,
> +	.pwrc_int_mask_set = 0x10,
> +	.pwrc_pin_status = 0x14,
> +	.pwrc_scratch_pad1 = 0x18,
> +	.pwrc_scratch_pad2 = 0x1c,
> +	.pwrc_scratch_pad3 = 0x20,
> +	.pwrc_scratch_pad4 = 0x24,
> +	.pwrc_scratch_pad5 = 0x28,
> +	.pwrc_scratch_pad6 = 0x2c,
> +	.pwrc_scratch_pad7 = 0x30,
> +	.pwrc_scratch_pad8 = 0x34,
> +	.pwrc_scratch_pad9 = 0x38,
> +	.pwrc_scratch_pad10 = 0x3c,
> +	.pwrc_scratch_pad11 = 0x40,
> +	.pwrc_scratch_pad12 = 0x44,
> +	.pwrc_gpio3_clk = 0x54,
> +	.pwrc_gpio_ds = 0x78,
> +};

This is not the way we usually define register addresses.

Please #define them properly.

> +static const struct regmap_irq pwrc_irqs[] = {
> +	/* INT0 */
> +	[PWRC_IRQ_ONKEY] = {
> +		.mask = BIT(PWRC_IRQ_ONKEY),

Better to do the BIT() shifting in the header file.

> +	},
> +	[PWRC_IRQ_EXT_ONKEY] = {
> +		.mask = BIT(PWRC_IRQ_EXT_ONKEY),
> +	},
> +};
> +
> +static struct regmap_irq_chip pwrc_irq_chip = {
> +	.name = "pwrc_irq",
> +	.irqs = pwrc_irqs,
> +	.num_irqs = ARRAY_SIZE(pwrc_irqs),
> +	.num_regs = 1,
> +	.ack_invert = 1,
> +	.init_ack_masked = true,
> +};
> +
> +static const struct of_device_id pwrc_ids[] = {
> +	{ .compatible = "sirf,prima2-pwrc", .data = &sirfsoc_prima2_pwrc},
> +	{ .compatible = "sirf,atlas7-pwrc", .data = &sirfsoc_a7da_pwrc},
> +	{}
> +};

Please put these _just_ before you use them, so just above .probe() in
this case.

> +static const struct mfd_cell pwrc_devs[] = {
> +	{
> +		.name = "rtcmclk",
> +		.of_compatible = "sirf,atlas7-rtcmclk",
> +	}, {
> +		.name = "sirf-sysctl",
> +		.of_compatible = "sirf,sirf-sysctl",
> +	}, {
> +		.name = "gps-power",
> +		.of_compatible = "sirf,gps-power",
> +	}, {
> +		.name = "onkey",
> +		.of_compatible = "sirf,prima2-onkey",
> +	},
> +};
> +
> +static const struct regmap_config pwrc_regmap_config = {
> +	.fast_io = true,
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct of_device_id *match;
> +	struct sirfsoc_pwrc_info *pwrcinfo;
> +	struct regmap_irq_chip *regmap_irq_chip;
> +	struct sirfsoc_pwrc_register *pwrc_reg;
> +	struct regmap *map;
> +	int ret;
> +	u32 base;
> +
> +	if (of_property_read_u32(np, "reg", &base))
> +		panic("unable to find base address of pwrc node in dtb\n");

It looks like this driver should depend on OF.

Why are you obtaining the base address manually? Use:

  res = platform_get_resource();
  devm_ioremap_resource(res);

... instead.

> +	pwrcinfo = devm_kzalloc(&pdev->dev,
> +			sizeof(struct sirfsoc_pwrc_info), GFP_KERNEL);

sizeof(pwrcinfo)

> +	if (!pwrcinfo)
> +		return -ENOMEM;
> +	pwrcinfo->base = base;
> +
> +	/*
> +	 * pwrc behind rtciobrg offset is diff between prima2 and atlas7
> +	 * here match to each ids data for it.
> +	 */

Use uppercase characters for abbreviations.

Besides, I have no idea what you're trying to say.

> +	match = of_match_node(pwrc_ids, np);
> +	pwrcinfo->pwrc_reg = (struct sirfsoc_pwrc_register *)match->data;

It looks like you're trying to use the same variables two different
device's register sets.  That's asking for trouble, please don't do
that.

> +	if (of_device_is_compatible(np, "sirf,atlas7-pwrc"))
> +		pwrcinfo->ver = PWRC_ATLAS7_VER;
> +	else if (of_device_is_compatible(np, "sirf,prima2-pwrc"))
> +		pwrcinfo->ver = PWRC_PRIMA2_VER;
> +	else
> +		return -EINVAL;

These aren't versions, are they?  It's different hardware.

Can't you probe for this at runtime?

> +	of_node_put(np);

What are you putting here?

> +	map = (struct regmap *)devm_regmap_init_iobg(&pdev->dev,
> +			&pwrc_regmap_config);

Why are you casting?

> +	if (IS_ERR(map)) {
> +		ret = PTR_ERR(map);
> +		dev_err(&pdev->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		goto err;

Please remove the 'err' label and just return from here.

> +	}
> +
> +	pwrcinfo->regmap = map;
> +	pwrcinfo->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, pwrcinfo);
> +
> +	ret = mfd_add_devices(pwrcinfo->dev, 0, pwrc_devs,
> +		ARRAY_SIZE(pwrc_devs), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add all pwrc subdev\n");

Capitalise all abbreviations.

Don't abbreviate things like "sub devices".

> +		goto err;

Just return.

> +	}
> +
> +	ret = of_irq_get(pdev->dev.of_node, 0);

You already put this in to a succinct variable 'np'.  Please use it.

> +	if (ret <= 0) {
> +		dev_info(&pdev->dev,
> +			"Unable to find IRQ for pwrc. ret=%d\n", ret);

Just print ret.  No need for the ugly "ret=".

> +		goto err_irq;
> +	}
> +
> +	pwrcinfo->irq = ret;

Better change this to 'irq' instead of using 'ret'.

> +	regmap_irq_chip = &pwrc_irq_chip;
> +	pwrcinfo->regmap_irq_chip = regmap_irq_chip;
> +
> +	pwrc_reg = pwrcinfo->pwrc_reg;
> +	regmap_irq_chip->mask_base = pwrcinfo->base +
> +						pwrc_reg->pwrc_int_mask_set;
> +	regmap_irq_chip->unmask_base = pwrcinfo->base +
> +				pwrc_reg->pwrc_int_mask_clr;
> +	regmap_irq_chip->status_base = pwrcinfo->base +
> +						pwrc_reg->pwrc_int_status;
> +	regmap_irq_chip->ack_base = pwrcinfo->base +
> +						pwrc_reg->pwrc_int_status;

This is ugly.

Better to create 2 regmap_irq_chip structures, one for each device.

> +	/* enable onkey trigger interrupt controller */

Capital letters to start a sentence. 

> +	ret = regmap_update_bits(map,
> +			pwrcinfo->base +
> +			pwrc_reg->pwrc_trigger_en_set,
> +			BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY),
> +			BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY));
> +	if (ret < 0)
> +		goto err_irq;
> +
> +	/* add irq controller for pwrc */

Capital letters to start a sentence and for abbreviations.

> +	ret = regmap_add_irq_chip(map, pwrcinfo->irq, IRQF_ONESHOT,
> +				-1, pwrcinfo->regmap_irq_chip,
> +				&pwrcinfo->irq_data);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add regmap irq controller for pwrc\n");
> +		goto err;
> +	}
> +
> +	return 0;
> +err_irq:
> +	mfd_remove_devices(pwrcinfo->dev);
> +err:

Remove this label, it's not helpful.

> +	return ret;
> +}
> +
> +static struct platform_driver sirfsoc_pwrc_driver = {
> +	.probe	= sirfsoc_pwrc_probe,

.remove?

> +	.driver	= {
> +		.name = "sirfsoc_pwrc",
> +		.of_match_table = pwrc_ids,

of_match_ptr()

> +	},
> +};
> +module_platform_driver(sirfsoc_pwrc_driver);

This isn't a module.

> diff --git a/include/linux/mfd/sirfsoc_pwrc.h b/include/linux/mfd/sirfsoc_pwrc.h
> new file mode 100644
> index 0000000..ee2b3d5
> --- /dev/null
> +++ b/include/linux/mfd/sirfsoc_pwrc.h
> @@ -0,0 +1,97 @@
> +/*
> + * CSR SiRFSoC power control module MFD interface
> + *
> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.

Out of date.

> + * Licensed under GPLv2 or later.
> + */

'\n' here.

> +#ifndef _SIRFSOC_PWRC_H_
> +#define _SIRFSOC_PWRC_H_

'\n' here.

> +#include <linux/interrupt.h>

What's this for?

> +#include <linux/regmap.h>
> +
> +#define PWRC_PDN_CTRL_OFFSET	0
> +#define AUDIO_POWER_EN_BIT	14
> +
> +struct sirfsoc_pwrc_register {
> +	/* hardware pwrc specific */
> +	u32 pwrc_pdn_ctrl_set;
> +	u32 pwrc_pdn_ctrl_clr;
> +	u32 pwrc_pon_status;
> +	u32 pwrc_trigger_en_set;
> +	u32 pwrc_trigger_en_clr;
> +	u32 pwrc_int_mask_set;
> +	u32 pwrc_int_mask_clr;
> +	u32 pwrc_int_status;
> +	u32 pwrc_pin_status;
> +	u32 pwrc_rtc_pll_ctrl;
> +	u32 pwrc_gpio3_debug;
> +	u32 pwrc_rtc_noc_pwrctl_set;
> +	u32 pwrc_rtc_noc_pwrctl_clr;
> +	u32 pwrc_rtc_can_ctrl;
> +	u32 pwrc_rtc_can_status;
> +	u32 pwrc_fsm_m3_ctrl;
> +	u32 pwrc_fsm_state;
> +	u32 pwrc_rtcldo_reg;
> +	u32 pwrc_gnss_ctrl;
> +	u32 pwrc_gnss_status;
> +	u32 pwrc_xtal_reg;
> +	u32 pwrc_xtal_ldo_mux_sel;
> +	u32 pwrc_rtc_sw_rstc_set;
> +	u32 pwrc_rtc_sw_rstc_clr;
> +	u32 pwrc_power_sw_ctrl_set;
> +	u32 pwrc_power_sw_ctrl_clr;
> +	u32 pwrc_rtc_dcog;
> +	u32 pwrc_m3_memories;
> +	u32 pwrc_can0_memory;
> +	u32 pwrc_rtc_gnss_memory;
> +	u32 pwrc_m3_clk_en;
> +	u32 pwrc_can0_clk_en;
> +	u32 pwrc_spi0_clk_en;
> +	u32 pwrc_rtc_sec_clk_en;
> +	u32 pwrc_rtc_noc_clk_en;
> +
> +	/*only for prima2*/
> +	u32 pwrc_scratch_pad1;
> +	u32 pwrc_scratch_pad2;
> +	u32 pwrc_scratch_pad3;
> +	u32 pwrc_scratch_pad4;
> +	u32 pwrc_scratch_pad5;
> +	u32 pwrc_scratch_pad6;
> +	u32 pwrc_scratch_pad7;
> +	u32 pwrc_scratch_pad8;
> +	u32 pwrc_scratch_pad9;
> +	u32 pwrc_scratch_pad10;
> +	u32 pwrc_scratch_pad11;
> +	u32 pwrc_scratch_pad12;
> +	u32 pwrc_gpio3_clk;
> +	u32 pwrc_gpio_ds;
> +
> +};

Please #define these instead.

> +enum pwrc_version {
> +	PWRC_MARCO_VER,
> +	PWRC_PRIMA2_VER,
> +	PWRC_ATLAS7_VER,
> +};

Kerneldoc header?

> +struct sirfsoc_pwrc_info {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct sirfsoc_pwrc_register *pwrc_reg;
> +	struct regmap_irq_chip *regmap_irq_chip;
> +	struct regmap_irq_chip_data *irq_data;

Where are these reused?

> +	u32 ver;

What is this used for?

> +	u32 base;

Is this a u32 or a memory address?

> +	int irq;
> +};
> +
> +enum {
> +	PWRC_IRQ_ONKEY = 0,
> +	PWRC_IRQ_EXT_ONKEY,
> +	PWRC_MAX_IRQ,
> +};
> +
> +extern struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc;
> +extern struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc;
> +#endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Barry Song]

hi Lee,
 thanks very much!

2015-09-20 12:15 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> On Thu, 17 Sep 2015, Barry Song wrote:
>
>> From: Guo Zeng <Guo.Zeng@csr.com>
>>
>> CSR SiRFSoC Power Control Module includes power on or power
>> off for sysctl power and gnss, it also includes onkey, RTC
>> domain clock controllers and interrupt controller for power
>> events.
>
> There are lots of Three (and four) Letter Abbreviations (TLAs) here,
> which mean nothing to the average reader.  Please break them out in
> the commit log as I have done i.e. "Long Abbreviated Word (LAW)", so
> us normies can see what they mean.
>
>> Signed-off-by: Guo Zeng <Guo.Zeng@csr.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>>  .../devicetree/bindings/mfd/sirf-pwrc.txt          |  37 ++++
>
> This should be in a separate patch.
>
>>  drivers/mfd/Kconfig                                |  12 ++
>>  drivers/mfd/Makefile                               |   2 +
>>  drivers/mfd/sirfsoc_pwrc.c                         | 238 +++++++++++++++++++++
>>  include/linux/mfd/sirfsoc_pwrc.h                   |  97 +++++++++
>>  5 files changed, 386 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
>>  create mode 100644 drivers/mfd/sirfsoc_pwrc.c
>>  create mode 100644 include/linux/mfd/sirfsoc_pwrc.h
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
>> new file mode 100644
>> index 0000000..b919cdd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
>> @@ -0,0 +1,37 @@
>> +SiRFSoC Power Controller (PWRC) module
>> +
>> +Power Controller is used to control the whole SoC power process.
>> +The power controller controls the process of Power-On sequence,
>
> s/power controller/Power Controller/
>
>> +Power-Off sequence, different power modes switching and power
>> +status configuration. the pwrc is access by io bridge, so the
>
> s/the pwrc/The PWRC/
>
> s/access/accessed/
>
> s/io/IO/
>
>> +node's parent should be io bridge.
>
> s/io/IO/
>
> Not quite sure what an IO bridge is though.
>
>> +Required properties:
>> +- compatible: "sirf,prima2-pwrc", or "sirf,atlas7-pwrc"
>> +- reg: Address range of pwrc register set
>
> Address start and size.
>
>> +- sysctl:mfd cell device of pwrc
>> +- rtcm-clk:mfd cell device of pwrc
>> +- onkey:mfd cell device of pwrc
>
> MFD is a Linuxisum.  Please find another way to document them.
>
> I always find documentation easier to read then it's tabbed out
> nicely.  Like:
>
> Required properties:
> - compatible    : "sirf,prima2-pwrc", or "sirf,atlas7-pwrc"
> - reg           : Address range of pwrc register set
> - sysctl        : mfd cell device of pwrc
> - rtcm-clk      : mfd cell device of pwrc
> - onkey         : mfd cell device of pwrc
>
>> +Example:
>> +
>> +pwrc@3000 {
>> +     compatible = "sirf,atlas7-pwrc";
>> +     reg = <0x3000 0x100>;
>
> This doesn't look like a real address.  It looks like an offset.
> Please provide a full example, complete with the parent node (which I
> assume has ranges set-up).
>
>> +     sysctl {
>> +             compatible = "sirf,sirf-sysctl";
>> +     };
>> +
>> +     rtcm-clk {
>> +             compatible = "sirf,atlas7-rtcmclk";
>> +     };
>> +
>> +     onkey {
>> +             compatible = "sirf,prima2-onkey";
>> +     };
>
> Why do these require their own cells?
>
> Do they have their own properties?  If so, where are they documented?
>
>> +};
>> +
>> +pwrc@3000 {
>> +     compatible = "sirf,prima2-pwrc";
>> +     reg = <0x3000 0x100>;
>> +};
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 99d6367..5b67bee 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1515,5 +1515,17 @@ config MFD_VEXPRESS_SYSREG
>>         System Registers are the platform configuration block
>>         on the ARM Ltd. Versatile Express board.
>>
>> +config MFD_SIRFSOC_PWRC
>> +        bool "CSR SiRFSoC on-chip Power Control Module"
>> +     depends on ARCH_SIRF
>> +     default y
>> +        select MFD_CORE
>> +        select REGMAP_IRQ
>> +        help
>> +         CSR SiRFSoC Power Control Module includes power on or power
>> +      off for sysctl power and gnss, it also includes onkey, RTC
>> +      domain clock controllers and interrupt controller for power
>> +      events.
>
> Break out the TLAs.
>
> Your tabbing is all over the place here.  Please match existing
> entries.
>
>>  endmenu
>>  endif
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index a59e3fc..255fb80 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -192,3 +192,5 @@ obj-$(CONFIG_MFD_SKY81452)        += sky81452.o
>>  intel-soc-pmic-objs          := intel_soc_pmic_core.o intel_soc_pmic_crc.o
>>  obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
>>  obj-$(CONFIG_MFD_MT6397)     += mt6397-core.o
>> +
>> +obj-$(CONFIG_MFD_SIRFSOC_PWRC)       += sirfsoc_pwrc.o
>> diff --git a/drivers/mfd/sirfsoc_pwrc.c b/drivers/mfd/sirfsoc_pwrc.c
>> new file mode 100644
>> index 0000000..b43f8b4
>> --- /dev/null
>> +++ b/drivers/mfd/sirfsoc_pwrc.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * power management mfd for CSR SiRFSoC chips
>
> "Power Management MFD for CSR SiRFSoC chips"
>
>> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
>
> This is out of date.
>
>> + * Licensed under GPLv2 or later.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/io.h>
>> +#include <linux/regmap.h>
>> +#include <linux/module.h>
>
> This isn't a module.
>
>> +#include <linux/rtc/sirfsoc_rtciobrg.h>
>
> What's this for?

the registers are behind a rtc io bridge. HW needs to access the
bridge to access registers.
another example similar with pwrc MFD is sysrtc as below:

2019                         rtc-iobg@18840000 {
2020                                 compatible = "sirf,prima2-rtciobg",
2021                                         "sirf-prima2-rtciobg-bus",
2022                                         "simple-bus";
2023                                 #address-cells = <1>;
2024                                 #size-cells = <1>;
2025                                 reg = <0x18840000 0x1000>;
2026
2027                                 sysrtc@2000 {
2028                                         compatible = "sirf,prima2-sysrtc";
2029                                         reg = <0x2000 0x100>;
2030                                         interrupts = <0 52 0>;
2031                                 };
2032                                 pwrc@3000 {
2033                                         compatible = "sirf,atlas7-pwrc";
2034                                         reg = <0x3000 0x100>;
2035                                         interrupts = <0 32 0>;
2036                                         rtcmclk: clock-controller {

its driver is at
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-sirfsoc.c

>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/sirfsoc_pwrc.h>
>
> Header files should be in alphabetical order.
>
>> +struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc = {
>> +     .pwrc_pdn_ctrl_set = 0x0,
>> +     .pwrc_pdn_ctrl_clr = 0x4,
>> +     .pwrc_pon_status = 0x8,
>> +     .pwrc_trigger_en_set = 0xc,
>> +     .pwrc_trigger_en_clr = 0x10,
>> +     .pwrc_int_mask_set = 0x14,
>> +     .pwrc_int_mask_clr = 0x18,
>> +     .pwrc_int_status = 0x1c,
>> +     .pwrc_pin_status = 0x20,
>> +     .pwrc_rtc_pll_ctrl = 0x28,
>> +     .pwrc_gpio3_debug = 0x34,
>> +     .pwrc_rtc_noc_pwrctl_set = 0x38,
>> +     .pwrc_rtc_noc_pwrctl_clr = 0x3c,
>> +     .pwrc_rtc_can_ctrl = 0x48,
>> +     .pwrc_rtc_can_status = 0x4c,
>> +     .pwrc_fsm_m3_ctrl = 0x50,
>> +     .pwrc_fsm_state = 0x54,
>> +     .pwrc_rtcldo_reg = 0x58,
>> +     .pwrc_gnss_ctrl = 0x5c,
>> +     .pwrc_gnss_status = 0x60,
>> +     .pwrc_xtal_reg = 0x64,
>> +     .pwrc_xtal_ldo_mux_sel = 0x68,
>> +     .pwrc_rtc_sw_rstc_set = 0x6c,
>> +     .pwrc_rtc_sw_rstc_clr = 0x70,
>> +     .pwrc_power_sw_ctrl_set = 0x74,
>> +     .pwrc_power_sw_ctrl_clr = 0x78,
>> +     .pwrc_rtc_dcog = 0x7c,
>> +     .pwrc_m3_memories = 0x80,
>> +     .pwrc_can0_memory = 0x84,
>> +     .pwrc_rtc_gnss_memory = 0x88,
>> +     .pwrc_m3_clk_en = 0x8c,
>> +     .pwrc_can0_clk_en = 0x90,
>> +     .pwrc_spi0_clk_en = 0x94,
>> +     .pwrc_rtc_sec_clk_en = 0x98,
>> +     .pwrc_rtc_noc_clk_en = 0x9c,
>> +};
>> +
>> +struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc = {
>> +     .pwrc_pdn_ctrl_set = 0x0,
>> +     .pwrc_pon_status = 0x4,
>> +     .pwrc_trigger_en_set = 0x8,
>> +     .pwrc_int_status = 0xc,
>> +     .pwrc_int_mask_set = 0x10,
>> +     .pwrc_pin_status = 0x14,
>> +     .pwrc_scratch_pad1 = 0x18,
>> +     .pwrc_scratch_pad2 = 0x1c,
>> +     .pwrc_scratch_pad3 = 0x20,
>> +     .pwrc_scratch_pad4 = 0x24,
>> +     .pwrc_scratch_pad5 = 0x28,
>> +     .pwrc_scratch_pad6 = 0x2c,
>> +     .pwrc_scratch_pad7 = 0x30,
>> +     .pwrc_scratch_pad8 = 0x34,
>> +     .pwrc_scratch_pad9 = 0x38,
>> +     .pwrc_scratch_pad10 = 0x3c,
>> +     .pwrc_scratch_pad11 = 0x40,
>> +     .pwrc_scratch_pad12 = 0x44,
>> +     .pwrc_gpio3_clk = 0x54,
>> +     .pwrc_gpio_ds = 0x78,
>> +};
>
> This is not the way we usually define register addresses.

i agree using MARCO is the general rules. but here moving to struct is
for avoiding things like:

if (prima2...)
   ...
elif(atlas7)
  ...

it is making registers offset private data which can be a member of the object.

>
> Please #define them properly.
>
>> +static const struct regmap_irq pwrc_irqs[] = {
>> +     /* INT0 */
>> +     [PWRC_IRQ_ONKEY] = {
>> +             .mask = BIT(PWRC_IRQ_ONKEY),
>
> Better to do the BIT() shifting in the header file.
>
>> +     },
>> +     [PWRC_IRQ_EXT_ONKEY] = {
>> +             .mask = BIT(PWRC_IRQ_EXT_ONKEY),
>> +     },
>> +};
>> +
>> +static struct regmap_irq_chip pwrc_irq_chip = {
>> +     .name = "pwrc_irq",
>> +     .irqs = pwrc_irqs,
>> +     .num_irqs = ARRAY_SIZE(pwrc_irqs),
>> +     .num_regs = 1,
>> +     .ack_invert = 1,
>> +     .init_ack_masked = true,
>> +};
>> +
>> +static const struct of_device_id pwrc_ids[] = {
>> +     { .compatible = "sirf,prima2-pwrc", .data = &sirfsoc_prima2_pwrc},
>> +     { .compatible = "sirf,atlas7-pwrc", .data = &sirfsoc_a7da_pwrc},
>> +     {}
>> +};
>
> Please put these _just_ before you use them, so just above .probe() in
> this case.
>
>> +static const struct mfd_cell pwrc_devs[] = {
>> +     {
>> +             .name = "rtcmclk",
>> +             .of_compatible = "sirf,atlas7-rtcmclk",
>> +     }, {
>> +             .name = "sirf-sysctl",
>> +             .of_compatible = "sirf,sirf-sysctl",
>> +     }, {
>> +             .name = "gps-power",
>> +             .of_compatible = "sirf,gps-power",
>> +     }, {
>> +             .name = "onkey",
>> +             .of_compatible = "sirf,prima2-onkey",
>> +     },
>> +};
>> +
>> +static const struct regmap_config pwrc_regmap_config = {
>> +     .fast_io = true,
>> +     .reg_bits = 32,
>> +     .val_bits = 32,
>> +     .reg_stride = 4,
>> +};
>> +
>> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     const struct of_device_id *match;
>> +     struct sirfsoc_pwrc_info *pwrcinfo;
>> +     struct regmap_irq_chip *regmap_irq_chip;
>> +     struct sirfsoc_pwrc_register *pwrc_reg;
>> +     struct regmap *map;
>> +     int ret;
>> +     u32 base;
>> +
>> +     if (of_property_read_u32(np, "reg", &base))
>> +             panic("unable to find base address of pwrc node in dtb\n");
>
> It looks like this driver should depend on OF.
>
> Why are you obtaining the base address manually? Use:
>
>   res = platform_get_resource();
>   devm_ioremap_resource(res);
>
> ... instead.

this was explained as they are not in memory space, they are behind a
bus bridge.

>
>> +     pwrcinfo = devm_kzalloc(&pdev->dev,
>> +                     sizeof(struct sirfsoc_pwrc_info), GFP_KERNEL);
>
> sizeof(pwrcinfo)
>
>> +     if (!pwrcinfo)
>> +             return -ENOMEM;
>> +     pwrcinfo->base = base;
>> +
>> +     /*
>> +      * pwrc behind rtciobrg offset is diff between prima2 and atlas7
>> +      * here match to each ids data for it.
>> +      */
>
> Use uppercase characters for abbreviations.
>
> Besides, I have no idea what you're trying to say.
>
>> +     match = of_match_node(pwrc_ids, np);
>> +     pwrcinfo->pwrc_reg = (struct sirfsoc_pwrc_register *)match->data;
>
> It looks like you're trying to use the same variables two different
> device's register sets.  That's asking for trouble, please don't do
> that.
>
>> +     if (of_device_is_compatible(np, "sirf,atlas7-pwrc"))
>> +             pwrcinfo->ver = PWRC_ATLAS7_VER;
>> +     else if (of_device_is_compatible(np, "sirf,prima2-pwrc"))
>> +             pwrcinfo->ver = PWRC_PRIMA2_VER;
>> +     else
>> +             return -EINVAL;
>
> These aren't versions, are they?  It's different hardware.
>
> Can't you probe for this at runtime?
>
>> +     of_node_put(np);
>
> What are you putting here?
>
>> +     map = (struct regmap *)devm_regmap_init_iobg(&pdev->dev,
>> +                     &pwrc_regmap_config);
>
> Why are you casting?
>
>> +     if (IS_ERR(map)) {
>> +             ret = PTR_ERR(map);
>> +             dev_err(&pdev->dev, "Failed to allocate register map: %d\n",
>> +                     ret);
>> +             goto err;
>
> Please remove the 'err' label and just return from here.
>
>> +     }
>> +
>> +     pwrcinfo->regmap = map;
>> +     pwrcinfo->dev = &pdev->dev;
>> +     platform_set_drvdata(pdev, pwrcinfo);
>> +
>> +     ret = mfd_add_devices(pwrcinfo->dev, 0, pwrc_devs,
>> +             ARRAY_SIZE(pwrc_devs), NULL, 0, NULL);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to add all pwrc subdev\n");
>
> Capitalise all abbreviations.
>
> Don't abbreviate things like "sub devices".
>
>> +             goto err;
>
> Just return.
>
>> +     }
>> +
>> +     ret = of_irq_get(pdev->dev.of_node, 0);
>
> You already put this in to a succinct variable 'np'.  Please use it.
>
>> +     if (ret <= 0) {
>> +             dev_info(&pdev->dev,
>> +                     "Unable to find IRQ for pwrc. ret=%d\n", ret);
>
> Just print ret.  No need for the ugly "ret=".
>
>> +             goto err_irq;
>> +     }
>> +
>> +     pwrcinfo->irq = ret;
>
> Better change this to 'irq' instead of using 'ret'.
>
>> +     regmap_irq_chip = &pwrc_irq_chip;
>> +     pwrcinfo->regmap_irq_chip = regmap_irq_chip;
>> +
>> +     pwrc_reg = pwrcinfo->pwrc_reg;
>> +     regmap_irq_chip->mask_base = pwrcinfo->base +
>> +                                             pwrc_reg->pwrc_int_mask_set;
>> +     regmap_irq_chip->unmask_base = pwrcinfo->base +
>> +                             pwrc_reg->pwrc_int_mask_clr;
>> +     regmap_irq_chip->status_base = pwrcinfo->base +
>> +                                             pwrc_reg->pwrc_int_status;
>> +     regmap_irq_chip->ack_base = pwrcinfo->base +
>> +                                             pwrc_reg->pwrc_int_status;
>
> This is ugly.
>
> Better to create 2 regmap_irq_chip structures, one for each device.

there is only one device. why two regmap_irq_chip structures?

the driver is compatible with prima2 and atlas7, but any time there is
only one of them,
and the register needs to be adjust from dts and offset table.

>
>> +     /* enable onkey trigger interrupt controller */
>
> Capital letters to start a sentence.
>
>> +     ret = regmap_update_bits(map,
>> +                     pwrcinfo->base +
>> +                     pwrc_reg->pwrc_trigger_en_set,
>> +                     BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY),
>> +                     BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY));
>> +     if (ret < 0)
>> +             goto err_irq;
>> +
>> +     /* add irq controller for pwrc */
>
> Capital letters to start a sentence and for abbreviations.
>
>> +     ret = regmap_add_irq_chip(map, pwrcinfo->irq, IRQF_ONESHOT,
>> +                             -1, pwrcinfo->regmap_irq_chip,
>> +                             &pwrcinfo->irq_data);
>> +
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to add regmap irq controller for pwrc\n");
>> +             goto err;
>> +     }
>> +
>> +     return 0;
>> +err_irq:
>> +     mfd_remove_devices(pwrcinfo->dev);
>> +err:
>
> Remove this label, it's not helpful.
>
>> +     return ret;
>> +}
>> +
>> +static struct platform_driver sirfsoc_pwrc_driver = {
>> +     .probe  = sirfsoc_pwrc_probe,
>
> .remove?
>
>> +     .driver = {
>> +             .name = "sirfsoc_pwrc",
>> +             .of_match_table = pwrc_ids,
>
> of_match_ptr()
>
>> +     },
>> +};
>> +module_platform_driver(sirfsoc_pwrc_driver);
>
> This isn't a module.


so do you think it is still a platform, what is the best way to probe them?

>
>> diff --git a/include/linux/mfd/sirfsoc_pwrc.h b/include/linux/mfd/sirfsoc_pwrc.h
>> new file mode 100644
>> index 0000000..ee2b3d5
>> --- /dev/null
>> +++ b/include/linux/mfd/sirfsoc_pwrc.h
>> @@ -0,0 +1,97 @@
>> +/*
>> + * CSR SiRFSoC power control module MFD interface
>> + *
>> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
>
> Out of date.
>
>> + * Licensed under GPLv2 or later.
>> + */
>
> '\n' here.
>
>> +#ifndef _SIRFSOC_PWRC_H_
>> +#define _SIRFSOC_PWRC_H_
>
> '\n' here.
>
>> +#include <linux/interrupt.h>
>
> What's this for?
>
>> +#include <linux/regmap.h>
>> +
>> +#define PWRC_PDN_CTRL_OFFSET 0
>> +#define AUDIO_POWER_EN_BIT   14
>> +
>> +struct sirfsoc_pwrc_register {
>> +     /* hardware pwrc specific */
>> +     u32 pwrc_pdn_ctrl_set;
>> +     u32 pwrc_pdn_ctrl_clr;
>> +     u32 pwrc_pon_status;
>> +     u32 pwrc_trigger_en_set;
>> +     u32 pwrc_trigger_en_clr;
>> +     u32 pwrc_int_mask_set;
>> +     u32 pwrc_int_mask_clr;
>> +     u32 pwrc_int_status;
>> +     u32 pwrc_pin_status;
>> +     u32 pwrc_rtc_pll_ctrl;
>> +     u32 pwrc_gpio3_debug;
>> +     u32 pwrc_rtc_noc_pwrctl_set;
>> +     u32 pwrc_rtc_noc_pwrctl_clr;
>> +     u32 pwrc_rtc_can_ctrl;
>> +     u32 pwrc_rtc_can_status;
>> +     u32 pwrc_fsm_m3_ctrl;
>> +     u32 pwrc_fsm_state;
>> +     u32 pwrc_rtcldo_reg;
>> +     u32 pwrc_gnss_ctrl;
>> +     u32 pwrc_gnss_status;
>> +     u32 pwrc_xtal_reg;
>> +     u32 pwrc_xtal_ldo_mux_sel;
>> +     u32 pwrc_rtc_sw_rstc_set;
>> +     u32 pwrc_rtc_sw_rstc_clr;
>> +     u32 pwrc_power_sw_ctrl_set;
>> +     u32 pwrc_power_sw_ctrl_clr;
>> +     u32 pwrc_rtc_dcog;
>> +     u32 pwrc_m3_memories;
>> +     u32 pwrc_can0_memory;
>> +     u32 pwrc_rtc_gnss_memory;
>> +     u32 pwrc_m3_clk_en;
>> +     u32 pwrc_can0_clk_en;
>> +     u32 pwrc_spi0_clk_en;
>> +     u32 pwrc_rtc_sec_clk_en;
>> +     u32 pwrc_rtc_noc_clk_en;
>> +
>> +     /*only for prima2*/
>> +     u32 pwrc_scratch_pad1;
>> +     u32 pwrc_scratch_pad2;
>> +     u32 pwrc_scratch_pad3;
>> +     u32 pwrc_scratch_pad4;
>> +     u32 pwrc_scratch_pad5;
>> +     u32 pwrc_scratch_pad6;
>> +     u32 pwrc_scratch_pad7;
>> +     u32 pwrc_scratch_pad8;
>> +     u32 pwrc_scratch_pad9;
>> +     u32 pwrc_scratch_pad10;
>> +     u32 pwrc_scratch_pad11;
>> +     u32 pwrc_scratch_pad12;
>> +     u32 pwrc_gpio3_clk;
>> +     u32 pwrc_gpio_ds;
>> +
>> +};
>
> Please #define these instead.
>
>> +enum pwrc_version {
>> +     PWRC_MARCO_VER,
>> +     PWRC_PRIMA2_VER,
>> +     PWRC_ATLAS7_VER,
>> +};
>
> Kerneldoc header?
>
>> +struct sirfsoc_pwrc_info {
>> +     struct device *dev;
>> +     struct regmap *regmap;
>> +     struct sirfsoc_pwrc_register *pwrc_reg;
>> +     struct regmap_irq_chip *regmap_irq_chip;
>> +     struct regmap_irq_chip_data *irq_data;
>
> Where are these reused?
>
>> +     u32 ver;
>
> What is this used for?
>
>> +     u32 base;
>
> Is this a u32 or a memory address?
>
>> +     int irq;
>> +};
>> +
>> +enum {
>> +     PWRC_IRQ_ONKEY = 0,
>> +     PWRC_IRQ_EXT_ONKEY,
>> +     PWRC_MAX_IRQ,
>> +};
>> +
>> +extern struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc;
>> +extern struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc;
>> +#endif
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog


-barry

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Lee Jones]

On Mon, 21 Sep 2015, Barry Song wrote:
> 2015-09-20 12:15 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> > On Thu, 17 Sep 2015, Barry Song wrote:
> >
> >> From: Guo Zeng <Guo.Zeng@csr.com>
> >>
> >> CSR SiRFSoC Power Control Module includes power on or power
> >> off for sysctl power and gnss, it also includes onkey, RTC
> >> domain clock controllers and interrupt controller for power
> >> events.
> >
> > There are lots of Three (and four) Letter Abbreviations (TLAs) here,
> > which mean nothing to the average reader.  Please break them out in
> > the commit log as I have done i.e. "Long Abbreviated Word (LAW)", so
> > us normies can see what they mean.
> >
> >> Signed-off-by: Guo Zeng <Guo.Zeng@csr.com>
> >> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> >> ---
> >>  .../devicetree/bindings/mfd/sirf-pwrc.txt          |  37 ++++
> >
> > This should be in a separate patch.
> >
> >>  drivers/mfd/Kconfig                                |  12 ++
> >>  drivers/mfd/Makefile                               |   2 +
> >>  drivers/mfd/sirfsoc_pwrc.c                         | 238 +++++++++++++++++++++
> >>  include/linux/mfd/sirfsoc_pwrc.h                   |  97 +++++++++
> >>  5 files changed, 386 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
> >>  create mode 100644 drivers/mfd/sirfsoc_pwrc.c
> >>  create mode 100644 include/linux/mfd/sirfsoc_pwrc.h

[...]

> >> +#include <linux/rtc/sirfsoc_rtciobrg.h>
> >
> > What's this for?
> 
> the registers are behind a rtc io bridge. HW needs to access the
> bridge to access registers.
> another example similar with pwrc MFD is sysrtc as below:
> 
> 2019                         rtc-iobg@18840000 {
> 2020                                 compatible = "sirf,prima2-rtciobg",
> 2021                                         "sirf-prima2-rtciobg-bus",
> 2022                                         "simple-bus";
> 2023                                 #address-cells = <1>;
> 2024                                 #size-cells = <1>;
> 2025                                 reg = <0x18840000 0x1000>;
> 2026
> 2027                                 sysrtc@2000 {
> 2028                                         compatible = "sirf,prima2-sysrtc";
> 2029                                         reg = <0x2000 0x100>;
> 2030                                         interrupts = <0 52 0>;
> 2031                                 };
> 2032                                 pwrc@3000 {
> 2033                                         compatible = "sirf,atlas7-pwrc";
> 2034                                         reg = <0x3000 0x100>;
> 2035                                         interrupts = <0 32 0>;
> 2036                                         rtcmclk: clock-controller {
> 
> its driver is at
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-sirfsoc.c

I meant what are you using in the header file, but I looked it up.

> >> +#include <linux/mfd/core.h>
> >> +#include <linux/mfd/sirfsoc_pwrc.h>
> >
> > Header files should be in alphabetical order.
> >
> >> +struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc = {
> >> +     .pwrc_pdn_ctrl_set = 0x0,
> >> +     .pwrc_pdn_ctrl_clr = 0x4,
> >> +     .pwrc_pon_status = 0x8,
> >> +     .pwrc_trigger_en_set = 0xc,
> >> +     .pwrc_trigger_en_clr = 0x10,
> >> +     .pwrc_int_mask_set = 0x14,
> >> +     .pwrc_int_mask_clr = 0x18,
> >> +     .pwrc_int_status = 0x1c,
> >> +     .pwrc_pin_status = 0x20,
> >> +     .pwrc_rtc_pll_ctrl = 0x28,
> >> +     .pwrc_gpio3_debug = 0x34,
> >> +     .pwrc_rtc_noc_pwrctl_set = 0x38,
> >> +     .pwrc_rtc_noc_pwrctl_clr = 0x3c,
> >> +     .pwrc_rtc_can_ctrl = 0x48,
> >> +     .pwrc_rtc_can_status = 0x4c,
> >> +     .pwrc_fsm_m3_ctrl = 0x50,
> >> +     .pwrc_fsm_state = 0x54,
> >> +     .pwrc_rtcldo_reg = 0x58,
> >> +     .pwrc_gnss_ctrl = 0x5c,
> >> +     .pwrc_gnss_status = 0x60,
> >> +     .pwrc_xtal_reg = 0x64,
> >> +     .pwrc_xtal_ldo_mux_sel = 0x68,
> >> +     .pwrc_rtc_sw_rstc_set = 0x6c,
> >> +     .pwrc_rtc_sw_rstc_clr = 0x70,
> >> +     .pwrc_power_sw_ctrl_set = 0x74,
> >> +     .pwrc_power_sw_ctrl_clr = 0x78,
> >> +     .pwrc_rtc_dcog = 0x7c,
> >> +     .pwrc_m3_memories = 0x80,
> >> +     .pwrc_can0_memory = 0x84,
> >> +     .pwrc_rtc_gnss_memory = 0x88,
> >> +     .pwrc_m3_clk_en = 0x8c,
> >> +     .pwrc_can0_clk_en = 0x90,
> >> +     .pwrc_spi0_clk_en = 0x94,
> >> +     .pwrc_rtc_sec_clk_en = 0x98,
> >> +     .pwrc_rtc_noc_clk_en = 0x9c,
> >> +};
> >> +
> >> +struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc = {
> >> +     .pwrc_pdn_ctrl_set = 0x0,
> >> +     .pwrc_pon_status = 0x4,
> >> +     .pwrc_trigger_en_set = 0x8,
> >> +     .pwrc_int_status = 0xc,
> >> +     .pwrc_int_mask_set = 0x10,
> >> +     .pwrc_pin_status = 0x14,
> >> +     .pwrc_scratch_pad1 = 0x18,
> >> +     .pwrc_scratch_pad2 = 0x1c,
> >> +     .pwrc_scratch_pad3 = 0x20,
> >> +     .pwrc_scratch_pad4 = 0x24,
> >> +     .pwrc_scratch_pad5 = 0x28,
> >> +     .pwrc_scratch_pad6 = 0x2c,
> >> +     .pwrc_scratch_pad7 = 0x30,
> >> +     .pwrc_scratch_pad8 = 0x34,
> >> +     .pwrc_scratch_pad9 = 0x38,
> >> +     .pwrc_scratch_pad10 = 0x3c,
> >> +     .pwrc_scratch_pad11 = 0x40,
> >> +     .pwrc_scratch_pad12 = 0x44,
> >> +     .pwrc_gpio3_clk = 0x54,
> >> +     .pwrc_gpio_ds = 0x78,
> >> +};
> >
> > This is not the way we usually define register addresses.
> 
> i agree using MARCO is the general rules. but here moving to struct is
> for avoiding things like:
> 
> if (prima2...)
>    ...
> elif(atlas7)
>   ...
> 
> it is making registers offset private data which can be a member of the object.

Hmm... it's pretty unusual, but I can't think of a better way of doing
it yet.

[...]

> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >> +{
> >> +     struct device_node *np = pdev->dev.of_node;
> >> +     const struct of_device_id *match;
> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
> >> +     struct regmap_irq_chip *regmap_irq_chip;
> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
> >> +     struct regmap *map;
> >> +     int ret;
> >> +     u32 base;
> >> +
> >> +     if (of_property_read_u32(np, "reg", &base))
> >> +             panic("unable to find base address of pwrc node in dtb\n");
> >
> > It looks like this driver should depend on OF.
> >
> > Why are you obtaining the base address manually? Use:
> >
> >   res = platform_get_resource();
> >   devm_ioremap_resource(res);
> >
> > ... instead.
> 
> this was explained as they are not in memory space, they are behind a
> bus bridge.

Use 'ranges' in the DT, then you can pull out the proper address
without hand rolling your own method.

[...]

> >> +     regmap_irq_chip = &pwrc_irq_chip;
> >> +     pwrcinfo->regmap_irq_chip = regmap_irq_chip;
> >> +
> >> +     pwrc_reg = pwrcinfo->pwrc_reg;
> >> +     regmap_irq_chip->mask_base = pwrcinfo->base +
> >> +                                             pwrc_reg->pwrc_int_mask_set;
> >> +     regmap_irq_chip->unmask_base = pwrcinfo->base +
> >> +                             pwrc_reg->pwrc_int_mask_clr;
> >> +     regmap_irq_chip->status_base = pwrcinfo->base +
> >> +                                             pwrc_reg->pwrc_int_status;
> >> +     regmap_irq_chip->ack_base = pwrcinfo->base +
> >> +                                             pwrc_reg->pwrc_int_status;
> >
> > This is ugly.
> >
> > Better to create 2 regmap_irq_chip structures, one for each device.
> 
> there is only one device. why two regmap_irq_chip structures?
> 
> the driver is compatible with prima2 and atlas7, but any time there is
> only one of them,
> and the register needs to be adjust from dts and offset table.

Why does the 'base' offset have to be drawn from DT?  Does it change?

I think you should create two static regmap_irq_chip structures and do
only pass the relevant one to regmap.

See how everyone else does it.

[...]

> >> +static struct platform_driver sirfsoc_pwrc_driver = {
> >> +     .probe  = sirfsoc_pwrc_probe,
> >
> > .remove?
> >
> >> +     .driver = {
> >> +             .name = "sirfsoc_pwrc",
> >> +             .of_match_table = pwrc_ids,
> >
> > of_match_ptr()
> >
> >> +     },
> >> +};
> >> +module_platform_driver(sirfsoc_pwrc_driver);
> >
> > This isn't a module.
> 
> 
> so do you think it is still a platform, what is the best way to probe them?

Yes, it's still a platform.  It's just not a module.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Barry Song]

>> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> >> +{
>> >> +     struct device_node *np = pdev->dev.of_node;
>> >> +     const struct of_device_id *match;
>> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
>> >> +     struct regmap_irq_chip *regmap_irq_chip;
>> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
>> >> +     struct regmap *map;
>> >> +     int ret;
>> >> +     u32 base;
>> >> +
>> >> +     if (of_property_read_u32(np, "reg", &base))
>> >> +             panic("unable to find base address of pwrc node in dtb\n");
>> >
>> > It looks like this driver should depend on OF.
>> >
>> > Why are you obtaining the base address manually? Use:
>> >
>> >   res = platform_get_resource();
>> >   devm_ioremap_resource(res);
>> >
>> > ... instead.
>>
>> this was explained as they are not in memory space, they are behind a
>> bus bridge.
>
> Use 'ranges' in the DT, then you can pull out the proper address
> without hand rolling your own method.


it seems it is not a "ranges" thing,  things behind rtciobrg is much
like things behind USB or sdio. we need to use a rtciobrg protocol to
do read/write.
they can not be randomly accessed by load/store, and can't be XIP.
they don't have any ranges in CPU memory space.

>
> [...]
>
>> >> +     regmap_irq_chip = &pwrc_irq_chip;
>> >> +     pwrcinfo->regmap_irq_chip = regmap_irq_chip;
>> >> +
>> >> +     pwrc_reg = pwrcinfo->pwrc_reg;
>> >> +     regmap_irq_chip->mask_base = pwrcinfo->base +
>> >> +                                             pwrc_reg->pwrc_int_mask_set;
>> >> +     regmap_irq_chip->unmask_base = pwrcinfo->base +
>> >> +                             pwrc_reg->pwrc_int_mask_clr;
>> >> +     regmap_irq_chip->status_base = pwrcinfo->base +
>> >> +                                             pwrc_reg->pwrc_int_status;
>> >> +     regmap_irq_chip->ack_base = pwrcinfo->base +
>> >> +                                             pwrc_reg->pwrc_int_status;
>> >
>> > This is ugly.
>> >
>> > Better to create 2 regmap_irq_chip structures, one for each device.
>>
>> there is only one device. why two regmap_irq_chip structures?
>>
>> the driver is compatible with prima2 and atlas7, but any time there is
>> only one of them,
>> and the register needs to be adjust from dts and offset table.
>
> Why does the 'base' offset have to be drawn from DT?  Does it change?
>
> I think you should create two static regmap_irq_chip structures and do
> only pass the relevant one to regmap.
>
> See how everyone else does it.

that is ok, if this driver picks up one regmap_irq_chip from two
according to of compatible strings.

>
> [...]
>
>> >> +static struct platform_driver sirfsoc_pwrc_driver = {
>> >> +     .probe  = sirfsoc_pwrc_probe,
>> >
>> > .remove?
>> >
>> >> +     .driver = {
>> >> +             .name = "sirfsoc_pwrc",
>> >> +             .of_match_table = pwrc_ids,
>> >
>> > of_match_ptr()
>> >
>> >> +     },
>> >> +};
>> >> +module_platform_driver(sirfsoc_pwrc_driver);
>> >
>> > This isn't a module.
>>
>>
>> so do you think it is still a platform, what is the best way to probe them?
>
> Yes, it's still a platform.  It's just not a module.

Lee, i don't have idea on this.

as a module, if it is built-in, it is initilized during
device_initcall, if it is not built-in, it is initilized during
insmod.
when you say it is not a module,
do you mean it must be built-in, or do you mean it is not a device_initcall?

-barry

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Lee Jones]

On Tue, 29 Sep 2015, Barry Song wrote:
> >> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >> >> +{
> >> >> +     struct device_node *np = pdev->dev.of_node;
> >> >> +     const struct of_device_id *match;
> >> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
> >> >> +     struct regmap_irq_chip *regmap_irq_chip;
> >> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
> >> >> +     struct regmap *map;
> >> >> +     int ret;
> >> >> +     u32 base;
> >> >> +
> >> >> +     if (of_property_read_u32(np, "reg", &base))
> >> >> +             panic("unable to find base address of pwrc node in dtb\n");
> >> >
> >> > It looks like this driver should depend on OF.
> >> >
> >> > Why are you obtaining the base address manually? Use:
> >> >
> >> >   res = platform_get_resource();
> >> >   devm_ioremap_resource(res);
> >> >
> >> > ... instead.
> >>
> >> this was explained as they are not in memory space, they are behind a
> >> bus bridge.
> >
> > Use 'ranges' in the DT, then you can pull out the proper address
> > without hand rolling your own method.
> 
> it seems it is not a "ranges" thing,  things behind rtciobrg is much
> like things behind USB or sdio. we need to use a rtciobrg protocol to
> do read/write.
> they can not be randomly accessed by load/store, and can't be XIP.
> they don't have any ranges in CPU memory space.

So what's the point of 'base' then?  I assumed this was the base of
the IP registers which where memory mapped? 

> > [...]
> >
> >> >> +     regmap_irq_chip = &pwrc_irq_chip;
> >> >> +     pwrcinfo->regmap_irq_chip = regmap_irq_chip;
> >> >> +
> >> >> +     pwrc_reg = pwrcinfo->pwrc_reg;
> >> >> +     regmap_irq_chip->mask_base = pwrcinfo->base +
> >> >> +                                             pwrc_reg->pwrc_int_mask_set;
> >> >> +     regmap_irq_chip->unmask_base = pwrcinfo->base +
> >> >> +                             pwrc_reg->pwrc_int_mask_clr;
> >> >> +     regmap_irq_chip->status_base = pwrcinfo->base +
> >> >> +                                             pwrc_reg->pwrc_int_status;
> >> >> +     regmap_irq_chip->ack_base = pwrcinfo->base +
> >> >> +                                             pwrc_reg->pwrc_int_status;
> >> >
> >> > This is ugly.
> >> >
> >> > Better to create 2 regmap_irq_chip structures, one for each device.
> >>
> >> there is only one device. why two regmap_irq_chip structures?
> >>
> >> the driver is compatible with prima2 and atlas7, but any time there is
> >> only one of them,
> >> and the register needs to be adjust from dts and offset table.
> >
> > Why does the 'base' offset have to be drawn from DT?  Does it change?
> >
> > I think you should create two static regmap_irq_chip structures and do
> > only pass the relevant one to regmap.
> >
> > See how everyone else does it.
> 
> that is ok, if this driver picks up one regmap_irq_chip from two
> according to of compatible strings.

Okay, great.

> > [...]
> >
> >> >> +static struct platform_driver sirfsoc_pwrc_driver = {
> >> >> +     .probe  = sirfsoc_pwrc_probe,
> >> >
> >> > .remove?
> >> >
> >> >> +     .driver = {
> >> >> +             .name = "sirfsoc_pwrc",
> >> >> +             .of_match_table = pwrc_ids,
> >> >
> >> > of_match_ptr()
> >> >
> >> >> +     },
> >> >> +};
> >> >> +module_platform_driver(sirfsoc_pwrc_driver);
> >> >
> >> > This isn't a module.
> >>
> >>
> >> so do you think it is still a platform, what is the best way to probe them?
> >
> > Yes, it's still a platform.  It's just not a module.
> 
> Lee, i don't have idea on this.
> 
> as a module, if it is built-in, it is initilized during
> device_initcall, if it is not built-in, it is initilized during
> insmod.
> when you say it is not a module,
> do you mean it must be built-in, or do you mean it is not a device_initcall?

Use builtin_platform_driver_probe().

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Barry Song]

2015-09-29 15:16 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 29 Sep 2015, Barry Song wrote:
>> >> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> >> >> +{
>> >> >> +     struct device_node *np = pdev->dev.of_node;
>> >> >> +     const struct of_device_id *match;
>> >> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
>> >> >> +     struct regmap_irq_chip *regmap_irq_chip;
>> >> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
>> >> >> +     struct regmap *map;
>> >> >> +     int ret;
>> >> >> +     u32 base;
>> >> >> +
>> >> >> +     if (of_property_read_u32(np, "reg", &base))
>> >> >> +             panic("unable to find base address of pwrc node in dtb\n");
>> >> >
>> >> > It looks like this driver should depend on OF.
>> >> >
>> >> > Why are you obtaining the base address manually? Use:
>> >> >
>> >> >   res = platform_get_resource();
>> >> >   devm_ioremap_resource(res);
>> >> >
>> >> > ... instead.
>> >>
>> >> this was explained as they are not in memory space, they are behind a
>> >> bus bridge.
>> >
>> > Use 'ranges' in the DT, then you can pull out the proper address
>> > without hand rolling your own method.
>>
>> it seems it is not a "ranges" thing,  things behind rtciobrg is much
>> like things behind USB or sdio. we need to use a rtciobrg protocol to
>> do read/write.
>> they can not be randomly accessed by load/store, and can't be XIP.
>> they don't have any ranges in CPU memory space.
>
> So what's the point of 'base' then?  I assumed this was the base of
> the IP registers which where memory mapped?

just think we have a i2c device, and this i2c device has multi-functions.
each function has a base of its register offset.
actually, the base is the offset of 1st register.


-barry

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Lee Jones]

On Tue, 29 Sep 2015, Barry Song wrote:

> 2015-09-29 15:16 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> > On Tue, 29 Sep 2015, Barry Song wrote:
> >> >> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >> >> >> +{
> >> >> >> +     struct device_node *np = pdev->dev.of_node;
> >> >> >> +     const struct of_device_id *match;
> >> >> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
> >> >> >> +     struct regmap_irq_chip *regmap_irq_chip;
> >> >> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
> >> >> >> +     struct regmap *map;
> >> >> >> +     int ret;
> >> >> >> +     u32 base;
> >> >> >> +
> >> >> >> +     if (of_property_read_u32(np, "reg", &base))
> >> >> >> +             panic("unable to find base address of pwrc node in dtb\n");
> >> >> >
> >> >> > It looks like this driver should depend on OF.
> >> >> >
> >> >> > Why are you obtaining the base address manually? Use:
> >> >> >
> >> >> >   res = platform_get_resource();
> >> >> >   devm_ioremap_resource(res);
> >> >> >
> >> >> > ... instead.
> >> >>
> >> >> this was explained as they are not in memory space, they are behind a
> >> >> bus bridge.
> >> >
> >> > Use 'ranges' in the DT, then you can pull out the proper address
> >> > without hand rolling your own method.
> >>
> >> it seems it is not a "ranges" thing,  things behind rtciobrg is much
> >> like things behind USB or sdio. we need to use a rtciobrg protocol to
> >> do read/write.
> >> they can not be randomly accessed by load/store, and can't be XIP.
> >> they don't have any ranges in CPU memory space.
> >
> > So what's the point of 'base' then?  I assumed this was the base of
> > the IP registers which where memory mapped?
> 
> just think we have a i2c device, and this i2c device has multi-functions.
> each function has a base of its register offset.
> actually, the base is the offset of 1st register.

Does it every change, from device to device?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Barry Song]

2015-09-29 16:55 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 29 Sep 2015, Barry Song wrote:
>
>> 2015-09-29 15:16 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
>> > On Tue, 29 Sep 2015, Barry Song wrote:
>> >> >> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> >> >> >> +{
>> >> >> >> +     struct device_node *np = pdev->dev.of_node;
>> >> >> >> +     const struct of_device_id *match;
>> >> >> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
>> >> >> >> +     struct regmap_irq_chip *regmap_irq_chip;
>> >> >> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
>> >> >> >> +     struct regmap *map;
>> >> >> >> +     int ret;
>> >> >> >> +     u32 base;
>> >> >> >> +
>> >> >> >> +     if (of_property_read_u32(np, "reg", &base))
>> >> >> >> +             panic("unable to find base address of pwrc node in dtb\n");
>> >> >> >
>> >> >> > It looks like this driver should depend on OF.
>> >> >> >
>> >> >> > Why are you obtaining the base address manually? Use:
>> >> >> >
>> >> >> >   res = platform_get_resource();
>> >> >> >   devm_ioremap_resource(res);
>> >> >> >
>> >> >> > ... instead.
>> >> >>
>> >> >> this was explained as they are not in memory space, they are behind a
>> >> >> bus bridge.
>> >> >
>> >> > Use 'ranges' in the DT, then you can pull out the proper address
>> >> > without hand rolling your own method.
>> >>
>> >> it seems it is not a "ranges" thing,  things behind rtciobrg is much
>> >> like things behind USB or sdio. we need to use a rtciobrg protocol to
>> >> do read/write.
>> >> they can not be randomly accessed by load/store, and can't be XIP.
>> >> they don't have any ranges in CPU memory space.
>> >
>> > So what's the point of 'base' then?  I assumed this was the base of
>> > the IP registers which where memory mapped?
>>
>> just think we have a i2c device, and this i2c device has multi-functions.
>> each function has a base of its register offset.
>> actually, the base is the offset of 1st register.
>
> Does it every change, from device to device?

yes. Lee. e.g:

                        rtc-iobg@18840000 {
                                reg = <0x18840000 0x1000>;

                                sysrtc@2000 {
                                        compatible = "sirf,prima2-sysrtc";
                                        reg = <0x2000 0x100>;
                                        interrupts = <0 52 0>;
                                };
                                pwrc@3000 {
                                        compatible = "sirf,atlas7-pwrc";
                                        reg = <0x3000 0x100>;
                                        interrupts = <0 32 0>;
                                };

                        }


-barry

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Lee Jones]

On Sun, 04 Oct 2015, Barry Song wrote:
> 2015-09-29 16:55 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> > On Tue, 29 Sep 2015, Barry Song wrote:
> >
> >> 2015-09-29 15:16 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> >> > On Tue, 29 Sep 2015, Barry Song wrote:
> >> >> >> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >> >> >> >> +{
> >> >> >> >> +     struct device_node *np = pdev->dev.of_node;
> >> >> >> >> +     const struct of_device_id *match;
> >> >> >> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
> >> >> >> >> +     struct regmap_irq_chip *regmap_irq_chip;
> >> >> >> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
> >> >> >> >> +     struct regmap *map;
> >> >> >> >> +     int ret;
> >> >> >> >> +     u32 base;
> >> >> >> >> +
> >> >> >> >> +     if (of_property_read_u32(np, "reg", &base))
> >> >> >> >> +             panic("unable to find base address of pwrc node in dtb\n");
> >> >> >> >
> >> >> >> > It looks like this driver should depend on OF.
> >> >> >> >
> >> >> >> > Why are you obtaining the base address manually? Use:
> >> >> >> >
> >> >> >> >   res = platform_get_resource();
> >> >> >> >   devm_ioremap_resource(res);
> >> >> >> >
> >> >> >> > ... instead.
> >> >> >>
> >> >> >> this was explained as they are not in memory space, they are behind a
> >> >> >> bus bridge.
> >> >> >
> >> >> > Use 'ranges' in the DT, then you can pull out the proper address
> >> >> > without hand rolling your own method.
> >> >>
> >> >> it seems it is not a "ranges" thing,  things behind rtciobrg is much
> >> >> like things behind USB or sdio. we need to use a rtciobrg protocol to
> >> >> do read/write.
> >> >> they can not be randomly accessed by load/store, and can't be XIP.
> >> >> they don't have any ranges in CPU memory space.
> >> >
> >> > So what's the point of 'base' then?  I assumed this was the base of
> >> > the IP registers which where memory mapped?
> >>
> >> just think we have a i2c device, and this i2c device has multi-functions.
> >> each function has a base of its register offset.
> >> actually, the base is the offset of 1st register.
> >
> > Does it every change, from device to device?

What I mean is ...

> yes. Lee. e.g:
> 
>                         rtc-iobg@18840000 {
>                                 reg = <0x18840000 0x1000>;
> 
>                                 sysrtc@2000 {

Is sysrtc always @2000, or might it be @4000 on some devices?

>                                         compatible = "sirf,prima2-sysrtc";
>                                         reg = <0x2000 0x100>;
>                                         interrupts = <0 52 0>;
>                                 };
>                                 pwrc@3000 {

Same for pwrc?

>                                         compatible = "sirf,atlas7-pwrc";
>                                         reg = <0x3000 0x100>;
>                                         interrupts = <0 32 0>;
>                                 };
> 
>                         }
> 
> 
> -barry

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Barry Song]

2015-10-05 16:21 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> On Sun, 04 Oct 2015, Barry Song wrote:
>> 2015-09-29 16:55 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
>> > On Tue, 29 Sep 2015, Barry Song wrote:
>> >
>> >> 2015-09-29 15:16 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
>> >> > On Tue, 29 Sep 2015, Barry Song wrote:
>> >> >> >> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> >> >> >> >> +{
>> >> >> >> >> +     struct device_node *np = pdev->dev.of_node;
>> >> >> >> >> +     const struct of_device_id *match;
>> >> >> >> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
>> >> >> >> >> +     struct regmap_irq_chip *regmap_irq_chip;
>> >> >> >> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
>> >> >> >> >> +     struct regmap *map;
>> >> >> >> >> +     int ret;
>> >> >> >> >> +     u32 base;
>> >> >> >> >> +
>> >> >> >> >> +     if (of_property_read_u32(np, "reg", &base))
>> >> >> >> >> +             panic("unable to find base address of pwrc node in dtb\n");
>> >> >> >> >
>> >> >> >> > It looks like this driver should depend on OF.
>> >> >> >> >
>> >> >> >> > Why are you obtaining the base address manually? Use:
>> >> >> >> >
>> >> >> >> >   res = platform_get_resource();
>> >> >> >> >   devm_ioremap_resource(res);
>> >> >> >> >
>> >> >> >> > ... instead.
>> >> >> >>
>> >> >> >> this was explained as they are not in memory space, they are behind a
>> >> >> >> bus bridge.
>> >> >> >
>> >> >> > Use 'ranges' in the DT, then you can pull out the proper address
>> >> >> > without hand rolling your own method.
>> >> >>
>> >> >> it seems it is not a "ranges" thing,  things behind rtciobrg is much
>> >> >> like things behind USB or sdio. we need to use a rtciobrg protocol to
>> >> >> do read/write.
>> >> >> they can not be randomly accessed by load/store, and can't be XIP.
>> >> >> they don't have any ranges in CPU memory space.
>> >> >
>> >> > So what's the point of 'base' then?  I assumed this was the base of
>> >> > the IP registers which where memory mapped?
>> >>
>> >> just think we have a i2c device, and this i2c device has multi-functions.
>> >> each function has a base of its register offset.
>> >> actually, the base is the offset of 1st register.
>> >
>> > Does it every change, from device to device?
>
> What I mean is ...

i get you now :-)

>
>> yes. Lee. e.g:
>>
>>                         rtc-iobg@18840000 {
>>                                 reg = <0x18840000 0x1000>;
>>
>>                                 sysrtc@2000 {
>
> Is sysrtc always @2000, or might it be @4000 on some devices?


that depends on the IC design. technically, they can be anywhere and
in any order behind rtc-iobridge.
good luck is the existing SoCs such as prima2, atlas6, atlas7 all put
it at 2000.

>
>>                                         compatible = "sirf,prima2-sysrtc";
>>                                         reg = <0x2000 0x100>;
>>                                         interrupts = <0 52 0>;
>>                                 };
>>                                 pwrc@3000 {
>
> Same for pwrc?

all of the existing prima2, atlas6, atlas7 chips put it at 3000. so
the problem is it a hardware OF property or a const/MARCO?
i feel it is a OF property. it is pretty much similar with we can put
a i2c, spi ,usb controller in different memory place in memory bus.

>
>>                                         compatible = "sirf,atlas7-pwrc";
>>                                         reg = <0x3000 0x100>;
>>                                         interrupts = <0 32 0>;
>>                                 };
>>
>>                         }
>>

-barry

Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Lee Jones]

On Mon, 05 Oct 2015, Barry Song wrote:

> 2015-10-05 16:21 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> > On Sun, 04 Oct 2015, Barry Song wrote:
> >> 2015-09-29 16:55 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> >> > On Tue, 29 Sep 2015, Barry Song wrote:
> >> >
> >> >> 2015-09-29 15:16 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> >> >> > On Tue, 29 Sep 2015, Barry Song wrote:
> >> >> >> >> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >> >> >> >> >> +{
> >> >> >> >> >> +     struct device_node *np = pdev->dev.of_node;
> >> >> >> >> >> +     const struct of_device_id *match;
> >> >> >> >> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
> >> >> >> >> >> +     struct regmap_irq_chip *regmap_irq_chip;
> >> >> >> >> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
> >> >> >> >> >> +     struct regmap *map;
> >> >> >> >> >> +     int ret;
> >> >> >> >> >> +     u32 base;
> >> >> >> >> >> +
> >> >> >> >> >> +     if (of_property_read_u32(np, "reg", &base))
> >> >> >> >> >> +             panic("unable to find base address of pwrc node in dtb\n");
> >> >> >> >> >
> >> >> >> >> > It looks like this driver should depend on OF.
> >> >> >> >> >
> >> >> >> >> > Why are you obtaining the base address manually? Use:
> >> >> >> >> >
> >> >> >> >> >   res = platform_get_resource();
> >> >> >> >> >   devm_ioremap_resource(res);
> >> >> >> >> >
> >> >> >> >> > ... instead.
> >> >> >> >>
> >> >> >> >> this was explained as they are not in memory space, they are behind a
> >> >> >> >> bus bridge.
> >> >> >> >
> >> >> >> > Use 'ranges' in the DT, then you can pull out the proper address
> >> >> >> > without hand rolling your own method.
> >> >> >>
> >> >> >> it seems it is not a "ranges" thing,  things behind rtciobrg is much
> >> >> >> like things behind USB or sdio. we need to use a rtciobrg protocol to
> >> >> >> do read/write.
> >> >> >> they can not be randomly accessed by load/store, and can't be XIP.
> >> >> >> they don't have any ranges in CPU memory space.
> >> >> >
> >> >> > So what's the point of 'base' then?  I assumed this was the base of
> >> >> > the IP registers which where memory mapped?
> >> >>
> >> >> just think we have a i2c device, and this i2c device has multi-functions.
> >> >> each function has a base of its register offset.
> >> >> actually, the base is the offset of 1st register.
> >> >
> >> > Does it every change, from device to device?
> >
> > What I mean is ...
> 
> i get you now :-)
> 
> >
> >> yes. Lee. e.g:
> >>
> >>                         rtc-iobg@18840000 {
> >>                                 reg = <0x18840000 0x1000>;
> >>
> >>                                 sysrtc@2000 {
> >
> > Is sysrtc always @2000, or might it be @4000 on some devices?
> 
> 
> that depends on the IC design. technically, they can be anywhere and
> in any order behind rtc-iobridge.
> good luck is the existing SoCs such as prima2, atlas6, atlas7 all put
> it at 2000.
> 
> >
> >>                                         compatible = "sirf,prima2-sysrtc";
> >>                                         reg = <0x2000 0x100>;
> >>                                         interrupts = <0 52 0>;
> >>                                 };
> >>                                 pwrc@3000 {
> >
> > Same for pwrc?
> 
> all of the existing prima2, atlas6, atlas7 chips put it at 3000. so
> the problem is it a hardware OF property or a const/MARCO?
> i feel it is a OF property. it is pretty much similar with we can put
> a i2c, spi ,usb controller in different memory place in memory bus.

Right, this is what I was getting at.  I think the address is unlikely
to change, and if (and that's a big IF) it does we can rework things.

It will reduced code and effort if you remove these from DT and
hard-code them into your driver as a #define.

Please rework accordingly.

> >>                                         compatible = "sirf,atlas7-pwrc";
> >>                                         reg = <0x3000 0x100>;
> >>                                         interrupts = <0 32 0>;
> >>                                 };
> >>
> >>                         }
> >>
> 
> -barry

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog