[PATCH v4 0/4] Mediatek MT2701 SCPSYS power domain support

STALE3869d

Revision v4 of 9 in this series.

16 messages, 4 authors, 2016-02-15 · open the first message on its own page

[PATCH v4 0/4] Mediatek MT2701 SCPSYS power domain support

From: James Liao <hidden>
Date: 2016-01-20 06:08:37 

This patchset is based on v4.4-next/soc from Matthias [1], which is
4.4-rc1 plus 2 patches. This patchset adds scpsys power domain support
for Mediatek MT2701.

The major change from v3 is merging MT2701 and MT8173 scpsys support
in a single file (mtk-scpsys.c).

To share the code between MT2701 and MT8173, this patchset also refined
original mtk-scpsys.c to separate common codes and platform codes, so
that mtk-scpsys.c can support new SoCs more easily.

MT8173 and MT2701 scpsys init level are now subsys_init. Please refer to [2]
to see discussion details.

changes since v3:
- Implement MT8173 and MT2701 scpsys drivers in a signle file.
- Remove naming of registers that can't be shared among SoCs.

changes since v2:
- Rebase to mbgg/linux-mediatek v4.4-next/soc [1].
- Remove MTK_SCPSYS_MT8173 and MTK_SCPSYS_MT2701.
- Modify scpsys dt-binding document to support MT2701.

changes since v1:
- Make MTK_SCPSYS in Kconfig invisible from users.
- Add comments for changing scpsys init level to subsys_init.

[1] https://github.com/mbgg/linux-mediatek/commits/v4.4-next/soc
[2] http://lists.infradead.org/pipermail/linux-mediatek/2015-December/003416.html

James Liao (2):
  soc: mediatek: Refine scpsys to support multiple platform
  soc: mediatek: Init MT8173 scpsys driver earlier

Shunli Wang (2):
  soc: mediatek: Add MT2701 power dt-bindings
  soc: mediatek: Add MT2701 scpsys driver

 .../devicetree/bindings/soc/mediatek/scpsys.txt    |   6 +-
 drivers/soc/mediatek/mtk-scpsys.c                  | 534 +++++++++++++--------
 drivers/soc/mediatek/mtk-scpsys.h                  |  55 +++
 include/dt-bindings/power/mt2701-power.h           |  27 ++
 4 files changed, 416 insertions(+), 206 deletions(-)
 create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
 create mode 100644 include/dt-bindings/power/mt2701-power.h

--
1.9.1

[PATCH v4 3/4] soc: mediatek: Add MT2701 power dt-bindings

From: James Liao <hidden>
Date: 2016-01-20 06:08:49 

From: Shunli Wang <redacted>

Add power dt-bindings for MT2701.

Signed-off-by: Shunli Wang <redacted>
Signed-off-by: James Liao <redacted>
---
 .../devicetree/bindings/soc/mediatek/scpsys.txt    |  6 +++--
 include/dt-bindings/power/mt2701-power.h           | 27 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 include/dt-bindings/power/mt2701-power.h

diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
index a6c8afc..807d87f 100644
--- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
+++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
@@ -9,10 +9,12 @@ domain control.
 
 The driver implements the Generic PM domain bindings described in
 power/power_domain.txt. It provides the power domains defined in
-include/dt-bindings/power/mt8173-power.h.
+include/dt-bindings/power/mt8173-power.h and mt2701-power.h.
 
 Required properties:
-- compatible: Must be "mediatek,mt8173-scpsys"
+- compatible: Should be:
+	- "mediatek,mt8173-scpsys"
+	- "mediatek,mt2701-scpsys"
 - #power-domain-cells: Must be 1
 - reg: Address range of the SCPSYS unit
 - infracfg: must contain a phandle to the infracfg controller
diff --git a/include/dt-bindings/power/mt2701-power.h b/include/dt-bindings/power/mt2701-power.h
new file mode 100644
index 0000000..64cc826
--- /dev/null
+++ b/include/dt-bindings/power/mt2701-power.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2015 MediaTek Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _DT_BINDINGS_POWER_MT2701_POWER_H
+#define _DT_BINDINGS_POWER_MT2701_POWER_H
+
+#define MT2701_POWER_DOMAIN_CONN	0
+#define MT2701_POWER_DOMAIN_DISP	1
+#define MT2701_POWER_DOMAIN_MFG		2
+#define MT2701_POWER_DOMAIN_VDEC	3
+#define MT2701_POWER_DOMAIN_ISP		4
+#define MT2701_POWER_DOMAIN_BDP		5
+#define MT2701_POWER_DOMAIN_ETH		6
+#define MT2701_POWER_DOMAIN_HIF		7
+#define MT2701_POWER_DOMAIN_IFR_MSC	8
+
+#endif /* _DT_BINDINGS_POWER_MT2701_POWER_H */
-- 
1.9.1

[PATCH v4 2/4] soc: mediatek: Init MT8173 scpsys driver earlier

From: James Liao <hidden>
Date: 2016-01-20 06:09:01 

Some power domain comsumers may init before module_init.
So the power domain provider (scpsys) need to be initialized
earlier too.

Take an example for our IOMMU (M4U) and SMI. SMI is a bridge
between IOMMU and multimedia HW. SMI is responsible to
enable/disable iommu and help transfer data for each multimedia
HW. Both of them have to wait until the power and clocks are
enabled.

So scpsys driver should be initialized before SMI, and SMI should
be initialized before IOMMU, and then init IOMMU consumers
(display/vdec/venc/camera etc.).

IOMMU is subsys_init by default. So we need to init scpsys driver
before subsys_init.

Signed-off-by: James Liao <redacted>
---
 drivers/soc/mediatek/mtk-scpsys.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 339adfc..0490a93 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -560,4 +560,21 @@ static struct platform_driver scpsys_drv = {
 		.of_match_table = of_match_ptr(of_scpsys_match_tbl),
 	},
 };
-builtin_platform_driver_probe(scpsys_drv, scpsys_probe);
+
+static int __init scpsys_drv_init(void)
+{
+	return platform_driver_probe(&scpsys_drv, scpsys_probe);
+}
+
+/*
+ * There are some Mediatek drivers which depend on the power domain driver need
+ * to probe in earlier initcall levels. So scpsys driver also need to probe
+ * earlier.
+ *
+ * IOMMU(M4U) and SMI drivers for example. SMI is a bridge between IOMMU and
+ * multimedia HW. IOMMU depends on SMI, and SMI is a power domain consumer,
+ * so the proper probe sequence should be scpsys -> SMI -> IOMMU driver.
+ * IOMMU drivers are initialized during subsys_init by default, so we need to
+ * move SMI and scpsys drivers to subsys_init or earlier init levels.
+ */
+subsys_initcall(scpsys_drv_init);
-- 
1.9.1

[PATCH v4 4/4] soc: mediatek: Add MT2701 scpsys driver

From: James Liao <hidden>
Date: 2016-01-20 06:09:18 

From: Shunli Wang <redacted>

Add scpsys driver for MT2701.

Signed-off-by: Shunli Wang <redacted>
Signed-off-by: James Liao <redacted>
---
 drivers/soc/mediatek/mtk-scpsys.c | 97 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0490a93..ee193aa 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -402,6 +402,100 @@ void mtk_register_power_domains(struct platform_device *pdev,
 }
 
 /*
+ * MT2701 power domain support
+ */
+
+#include <dt-bindings/power/mt2701-power.h>
+
+static const struct scp_domain_data scp_domain_data_mt2701[] __initconst = {
+	[MT2701_POWER_DOMAIN_CONN] = {
+		.name = "conn",
+		.sta_mask = BIT(1),
+		.ctl_offs = 0x0280,
+		.bus_prot_mask = 0x0104,
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_DISP] = {
+		.name = "disp",
+		.sta_mask = BIT(3),
+		.ctl_offs = 0x023c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.clk_id = {CLK_MM},
+		.bus_prot_mask = 0x0002,
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_MFG] = {
+		.name = "mfg",
+		.sta_mask = BIT(4),
+		.ctl_offs = 0x0214,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_VDEC] = {
+		.name = "vdec",
+		.sta_mask = BIT(7),
+		.ctl_offs = 0x0210,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MM},
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_ISP] = {
+		.name = "isp",
+		.sta_mask = BIT(5),
+		.ctl_offs = 0x0238,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(13, 12),
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_BDP] = {
+		.name = "bdp",
+		.sta_mask = BIT(14),
+		.ctl_offs = 0x029c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_ETH] = {
+		.name = "eth",
+		.sta_mask = BIT(15),
+		.ctl_offs = 0x02a0,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_HIF] = {
+		.name = "hif",
+		.sta_mask = BIT(16),
+		.ctl_offs = 0x02a4,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_IFR_MSC] = {
+		.name = "ifr_msc",
+		.sta_mask = BIT(17),
+		.ctl_offs = 0x02a8,
+		.active_wakeup = true,
+	},
+};
+
+#define NUM_DOMAINS_MT2701	ARRAY_SIZE(scp_domain_data_mt2701)
+
+static int __init scpsys_probe_mt2701(struct platform_device *pdev)
+{
+	struct scp *scp;
+
+	scp = init_scp(pdev, scp_domain_data_mt2701, NUM_DOMAINS_MT2701);
+	if (IS_ERR(scp))
+		return PTR_ERR(scp);
+
+	mtk_register_power_domains(pdev, scp, NUM_DOMAINS_MT2701);
+
+	return 0;
+}
+
+/*
  * MT8173 power domain support
  */
 
@@ -532,6 +626,9 @@ static int __init scpsys_probe_mt8173(struct platform_device *pdev)
 
 static const struct of_device_id of_scpsys_match_tbl[] = {
 	{
+		.compatible = "mediatek,mt2701-scpsys",
+		.data = scpsys_probe_mt2701,
+	}, {
 		.compatible = "mediatek,mt8173-scpsys",
 		.data = scpsys_probe_mt8173,
 	}, {
-- 
1.9.1

[PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

From: James Liao <hidden>
Date: 2016-01-20 06:09:30 

Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao <redacted>
---
 drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
 drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
 2 files changed, 270 insertions(+), 203 deletions(-)
 create mode 100644 drivers/soc/mediatek/mtk-scpsys.h

diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0221387..339adfc 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,29 +11,17 @@
  * GNU General Public License for more details.
  */
 #include <linux/clk.h>
-#include <linux/delay.h>
+#include <linux/init.h>
 #include <linux/io.h>
-#include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
-#include <linux/init.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
-#include <linux/regmap.h>
-#include <linux/soc/mediatek/infracfg.h>
 #include <linux/regulator/consumer.h>
-#include <dt-bindings/power/mt8173-power.h>
+#include <linux/soc/mediatek/infracfg.h>
+
+#include "mtk-scpsys.h"
 
-#define SPM_VDE_PWR_CON			0x0210
-#define SPM_MFG_PWR_CON			0x0214
-#define SPM_VEN_PWR_CON			0x0230
-#define SPM_ISP_PWR_CON			0x0238
-#define SPM_DIS_PWR_CON			0x023c
-#define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
-#define SPM_MFG_2D_PWR_CON		0x02c0
-#define SPM_MFG_ASYNC_PWR_CON		0x02c4
-#define SPM_USB_PWR_CON			0x02cc
 #define SPM_PWR_STATUS			0x060c
 #define SPM_PWR_STATUS_2ND		0x0610
 
@@ -43,154 +31,6 @@
 #define PWR_ON_2ND_BIT			BIT(3)
 #define PWR_CLK_DIS_BIT			BIT(4)
 
-#define PWR_STATUS_DISP			BIT(3)
-#define PWR_STATUS_MFG			BIT(4)
-#define PWR_STATUS_ISP			BIT(5)
-#define PWR_STATUS_VDEC			BIT(7)
-#define PWR_STATUS_VENC_LT		BIT(20)
-#define PWR_STATUS_VENC			BIT(21)
-#define PWR_STATUS_MFG_2D		BIT(22)
-#define PWR_STATUS_MFG_ASYNC		BIT(23)
-#define PWR_STATUS_AUDIO		BIT(24)
-#define PWR_STATUS_USB			BIT(25)
-
-enum clk_id {
-	MT8173_CLK_NONE,
-	MT8173_CLK_MM,
-	MT8173_CLK_MFG,
-	MT8173_CLK_VENC,
-	MT8173_CLK_VENC_LT,
-	MT8173_CLK_MAX,
-};
-
-#define MAX_CLKS	2
-
-struct scp_domain_data {
-	const char *name;
-	u32 sta_mask;
-	int ctl_offs;
-	u32 sram_pdn_bits;
-	u32 sram_pdn_ack_bits;
-	u32 bus_prot_mask;
-	enum clk_id clk_id[MAX_CLKS];
-	bool active_wakeup;
-};
-
-static const struct scp_domain_data scp_domain_data[] __initconst = {
-	[MT8173_POWER_DOMAIN_VDEC] = {
-		.name = "vdec",
-		.sta_mask = PWR_STATUS_VDEC,
-		.ctl_offs = SPM_VDE_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(12, 12),
-		.clk_id = {MT8173_CLK_MM},
-	},
-	[MT8173_POWER_DOMAIN_VENC] = {
-		.name = "venc",
-		.sta_mask = PWR_STATUS_VENC,
-		.ctl_offs = SPM_VEN_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_MM, MT8173_CLK_VENC},
-	},
-	[MT8173_POWER_DOMAIN_ISP] = {
-		.name = "isp",
-		.sta_mask = PWR_STATUS_ISP,
-		.ctl_offs = SPM_ISP_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(13, 12),
-		.clk_id = {MT8173_CLK_MM},
-	},
-	[MT8173_POWER_DOMAIN_MM] = {
-		.name = "mm",
-		.sta_mask = PWR_STATUS_DISP,
-		.ctl_offs = SPM_DIS_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(12, 12),
-		.clk_id = {MT8173_CLK_MM},
-		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
-			MT8173_TOP_AXI_PROT_EN_MM_M1,
-	},
-	[MT8173_POWER_DOMAIN_VENC_LT] = {
-		.name = "venc_lt",
-		.sta_mask = PWR_STATUS_VENC_LT,
-		.ctl_offs = SPM_VEN2_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_MM, MT8173_CLK_VENC_LT},
-	},
-	[MT8173_POWER_DOMAIN_AUDIO] = {
-		.name = "audio",
-		.sta_mask = PWR_STATUS_AUDIO,
-		.ctl_offs = SPM_AUDIO_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_NONE},
-	},
-	[MT8173_POWER_DOMAIN_USB] = {
-		.name = "usb",
-		.sta_mask = PWR_STATUS_USB,
-		.ctl_offs = SPM_USB_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_NONE},
-		.active_wakeup = true,
-	},
-	[MT8173_POWER_DOMAIN_MFG_ASYNC] = {
-		.name = "mfg_async",
-		.sta_mask = PWR_STATUS_MFG_ASYNC,
-		.ctl_offs = SPM_MFG_ASYNC_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = 0,
-		.clk_id = {MT8173_CLK_MFG},
-	},
-	[MT8173_POWER_DOMAIN_MFG_2D] = {
-		.name = "mfg_2d",
-		.sta_mask = PWR_STATUS_MFG_2D,
-		.ctl_offs = SPM_MFG_2D_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(13, 12),
-		.clk_id = {MT8173_CLK_NONE},
-	},
-	[MT8173_POWER_DOMAIN_MFG] = {
-		.name = "mfg",
-		.sta_mask = PWR_STATUS_MFG,
-		.ctl_offs = SPM_MFG_PWR_CON,
-		.sram_pdn_bits = GENMASK(13, 8),
-		.sram_pdn_ack_bits = GENMASK(21, 16),
-		.clk_id = {MT8173_CLK_NONE},
-		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
-			MT8173_TOP_AXI_PROT_EN_MFG_M0 |
-			MT8173_TOP_AXI_PROT_EN_MFG_M1 |
-			MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
-	},
-};
-
-#define NUM_DOMAINS	ARRAY_SIZE(scp_domain_data)
-
-struct scp;
-
-struct scp_domain {
-	struct generic_pm_domain genpd;
-	struct scp *scp;
-	struct clk *clk[MAX_CLKS];
-	u32 sta_mask;
-	void __iomem *ctl_addr;
-	u32 sram_pdn_bits;
-	u32 sram_pdn_ack_bits;
-	u32 bus_prot_mask;
-	bool active_wakeup;
-	struct regulator *supply;
-};
-
-struct scp {
-	struct scp_domain domains[NUM_DOMAINS];
-	struct genpd_onecell_data pd_data;
-	struct device *dev;
-	void __iomem *base;
-	struct regmap *infracfg;
-};
-
 static int scpsys_domain_is_on(struct scp_domain *scpd)
 {
 	struct scp *scp = scpd->scp;
@@ -412,57 +252,69 @@ static bool scpsys_active_wakeup(struct device *dev)
 	return scpd->active_wakeup;
 }
 
-static int __init scpsys_probe(struct platform_device *pdev)
+static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
+{
+	enum clk_id clk_ids[] = {
+		CLK_MM,
+		CLK_MFG,
+		CLK_VENC,
+		CLK_VENC_LT
+	};
+
+	static const char * const clk_names[] = {
+		"mm",
+		"mfg",
+		"venc",
+		"venc_lt",
+	};
+
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(clk_ids); i++)
+		clk[clk_ids[i]] = devm_clk_get(&pdev->dev, clk_names[i]);
+}
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num)
 {
 	struct genpd_onecell_data *pd_data;
 	struct resource *res;
-	int i, j, ret;
+	int i, j;
 	struct scp *scp;
-	struct clk *clk[MT8173_CLK_MAX];
+	struct clk *clk[CLK_MAX];
 
 	scp = devm_kzalloc(&pdev->dev, sizeof(*scp), GFP_KERNEL);
 	if (!scp)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	scp->dev = &pdev->dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	scp->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(scp->base))
-		return PTR_ERR(scp->base);
-
-	pd_data = &scp->pd_data;
-
-	pd_data->domains = devm_kzalloc(&pdev->dev,
-			sizeof(*pd_data->domains) * NUM_DOMAINS, GFP_KERNEL);
-	if (!pd_data->domains)
-		return -ENOMEM;
-
-	clk[MT8173_CLK_MM] = devm_clk_get(&pdev->dev, "mm");
-	if (IS_ERR(clk[MT8173_CLK_MM]))
-		return PTR_ERR(clk[MT8173_CLK_MM]);
-
-	clk[MT8173_CLK_MFG] = devm_clk_get(&pdev->dev, "mfg");
-	if (IS_ERR(clk[MT8173_CLK_MFG]))
-		return PTR_ERR(clk[MT8173_CLK_MFG]);
-
-	clk[MT8173_CLK_VENC] = devm_clk_get(&pdev->dev, "venc");
-	if (IS_ERR(clk[MT8173_CLK_VENC]))
-		return PTR_ERR(clk[MT8173_CLK_VENC]);
-
-	clk[MT8173_CLK_VENC_LT] = devm_clk_get(&pdev->dev, "venc_lt");
-	if (IS_ERR(clk[MT8173_CLK_VENC_LT]))
-		return PTR_ERR(clk[MT8173_CLK_VENC_LT]);
+		return ERR_CAST(scp->base);
 
 	scp->infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
 			"infracfg");
 	if (IS_ERR(scp->infracfg)) {
 		dev_err(&pdev->dev, "Cannot find infracfg controller: %ld\n",
 				PTR_ERR(scp->infracfg));
-		return PTR_ERR(scp->infracfg);
+		return ERR_CAST(scp->infracfg);
 	}
 
-	for (i = 0; i < NUM_DOMAINS; i++) {
+	scp->domains = devm_kzalloc(&pdev->dev,
+				sizeof(*scp->domains) * num, GFP_KERNEL);
+	if (!scp->domains)
+		return ERR_PTR(-ENOMEM);
+
+	pd_data = &scp->pd_data;
+
+	pd_data->domains = devm_kzalloc(&pdev->dev,
+			sizeof(*pd_data->domains) * num, GFP_KERNEL);
+	if (!pd_data->domains)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < num; i++) {
 		struct scp_domain *scpd = &scp->domains[i];
 		const struct scp_domain_data *data = &scp_domain_data[i];
 
@@ -471,17 +323,31 @@ static int __init scpsys_probe(struct platform_device *pdev)
 			if (PTR_ERR(scpd->supply) == -ENODEV)
 				scpd->supply = NULL;
 			else
-				return PTR_ERR(scpd->supply);
+				return ERR_CAST(scpd->supply);
 		}
 	}
 
-	pd_data->num_domains = NUM_DOMAINS;
+	pd_data->num_domains = num;
 
-	for (i = 0; i < NUM_DOMAINS; i++) {
+	init_clks(pdev, clk);
+
+	for (i = 0; i < num; i++) {
 		struct scp_domain *scpd = &scp->domains[i];
 		struct generic_pm_domain *genpd = &scpd->genpd;
 		const struct scp_domain_data *data = &scp_domain_data[i];
 
+		for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++) {
+			struct clk *c = clk[data->clk_id[j]];
+
+			if (IS_ERR(c)) {
+				dev_err(&pdev->dev, "%s: clk unavailable\n",
+					data->name);
+				return ERR_CAST(c);
+			}
+
+			scpd->clk[j] = c;
+		}
+
 		pd_data->domains[i] = genpd;
 		scpd->scp = scp;
 
@@ -491,13 +357,25 @@ static int __init scpsys_probe(struct platform_device *pdev)
 		scpd->sram_pdn_ack_bits = data->sram_pdn_ack_bits;
 		scpd->bus_prot_mask = data->bus_prot_mask;
 		scpd->active_wakeup = data->active_wakeup;
-		for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++)
-			scpd->clk[j] = clk[data->clk_id[j]];
 
 		genpd->name = data->name;
 		genpd->power_off = scpsys_power_off;
 		genpd->power_on = scpsys_power_on;
 		genpd->dev_ops.active_wakeup = scpsys_active_wakeup;
+	}
+
+	return scp;
+}
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num)
+{
+	struct genpd_onecell_data *pd_data;
+	int i, ret;
+
+	for (i = 0; i < num; i++) {
+		struct scp_domain *scpd = &scp->domains[i];
+		struct generic_pm_domain *genpd = &scpd->genpd;
 
 		/*
 		 * Initially turn on all domains to make the domains usable
@@ -516,6 +394,125 @@ static int __init scpsys_probe(struct platform_device *pdev)
 	 * valid.
 	 */
 
+	pd_data = &scp->pd_data;
+
+	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to add OF provider: %d\n", ret);
+}
+
+/*
+ * MT8173 power domain support
+ */
+
+#include <dt-bindings/power/mt8173-power.h>
+
+static const struct scp_domain_data scp_domain_data_mt8173[] __initconst = {
+	[MT8173_POWER_DOMAIN_VDEC] = {
+		.name = "vdec",
+		.sta_mask = BIT(7),
+		.ctl_offs = 0x0210,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MM},
+	},
+	[MT8173_POWER_DOMAIN_VENC] = {
+		.name = "venc",
+		.sta_mask = BIT(21),
+		.ctl_offs = 0x0230,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_MM, CLK_VENC},
+	},
+	[MT8173_POWER_DOMAIN_ISP] = {
+		.name = "isp",
+		.sta_mask = BIT(5),
+		.ctl_offs = 0x0238,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(13, 12),
+		.clk_id = {CLK_MM},
+	},
+	[MT8173_POWER_DOMAIN_MM] = {
+		.name = "mm",
+		.sta_mask = BIT(3),
+		.ctl_offs = 0x023c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MM},
+		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
+			MT8173_TOP_AXI_PROT_EN_MM_M1,
+	},
+	[MT8173_POWER_DOMAIN_VENC_LT] = {
+		.name = "venc_lt",
+		.sta_mask = BIT(20),
+		.ctl_offs = 0x0298,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_MM, CLK_VENC_LT},
+	},
+	[MT8173_POWER_DOMAIN_AUDIO] = {
+		.name = "audio",
+		.sta_mask = BIT(24),
+		.ctl_offs = 0x029c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_NONE},
+	},
+	[MT8173_POWER_DOMAIN_USB] = {
+		.name = "usb",
+		.sta_mask = BIT(25),
+		.ctl_offs = 0x02cc,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_NONE},
+		.active_wakeup = true,
+	},
+	[MT8173_POWER_DOMAIN_MFG_ASYNC] = {
+		.name = "mfg_async",
+		.sta_mask = BIT(23),
+		.ctl_offs = 0x02c4,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = 0,
+		.clk_id = {CLK_MFG},
+	},
+	[MT8173_POWER_DOMAIN_MFG_2D] = {
+		.name = "mfg_2d",
+		.sta_mask = BIT(22),
+		.ctl_offs = 0x02c0,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(13, 12),
+		.clk_id = {CLK_NONE},
+	},
+	[MT8173_POWER_DOMAIN_MFG] = {
+		.name = "mfg",
+		.sta_mask = BIT(4),
+		.ctl_offs = 0x0214,
+		.sram_pdn_bits = GENMASK(13, 8),
+		.sram_pdn_ack_bits = GENMASK(21, 16),
+		.clk_id = {CLK_NONE},
+		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
+			MT8173_TOP_AXI_PROT_EN_MFG_M0 |
+			MT8173_TOP_AXI_PROT_EN_MFG_M1 |
+			MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
+	},
+};
+
+#define NUM_DOMAINS_MT8173	ARRAY_SIZE(scp_domain_data_mt8173)
+
+static int __init scpsys_probe_mt8173(struct platform_device *pdev)
+{
+	struct scp *scp;
+	struct genpd_onecell_data *pd_data;
+	int ret;
+
+	scp = init_scp(pdev, scp_domain_data_mt8173, NUM_DOMAINS_MT8173);
+	if (IS_ERR(scp))
+		return PTR_ERR(scp);
+
+	mtk_register_power_domains(pdev, scp, NUM_DOMAINS_MT8173);
+
+	pd_data = &scp->pd_data;
+
 	ret = pm_genpd_add_subdomain(pd_data->domains[MT8173_POWER_DOMAIN_MFG_ASYNC],
 		pd_data->domains[MT8173_POWER_DOMAIN_MFG_2D]);
 	if (ret && IS_ENABLED(CONFIG_PM))
@@ -526,21 +523,36 @@ static int __init scpsys_probe(struct platform_device *pdev)
 	if (ret && IS_ENABLED(CONFIG_PM))
 		dev_err(&pdev->dev, "Failed to add subdomain: %d\n", ret);
 
-	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data);
-	if (ret)
-		dev_err(&pdev->dev, "Failed to add OF provider: %d\n", ret);
-
 	return 0;
 }
 
+/*
+ * scpsys driver init
+ */
+
 static const struct of_device_id of_scpsys_match_tbl[] = {
 	{
 		.compatible = "mediatek,mt8173-scpsys",
+		.data = scpsys_probe_mt8173,
 	}, {
 		/* sentinel */
 	}
 };
 
+static int __init scpsys_probe(struct platform_device *pdev)
+{
+	int (*probe)(struct platform_device *);
+	const struct of_device_id *of_id;
+
+	of_id = of_match_node(of_scpsys_match_tbl, pdev->dev.of_node);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+
+	probe = of_id->data;
+
+	return probe(pdev);
+}
+
 static struct platform_driver scpsys_drv = {
 	.driver = {
 		.name = "mtk-scpsys",
diff --git a/drivers/soc/mediatek/mtk-scpsys.h b/drivers/soc/mediatek/mtk-scpsys.h
new file mode 100644
index 0000000..e435bc3
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys.h
@@ -0,0 +1,55 @@
+#ifndef __DRV_SOC_MTK_H
+#define __DRV_SOC_MTK_H
+
+enum clk_id {
+	CLK_NONE,
+	CLK_MM,
+	CLK_MFG,
+	CLK_VENC,
+	CLK_VENC_LT,
+	CLK_MAX,
+};
+
+#define MAX_CLKS	2
+
+struct scp_domain_data {
+	const char *name;
+	u32 sta_mask;
+	int ctl_offs;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	enum clk_id clk_id[MAX_CLKS];
+	bool active_wakeup;
+};
+
+struct scp;
+
+struct scp_domain {
+	struct generic_pm_domain genpd;
+	struct scp *scp;
+	struct clk *clk[MAX_CLKS];
+	u32 sta_mask;
+	void __iomem *ctl_addr;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	bool active_wakeup;
+	struct regulator *supply;
+};
+
+struct scp {
+	struct scp_domain *domains;
+	struct genpd_onecell_data pd_data;
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *infracfg;
+};
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num);
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num);
+
+#endif /* __DRV_SOC_MTK_H */
-- 
1.9.1

Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

From: Yingjoe Chen <hidden>
Date: 2016-01-20 09:14:22 

On Wed, 2016-01-20 at 14:08 +0800, James Liao wrote:
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao <redacted>
<...>
quoted hunk
diff --git a/drivers/soc/mediatek/mtk-scpsys.h b/drivers/soc/mediatek/mtk-scpsys.h
new file mode 100644
index 0000000..e435bc3
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys.h
@@ -0,0 +1,55 @@
+#ifndef __DRV_SOC_MTK_H
+#define __DRV_SOC_MTK_H
+
+enum clk_id {
+	CLK_NONE,
+	CLK_MM,
+	CLK_MFG,
+	CLK_VENC,
+	CLK_VENC_LT,
+	CLK_MAX,
+};
+
+#define MAX_CLKS	2
+
+struct scp_domain_data {
+	const char *name;
+	u32 sta_mask;
+	int ctl_offs;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	enum clk_id clk_id[MAX_CLKS];
+	bool active_wakeup;
+};
+
+struct scp;
+
+struct scp_domain {
+	struct generic_pm_domain genpd;
+	struct scp *scp;
+	struct clk *clk[MAX_CLKS];
+	u32 sta_mask;
+	void __iomem *ctl_addr;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	bool active_wakeup;
+	struct regulator *supply;
+};
+
+struct scp {
+	struct scp_domain *domains;
+	struct genpd_onecell_data pd_data;
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *infracfg;
+};
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num);
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num);
+
+#endif /* __DRV_SOC_MTK_H */
After merge, only mtk-scpsys.c will use this file. I think it make sense
to just include them in mtk-scpsys.c. Also init_scp and
mtk_register_power_domains can be static now.

Joe.C

Re: [PATCH v4 3/4] soc: mediatek: Add MT2701 power dt-bindings

From: Yingjoe Chen <hidden>
Date: 2016-01-20 09:29:32 

On Wed, 2016-01-20 at 14:08 +0800, James Liao wrote:
quoted hunk
From: Shunli Wang <redacted>

Add power dt-bindings for MT2701.

Signed-off-by: Shunli Wang <redacted>
Signed-off-by: James Liao <redacted>
---
 .../devicetree/bindings/soc/mediatek/scpsys.txt    |  6 +++--
 include/dt-bindings/power/mt2701-power.h           | 27 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 include/dt-bindings/power/mt2701-power.h

diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
index a6c8afc..807d87f 100644
--- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
+++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
@@ -9,10 +9,12 @@ domain control.
 
 The driver implements the Generic PM domain bindings described in
 power/power_domain.txt. It provides the power domains defined in
-include/dt-bindings/power/mt8173-power.h.
+include/dt-bindings/power/mt8173-power.h and mt2701-power.h.
 
 Required properties:
-- compatible: Must be "mediatek,mt8173-scpsys"
+- compatible: Should be:
+	- "mediatek,mt8173-scpsys"
+	- "mediatek,mt2701-scpsys"
 - #power-domain-cells: Must be 1
 - reg: Address range of the SCPSYS unit
 - infracfg: must contain a phandle to the infracfg controller
Please sort the list.

Joe.C

Re: [PATCH v4 3/4] soc: mediatek: Add MT2701 power dt-bindings

From: Rob Herring <robh@kernel.org>
Date: 2016-01-20 16:35:16 

On Wed, Jan 20, 2016 at 05:29:21PM +0800, Yingjoe Chen wrote:
On Wed, 2016-01-20 at 14:08 +0800, James Liao wrote:
quoted
From: Shunli Wang <redacted>

Add power dt-bindings for MT2701.

Signed-off-by: Shunli Wang <redacted>
Signed-off-by: James Liao <redacted>
---
 .../devicetree/bindings/soc/mediatek/scpsys.txt    |  6 +++--
 include/dt-bindings/power/mt2701-power.h           | 27 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 include/dt-bindings/power/mt2701-power.h

diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
index a6c8afc..807d87f 100644
--- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
+++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
@@ -9,10 +9,12 @@ domain control.
 
 The driver implements the Generic PM domain bindings described in
 power/power_domain.txt. It provides the power domains defined in
-include/dt-bindings/power/mt8173-power.h.
+include/dt-bindings/power/mt8173-power.h and mt2701-power.h.
 
 Required properties:
-- compatible: Must be "mediatek,mt8173-scpsys"
+- compatible: Should be:
+	- "mediatek,mt8173-scpsys"
+	- "mediatek,mt2701-scpsys"
 - #power-domain-cells: Must be 1
 - reg: Address range of the SCPSYS unit
 - infracfg: must contain a phandle to the infracfg controller
Please sort the list.
And s/Should be/Should be one of/

Rob

Re: [PATCH v4 3/4] soc: mediatek: Add MT2701 power dt-bindings

From: James Liao <hidden>
Date: 2016-01-21 05:22:35 

Hi Yingjoe, Rob,

On Wed, 2016-01-20 at 10:35 -0600, Rob Herring wrote:
On Wed, Jan 20, 2016 at 05:29:21PM +0800, Yingjoe Chen wrote:
quoted
On Wed, 2016-01-20 at 14:08 +0800, James Liao wrote:
quoted
From: Shunli Wang <redacted>

Add power dt-bindings for MT2701.

Signed-off-by: Shunli Wang <redacted>
Signed-off-by: James Liao <redacted>
---
 .../devicetree/bindings/soc/mediatek/scpsys.txt    |  6 +++--
 include/dt-bindings/power/mt2701-power.h           | 27 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 include/dt-bindings/power/mt2701-power.h

diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
index a6c8afc..807d87f 100644
--- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
+++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
@@ -9,10 +9,12 @@ domain control.
 
 The driver implements the Generic PM domain bindings described in
 power/power_domain.txt. It provides the power domains defined in
-include/dt-bindings/power/mt8173-power.h.
+include/dt-bindings/power/mt8173-power.h and mt2701-power.h.
 
 Required properties:
-- compatible: Must be "mediatek,mt8173-scpsys"
+- compatible: Should be:
+	- "mediatek,mt8173-scpsys"
+	- "mediatek,mt2701-scpsys"
 - #power-domain-cells: Must be 1
 - reg: Address range of the SCPSYS unit
 - infracfg: must contain a phandle to the infracfg controller
Please sort the list.
And s/Should be/Should be one of/
OK. I'll modify it in next patch.


Best regards,

James

Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

From: James Liao <hidden>
Date: 2016-01-21 05:27:52 

On Wed, 2016-01-20 at 17:14 +0800, Yingjoe Chen wrote:
On Wed, 2016-01-20 at 14:08 +0800, James Liao wrote:
quoted
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao <redacted>
<...>
quoted
diff --git a/drivers/soc/mediatek/mtk-scpsys.h b/drivers/soc/mediatek/mtk-scpsys.h
new file mode 100644
index 0000000..e435bc3
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys.h
@@ -0,0 +1,55 @@
+#ifndef __DRV_SOC_MTK_H
+#define __DRV_SOC_MTK_H
+
+enum clk_id {
+	CLK_NONE,
+	CLK_MM,
+	CLK_MFG,
+	CLK_VENC,
+	CLK_VENC_LT,
+	CLK_MAX,
+};
+
+#define MAX_CLKS	2
+
+struct scp_domain_data {
+	const char *name;
+	u32 sta_mask;
+	int ctl_offs;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	enum clk_id clk_id[MAX_CLKS];
+	bool active_wakeup;
+};
+
+struct scp;
+
+struct scp_domain {
+	struct generic_pm_domain genpd;
+	struct scp *scp;
+	struct clk *clk[MAX_CLKS];
+	u32 sta_mask;
+	void __iomem *ctl_addr;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	bool active_wakeup;
+	struct regulator *supply;
+};
+
+struct scp {
+	struct scp_domain *domains;
+	struct genpd_onecell_data pd_data;
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *infracfg;
+};
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num);
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num);
+
+#endif /* __DRV_SOC_MTK_H */
After merge, only mtk-scpsys.c will use this file. I think it make sense
to just include them in mtk-scpsys.c. Also init_scp and
mtk_register_power_domains can be static now.
Hi Yingjoe,

OK. I can merge this header file into mtk-scpsys.c when we confirmed the
MT8173 + MT2701 implementation is OK.


Hi Matthias,

Do you have suggestions for this implementation that merge MT8173 and
MT2701 support in the same file?


Best regards,

James

Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

From: Matthias Brugger <matthias.bgg@gmail.com>
Date: 2016-01-31 11:51:56 


On 20/01/16 07:08, James Liao wrote:
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao <redacted>
---
  drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
  drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
  2 files changed, 270 insertions(+), 203 deletions(-)
  create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
In general this approach looks fine to me, comments below.
quoted hunk
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0221387..339adfc 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,29 +11,17 @@
   * GNU General Public License for more details.
   */
  #include <linux/clk.h>
-#include <linux/delay.h>
+#include <linux/init.h>
  #include <linux/io.h>
-#include <linux/kernel.h>
  #include <linux/mfd/syscon.h>
When at it, do we need this include?

-#include <linux/init.h>
  #include <linux/of_device.h>
  #include <linux/platform_device.h>
  #include <linux/pm_domain.h>
-#include <linux/regmap.h>
-#include <linux/soc/mediatek/infracfg.h>
  #include <linux/regulator/consumer.h>
-#include <dt-bindings/power/mt8173-power.h>
+#include <linux/soc/mediatek/infracfg.h>
+
+#include "mtk-scpsys.h"

-#define SPM_VDE_PWR_CON			0x0210
-#define SPM_MFG_PWR_CON			0x0214
-#define SPM_VEN_PWR_CON			0x0230
-#define SPM_ISP_PWR_CON			0x0238
-#define SPM_DIS_PWR_CON			0x023c
-#define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
-#define SPM_MFG_2D_PWR_CON		0x02c0
-#define SPM_MFG_ASYNC_PWR_CON		0x02c4
-#define SPM_USB_PWR_CON			0x02cc
I would prefer to keep this defines and declare SoC specific ones where 
necessary. It makes the code more readable.

quoted hunk
  #define SPM_PWR_STATUS			0x060c
  #define SPM_PWR_STATUS_2ND		0x0610

@@ -43,154 +31,6 @@
  #define PWR_ON_2ND_BIT			BIT(3)
  #define PWR_CLK_DIS_BIT			BIT(4)

-#define PWR_STATUS_DISP			BIT(3)
-#define PWR_STATUS_MFG			BIT(4)
-#define PWR_STATUS_ISP			BIT(5)
-#define PWR_STATUS_VDEC			BIT(7)
-#define PWR_STATUS_VENC_LT		BIT(20)
-#define PWR_STATUS_VENC			BIT(21)
-#define PWR_STATUS_MFG_2D		BIT(22)
-#define PWR_STATUS_MFG_ASYNC		BIT(23)
-#define PWR_STATUS_AUDIO		BIT(24)
-#define PWR_STATUS_USB			BIT(25)
-
Same here.

quoted hunk
-enum clk_id {
-	MT8173_CLK_NONE,
-	MT8173_CLK_MM,
-	MT8173_CLK_MFG,
-	MT8173_CLK_VENC,
-	MT8173_CLK_VENC_LT,
-	MT8173_CLK_MAX,
-};
-
-#define MAX_CLKS	2
-
-struct scp_domain_data {
-	const char *name;
-	u32 sta_mask;
-	int ctl_offs;
-	u32 sram_pdn_bits;
-	u32 sram_pdn_ack_bits;
-	u32 bus_prot_mask;
-	enum clk_id clk_id[MAX_CLKS];
-	bool active_wakeup;
-};
-
-static const struct scp_domain_data scp_domain_data[] __initconst = {
-	[MT8173_POWER_DOMAIN_VDEC] = {
-		.name = "vdec",
-		.sta_mask = PWR_STATUS_VDEC,
-		.ctl_offs = SPM_VDE_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(12, 12),
-		.clk_id = {MT8173_CLK_MM},
-	},
-	[MT8173_POWER_DOMAIN_VENC] = {
-		.name = "venc",
-		.sta_mask = PWR_STATUS_VENC,
-		.ctl_offs = SPM_VEN_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_MM, MT8173_CLK_VENC},
-	},
-	[MT8173_POWER_DOMAIN_ISP] = {
-		.name = "isp",
-		.sta_mask = PWR_STATUS_ISP,
-		.ctl_offs = SPM_ISP_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(13, 12),
-		.clk_id = {MT8173_CLK_MM},
-	},
-	[MT8173_POWER_DOMAIN_MM] = {
-		.name = "mm",
-		.sta_mask = PWR_STATUS_DISP,
-		.ctl_offs = SPM_DIS_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(12, 12),
-		.clk_id = {MT8173_CLK_MM},
-		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
-			MT8173_TOP_AXI_PROT_EN_MM_M1,
-	},
-	[MT8173_POWER_DOMAIN_VENC_LT] = {
-		.name = "venc_lt",
-		.sta_mask = PWR_STATUS_VENC_LT,
-		.ctl_offs = SPM_VEN2_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_MM, MT8173_CLK_VENC_LT},
-	},
-	[MT8173_POWER_DOMAIN_AUDIO] = {
-		.name = "audio",
-		.sta_mask = PWR_STATUS_AUDIO,
-		.ctl_offs = SPM_AUDIO_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_NONE},
-	},
-	[MT8173_POWER_DOMAIN_USB] = {
-		.name = "usb",
-		.sta_mask = PWR_STATUS_USB,
-		.ctl_offs = SPM_USB_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_NONE},
-		.active_wakeup = true,
-	},
-	[MT8173_POWER_DOMAIN_MFG_ASYNC] = {
-		.name = "mfg_async",
-		.sta_mask = PWR_STATUS_MFG_ASYNC,
-		.ctl_offs = SPM_MFG_ASYNC_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = 0,
-		.clk_id = {MT8173_CLK_MFG},
-	},
-	[MT8173_POWER_DOMAIN_MFG_2D] = {
-		.name = "mfg_2d",
-		.sta_mask = PWR_STATUS_MFG_2D,
-		.ctl_offs = SPM_MFG_2D_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(13, 12),
-		.clk_id = {MT8173_CLK_NONE},
-	},
-	[MT8173_POWER_DOMAIN_MFG] = {
-		.name = "mfg",
-		.sta_mask = PWR_STATUS_MFG,
-		.ctl_offs = SPM_MFG_PWR_CON,
-		.sram_pdn_bits = GENMASK(13, 8),
-		.sram_pdn_ack_bits = GENMASK(21, 16),
-		.clk_id = {MT8173_CLK_NONE},
-		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
-			MT8173_TOP_AXI_PROT_EN_MFG_M0 |
-			MT8173_TOP_AXI_PROT_EN_MFG_M1 |
-			MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
-	},
-};
-
-#define NUM_DOMAINS	ARRAY_SIZE(scp_domain_data)
-
-struct scp;
-
-struct scp_domain {
-	struct generic_pm_domain genpd;
-	struct scp *scp;
-	struct clk *clk[MAX_CLKS];
-	u32 sta_mask;
-	void __iomem *ctl_addr;
-	u32 sram_pdn_bits;
-	u32 sram_pdn_ack_bits;
-	u32 bus_prot_mask;
-	bool active_wakeup;
-	struct regulator *supply;
-};
-
-struct scp {
-	struct scp_domain domains[NUM_DOMAINS];
-	struct genpd_onecell_data pd_data;
-	struct device *dev;
-	void __iomem *base;
-	struct regmap *infracfg;
-};
-
  static int scpsys_domain_is_on(struct scp_domain *scpd)
  {
  	struct scp *scp = scpd->scp;
@@ -412,57 +252,69 @@ static bool scpsys_active_wakeup(struct device *dev)
  	return scpd->active_wakeup;
  }

-static int __init scpsys_probe(struct platform_device *pdev)
+static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
+{
+	enum clk_id clk_ids[] = {
+		CLK_MM,
+		CLK_MFG,
+		CLK_VENC,
+		CLK_VENC_LT
+	};
+
+	static const char * const clk_names[] = {
+		"mm",
+		"mfg",
+		"venc",
+		"venc_lt",
+	};
+
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(clk_ids); i++)
+		clk[clk_ids[i]] = devm_clk_get(&pdev->dev, clk_names[i]);
+}
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num)
  {
  	struct genpd_onecell_data *pd_data;
  	struct resource *res;
-	int i, j, ret;
+	int i, j;
  	struct scp *scp;
-	struct clk *clk[MT8173_CLK_MAX];
+	struct clk *clk[CLK_MAX];

  	scp = devm_kzalloc(&pdev->dev, sizeof(*scp), GFP_KERNEL);
  	if (!scp)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);

  	scp->dev = &pdev->dev;

  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	scp->base = devm_ioremap_resource(&pdev->dev, res);
  	if (IS_ERR(scp->base))
-		return PTR_ERR(scp->base);
-
-	pd_data = &scp->pd_data;
-
-	pd_data->domains = devm_kzalloc(&pdev->dev,
-			sizeof(*pd_data->domains) * NUM_DOMAINS, GFP_KERNEL);
-	if (!pd_data->domains)
-		return -ENOMEM;
-
-	clk[MT8173_CLK_MM] = devm_clk_get(&pdev->dev, "mm");
-	if (IS_ERR(clk[MT8173_CLK_MM]))
-		return PTR_ERR(clk[MT8173_CLK_MM]);
-
-	clk[MT8173_CLK_MFG] = devm_clk_get(&pdev->dev, "mfg");
-	if (IS_ERR(clk[MT8173_CLK_MFG]))
-		return PTR_ERR(clk[MT8173_CLK_MFG]);
-
-	clk[MT8173_CLK_VENC] = devm_clk_get(&pdev->dev, "venc");
-	if (IS_ERR(clk[MT8173_CLK_VENC]))
-		return PTR_ERR(clk[MT8173_CLK_VENC]);
-
-	clk[MT8173_CLK_VENC_LT] = devm_clk_get(&pdev->dev, "venc_lt");
-	if (IS_ERR(clk[MT8173_CLK_VENC_LT]))
-		return PTR_ERR(clk[MT8173_CLK_VENC_LT]);
+		return ERR_CAST(scp->base);

  	scp->infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
  			"infracfg");
  	if (IS_ERR(scp->infracfg)) {
  		dev_err(&pdev->dev, "Cannot find infracfg controller: %ld\n",
  				PTR_ERR(scp->infracfg));
-		return PTR_ERR(scp->infracfg);
+		return ERR_CAST(scp->infracfg);
  	}

-	for (i = 0; i < NUM_DOMAINS; i++) {
+	scp->domains = devm_kzalloc(&pdev->dev,
+				sizeof(*scp->domains) * num, GFP_KERNEL);
+	if (!scp->domains)
+		return ERR_PTR(-ENOMEM);
+
+	pd_data = &scp->pd_data;
+
+	pd_data->domains = devm_kzalloc(&pdev->dev,
+			sizeof(*pd_data->domains) * num, GFP_KERNEL);
+	if (!pd_data->domains)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < num; i++) {
  		struct scp_domain *scpd = &scp->domains[i];
  		const struct scp_domain_data *data = &scp_domain_data[i];

@@ -471,17 +323,31 @@ static int __init scpsys_probe(struct platform_device *pdev)
  			if (PTR_ERR(scpd->supply) == -ENODEV)
  				scpd->supply = NULL;
  			else
-				return PTR_ERR(scpd->supply);
+				return ERR_CAST(scpd->supply);
  		}
  	}

-	pd_data->num_domains = NUM_DOMAINS;
+	pd_data->num_domains = num;

-	for (i = 0; i < NUM_DOMAINS; i++) {
+	init_clks(pdev, clk);
+
+	for (i = 0; i < num; i++) {
  		struct scp_domain *scpd = &scp->domains[i];
  		struct generic_pm_domain *genpd = &scpd->genpd;
  		const struct scp_domain_data *data = &scp_domain_data[i];

+		for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++) {
+			struct clk *c = clk[data->clk_id[j]];
+
+			if (IS_ERR(c)) {
+				dev_err(&pdev->dev, "%s: clk unavailable\n",
+					data->name);
+				return ERR_CAST(c);
+			}
+
+			scpd->clk[j] = c;
+		}
+
  		pd_data->domains[i] = genpd;
  		scpd->scp = scp;

@@ -491,13 +357,25 @@ static int __init scpsys_probe(struct platform_device *pdev)
  		scpd->sram_pdn_ack_bits = data->sram_pdn_ack_bits;
  		scpd->bus_prot_mask = data->bus_prot_mask;
  		scpd->active_wakeup = data->active_wakeup;
-		for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++)
-			scpd->clk[j] = clk[data->clk_id[j]];

  		genpd->name = data->name;
  		genpd->power_off = scpsys_power_off;
  		genpd->power_on = scpsys_power_on;
  		genpd->dev_ops.active_wakeup = scpsys_active_wakeup;
+	}
+
+	return scp;
+}
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num)
+{
+	struct genpd_onecell_data *pd_data;
+	int i, ret;
+
+	for (i = 0; i < num; i++) {
+		struct scp_domain *scpd = &scp->domains[i];
+		struct generic_pm_domain *genpd = &scpd->genpd;

  		/*
  		 * Initially turn on all domains to make the domains usable
@@ -516,6 +394,125 @@ static int __init scpsys_probe(struct platform_device *pdev)
  	 * valid.
  	 */

+	pd_data = &scp->pd_data;
+
+	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to add OF provider: %d\n", ret);
+}
+
+/*
+ * MT8173 power domain support
+ */
+
+#include <dt-bindings/power/mt8173-power.h>
Please put the includes at the beginning of the file.
Same for mt2701 of course.

Thanks,
Matthias

quoted hunk
+
+static const struct scp_domain_data scp_domain_data_mt8173[] __initconst = {
+	[MT8173_POWER_DOMAIN_VDEC] = {
+		.name = "vdec",
+		.sta_mask = BIT(7),
+		.ctl_offs = 0x0210,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MM},
+	},
+	[MT8173_POWER_DOMAIN_VENC] = {
+		.name = "venc",
+		.sta_mask = BIT(21),
+		.ctl_offs = 0x0230,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_MM, CLK_VENC},
+	},
+	[MT8173_POWER_DOMAIN_ISP] = {
+		.name = "isp",
+		.sta_mask = BIT(5),
+		.ctl_offs = 0x0238,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(13, 12),
+		.clk_id = {CLK_MM},
+	},
+	[MT8173_POWER_DOMAIN_MM] = {
+		.name = "mm",
+		.sta_mask = BIT(3),
+		.ctl_offs = 0x023c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MM},
+		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
+			MT8173_TOP_AXI_PROT_EN_MM_M1,
+	},
+	[MT8173_POWER_DOMAIN_VENC_LT] = {
+		.name = "venc_lt",
+		.sta_mask = BIT(20),
+		.ctl_offs = 0x0298,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_MM, CLK_VENC_LT},
+	},
+	[MT8173_POWER_DOMAIN_AUDIO] = {
+		.name = "audio",
+		.sta_mask = BIT(24),
+		.ctl_offs = 0x029c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_NONE},
+	},
+	[MT8173_POWER_DOMAIN_USB] = {
+		.name = "usb",
+		.sta_mask = BIT(25),
+		.ctl_offs = 0x02cc,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_NONE},
+		.active_wakeup = true,
+	},
+	[MT8173_POWER_DOMAIN_MFG_ASYNC] = {
+		.name = "mfg_async",
+		.sta_mask = BIT(23),
+		.ctl_offs = 0x02c4,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = 0,
+		.clk_id = {CLK_MFG},
+	},
+	[MT8173_POWER_DOMAIN_MFG_2D] = {
+		.name = "mfg_2d",
+		.sta_mask = BIT(22),
+		.ctl_offs = 0x02c0,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(13, 12),
+		.clk_id = {CLK_NONE},
+	},
+	[MT8173_POWER_DOMAIN_MFG] = {
+		.name = "mfg",
+		.sta_mask = BIT(4),
+		.ctl_offs = 0x0214,
+		.sram_pdn_bits = GENMASK(13, 8),
+		.sram_pdn_ack_bits = GENMASK(21, 16),
+		.clk_id = {CLK_NONE},
+		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
+			MT8173_TOP_AXI_PROT_EN_MFG_M0 |
+			MT8173_TOP_AXI_PROT_EN_MFG_M1 |
+			MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
+	},
+};
+
+#define NUM_DOMAINS_MT8173	ARRAY_SIZE(scp_domain_data_mt8173)
+
+static int __init scpsys_probe_mt8173(struct platform_device *pdev)
+{
+	struct scp *scp;
+	struct genpd_onecell_data *pd_data;
+	int ret;
+
+	scp = init_scp(pdev, scp_domain_data_mt8173, NUM_DOMAINS_MT8173);
+	if (IS_ERR(scp))
+		return PTR_ERR(scp);
+
+	mtk_register_power_domains(pdev, scp, NUM_DOMAINS_MT8173);
+
+	pd_data = &scp->pd_data;
+
  	ret = pm_genpd_add_subdomain(pd_data->domains[MT8173_POWER_DOMAIN_MFG_ASYNC],
  		pd_data->domains[MT8173_POWER_DOMAIN_MFG_2D]);
  	if (ret && IS_ENABLED(CONFIG_PM))
@@ -526,21 +523,36 @@ static int __init scpsys_probe(struct platform_device *pdev)
  	if (ret && IS_ENABLED(CONFIG_PM))
  		dev_err(&pdev->dev, "Failed to add subdomain: %d\n", ret);

-	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data);
-	if (ret)
-		dev_err(&pdev->dev, "Failed to add OF provider: %d\n", ret);
-
  	return 0;
  }

+/*
+ * scpsys driver init
+ */
+
  static const struct of_device_id of_scpsys_match_tbl[] = {
  	{
  		.compatible = "mediatek,mt8173-scpsys",
+		.data = scpsys_probe_mt8173,
  	}, {
  		/* sentinel */
  	}
  };

+static int __init scpsys_probe(struct platform_device *pdev)
+{
+	int (*probe)(struct platform_device *);
+	const struct of_device_id *of_id;
+
+	of_id = of_match_node(of_scpsys_match_tbl, pdev->dev.of_node);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+
+	probe = of_id->data;
+
+	return probe(pdev);
+}
+
  static struct platform_driver scpsys_drv = {
  	.driver = {
  		.name = "mtk-scpsys",
diff --git a/drivers/soc/mediatek/mtk-scpsys.h b/drivers/soc/mediatek/mtk-scpsys.h
new file mode 100644
index 0000000..e435bc3
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys.h
@@ -0,0 +1,55 @@
+#ifndef __DRV_SOC_MTK_H
+#define __DRV_SOC_MTK_H
+
+enum clk_id {
+	CLK_NONE,
+	CLK_MM,
+	CLK_MFG,
+	CLK_VENC,
+	CLK_VENC_LT,
+	CLK_MAX,
+};
+
+#define MAX_CLKS	2
+
+struct scp_domain_data {
+	const char *name;
+	u32 sta_mask;
+	int ctl_offs;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	enum clk_id clk_id[MAX_CLKS];
+	bool active_wakeup;
+};
+
+struct scp;
+
+struct scp_domain {
+	struct generic_pm_domain genpd;
+	struct scp *scp;
+	struct clk *clk[MAX_CLKS];
+	u32 sta_mask;
+	void __iomem *ctl_addr;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	bool active_wakeup;
+	struct regulator *supply;
+};
+
+struct scp {
+	struct scp_domain *domains;
+	struct genpd_onecell_data pd_data;
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *infracfg;
+};
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num);
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num);
+
+#endif /* __DRV_SOC_MTK_H */





  

    
Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

    

      From: James Liao <hidden>

      Date: 2016-02-02 06:56:58 
    

  

  
Hi Matthias,

On Sun, 2016-01-31 at 12:51 +0100, Matthias Brugger wrote:
On 20/01/16 07:08, James Liao wrote:
quoted
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao <redacted>
---
  drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
  drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
  2 files changed, 270 insertions(+), 203 deletions(-)
  create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
In general this approach looks fine to me, comments below.

quoted
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0221387..339adfc 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,29 +11,17 @@
   * GNU General Public License for more details.
   */
  #include <linux/clk.h>
-#include <linux/delay.h>
+#include <linux/init.h>
  #include <linux/io.h>
-#include <linux/kernel.h>
  #include <linux/mfd/syscon.h>
When at it, do we need this include?
syscon_regmap_lookup_by_phandle() is declared in this head file.

quoted
-#include <linux/init.h>
  #include <linux/of_device.h>
  #include <linux/platform_device.h>
  #include <linux/pm_domain.h>
-#include <linux/regmap.h>
-#include <linux/soc/mediatek/infracfg.h>
  #include <linux/regulator/consumer.h>
-#include <dt-bindings/power/mt8173-power.h>
+#include <linux/soc/mediatek/infracfg.h>
+
+#include "mtk-scpsys.h"

-#define SPM_VDE_PWR_CON			0x0210
-#define SPM_MFG_PWR_CON			0x0214
-#define SPM_VEN_PWR_CON			0x0230
-#define SPM_ISP_PWR_CON			0x0238
-#define SPM_DIS_PWR_CON			0x023c
-#define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
-#define SPM_MFG_2D_PWR_CON		0x02c0
-#define SPM_MFG_ASYNC_PWR_CON		0x02c4
-#define SPM_USB_PWR_CON			0x02cc
I would prefer to keep this defines and declare SoC specific ones where 
necessary. It makes the code more readable.
Some register address may be reused by other modules among SoCs, so it's
not easy to maintain the defines when we implement multiple SoC drivers
in the same file. For example, offset 0x0298 is VEN2_PWR_CON on MT8173,
but it is MJC_PWR_CON on other chips.

Furthermore, these register offsets are only used in scp_domain_data[],
and each element has its own power domain name. So I think it's enough
to know which power domain are using these registers and status bits.

quoted
  #define SPM_PWR_STATUS			0x060c
  #define SPM_PWR_STATUS_2ND		0x0610

@@ -43,154 +31,6 @@
  #define PWR_ON_2ND_BIT			BIT(3)
  #define PWR_CLK_DIS_BIT			BIT(4)

-#define PWR_STATUS_DISP			BIT(3)
-#define PWR_STATUS_MFG			BIT(4)
-#define PWR_STATUS_ISP			BIT(5)
-#define PWR_STATUS_VDEC			BIT(7)
-#define PWR_STATUS_VENC_LT		BIT(20)
-#define PWR_STATUS_VENC			BIT(21)
-#define PWR_STATUS_MFG_2D		BIT(22)
-#define PWR_STATUS_MFG_ASYNC		BIT(23)
-#define PWR_STATUS_AUDIO		BIT(24)
-#define PWR_STATUS_USB			BIT(25)
-
Same here.

quoted
-enum clk_id {
-	MT8173_CLK_NONE,
-	MT8173_CLK_MM,
-	MT8173_CLK_MFG,
-	MT8173_CLK_VENC,
-	MT8173_CLK_VENC_LT,
-	MT8173_CLK_MAX,
-};
-
-#define MAX_CLKS	2
-
-struct scp_domain_data {
-	const char *name;
-	u32 sta_mask;
-	int ctl_offs;
-	u32 sram_pdn_bits;
-	u32 sram_pdn_ack_bits;
-	u32 bus_prot_mask;
-	enum clk_id clk_id[MAX_CLKS];
-	bool active_wakeup;
-};
-
-static const struct scp_domain_data scp_domain_data[] __initconst = {
-	[MT8173_POWER_DOMAIN_VDEC] = {
-		.name = "vdec",
-		.sta_mask = PWR_STATUS_VDEC,
-		.ctl_offs = SPM_VDE_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(12, 12),
-		.clk_id = {MT8173_CLK_MM},
-	},
-	[MT8173_POWER_DOMAIN_VENC] = {
-		.name = "venc",
-		.sta_mask = PWR_STATUS_VENC,
-		.ctl_offs = SPM_VEN_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_MM, MT8173_CLK_VENC},
-	},
-	[MT8173_POWER_DOMAIN_ISP] = {
-		.name = "isp",
-		.sta_mask = PWR_STATUS_ISP,
-		.ctl_offs = SPM_ISP_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(13, 12),
-		.clk_id = {MT8173_CLK_MM},
-	},
-	[MT8173_POWER_DOMAIN_MM] = {
-		.name = "mm",
-		.sta_mask = PWR_STATUS_DISP,
-		.ctl_offs = SPM_DIS_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(12, 12),
-		.clk_id = {MT8173_CLK_MM},
-		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
-			MT8173_TOP_AXI_PROT_EN_MM_M1,
-	},
-	[MT8173_POWER_DOMAIN_VENC_LT] = {
-		.name = "venc_lt",
-		.sta_mask = PWR_STATUS_VENC_LT,
-		.ctl_offs = SPM_VEN2_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_MM, MT8173_CLK_VENC_LT},
-	},
-	[MT8173_POWER_DOMAIN_AUDIO] = {
-		.name = "audio",
-		.sta_mask = PWR_STATUS_AUDIO,
-		.ctl_offs = SPM_AUDIO_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_NONE},
-	},
-	[MT8173_POWER_DOMAIN_USB] = {
-		.name = "usb",
-		.sta_mask = PWR_STATUS_USB,
-		.ctl_offs = SPM_USB_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(15, 12),
-		.clk_id = {MT8173_CLK_NONE},
-		.active_wakeup = true,
-	},
-	[MT8173_POWER_DOMAIN_MFG_ASYNC] = {
-		.name = "mfg_async",
-		.sta_mask = PWR_STATUS_MFG_ASYNC,
-		.ctl_offs = SPM_MFG_ASYNC_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = 0,
-		.clk_id = {MT8173_CLK_MFG},
-	},
-	[MT8173_POWER_DOMAIN_MFG_2D] = {
-		.name = "mfg_2d",
-		.sta_mask = PWR_STATUS_MFG_2D,
-		.ctl_offs = SPM_MFG_2D_PWR_CON,
-		.sram_pdn_bits = GENMASK(11, 8),
-		.sram_pdn_ack_bits = GENMASK(13, 12),
-		.clk_id = {MT8173_CLK_NONE},
-	},
-	[MT8173_POWER_DOMAIN_MFG] = {
-		.name = "mfg",
-		.sta_mask = PWR_STATUS_MFG,
-		.ctl_offs = SPM_MFG_PWR_CON,
-		.sram_pdn_bits = GENMASK(13, 8),
-		.sram_pdn_ack_bits = GENMASK(21, 16),
-		.clk_id = {MT8173_CLK_NONE},
-		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
-			MT8173_TOP_AXI_PROT_EN_MFG_M0 |
-			MT8173_TOP_AXI_PROT_EN_MFG_M1 |
-			MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
-	},
-};
-
-#define NUM_DOMAINS	ARRAY_SIZE(scp_domain_data)
-
-struct scp;
-
-struct scp_domain {
-	struct generic_pm_domain genpd;
-	struct scp *scp;
-	struct clk *clk[MAX_CLKS];
-	u32 sta_mask;
-	void __iomem *ctl_addr;
-	u32 sram_pdn_bits;
-	u32 sram_pdn_ack_bits;
-	u32 bus_prot_mask;
-	bool active_wakeup;
-	struct regulator *supply;
-};
-
-struct scp {
-	struct scp_domain domains[NUM_DOMAINS];
-	struct genpd_onecell_data pd_data;
-	struct device *dev;
-	void __iomem *base;
-	struct regmap *infracfg;
-};
-
  static int scpsys_domain_is_on(struct scp_domain *scpd)
  {
  	struct scp *scp = scpd->scp;
@@ -412,57 +252,69 @@ static bool scpsys_active_wakeup(struct device *dev)
  	return scpd->active_wakeup;
  }

-static int __init scpsys_probe(struct platform_device *pdev)
+static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
+{
+	enum clk_id clk_ids[] = {
+		CLK_MM,
+		CLK_MFG,
+		CLK_VENC,
+		CLK_VENC_LT
+	};
+
+	static const char * const clk_names[] = {
+		"mm",
+		"mfg",
+		"venc",
+		"venc_lt",
+	};
+
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(clk_ids); i++)
+		clk[clk_ids[i]] = devm_clk_get(&pdev->dev, clk_names[i]);
+}
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num)
  {
  	struct genpd_onecell_data *pd_data;
  	struct resource *res;
-	int i, j, ret;
+	int i, j;
  	struct scp *scp;
-	struct clk *clk[MT8173_CLK_MAX];
+	struct clk *clk[CLK_MAX];

  	scp = devm_kzalloc(&pdev->dev, sizeof(*scp), GFP_KERNEL);
  	if (!scp)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);

  	scp->dev = &pdev->dev;

  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	scp->base = devm_ioremap_resource(&pdev->dev, res);
  	if (IS_ERR(scp->base))
-		return PTR_ERR(scp->base);
-
-	pd_data = &scp->pd_data;
-
-	pd_data->domains = devm_kzalloc(&pdev->dev,
-			sizeof(*pd_data->domains) * NUM_DOMAINS, GFP_KERNEL);
-	if (!pd_data->domains)
-		return -ENOMEM;
-
-	clk[MT8173_CLK_MM] = devm_clk_get(&pdev->dev, "mm");
-	if (IS_ERR(clk[MT8173_CLK_MM]))
-		return PTR_ERR(clk[MT8173_CLK_MM]);
-
-	clk[MT8173_CLK_MFG] = devm_clk_get(&pdev->dev, "mfg");
-	if (IS_ERR(clk[MT8173_CLK_MFG]))
-		return PTR_ERR(clk[MT8173_CLK_MFG]);
-
-	clk[MT8173_CLK_VENC] = devm_clk_get(&pdev->dev, "venc");
-	if (IS_ERR(clk[MT8173_CLK_VENC]))
-		return PTR_ERR(clk[MT8173_CLK_VENC]);
-
-	clk[MT8173_CLK_VENC_LT] = devm_clk_get(&pdev->dev, "venc_lt");
-	if (IS_ERR(clk[MT8173_CLK_VENC_LT]))
-		return PTR_ERR(clk[MT8173_CLK_VENC_LT]);
+		return ERR_CAST(scp->base);

  	scp->infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
  			"infracfg");
  	if (IS_ERR(scp->infracfg)) {
  		dev_err(&pdev->dev, "Cannot find infracfg controller: %ld\n",
  				PTR_ERR(scp->infracfg));
-		return PTR_ERR(scp->infracfg);
+		return ERR_CAST(scp->infracfg);
  	}

-	for (i = 0; i < NUM_DOMAINS; i++) {
+	scp->domains = devm_kzalloc(&pdev->dev,
+				sizeof(*scp->domains) * num, GFP_KERNEL);
+	if (!scp->domains)
+		return ERR_PTR(-ENOMEM);
+
+	pd_data = &scp->pd_data;
+
+	pd_data->domains = devm_kzalloc(&pdev->dev,
+			sizeof(*pd_data->domains) * num, GFP_KERNEL);
+	if (!pd_data->domains)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < num; i++) {
  		struct scp_domain *scpd = &scp->domains[i];
  		const struct scp_domain_data *data = &scp_domain_data[i];

@@ -471,17 +323,31 @@ static int __init scpsys_probe(struct platform_device *pdev)
  			if (PTR_ERR(scpd->supply) == -ENODEV)
  				scpd->supply = NULL;
  			else
-				return PTR_ERR(scpd->supply);
+				return ERR_CAST(scpd->supply);
  		}
  	}

-	pd_data->num_domains = NUM_DOMAINS;
+	pd_data->num_domains = num;

-	for (i = 0; i < NUM_DOMAINS; i++) {
+	init_clks(pdev, clk);
+
+	for (i = 0; i < num; i++) {
  		struct scp_domain *scpd = &scp->domains[i];
  		struct generic_pm_domain *genpd = &scpd->genpd;
  		const struct scp_domain_data *data = &scp_domain_data[i];

+		for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++) {
+			struct clk *c = clk[data->clk_id[j]];
+
+			if (IS_ERR(c)) {
+				dev_err(&pdev->dev, "%s: clk unavailable\n",
+					data->name);
+				return ERR_CAST(c);
+			}
+
+			scpd->clk[j] = c;
+		}
+
  		pd_data->domains[i] = genpd;
  		scpd->scp = scp;

@@ -491,13 +357,25 @@ static int __init scpsys_probe(struct platform_device *pdev)
  		scpd->sram_pdn_ack_bits = data->sram_pdn_ack_bits;
  		scpd->bus_prot_mask = data->bus_prot_mask;
  		scpd->active_wakeup = data->active_wakeup;
-		for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++)
-			scpd->clk[j] = clk[data->clk_id[j]];

  		genpd->name = data->name;
  		genpd->power_off = scpsys_power_off;
  		genpd->power_on = scpsys_power_on;
  		genpd->dev_ops.active_wakeup = scpsys_active_wakeup;
+	}
+
+	return scp;
+}
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num)
+{
+	struct genpd_onecell_data *pd_data;
+	int i, ret;
+
+	for (i = 0; i < num; i++) {
+		struct scp_domain *scpd = &scp->domains[i];
+		struct generic_pm_domain *genpd = &scpd->genpd;

  		/*
  		 * Initially turn on all domains to make the domains usable
@@ -516,6 +394,125 @@ static int __init scpsys_probe(struct platform_device *pdev)
  	 * valid.
  	 */

+	pd_data = &scp->pd_data;
+
+	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to add OF provider: %d\n", ret);
+}
+
+/*
+ * MT8173 power domain support
+ */
+
+#include <dt-bindings/power/mt8173-power.h>
Please put the includes at the beginning of the file.
Same for mt2701 of course.
OK, I'll change it in next patch.


Best regards,

James

quoted
+
+static const struct scp_domain_data scp_domain_data_mt8173[] __initconst = {
+	[MT8173_POWER_DOMAIN_VDEC] = {
+		.name = "vdec",
+		.sta_mask = BIT(7),
+		.ctl_offs = 0x0210,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MM},
+	},
+	[MT8173_POWER_DOMAIN_VENC] = {
+		.name = "venc",
+		.sta_mask = BIT(21),
+		.ctl_offs = 0x0230,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_MM, CLK_VENC},
+	},
+	[MT8173_POWER_DOMAIN_ISP] = {
+		.name = "isp",
+		.sta_mask = BIT(5),
+		.ctl_offs = 0x0238,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(13, 12),
+		.clk_id = {CLK_MM},
+	},
+	[MT8173_POWER_DOMAIN_MM] = {
+		.name = "mm",
+		.sta_mask = BIT(3),
+		.ctl_offs = 0x023c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MM},
+		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
+			MT8173_TOP_AXI_PROT_EN_MM_M1,
+	},
+	[MT8173_POWER_DOMAIN_VENC_LT] = {
+		.name = "venc_lt",
+		.sta_mask = BIT(20),
+		.ctl_offs = 0x0298,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_MM, CLK_VENC_LT},
+	},
+	[MT8173_POWER_DOMAIN_AUDIO] = {
+		.name = "audio",
+		.sta_mask = BIT(24),
+		.ctl_offs = 0x029c,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_NONE},
+	},
+	[MT8173_POWER_DOMAIN_USB] = {
+		.name = "usb",
+		.sta_mask = BIT(25),
+		.ctl_offs = 0x02cc,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_NONE},
+		.active_wakeup = true,
+	},
+	[MT8173_POWER_DOMAIN_MFG_ASYNC] = {
+		.name = "mfg_async",
+		.sta_mask = BIT(23),
+		.ctl_offs = 0x02c4,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = 0,
+		.clk_id = {CLK_MFG},
+	},
+	[MT8173_POWER_DOMAIN_MFG_2D] = {
+		.name = "mfg_2d",
+		.sta_mask = BIT(22),
+		.ctl_offs = 0x02c0,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(13, 12),
+		.clk_id = {CLK_NONE},
+	},
+	[MT8173_POWER_DOMAIN_MFG] = {
+		.name = "mfg",
+		.sta_mask = BIT(4),
+		.ctl_offs = 0x0214,
+		.sram_pdn_bits = GENMASK(13, 8),
+		.sram_pdn_ack_bits = GENMASK(21, 16),
+		.clk_id = {CLK_NONE},
+		.bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
+			MT8173_TOP_AXI_PROT_EN_MFG_M0 |
+			MT8173_TOP_AXI_PROT_EN_MFG_M1 |
+			MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
+	},
+};
+
+#define NUM_DOMAINS_MT8173	ARRAY_SIZE(scp_domain_data_mt8173)
+
+static int __init scpsys_probe_mt8173(struct platform_device *pdev)
+{
+	struct scp *scp;
+	struct genpd_onecell_data *pd_data;
+	int ret;
+
+	scp = init_scp(pdev, scp_domain_data_mt8173, NUM_DOMAINS_MT8173);
+	if (IS_ERR(scp))
+		return PTR_ERR(scp);
+
+	mtk_register_power_domains(pdev, scp, NUM_DOMAINS_MT8173);
+
+	pd_data = &scp->pd_data;
+
  	ret = pm_genpd_add_subdomain(pd_data->domains[MT8173_POWER_DOMAIN_MFG_ASYNC],
  		pd_data->domains[MT8173_POWER_DOMAIN_MFG_2D]);
  	if (ret && IS_ENABLED(CONFIG_PM))
@@ -526,21 +523,36 @@ static int __init scpsys_probe(struct platform_device *pdev)
  	if (ret && IS_ENABLED(CONFIG_PM))
  		dev_err(&pdev->dev, "Failed to add subdomain: %d\n", ret);

-	ret = of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data);
-	if (ret)
-		dev_err(&pdev->dev, "Failed to add OF provider: %d\n", ret);
-
  	return 0;
  }

+/*
+ * scpsys driver init
+ */
+
  static const struct of_device_id of_scpsys_match_tbl[] = {
  	{
  		.compatible = "mediatek,mt8173-scpsys",
+		.data = scpsys_probe_mt8173,
  	}, {
  		/* sentinel */
  	}
  };

+static int __init scpsys_probe(struct platform_device *pdev)
+{
+	int (*probe)(struct platform_device *);
+	const struct of_device_id *of_id;
+
+	of_id = of_match_node(of_scpsys_match_tbl, pdev->dev.of_node);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+
+	probe = of_id->data;
+
+	return probe(pdev);
+}
+
  static struct platform_driver scpsys_drv = {
  	.driver = {
  		.name = "mtk-scpsys",
diff --git a/drivers/soc/mediatek/mtk-scpsys.h b/drivers/soc/mediatek/mtk-scpsys.h
new file mode 100644
index 0000000..e435bc3
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys.h
@@ -0,0 +1,55 @@
+#ifndef __DRV_SOC_MTK_H
+#define __DRV_SOC_MTK_H
+
+enum clk_id {
+	CLK_NONE,
+	CLK_MM,
+	CLK_MFG,
+	CLK_VENC,
+	CLK_VENC_LT,
+	CLK_MAX,
+};
+
+#define MAX_CLKS	2
+
+struct scp_domain_data {
+	const char *name;
+	u32 sta_mask;
+	int ctl_offs;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	enum clk_id clk_id[MAX_CLKS];
+	bool active_wakeup;
+};
+
+struct scp;
+
+struct scp_domain {
+	struct generic_pm_domain genpd;
+	struct scp *scp;
+	struct clk *clk[MAX_CLKS];
+	u32 sta_mask;
+	void __iomem *ctl_addr;
+	u32 sram_pdn_bits;
+	u32 sram_pdn_ack_bits;
+	u32 bus_prot_mask;
+	bool active_wakeup;
+	struct regulator *supply;
+};
+
+struct scp {
+	struct scp_domain *domains;
+	struct genpd_onecell_data pd_data;
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *infracfg;
+};
+
+struct scp *init_scp(struct platform_device *pdev,
+			const struct scp_domain_data *scp_domain_data, int num);
+
+void mtk_register_power_domains(struct platform_device *pdev,
+				struct scp *scp, int num);
+
+#endif /* __DRV_SOC_MTK_H */





  

    
Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

    

      From: Matthias Brugger <matthias.bgg@gmail.com>

      Date: 2016-02-02 10:44:48 
    

  

  

On 02/02/16 07:56, James Liao wrote:
Hi Matthias,

On Sun, 2016-01-31 at 12:51 +0100, Matthias Brugger wrote:
quoted
quoted
On 20/01/16 07:08, James Liao wrote:
quoted
quoted
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao<redacted>
---
  drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
  drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
  2 files changed, 270 insertions(+), 203 deletions(-)
  create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
In general this approach looks fine to me, comments below.

quoted
quoted
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0221387..339adfc 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,29 +11,17 @@
   * GNU General Public License for more details.
   */
  #include <linux/clk.h>
-#include <linux/delay.h>
+#include <linux/init.h>
  #include <linux/io.h>
-#include <linux/kernel.h>
  #include <linux/mfd/syscon.h>
When at it, do we need this include?
syscon_regmap_lookup_by_phandle() is declared in this head file.

quoted
quoted
quoted
quoted
-#include <linux/init.h>
  #include <linux/of_device.h>
  #include <linux/platform_device.h>
  #include <linux/pm_domain.h>
-#include <linux/regmap.h>
-#include <linux/soc/mediatek/infracfg.h>
  #include <linux/regulator/consumer.h>
-#include <dt-bindings/power/mt8173-power.h>
+#include <linux/soc/mediatek/infracfg.h>
+
+#include "mtk-scpsys.h"

-#define SPM_VDE_PWR_CON			0x0210
-#define SPM_MFG_PWR_CON			0x0214
-#define SPM_VEN_PWR_CON			0x0230
-#define SPM_ISP_PWR_CON			0x0238
-#define SPM_DIS_PWR_CON			0x023c
-#define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
-#define SPM_MFG_2D_PWR_CON		0x02c0
-#define SPM_MFG_ASYNC_PWR_CON		0x02c4
-#define SPM_USB_PWR_CON			0x02cc
I would prefer to keep this defines and declare SoC specific ones where
necessary. It makes the code more readable.
Some register address may be reused by other modules among SoCs, so it's
not easy to maintain the defines when we implement multiple SoC drivers
in the same file. For example, offset 0x0298 is VEN2_PWR_CON on MT8173,
but it is MJC_PWR_CON on other chips.
So that sounds as if 0x0298 offset is MT8173 specific.
I checked [VDE, MFG, VEN, IFR, ISP, DIS, DPY]_PWR_CON on mt8173, mt8135 
and mt6589 and they all have the same offset. So it doesn't seem as if 
the offset randomly changes for every SoC.

Furthermore, these register offsets are only used in scp_domain_data[],
and each element has its own power domain name. So I think it's enough
to know which power domain are using these registers and status bits.
Yes that's true, but it will make it easier for another person to 
understand the driver, especially if he want's to implement the driver 
for a new SoC.

Regards,
Matthias





  

    
Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

    

      From: James Liao <hidden>

      Date: 2016-02-03 05:22:56 
    

  

  
Hi Matthias,

On Tue, 2016-02-02 at 11:44 +0100, Matthias Brugger wrote:
On 02/02/16 07:56, James Liao wrote:
quoted
On Sun, 2016-01-31 at 12:51 +0100, Matthias Brugger wrote:
quoted
quoted
On 20/01/16 07:08, James Liao wrote:
quoted
quoted
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao<redacted>
---
  drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
  drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
  2 files changed, 270 insertions(+), 203 deletions(-)
  create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
In general this approach looks fine to me, comments below.

quoted
quoted
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0221387..339adfc 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,29 +11,17 @@
   * GNU General Public License for more details.
   */
  #include <linux/clk.h>
-#include <linux/delay.h>
+#include <linux/init.h>
  #include <linux/io.h>
-#include <linux/kernel.h>
  #include <linux/mfd/syscon.h>
When at it, do we need this include?
syscon_regmap_lookup_by_phandle() is declared in this head file.

quoted
quoted
quoted
quoted
-#include <linux/init.h>
  #include <linux/of_device.h>
  #include <linux/platform_device.h>
  #include <linux/pm_domain.h>
-#include <linux/regmap.h>
-#include <linux/soc/mediatek/infracfg.h>
  #include <linux/regulator/consumer.h>
-#include <dt-bindings/power/mt8173-power.h>
+#include <linux/soc/mediatek/infracfg.h>
+
+#include "mtk-scpsys.h"

-#define SPM_VDE_PWR_CON			0x0210
-#define SPM_MFG_PWR_CON			0x0214
-#define SPM_VEN_PWR_CON			0x0230
-#define SPM_ISP_PWR_CON			0x0238
-#define SPM_DIS_PWR_CON			0x023c
-#define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
-#define SPM_MFG_2D_PWR_CON		0x02c0
-#define SPM_MFG_ASYNC_PWR_CON		0x02c4
-#define SPM_USB_PWR_CON			0x02cc
I would prefer to keep this defines and declare SoC specific ones where
necessary. It makes the code more readable.
Some register address may be reused by other modules among SoCs, so it's
not easy to maintain the defines when we implement multiple SoC drivers
in the same file. For example, offset 0x0298 is VEN2_PWR_CON on MT8173,
but it is MJC_PWR_CON on other chips.
So that sounds as if 0x0298 offset is MT8173 specific.
I checked [VDE, MFG, VEN, IFR, ISP, DIS, DPY]_PWR_CON on mt8173, mt8135 
and mt6589 and they all have the same offset. So it doesn't seem as if 
the offset randomly changes for every SoC.

quoted
Furthermore, these register offsets are only used in scp_domain_data[],
and each element has its own power domain name. So I think it's enough
to know which power domain are using these registers and status bits.
Yes that's true, but it will make it easier for another person to 
understand the driver, especially if he want's to implement the driver 
for a new SoC.
There are two kinds of conflicts may happen:

1. Different modules use the same register address.
2. Different register addresses are used by the same module (on
different IC).

Type 1. for example:

	#define SPM_BDP_PWR_CON			0x029c /* 2701 */
	#define SPM_AUDIO_PWR_CON		0x029c /* 8173 */

We can resolve this conflict easily, such as define these two register
name to the same register address.

Type 2. for example:

	#define SPM_VDE_PWR_CON			0x0300 /* 6755 */
	#define SPM_VDE_PWR_CON			0x0210 /* 8173 */

We can not reuse the register defines in this case. We may need to name
the registers with its IC name, such as MT8173_SPM_VDE_PWR_CON and
MT6755_VDE_PWR_CON. But it will increase the maintain effort. That's why
I prefer to remove register defines if we implement multiple SoC's
scpsys in a single file.


Best regards,

James





  

    
Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

    

      From: Matthias Brugger <matthias.bgg@gmail.com>

      Date: 2016-02-03 09:00:23 
    

  

  

On 03/02/16 06:22, James Liao wrote:
Hi Matthias,

On Tue, 2016-02-02 at 11:44 +0100, Matthias Brugger wrote:
quoted
On 02/02/16 07:56, James Liao wrote:
quoted
On Sun, 2016-01-31 at 12:51 +0100, Matthias Brugger wrote:
quoted
quoted
On 20/01/16 07:08, James Liao wrote:
quoted
quoted
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao<redacted>
---
   drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
   drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
   2 files changed, 270 insertions(+), 203 deletions(-)
   create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
In general this approach looks fine to me, comments below.

quoted
quoted
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0221387..339adfc 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,29 +11,17 @@
    * GNU General Public License for more details.
    */
   #include <linux/clk.h>
-#include <linux/delay.h>
+#include <linux/init.h>
   #include <linux/io.h>
-#include <linux/kernel.h>
   #include <linux/mfd/syscon.h>
When at it, do we need this include?
syscon_regmap_lookup_by_phandle() is declared in this head file.

quoted
quoted
quoted
quoted
-#include <linux/init.h>
   #include <linux/of_device.h>
   #include <linux/platform_device.h>
   #include <linux/pm_domain.h>
-#include <linux/regmap.h>
-#include <linux/soc/mediatek/infracfg.h>
   #include <linux/regulator/consumer.h>
-#include <dt-bindings/power/mt8173-power.h>
+#include <linux/soc/mediatek/infracfg.h>
+
+#include "mtk-scpsys.h"

-#define SPM_VDE_PWR_CON			0x0210
-#define SPM_MFG_PWR_CON			0x0214
-#define SPM_VEN_PWR_CON			0x0230
-#define SPM_ISP_PWR_CON			0x0238
-#define SPM_DIS_PWR_CON			0x023c
-#define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
-#define SPM_MFG_2D_PWR_CON		0x02c0
-#define SPM_MFG_ASYNC_PWR_CON		0x02c4
-#define SPM_USB_PWR_CON			0x02cc
I would prefer to keep this defines and declare SoC specific ones where
necessary. It makes the code more readable.
Some register address may be reused by other modules among SoCs, so it's
not easy to maintain the defines when we implement multiple SoC drivers
in the same file. For example, offset 0x0298 is VEN2_PWR_CON on MT8173,
but it is MJC_PWR_CON on other chips.
So that sounds as if 0x0298 offset is MT8173 specific.
I checked [VDE, MFG, VEN, IFR, ISP, DIS, DPY]_PWR_CON on mt8173, mt8135
and mt6589 and they all have the same offset. So it doesn't seem as if
the offset randomly changes for every SoC.

quoted
Furthermore, these register offsets are only used in scp_domain_data[],
and each element has its own power domain name. So I think it's enough
to know which power domain are using these registers and status bits.
Yes that's true, but it will make it easier for another person to
understand the driver, especially if he want's to implement the driver
for a new SoC.
There are two kinds of conflicts may happen:

1. Different modules use the same register address.
2. Different register addresses are used by the same module (on
different IC).

Type 1. for example:

	#define SPM_BDP_PWR_CON			0x029c /* 2701 */
	#define SPM_AUDIO_PWR_CON		0x029c /* 8173 */

We can resolve this conflict easily, such as define these two register
name to the same register address.

Type 2. for example:

	#define SPM_VDE_PWR_CON			0x0300 /* 6755 */
	#define SPM_VDE_PWR_CON			0x0210 /* 8173 */

We can not reuse the register defines in this case. We may need to name
the registers with its IC name, such as MT8173_SPM_VDE_PWR_CON and
MT6755_VDE_PWR_CON. But it will increase the maintain effort. That's why
I prefer to remove register defines if we implement multiple SoC's
scpsys in a single file.

Well type 2 for me is no problem at all. As stated in my last mail, 
mt6755 would get the SoC name in the define (as a postfix preferably).
I don't think that this will make a lot of pain regarding maintaining 
it. Even less if we have the defines in alphabetic order.

I we see in the future that this converts to a mess, we always can get 
rid of the defines quite easily.

Regards,
Matthias





  

    
Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

    

      From: James Liao <hidden>

      Date: 2016-02-15 09:09:40 
    

  

  
Hi Matthias,

On Wed, 2016-02-03 at 10:00 +0100, Matthias Brugger wrote:
On 03/02/16 06:22, James Liao wrote:
quoted
On Tue, 2016-02-02 at 11:44 +0100, Matthias Brugger wrote:
quoted
On 02/02/16 07:56, James Liao wrote:
quoted
On Sun, 2016-01-31 at 12:51 +0100, Matthias Brugger wrote:
quoted
quoted
On 20/01/16 07:08, James Liao wrote:
quoted
quoted
Refine scpsys driver common code to support multiple SoC / platform.

Signed-off-by: James Liao<redacted>
---
   drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
   drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
   2 files changed, 270 insertions(+), 203 deletions(-)
   create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
In general this approach looks fine to me, comments below.

quoted
quoted
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 0221387..339adfc 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,29 +11,17 @@
    * GNU General Public License for more details.
    */
   #include <linux/clk.h>
-#include <linux/delay.h>
+#include <linux/init.h>
   #include <linux/io.h>
-#include <linux/kernel.h>
   #include <linux/mfd/syscon.h>
When at it, do we need this include?
syscon_regmap_lookup_by_phandle() is declared in this head file.

quoted
quoted
quoted
quoted
-#include <linux/init.h>
   #include <linux/of_device.h>
   #include <linux/platform_device.h>
   #include <linux/pm_domain.h>
-#include <linux/regmap.h>
-#include <linux/soc/mediatek/infracfg.h>
   #include <linux/regulator/consumer.h>
-#include <dt-bindings/power/mt8173-power.h>
+#include <linux/soc/mediatek/infracfg.h>
+
+#include "mtk-scpsys.h"

-#define SPM_VDE_PWR_CON			0x0210
-#define SPM_MFG_PWR_CON			0x0214
-#define SPM_VEN_PWR_CON			0x0230
-#define SPM_ISP_PWR_CON			0x0238
-#define SPM_DIS_PWR_CON			0x023c
-#define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
-#define SPM_MFG_2D_PWR_CON		0x02c0
-#define SPM_MFG_ASYNC_PWR_CON		0x02c4
-#define SPM_USB_PWR_CON			0x02cc
I would prefer to keep this defines and declare SoC specific ones where
necessary. It makes the code more readable.
Some register address may be reused by other modules among SoCs, so it's
not easy to maintain the defines when we implement multiple SoC drivers
in the same file. For example, offset 0x0298 is VEN2_PWR_CON on MT8173,
but it is MJC_PWR_CON on other chips.
So that sounds as if 0x0298 offset is MT8173 specific.
I checked [VDE, MFG, VEN, IFR, ISP, DIS, DPY]_PWR_CON on mt8173, mt8135
and mt6589 and they all have the same offset. So it doesn't seem as if
the offset randomly changes for every SoC.

quoted
Furthermore, these register offsets are only used in scp_domain_data[],
and each element has its own power domain name. So I think it's enough
to know which power domain are using these registers and status bits.
Yes that's true, but it will make it easier for another person to
understand the driver, especially if he want's to implement the driver
for a new SoC.
There are two kinds of conflicts may happen:

1. Different modules use the same register address.
2. Different register addresses are used by the same module (on
different IC).

Type 1. for example:

	#define SPM_BDP_PWR_CON			0x029c /* 2701 */
	#define SPM_AUDIO_PWR_CON		0x029c /* 8173 */

We can resolve this conflict easily, such as define these two register
name to the same register address.

Type 2. for example:

	#define SPM_VDE_PWR_CON			0x0300 /* 6755 */
	#define SPM_VDE_PWR_CON			0x0210 /* 8173 */

We can not reuse the register defines in this case. We may need to name
the registers with its IC name, such as MT8173_SPM_VDE_PWR_CON and
MT6755_VDE_PWR_CON. But it will increase the maintain effort. That's why
I prefer to remove register defines if we implement multiple SoC's
scpsys in a single file.

Well type 2 for me is no problem at all. As stated in my last mail, 
mt6755 would get the SoC name in the define (as a postfix preferably).
I don't think that this will make a lot of pain regarding maintaining 
it. Even less if we have the defines in alphabetic order.

I we see in the future that this converts to a mess, we always can get 
rid of the defines quite easily.
OK, I'll add register names back in next patch.


Best regards,

James




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help