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

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

[James Liao]

This series is based on v4.6-rc1 and adds scpsys power domain support
for Mediatek MT2701.

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 [1]
to see discussion details.

changes since v6:
- Minor changes in the dt-binding document.

changes since v5:
- Rebase to v4.6-rc1.
- Add dependent clocks for MFG, ISP, ETH and HIF power domains.
- Add "ethif" as a dependent clock in scpsys dt-binding document.

changes since v4:
- Rebase to v4.5-rc4.
- Remove mtk-scpsys.h and Merge its code into mtk-scpsys.c.
- Add names for every controlling registers and bits.
- Include dt-bindings headers at the beginning of mtk-scpsys.c.
- Sort compatible string in dt-binding documents.

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] 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    |  13 +-
 drivers/soc/mediatek/Kconfig                       |   2 +-
 drivers/soc/mediatek/mtk-scpsys.c                  | 497 +++++++++++++++------
 include/dt-bindings/power/mt2701-power.h           |  27 ++
 4 files changed, 388 insertions(+), 151 deletions(-)
 create mode 100644 include/dt-bindings/power/mt2701-power.h

--
1.9.1

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

[James Liao]

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

Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/soc/mediatek/mtk-scpsys.c | 363 +++++++++++++++++++++++---------------
 1 file changed, 220 insertions(+), 143 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 57e781c..5870a24 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,17 +11,15 @@
  * 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 <linux/soc/mediatek/infracfg.h>
+
 #include <dt-bindings/power/mt8173-power.h>
 
 #define SPM_VDE_PWR_CON			0x0210
@@ -34,6 +32,7 @@
 #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
 
@@ -55,12 +54,12 @@
 #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,
+	CLK_NONE,
+	CLK_MM,
+	CLK_MFG,
+	CLK_VENC,
+	CLK_VENC_LT,
+	CLK_MAX,
 };
 
 #define MAX_CLKS	2
@@ -76,98 +75,6 @@ struct scp_domain_data {
 	bool active_wakeup;
 };
 
-static const struct scp_domain_data scp_domain_data[] = {
-	[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 {
@@ -179,7 +86,7 @@ struct scp_domain {
 };
 
 struct scp {
-	struct scp_domain domains[NUM_DOMAINS];
+	struct scp_domain *domains;
 	struct genpd_onecell_data pd_data;
 	struct device *dev;
 	void __iomem *base;
@@ -408,57 +315,69 @@ static bool scpsys_active_wakeup(struct device *dev)
 	return scpd->data->active_wakeup;
 }
 
-static int 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]);
+}
+
+static 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];
 
@@ -467,28 +386,54 @@ static int 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;
 
 		scpd->data = data;
-		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;
+}
+
+static 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;
 
 		/*
 		 * With CONFIG_PM disabled turn on all domains to make the
@@ -506,6 +451,123 @@ static int 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
+ */
+
+static const struct scp_domain_data scp_domain_data_mt8173[] = {
+	[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 = {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 = {CLK_MM, 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 = {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 = {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 = {CLK_MM, 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 = {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 = {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 = {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 = {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 = {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))
@@ -516,21 +578,36 @@ static int 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 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 = {
 	.probe = scpsys_probe,
 	.driver = {
-- 
1.9.1

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

[James Liao]

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 <jamesjj.liao@mediatek.com>
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
---
 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 5870a24..00c0adb 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -617,4 +617,21 @@ static struct platform_driver scpsys_drv = {
 		.of_match_table = of_match_ptr(of_scpsys_match_tbl),
 	},
 };
-builtin_platform_driver(scpsys_drv);
+
+static int __init scpsys_drv_init(void)
+{
+	return platform_driver_register(&scpsys_drv);
+}
+
+/*
+ * 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 v7 3/4] soc: mediatek: Add MT2701 power dt-bindings

[James Liao]

From: Shunli Wang <shunli.wang@mediatek.com>

Add power dt-bindings for MT2701.

Signed-off-by: Shunli Wang <shunli.wang@mediatek.com>
Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
---
 .../devicetree/bindings/soc/mediatek/scpsys.txt    | 13 +++++++----
 include/dt-bindings/power/mt2701-power.h           | 27 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 5 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 e8f15e3..16fe94d 100644
--- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
+++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt
@@ -9,17 +9,20 @@ 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 one of:
+	- "mediatek,mt2701-scpsys"
+	- "mediatek,mt8173-scpsys"
 - #power-domain-cells: Must be 1
 - reg: Address range of the SCPSYS unit
 - infracfg: must contain a phandle to the infracfg controller
 - clock, clock-names: clocks according to the common clock binding.
-                      The clocks needed "mm", "mfg", "venc" and "venc_lt".
-		      These are the clocks which hardware needs to be enabled
-		      before enabling certain power domains.
+                      These are clocks which hardware needs to be
+                      enabled before enabling certain power domains.
+	Required clocks for MT2701: "mm", "mfg", "ethif"
+	Required clocks for MT8173: "mm", "mfg", "venc", "venc_lt"
 
 Optional properties:
 - vdec-supply: Power supply for the vdec power domain
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 v7 4/4] soc: mediatek: Add MT2701 scpsys driver

[James Liao]

From: Shunli Wang <shunli.wang@mediatek.com>

Add scpsys driver for MT2701.

mtk-scpsys now supports MT8173 (arm64) and MT2701 (arm). So it should
be enabled on both arm64 and arm platforms.

Signed-off-by: Shunli Wang <shunli.wang@mediatek.com>
Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/soc/mediatek/Kconfig      |   2 +-
 drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 116 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index 0a4ea80..609bb34 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -23,7 +23,7 @@ config MTK_PMIC_WRAP
 config MTK_SCPSYS
 	bool "MediaTek SCPSYS Support"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
-	default ARM64 && ARCH_MEDIATEK
+	default ARCH_MEDIATEK
 	select REGMAP
 	select MTK_INFRACFG
 	select PM_GENERIC_DOMAINS if PM
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 00c0adb..f4d1230 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -20,6 +20,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/soc/mediatek/infracfg.h>
 
+#include <dt-bindings/power/mt2701-power.h>
 #include <dt-bindings/power/mt8173-power.h>
 
 #define SPM_VDE_PWR_CON			0x0210
@@ -27,8 +28,13 @@
 #define SPM_VEN_PWR_CON			0x0230
 #define SPM_ISP_PWR_CON			0x0238
 #define SPM_DIS_PWR_CON			0x023c
+#define SPM_CONN_PWR_CON		0x0280
 #define SPM_VEN2_PWR_CON		0x0298
-#define SPM_AUDIO_PWR_CON		0x029c
+#define SPM_AUDIO_PWR_CON		0x029c	/* MT8173 */
+#define SPM_BDP_PWR_CON			0x029c	/* MT2701 */
+#define SPM_ETH_PWR_CON			0x02a0
+#define SPM_HIF_PWR_CON			0x02a4
+#define SPM_IFR_MSC_PWR_CON		0x02a8
 #define SPM_MFG_2D_PWR_CON		0x02c0
 #define SPM_MFG_ASYNC_PWR_CON		0x02c4
 #define SPM_USB_PWR_CON			0x02cc
@@ -42,10 +48,15 @@
 #define PWR_ON_2ND_BIT			BIT(3)
 #define PWR_CLK_DIS_BIT			BIT(4)
 
+#define PWR_STATUS_CONN			BIT(1)
 #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_BDP			BIT(14)
+#define PWR_STATUS_ETH			BIT(15)
+#define PWR_STATUS_HIF			BIT(16)
+#define PWR_STATUS_IFR_MSC		BIT(17)
 #define PWR_STATUS_VENC_LT		BIT(20)
 #define PWR_STATUS_VENC			BIT(21)
 #define PWR_STATUS_MFG_2D		BIT(22)
@@ -59,6 +70,7 @@ enum clk_id {
 	CLK_MFG,
 	CLK_VENC,
 	CLK_VENC_LT,
+	CLK_ETHIF,
 	CLK_MAX,
 };
 
@@ -321,7 +333,8 @@ static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
 		CLK_MM,
 		CLK_MFG,
 		CLK_VENC,
-		CLK_VENC_LT
+		CLK_VENC_LT,
+		CLK_ETHIF
 	};
 
 	static const char * const clk_names[] = {
@@ -329,6 +342,7 @@ static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
 		"mfg",
 		"venc",
 		"venc_lt",
+		"ethif",
 	};
 
 	int i;
@@ -459,6 +473,102 @@ static void mtk_register_power_domains(struct platform_device *pdev,
 }
 
 /*
+ * MT2701 power domain support
+ */
+
+static const struct scp_domain_data scp_domain_data_mt2701[] = {
+	[MT2701_POWER_DOMAIN_CONN] = {
+		.name = "conn",
+		.sta_mask = PWR_STATUS_CONN,
+		.ctl_offs = SPM_CONN_PWR_CON,
+		.bus_prot_mask = 0x0104,
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_DISP] = {
+		.name = "disp",
+		.sta_mask = PWR_STATUS_DISP,
+		.ctl_offs = SPM_DIS_PWR_CON,
+		.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 = PWR_STATUS_MFG,
+		.ctl_offs = SPM_MFG_PWR_CON,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(12, 12),
+		.clk_id = {CLK_MFG},
+		.active_wakeup = true,
+	},
+	[MT2701_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 = {CLK_MM},
+		.active_wakeup = true,
+	},
+	[MT2701_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 = {CLK_MM},
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_BDP] = {
+		.name = "bdp",
+		.sta_mask = PWR_STATUS_BDP,
+		.ctl_offs = SPM_BDP_PWR_CON,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_ETH] = {
+		.name = "eth",
+		.sta_mask = PWR_STATUS_ETH,
+		.ctl_offs = SPM_ETH_PWR_CON,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_ETHIF},
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_HIF] = {
+		.name = "hif",
+		.sta_mask = PWR_STATUS_HIF,
+		.ctl_offs = SPM_HIF_PWR_CON,
+		.sram_pdn_bits = GENMASK(11, 8),
+		.sram_pdn_ack_bits = GENMASK(15, 12),
+		.clk_id = {CLK_ETHIF},
+		.active_wakeup = true,
+	},
+	[MT2701_POWER_DOMAIN_IFR_MSC] = {
+		.name = "ifr_msc",
+		.sta_mask = PWR_STATUS_IFR_MSC,
+		.ctl_offs = SPM_IFR_MSC_PWR_CON,
+		.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
  */
 
@@ -587,6 +697,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

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

[Matthias Brugger]

On 05/16/2016 11:28 AM, James Liao wrote:
> Refine scpsys driver common code to support multiple SoC / platform.
>
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  drivers/soc/mediatek/mtk-scpsys.c | 363 +++++++++++++++++++++++---------------
>  1 file changed, 220 insertions(+), 143 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index 57e781c..5870a24 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -11,17 +11,15 @@
>   * 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 <linux/soc/mediatek/infracfg.h>
> +
>  #include <dt-bindings/power/mt8173-power.h>
>
>  #define SPM_VDE_PWR_CON			0x0210
> @@ -34,6 +32,7 @@
>  #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
>
> @@ -55,12 +54,12 @@
>  #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,
> +	CLK_NONE,
> +	CLK_MM,
> +	CLK_MFG,
> +	CLK_VENC,
> +	CLK_VENC_LT,
> +	CLK_MAX,
>  };
>
>  #define MAX_CLKS	2
> @@ -76,98 +75,6 @@ struct scp_domain_data {
>  	bool active_wakeup;
>  };
>
> -static const struct scp_domain_data scp_domain_data[] = {
> -	[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 {
> @@ -179,7 +86,7 @@ struct scp_domain {
>  };
>
>  struct scp {
> -	struct scp_domain domains[NUM_DOMAINS];
> +	struct scp_domain *domains;
>  	struct genpd_onecell_data pd_data;
>  	struct device *dev;
>  	void __iomem *base;
> @@ -408,57 +315,69 @@ static bool scpsys_active_wakeup(struct device *dev)
>  	return scpd->data->active_wakeup;
>  }
>
> -static int 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]);

We can use the global enum clk_id and stat with i = 1, right?

> +}
> +
> +static 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];
>
> @@ -467,28 +386,54 @@ static int 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;

Put this in the else branch. Apart from that is there any reason you 
moved the for up in the function? If not, I would prefer not to move it, 
to make it easier to read the diff.

Apart from that, the patch looks good to me.
Sorry for the delay in responding.

Matthias

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

[Matthias Brugger]

On 05/16/2016 11:28 AM, James Liao wrote:
> 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 <jamesjj.liao@mediatek.com>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  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 5870a24..00c0adb 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -617,4 +617,21 @@ static struct platform_driver scpsys_drv = {
>  		.of_match_table = of_match_ptr(of_scpsys_match_tbl),
>  	},
>  };
> -builtin_platform_driver(scpsys_drv);
> +
> +static int __init scpsys_drv_init(void)
> +{
> +	return platform_driver_register(&scpsys_drv);
> +}
> +
> +/*
> + * 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);
>

Can't we achieve this with probe deferring? I'm not really keen on 
coding the order of the different drivers like this.

Regards,
Matthias

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

[Matthias Brugger]

On 05/16/2016 11:28 AM, James Liao wrote:
> From: Shunli Wang <shunli.wang@mediatek.com>
>
> Add scpsys driver for MT2701.
>
> mtk-scpsys now supports MT8173 (arm64) and MT2701 (arm). So it should
> be enabled on both arm64 and arm platforms.
>
> Signed-off-by: Shunli Wang <shunli.wang@mediatek.com>
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  drivers/soc/mediatek/Kconfig      |   2 +-
>  drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 116 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index 0a4ea80..609bb34 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -23,7 +23,7 @@ config MTK_PMIC_WRAP
>  config MTK_SCPSYS
>  	bool "MediaTek SCPSYS Support"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> -	default ARM64 && ARCH_MEDIATEK
> +	default ARCH_MEDIATEK
>  	select REGMAP
>  	select MTK_INFRACFG
>  	select PM_GENERIC_DOMAINS if PM
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index 00c0adb..f4d1230 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -20,6 +20,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/soc/mediatek/infracfg.h>
>
> +#include <dt-bindings/power/mt2701-power.h>
>  #include <dt-bindings/power/mt8173-power.h>
>
>  #define SPM_VDE_PWR_CON			0x0210
> @@ -27,8 +28,13 @@
>  #define SPM_VEN_PWR_CON			0x0230
>  #define SPM_ISP_PWR_CON			0x0238
>  #define SPM_DIS_PWR_CON			0x023c
> +#define SPM_CONN_PWR_CON		0x0280
>  #define SPM_VEN2_PWR_CON		0x0298
> -#define SPM_AUDIO_PWR_CON		0x029c
> +#define SPM_AUDIO_PWR_CON		0x029c	/* MT8173 */
> +#define SPM_BDP_PWR_CON			0x029c	/* MT2701 */
> +#define SPM_ETH_PWR_CON			0x02a0
> +#define SPM_HIF_PWR_CON			0x02a4
> +#define SPM_IFR_MSC_PWR_CON		0x02a8
>  #define SPM_MFG_2D_PWR_CON		0x02c0
>  #define SPM_MFG_ASYNC_PWR_CON		0x02c4
>  #define SPM_USB_PWR_CON			0x02cc
> @@ -42,10 +48,15 @@
>  #define PWR_ON_2ND_BIT			BIT(3)
>  #define PWR_CLK_DIS_BIT			BIT(4)
>
> +#define PWR_STATUS_CONN			BIT(1)
>  #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_BDP			BIT(14)
> +#define PWR_STATUS_ETH			BIT(15)
> +#define PWR_STATUS_HIF			BIT(16)
> +#define PWR_STATUS_IFR_MSC		BIT(17)
>  #define PWR_STATUS_VENC_LT		BIT(20)
>  #define PWR_STATUS_VENC			BIT(21)
>  #define PWR_STATUS_MFG_2D		BIT(22)
> @@ -59,6 +70,7 @@ enum clk_id {
>  	CLK_MFG,
>  	CLK_VENC,
>  	CLK_VENC_LT,
> +	CLK_ETHIF,
>  	CLK_MAX,
>  };
>
> @@ -321,7 +333,8 @@ static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
>  		CLK_MM,
>  		CLK_MFG,
>  		CLK_VENC,
> -		CLK_VENC_LT
> +		CLK_VENC_LT,
> +		CLK_ETHIF
>  	};
>
>  	static const char * const clk_names[] = {
> @@ -329,6 +342,7 @@ static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
>  		"mfg",
>  		"venc",
>  		"venc_lt",
> +		"ethif",
>  	};
>
>  	int i;
> @@ -459,6 +473,102 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>  }
>
>  /*
> + * MT2701 power domain support
> + */
> +
> +static const struct scp_domain_data scp_domain_data_mt2701[] = {
> +	[MT2701_POWER_DOMAIN_CONN] = {
> +		.name = "conn",
> +		.sta_mask = PWR_STATUS_CONN,
> +		.ctl_offs = SPM_CONN_PWR_CON,
> +		.bus_prot_mask = 0x0104,
> +		.active_wakeup = true,

.clk_id = {CLK_NONE},

> +	},
> +	[MT2701_POWER_DOMAIN_DISP] = {
> +		.name = "disp",
> +		.sta_mask = PWR_STATUS_DISP,
> +		.ctl_offs = SPM_DIS_PWR_CON,
> +		.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 = PWR_STATUS_MFG,
> +		.ctl_offs = SPM_MFG_PWR_CON,
> +		.sram_pdn_bits = GENMASK(11, 8),
> +		.sram_pdn_ack_bits = GENMASK(12, 12),
> +		.clk_id = {CLK_MFG},
> +		.active_wakeup = true,
> +	},
> +	[MT2701_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 = {CLK_MM},
> +		.active_wakeup = true,
> +	},
> +	[MT2701_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 = {CLK_MM},
> +		.active_wakeup = true,
> +	},
> +	[MT2701_POWER_DOMAIN_BDP] = {
> +		.name = "bdp",
> +		.sta_mask = PWR_STATUS_BDP,
> +		.ctl_offs = SPM_BDP_PWR_CON,
> +		.sram_pdn_bits = GENMASK(11, 8),
> +		.active_wakeup = true,

same here.

> +	},
> +	[MT2701_POWER_DOMAIN_ETH] = {
> +		.name = "eth",
> +		.sta_mask = PWR_STATUS_ETH,
> +		.ctl_offs = SPM_ETH_PWR_CON,
> +		.sram_pdn_bits = GENMASK(11, 8),
> +		.sram_pdn_ack_bits = GENMASK(15, 12),
> +		.clk_id = {CLK_ETHIF},
> +		.active_wakeup = true,
> +	},
> +	[MT2701_POWER_DOMAIN_HIF] = {
> +		.name = "hif",
> +		.sta_mask = PWR_STATUS_HIF,
> +		.ctl_offs = SPM_HIF_PWR_CON,
> +		.sram_pdn_bits = GENMASK(11, 8),
> +		.sram_pdn_ack_bits = GENMASK(15, 12),
> +		.clk_id = {CLK_ETHIF},
> +		.active_wakeup = true,
> +	},
> +	[MT2701_POWER_DOMAIN_IFR_MSC] = {
> +		.name = "ifr_msc",
> +		.sta_mask = PWR_STATUS_IFR_MSC,
> +		.ctl_offs = SPM_IFR_MSC_PWR_CON,
> +		.active_wakeup = true,

same here.

regards,
Matthias

> +	},
> +};
> +
> +#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
>   */
>
> @@ -587,6 +697,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,
>  	}, {
>

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

[James Liao]

On Sat, 2016-07-02 at 18:41 +0200, Matthias Brugger wrote:
> 
> On 05/16/2016 11:28 AM, James Liao wrote:
> > From: Shunli Wang <shunli.wang@mediatek.com>
> >
> > Add scpsys driver for MT2701.
> >
> > mtk-scpsys now supports MT8173 (arm64) and MT2701 (arm). So it should
> > be enabled on both arm64 and arm platforms.
> >
> > Signed-off-by: Shunli Wang <shunli.wang@mediatek.com>
> > Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> > Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> > ---
> >  drivers/soc/mediatek/Kconfig      |   2 +-
> >  drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 116 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > index 0a4ea80..609bb34 100644
> > --- a/drivers/soc/mediatek/Kconfig
> > +++ b/drivers/soc/mediatek/Kconfig
> > @@ -23,7 +23,7 @@ config MTK_PMIC_WRAP
> >  config MTK_SCPSYS
> >  	bool "MediaTek SCPSYS Support"
> >  	depends on ARCH_MEDIATEK || COMPILE_TEST
> > -	default ARM64 && ARCH_MEDIATEK
> > +	default ARCH_MEDIATEK
> >  	select REGMAP
> >  	select MTK_INFRACFG
> >  	select PM_GENERIC_DOMAINS if PM
> > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > index 00c0adb..f4d1230 100644
> > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/soc/mediatek/infracfg.h>
> >
> > +#include <dt-bindings/power/mt2701-power.h>
> >  #include <dt-bindings/power/mt8173-power.h>
> >
> >  #define SPM_VDE_PWR_CON			0x0210
> > @@ -27,8 +28,13 @@
> >  #define SPM_VEN_PWR_CON			0x0230
> >  #define SPM_ISP_PWR_CON			0x0238
> >  #define SPM_DIS_PWR_CON			0x023c
> > +#define SPM_CONN_PWR_CON		0x0280
> >  #define SPM_VEN2_PWR_CON		0x0298
> > -#define SPM_AUDIO_PWR_CON		0x029c
> > +#define SPM_AUDIO_PWR_CON		0x029c	/* MT8173 */
> > +#define SPM_BDP_PWR_CON			0x029c	/* MT2701 */
> > +#define SPM_ETH_PWR_CON			0x02a0
> > +#define SPM_HIF_PWR_CON			0x02a4
> > +#define SPM_IFR_MSC_PWR_CON		0x02a8
> >  #define SPM_MFG_2D_PWR_CON		0x02c0
> >  #define SPM_MFG_ASYNC_PWR_CON		0x02c4
> >  #define SPM_USB_PWR_CON			0x02cc
> > @@ -42,10 +48,15 @@
> >  #define PWR_ON_2ND_BIT			BIT(3)
> >  #define PWR_CLK_DIS_BIT			BIT(4)
> >
> > +#define PWR_STATUS_CONN			BIT(1)
> >  #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_BDP			BIT(14)
> > +#define PWR_STATUS_ETH			BIT(15)
> > +#define PWR_STATUS_HIF			BIT(16)
> > +#define PWR_STATUS_IFR_MSC		BIT(17)
> >  #define PWR_STATUS_VENC_LT		BIT(20)
> >  #define PWR_STATUS_VENC			BIT(21)
> >  #define PWR_STATUS_MFG_2D		BIT(22)
> > @@ -59,6 +70,7 @@ enum clk_id {
> >  	CLK_MFG,
> >  	CLK_VENC,
> >  	CLK_VENC_LT,
> > +	CLK_ETHIF,
> >  	CLK_MAX,
> >  };
> >
> > @@ -321,7 +333,8 @@ static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
> >  		CLK_MM,
> >  		CLK_MFG,
> >  		CLK_VENC,
> > -		CLK_VENC_LT
> > +		CLK_VENC_LT,
> > +		CLK_ETHIF
> >  	};
> >
> >  	static const char * const clk_names[] = {
> > @@ -329,6 +342,7 @@ static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
> >  		"mfg",
> >  		"venc",
> >  		"venc_lt",
> > +		"ethif",
> >  	};
> >
> >  	int i;
> > @@ -459,6 +473,102 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >  }
> >
> >  /*
> > + * MT2701 power domain support
> > + */
> > +
> > +static const struct scp_domain_data scp_domain_data_mt2701[] = {
> > +	[MT2701_POWER_DOMAIN_CONN] = {
> > +		.name = "conn",
> > +		.sta_mask = PWR_STATUS_CONN,
> > +		.ctl_offs = SPM_CONN_PWR_CON,
> > +		.bus_prot_mask = 0x0104,
> > +		.active_wakeup = true,
> 
> .clk_id = {CLK_NONE},
> 
> > +	},
> > +	[MT2701_POWER_DOMAIN_DISP] = {
> > +		.name = "disp",
> > +		.sta_mask = PWR_STATUS_DISP,
> > +		.ctl_offs = SPM_DIS_PWR_CON,
> > +		.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 = PWR_STATUS_MFG,
> > +		.ctl_offs = SPM_MFG_PWR_CON,
> > +		.sram_pdn_bits = GENMASK(11, 8),
> > +		.sram_pdn_ack_bits = GENMASK(12, 12),
> > +		.clk_id = {CLK_MFG},
> > +		.active_wakeup = true,
> > +	},
> > +	[MT2701_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 = {CLK_MM},
> > +		.active_wakeup = true,
> > +	},
> > +	[MT2701_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 = {CLK_MM},
> > +		.active_wakeup = true,
> > +	},
> > +	[MT2701_POWER_DOMAIN_BDP] = {
> > +		.name = "bdp",
> > +		.sta_mask = PWR_STATUS_BDP,
> > +		.ctl_offs = SPM_BDP_PWR_CON,
> > +		.sram_pdn_bits = GENMASK(11, 8),
> > +		.active_wakeup = true,
> 
> same here.
> 
> > +	},
> > +	[MT2701_POWER_DOMAIN_ETH] = {
> > +		.name = "eth",
> > +		.sta_mask = PWR_STATUS_ETH,
> > +		.ctl_offs = SPM_ETH_PWR_CON,
> > +		.sram_pdn_bits = GENMASK(11, 8),
> > +		.sram_pdn_ack_bits = GENMASK(15, 12),
> > +		.clk_id = {CLK_ETHIF},
> > +		.active_wakeup = true,
> > +	},
> > +	[MT2701_POWER_DOMAIN_HIF] = {
> > +		.name = "hif",
> > +		.sta_mask = PWR_STATUS_HIF,
> > +		.ctl_offs = SPM_HIF_PWR_CON,
> > +		.sram_pdn_bits = GENMASK(11, 8),
> > +		.sram_pdn_ack_bits = GENMASK(15, 12),
> > +		.clk_id = {CLK_ETHIF},
> > +		.active_wakeup = true,
> > +	},
> > +	[MT2701_POWER_DOMAIN_IFR_MSC] = {
> > +		.name = "ifr_msc",
> > +		.sta_mask = PWR_STATUS_IFR_MSC,
> > +		.ctl_offs = SPM_IFR_MSC_PWR_CON,
> > +		.active_wakeup = true,
> 
> same here.

Hi Matthias,

I'll add them in next patch.


Best regards,

James

> > +	},
> > +};
> > +
> > +#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
> >   */
> >
> > @@ -587,6 +697,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,
> >  	}, {
> >

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

[James Liao]

On Sat, 2016-07-02 at 18:35 +0200, Matthias Brugger wrote:
> 
> On 05/16/2016 11:28 AM, James Liao wrote:
> > 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 <jamesjj.liao@mediatek.com>
> > Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> > ---
> >  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 5870a24..00c0adb 100644
> > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > @@ -617,4 +617,21 @@ static struct platform_driver scpsys_drv = {
> >  		.of_match_table = of_match_ptr(of_scpsys_match_tbl),
> >  	},
> >  };
> > -builtin_platform_driver(scpsys_drv);
> > +
> > +static int __init scpsys_drv_init(void)
> > +{
> > +	return platform_driver_register(&scpsys_drv);
> > +}
> > +
> > +/*
> > + * 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);
> >
> 
> Can't we achieve this with probe deferring? I'm not really keen on 
> coding the order of the different drivers like this.

Hi Matthias,

Some drivers such as IOMMU don't support probe deferring. So scpsys need
to init before them by changing init level.


Best regards,

James

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

[James Liao]

Hi Matthias,

On Sat, 2016-07-02 at 18:33 +0200, Matthias Brugger wrote:
> 
> On 05/16/2016 11:28 AM, James Liao wrote:
> > Refine scpsys driver common code to support multiple SoC / platform.
> >
> > Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> > Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> > ---
> >  drivers/soc/mediatek/mtk-scpsys.c | 363 +++++++++++++++++++++++---------------
> >  1 file changed, 220 insertions(+), 143 deletions(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > index 57e781c..5870a24 100644
> > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > @@ -11,17 +11,15 @@
> >   * 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 <linux/soc/mediatek/infracfg.h>
> > +
> >  #include <dt-bindings/power/mt8173-power.h>
> >
> >  #define SPM_VDE_PWR_CON			0x0210
> > @@ -34,6 +32,7 @@
> >  #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
> >
> > @@ -55,12 +54,12 @@
> >  #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,
> > +	CLK_NONE,
> > +	CLK_MM,
> > +	CLK_MFG,
> > +	CLK_VENC,
> > +	CLK_VENC_LT,
> > +	CLK_MAX,
> >  };
> >
> >  #define MAX_CLKS	2
> > @@ -76,98 +75,6 @@ struct scp_domain_data {
> >  	bool active_wakeup;
> >  };
> >
> > -static const struct scp_domain_data scp_domain_data[] = {
> > -	[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 {
> > @@ -179,7 +86,7 @@ struct scp_domain {
> >  };
> >
> >  struct scp {
> > -	struct scp_domain domains[NUM_DOMAINS];
> > +	struct scp_domain *domains;
> >  	struct genpd_onecell_data pd_data;
> >  	struct device *dev;
> >  	void __iomem *base;
> > @@ -408,57 +315,69 @@ static bool scpsys_active_wakeup(struct device *dev)
> >  	return scpd->data->active_wakeup;
> >  }
> >
> > -static int 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]);
> 
> We can use the global enum clk_id and stat with i = 1, right?

You are right. I'll change the start value with i = CLK_NONE + 1 in next
patch.

> > +}
> > +
> > +static 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];
> >
> > @@ -467,28 +386,54 @@ static int 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;
> 
> Put this in the else branch. Apart from that is there any reason you 

Do you mean to change like this?

	if (IS_ERR(c)) {
		...
		return ERR_CAST(c);
	} else {
		scpd->clk[j] = c;
	}

checkpatch.pl will warn for above code due to it returns in 'if' branch.

> moved the for up in the function? If not, I would prefer not to move it, 
> to make it easier to read the diff.

The new 'for' block are far different from original one. And I think
it's easy to read if we keep simple assign statements in the same block.

> Apart from that, the patch looks good to me.
> Sorry for the delay in responding.

Thanks for your comments.


Best regards,

James

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

[Matthias Brugger]

On 06/07/16 07:39, James Liao wrote:
> Hi Matthias,
>
> On Sat, 2016-07-02 at 18:33 +0200, Matthias Brugger wrote:
>>
>> On 05/16/2016 11:28 AM, James Liao wrote:
>>> Refine scpsys driver common code to support multiple SoC / platform.
>>>
>>> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
>>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>>> ---
>>>   drivers/soc/mediatek/mtk-scpsys.c | 363 +++++++++++++++++++++++---------------
>>>   1 file changed, 220 insertions(+), 143 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
>>> index 57e781c..5870a24 100644
>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>> @@ -11,17 +11,15 @@
>>>    * 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 <linux/soc/mediatek/infracfg.h>
>>> +
>>>   #include <dt-bindings/power/mt8173-power.h>
>>>
>>>   #define SPM_VDE_PWR_CON			0x0210
>>> @@ -34,6 +32,7 @@
>>>   #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
>>>
>>> @@ -55,12 +54,12 @@
>>>   #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,
>>> +	CLK_NONE,
>>> +	CLK_MM,
>>> +	CLK_MFG,
>>> +	CLK_VENC,
>>> +	CLK_VENC_LT,
>>> +	CLK_MAX,
>>>   };
>>>
>>>   #define MAX_CLKS	2
>>> @@ -76,98 +75,6 @@ struct scp_domain_data {
>>>   	bool active_wakeup;
>>>   };
>>>
>>> -static const struct scp_domain_data scp_domain_data[] = {
>>> -	[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 {
>>> @@ -179,7 +86,7 @@ struct scp_domain {
>>>   };
>>>
>>>   struct scp {
>>> -	struct scp_domain domains[NUM_DOMAINS];
>>> +	struct scp_domain *domains;
>>>   	struct genpd_onecell_data pd_data;
>>>   	struct device *dev;
>>>   	void __iomem *base;
>>> @@ -408,57 +315,69 @@ static bool scpsys_active_wakeup(struct device *dev)
>>>   	return scpd->data->active_wakeup;
>>>   }
>>>
>>> -static int 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]);
>>
>> We can use the global enum clk_id and stat with i = 1, right?
>
> You are right. I'll change the start value with i = CLK_NONE + 1 in next
> patch.
>
>>> +}
>>> +
>>> +static 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];
>>>
>>> @@ -467,28 +386,54 @@ static int 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;
>>
>> Put this in the else branch. Apart from that is there any reason you
>
> Do you mean to change like this?
>
> 	if (IS_ERR(c)) {
> 		...
> 		return ERR_CAST(c);
> 	} else {
> 		scpd->clk[j] = c;
> 	}
>
> checkpatch.pl will warn for above code due to it returns in 'if' branch.
>

I tried that on top of next-20160706 and it checkpatch didn't throw any 
warning. Which kernel version are based on?

>> moved the for up in the function? If not, I would prefer not to move it,
>> to make it easier to read the diff.
>
> The new 'for' block are far different from original one. And I think
> it's easy to read if we keep simple assign statements in the same block.
>

It's different in the sense that it checks if struct clk *c is an error.
I don't see the reason why we need to move it up in the file.
It's not too important but I would prefer not to move it if there is no 
reason.

Regards,
Matthias

>> Apart from that, the patch looks good to me.
>> Sorry for the delay in responding.
>
> Thanks for your comments.
>
>
> Best regards,
>
> James
>

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

[Matthias Brugger]

On 06/07/16 07:22, James Liao wrote:
> On Sat, 2016-07-02 at 18:35 +0200, Matthias Brugger wrote:
>>
>> On 05/16/2016 11:28 AM, James Liao wrote:
>>> 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 <jamesjj.liao@mediatek.com>
>>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>>> ---
>>>   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 5870a24..00c0adb 100644
>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>> @@ -617,4 +617,21 @@ static struct platform_driver scpsys_drv = {
>>>   		.of_match_table = of_match_ptr(of_scpsys_match_tbl),
>>>   	},
>>>   };
>>> -builtin_platform_driver(scpsys_drv);
>>> +
>>> +static int __init scpsys_drv_init(void)
>>> +{
>>> +	return platform_driver_register(&scpsys_drv);
>>> +}
>>> +
>>> +/*
>>> + * 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);
>>>
>>
>> Can't we achieve this with probe deferring? I'm not really keen on
>> coding the order of the different drivers like this.
>
> Hi Matthias,
>
> Some drivers such as IOMMU don't support probe deferring. So scpsys need
> to init before them by changing init level.
>

SMI larbs return EPROBE_DEFER if it can't find the PM provider for it's 
domain, so this part should not be the problem. The larbs get added as 
components in the probe, when the PM provider is present.

The iommu driver uses the larbs as components. As long as not all 
components are present the iommu does not bind them. So from what I see 
this should work without any problem.

Can you please specify where the iommu dirver has a problem. Maybe we 
need to fix the driver rather then scpsys.

Regards,
Matthias

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

[James Liao]

Hi Matthias,

On Thu, 2016-07-07 at 13:20 +0200, Matthias Brugger wrote:
> 
> On 06/07/16 07:39, James Liao wrote:
> > Hi Matthias,
> >
> > On Sat, 2016-07-02 at 18:33 +0200, Matthias Brugger wrote:
> >>
> >> On 05/16/2016 11:28 AM, James Liao wrote:
> >>> Refine scpsys driver common code to support multiple SoC / platform.
> >>>
> >>> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> >>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> >>> ---
> >>>   drivers/soc/mediatek/mtk-scpsys.c | 363 +++++++++++++++++++++++---------------
> >>>   1 file changed, 220 insertions(+), 143 deletions(-)
> >>>
> >>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> >>> index 57e781c..5870a24 100644
> >>> --- a/drivers/soc/mediatek/mtk-scpsys.c
> >>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> >>> @@ -11,17 +11,15 @@
> >>>    * 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 <linux/soc/mediatek/infracfg.h>
> >>> +
> >>>   #include <dt-bindings/power/mt8173-power.h>
> >>>
> >>>   #define SPM_VDE_PWR_CON			0x0210
> >>> @@ -34,6 +32,7 @@
> >>>   #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
> >>>
> >>> @@ -55,12 +54,12 @@
> >>>   #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,
> >>> +	CLK_NONE,
> >>> +	CLK_MM,
> >>> +	CLK_MFG,
> >>> +	CLK_VENC,
> >>> +	CLK_VENC_LT,
> >>> +	CLK_MAX,
> >>>   };
> >>>
> >>>   #define MAX_CLKS	2
> >>> @@ -76,98 +75,6 @@ struct scp_domain_data {
> >>>   	bool active_wakeup;
> >>>   };
> >>>
> >>> -static const struct scp_domain_data scp_domain_data[] = {
> >>> -	[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 {
> >>> @@ -179,7 +86,7 @@ struct scp_domain {
> >>>   };
> >>>
> >>>   struct scp {
> >>> -	struct scp_domain domains[NUM_DOMAINS];
> >>> +	struct scp_domain *domains;
> >>>   	struct genpd_onecell_data pd_data;
> >>>   	struct device *dev;
> >>>   	void __iomem *base;
> >>> @@ -408,57 +315,69 @@ static bool scpsys_active_wakeup(struct device *dev)
> >>>   	return scpd->data->active_wakeup;
> >>>   }
> >>>
> >>> -static int 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]);
> >>
> >> We can use the global enum clk_id and stat with i = 1, right?
> >
> > You are right. I'll change the start value with i = CLK_NONE + 1 in next
> > patch.
> >
> >>> +}
> >>> +
> >>> +static 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];
> >>>
> >>> @@ -467,28 +386,54 @@ static int 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;
> >>
> >> Put this in the else branch. Apart from that is there any reason you
> >
> > Do you mean to change like this?
> >
> > 	if (IS_ERR(c)) {
> > 		...
> > 		return ERR_CAST(c);
> > 	} else {
> > 		scpd->clk[j] = c;
> > 	}
> >
> > checkpatch.pl will warn for above code due to it returns in 'if' branch.
> >
> 
> I tried that on top of next-20160706 and it checkpatch didn't throw any 
> warning. Which kernel version are based on?

I don't remember which version of checkpatch warn on this pattern. This
patch series develop across several kernel versions.

So do you prefer to put "scpd->clk[j] = c;" into 'else' branch?

> >> moved the for up in the function? If not, I would prefer not to move it,
> >> to make it easier to read the diff.
> >
> > The new 'for' block are far different from original one. And I think
> > it's easy to read if we keep simple assign statements in the same block.
> >
> 
> It's different in the sense that it checks if struct clk *c is an error.
> I don't see the reason why we need to move it up in the file.
> It's not too important but I would prefer not to move it if there is no 
> reason.

I think I may misunderstand your comments. Which 'for' block did you
mention for? 'for (i = 0; i < num ...' or 'for (j = 0; j < MAX_CLKS
&& ...' ?

The 'for(i)' exists in original code, this patch just change its counter
from 'NUM_DOMAINS' to 'num'. The 'for(j)' is a new for-block, so it was
not moved from other blocks.


Best regards,

James

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

[Matthias Brugger]

On 11/07/16 10:56, James Liao wrote:

[...]

>>>>> @@ -467,28 +386,54 @@ static int 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;
>>>>
>>>> Put this in the else branch. Apart from that is there any reason you
>>>
>>> Do you mean to change like this?
>>>
>>> 	if (IS_ERR(c)) {
>>> 		...
>>> 		return ERR_CAST(c);
>>> 	} else {
>>> 		scpd->clk[j] = c;
>>> 	}
>>>
>>> checkpatch.pl will warn for above code due to it returns in 'if' branch.
>>>
>>
>> I tried that on top of next-20160706 and it checkpatch didn't throw any
>> warning. Which kernel version are based on?
>
> I don't remember which version of checkpatch warn on this pattern. This
> patch series develop across several kernel versions.

We as the kernel community develop against master or linux-next. We only 
care about older kernel version in the sense that we intent hard not to 
break any userspace/kernel or firmware/kernel interfaces. Apart from 
that it's up to every individual to backport patches from mainline 
kernel to his respective version. But that's nothing the community as a 
hole can take care of.

>
> So do you prefer to put "scpd->clk[j] = c;" into 'else' branch?
>

Yes please :)

>>>> moved the for up in the function? If not, I would prefer not to move it,
>>>> to make it easier to read the diff.
>>>
>>> The new 'for' block are far different from original one. And I think
>>> it's easy to read if we keep simple assign statements in the same block.
>>>
>>
>> It's different in the sense that it checks if struct clk *c is an error.
>> I don't see the reason why we need to move it up in the file.
>> It's not too important but I would prefer not to move it if there is no
>> reason.
>
> I think I may misunderstand your comments. Which 'for' block did you
> mention for? 'for (i = 0; i < num ...' or 'for (j = 0; j < MAX_CLKS
> && ...' ?
>
> The 'for(i)' exists in original code, this patch just change its counter
> from 'NUM_DOMAINS' to 'num'. The 'for(j)' is a new for-block, so it was
> not moved from other blocks.
>

for (j = 0; j < MAX_CLKS... is present in the actual scpsys_probe in 
linux-next (line 485 as of today). This patch moves this for a few lines 
up, to be precise before executing this code sequence:
<code>
pd_data->domains[i] = genpd;
scpd->scp = scp;

scpd->data = data;
</code>

AFAIK there is no reason to do so. It adds unnecessary complexity to the 
patch. So please fix this together with the other comments you got.

Thanks a lot,
Matthias

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

[Yingjoe Chen]

On Mon, 2016-07-11 at 15:10 +0200, Matthias Brugger wrote:
> 
> On 11/07/16 10:56, James Liao wrote:
> 
> [...]
> 
> >>>>> @@ -467,28 +386,54 @@ static int 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;
> >>>>
> >>>> Put this in the else branch. Apart from that is there any reason you
> >>>
> >>> Do you mean to change like this?
> >>>
> >>> 	if (IS_ERR(c)) {
> >>> 		...
> >>> 		return ERR_CAST(c);
> >>> 	} else {
> >>> 		scpd->clk[j] = c;
> >>> 	}
> >>>
> >>> checkpatch.pl will warn for above code due to it returns in 'if' branch.
> >>>
> >>
> >> I tried that on top of next-20160706 and it checkpatch didn't throw any
> >> warning. Which kernel version are based on?
> >
> > I don't remember which version of checkpatch warn on this pattern. This
> > patch series develop across several kernel versions.
> 
> We as the kernel community develop against master or linux-next. We only 
> care about older kernel version in the sense that we intent hard not to 
> break any userspace/kernel or firmware/kernel interfaces. Apart from 
> that it's up to every individual to backport patches from mainline 
> kernel to his respective version. But that's nothing the community as a 
> hole can take care of.
> 
> >
> > So do you prefer to put "scpd->clk[j] = c;" into 'else' branch?
> >
> 
> Yes please :)


Hi,

I just got next-20160711 and change this chunk to:

+		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);
+			} else {
+				scpd->clk[j] = c;
+			}
+		}
+


and checkpatch give me this warning:

WARNING: else is not generally useful after a break or return
#313: FILE: drivers/soc/mediatek/mtk-scpsys.c:409:
+                               return ERR_CAST(c);
+                       } else {

Joe.C

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

[James Liao]

Hi Matthias,

On Mon, 2016-07-11 at 15:10 +0200, Matthias Brugger wrote:
> 
> On 11/07/16 10:56, James Liao wrote:
> 
> [...]
> 
> >>>>> @@ -467,28 +386,54 @@ static int 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;
> >>>>
> >>>> Put this in the else branch. Apart from that is there any reason you
> >>>
> >>> Do you mean to change like this?
> >>>
> >>> 	if (IS_ERR(c)) {
> >>> 		...
> >>> 		return ERR_CAST(c);
> >>> 	} else {
> >>> 		scpd->clk[j] = c;
> >>> 	}
> >>>
> >>> checkpatch.pl will warn for above code due to it returns in 'if' branch.
> >>>
> >>
> >> I tried that on top of next-20160706 and it checkpatch didn't throw any
> >> warning. Which kernel version are based on?
> >
> > I don't remember which version of checkpatch warn on this pattern. This
> > patch series develop across several kernel versions.
> 
> We as the kernel community develop against master or linux-next. We only 
> care about older kernel version in the sense that we intent hard not to 
> break any userspace/kernel or firmware/kernel interfaces. Apart from 
> that it's up to every individual to backport patches from mainline 
> kernel to his respective version. But that's nothing the community as a 
> hole can take care of.
> 
> >
> > So do you prefer to put "scpd->clk[j] = c;" into 'else' branch?
> >
> 
> Yes please :)

Yingjoe had tested in the latest checkpatch.pl and it showed checkpatch
warn on the else-branch. He had replied the results in previous email.
 
> >>>> moved the for up in the function? If not, I would prefer not to move it,
> >>>> to make it easier to read the diff.
> >>>
> >>> The new 'for' block are far different from original one. And I think
> >>> it's easy to read if we keep simple assign statements in the same block.
> >>>
> >>
> >> It's different in the sense that it checks if struct clk *c is an error.
> >> I don't see the reason why we need to move it up in the file.
> >> It's not too important but I would prefer not to move it if there is no
> >> reason.
> >
> > I think I may misunderstand your comments. Which 'for' block did you
> > mention for? 'for (i = 0; i < num ...' or 'for (j = 0; j < MAX_CLKS
> > && ...' ?
> >
> > The 'for(i)' exists in original code, this patch just change its counter
> > from 'NUM_DOMAINS' to 'num'. The 'for(j)' is a new for-block, so it was
> > not moved from other blocks.
> >
> 
> for (j = 0; j < MAX_CLKS... is present in the actual scpsys_probe in 
> linux-next (line 485 as of today). This patch moves this for a few lines 
> up, to be precise before executing this code sequence:
> <code>
> pd_data->domains[i] = genpd;
> scpd->scp = scp;
> 
> scpd->data = data;
> </code>
> 
> AFAIK there is no reason to do so. It adds unnecessary complexity to the 
> patch. So please fix this together with the other comments you got.

I see. So you prefer to put the for(j < MAX_CLKS) after 'scpd->data =
data' right? I can change it in next patch.


Best regards,

James

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

[Matthias Brugger]

On 12/07/16 05:34, James Liao wrote:
> Hi Matthias,
>
> On Mon, 2016-07-11 at 15:10 +0200, Matthias Brugger wrote:
>>
>> On 11/07/16 10:56, James Liao wrote:
>>
>> [...]
>>
>>>>>>> @@ -467,28 +386,54 @@ static int 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;
>>>>>>
>>>>>> Put this in the else branch. Apart from that is there any reason you
>>>>>
>>>>> Do you mean to change like this?
>>>>>
>>>>> 	if (IS_ERR(c)) {
>>>>> 		...
>>>>> 		return ERR_CAST(c);
>>>>> 	} else {
>>>>> 		scpd->clk[j] = c;
>>>>> 	}
>>>>>
>>>>> checkpatch.pl will warn for above code due to it returns in 'if' branch.
>>>>>
>>>>
>>>> I tried that on top of next-20160706 and it checkpatch didn't throw any
>>>> warning. Which kernel version are based on?
>>>
>>> I don't remember which version of checkpatch warn on this pattern. This
>>> patch series develop across several kernel versions.
>>
>> We as the kernel community develop against master or linux-next. We only
>> care about older kernel version in the sense that we intent hard not to
>> break any userspace/kernel or firmware/kernel interfaces. Apart from
>> that it's up to every individual to backport patches from mainline
>> kernel to his respective version. But that's nothing the community as a
>> hole can take care of.
>>
>>>
>>> So do you prefer to put "scpd->clk[j] = c;" into 'else' branch?
>>>
>>
>> Yes please :)
>
> Yingjoe had tested in the latest checkpatch.pl and it showed checkpatch
> warn on the else-branch. He had replied the results in previous email.
>

Yes you are right. Not sure what I was testing. Sorry for that.

>>>>>> moved the for up in the function? If not, I would prefer not to move it,
>>>>>> to make it easier to read the diff.
>>>>>
>>>>> The new 'for' block are far different from original one. And I think
>>>>> it's easy to read if we keep simple assign statements in the same block.
>>>>>
>>>>
>>>> It's different in the sense that it checks if struct clk *c is an error.
>>>> I don't see the reason why we need to move it up in the file.
>>>> It's not too important but I would prefer not to move it if there is no
>>>> reason.
>>>
>>> I think I may misunderstand your comments. Which 'for' block did you
>>> mention for? 'for (i = 0; i < num ...' or 'for (j = 0; j < MAX_CLKS
>>> && ...' ?
>>>
>>> The 'for(i)' exists in original code, this patch just change its counter
>>> from 'NUM_DOMAINS' to 'num'. The 'for(j)' is a new for-block, so it was
>>> not moved from other blocks.
>>>
>>
>> for (j = 0; j < MAX_CLKS... is present in the actual scpsys_probe in
>> linux-next (line 485 as of today). This patch moves this for a few lines
>> up, to be precise before executing this code sequence:
>> <code>
>> pd_data->domains[i] = genpd;
>> scpd->scp = scp;
>>
>> scpd->data = data;
>> </code>
>>
>> AFAIK there is no reason to do so. It adds unnecessary complexity to the
>> patch. So please fix this together with the other comments you got.
>
> I see. So you prefer to put the for(j < MAX_CLKS) after 'scpd->data =
> data' right? I can change it in next patch.
>

Ok, thanks.

Regards,
Matthias

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

[Yong Wu]

Hi Matthias,

On Fri, 2016-07-08 at 14:47 +0200, Matthias Brugger wrote:
> 
> On 06/07/16 07:22, James Liao wrote:
> > On Sat, 2016-07-02 at 18:35 +0200, Matthias Brugger wrote:
> >>
> >> On 05/16/2016 11:28 AM, James Liao wrote:
> >>> 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 <jamesjj.liao@mediatek.com>
> >>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> >>> ---
> >>>   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 5870a24..00c0adb 100644
> >>> --- a/drivers/soc/mediatek/mtk-scpsys.c
> >>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> >>> @@ -617,4 +617,21 @@ static struct platform_driver scpsys_drv = {
> >>>   		.of_match_table = of_match_ptr(of_scpsys_match_tbl),
> >>>   	},
> >>>   };
> >>> -builtin_platform_driver(scpsys_drv);
> >>> +
> >>> +static int __init scpsys_drv_init(void)
> >>> +{
> >>> +	return platform_driver_register(&scpsys_drv);
> >>> +}
> >>> +
> >>> +/*
> >>> + * 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);
> >>>
> >>
> >> Can't we achieve this with probe deferring? I'm not really keen on
> >> coding the order of the different drivers like this.
> >
> > Hi Matthias,
> >
> > Some drivers such as IOMMU don't support probe deferring. So scpsys need
> > to init before them by changing init level.
> >
> 
> SMI larbs return EPROBE_DEFER if it can't find the PM provider for it's 
> domain, so this part should not be the problem. The larbs get added as 
> components in the probe, when the PM provider is present.
> 
> The iommu driver uses the larbs as components. As long as not all 
> components are present the iommu does not bind them. So from what I see 
> this should work without any problem.
> 
> Can you please specify where the iommu dirver has a problem. Maybe we 
> need to fix the driver rather then scpsys.

We got a problem while bootup, the hang backtrace is like below(Sorry, I
can't find the full backtrace now):
 
[    7.832359] Call trace:
[    7.834778] [<ffffffc000764424>] mtk_smi_larb_get+0x24/0xa8
[    7.840300] [<ffffffc0005a1390>] mtk_drm_crtc_enable+0x6c/0x450

The reason is that "larb->mmu" is NULL while DRM call mtk_smi_larb_get.
 
DRM call iommu_present to wait for whether IOMMU is ready[1].
But in the  MTK-IOMMU, It will call
bus_set_iommu->mtk_iommu_add_device->mtk_iommu_attach_device, then it's
able to transfer "struct mtk_smi_iommu" to SMI whose probe maybe delayed
by power-domain, then SMI could finish its probe.

So If DRM probe is called after the time of calling bus_set_iommu and
before the time of SMI probe finish, this problem can be reproduced.

The root cause is that iommu_present cann't indicate that MTK-IOMMU and
SMI is ready actually. in other words, the DRM cann't detect
whether the SMI has probed done.

If we move scpsys_init from module_initcall to subsys_initcall, the
IOMMU and SMI could be probe done during the subsys_initcall period. 
this issue can be avoid.
And as we grep before, there are also some examples whose power-domain
is also not module_init[2].
core_initcall(exynos4_pm_init_power_domain);
subsys_initcall(imx_pgc_init);

If you think that this patch will hide the problem above, then I have to
add a new function, like below:
//==================
bool mtk_smi_larb_ready(struct device *larbdev)
{
	struct mtk_smi_larb *larb = dev_get_drvdata(larbdev);

	return larb && !!larb->mmu;
}
//=================

And in the probe of all the MTK-IOMMU consumer, we have to add this:
//====================
	if (!mtk_smi_larb_ready(&larb_pdev->dev))
		return -EPROBE_DEFER;
//====================

The sequence is a little complex, If I don't explain clearly, please
tell me.
Any other suggestion about this? Thanks.
 
[1]:https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.18/drivers/gpu/drm/mediatek/mtk_drm_drv.c#150 
[2]:http://lists.infradead.org/pipermail/linux-mediatek/2015-December/003416.html

> 
> Regards,
> Matthias
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Matthias Brugger]

Hi Yong,

On 07/12/2016 11:01 AM, Yong Wu wrote:
> Hi Matthias,
>
> On Fri, 2016-07-08 at 14:47 +0200, Matthias Brugger wrote:
>>
>> On 06/07/16 07:22, James Liao wrote:
>>> On Sat, 2016-07-02 at 18:35 +0200, Matthias Brugger wrote:
>>>>
>>>> On 05/16/2016 11:28 AM, James Liao wrote:
>>>>> 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 <jamesjj.liao@mediatek.com>
>>>>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>>>>> ---
>>>>>   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 5870a24..00c0adb 100644
>>>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>>>> @@ -617,4 +617,21 @@ static struct platform_driver scpsys_drv = {
>>>>>   		.of_match_table = of_match_ptr(of_scpsys_match_tbl),
>>>>>   	},
>>>>>   };
>>>>> -builtin_platform_driver(scpsys_drv);
>>>>> +
>>>>> +static int __init scpsys_drv_init(void)
>>>>> +{
>>>>> +	return platform_driver_register(&scpsys_drv);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * 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);
>>>>>
>>>>
>>>> Can't we achieve this with probe deferring? I'm not really keen on
>>>> coding the order of the different drivers like this.
>>>
>>> Hi Matthias,
>>>
>>> Some drivers such as IOMMU don't support probe deferring. So scpsys need
>>> to init before them by changing init level.
>>>
>>
>> SMI larbs return EPROBE_DEFER if it can't find the PM provider for it's
>> domain, so this part should not be the problem. The larbs get added as
>> components in the probe, when the PM provider is present.
>>
>> The iommu driver uses the larbs as components. As long as not all
>> components are present the iommu does not bind them. So from what I see
>> this should work without any problem.
>>
>> Can you please specify where the iommu dirver has a problem. Maybe we
>> need to fix the driver rather then scpsys.
>
> We got a problem while bootup, the hang backtrace is like below(Sorry, I
> can't find the full backtrace now):
>
> [    7.832359] Call trace:
> [    7.834778] [<ffffffc000764424>] mtk_smi_larb_get+0x24/0xa8
> [    7.840300] [<ffffffc0005a1390>] mtk_drm_crtc_enable+0x6c/0x450
>
> The reason is that "larb->mmu" is NULL while DRM call mtk_smi_larb_get.
>
> DRM call iommu_present to wait for whether IOMMU is ready[1].
> But in the  MTK-IOMMU, It will call
> bus_set_iommu->mtk_iommu_add_device->mtk_iommu_attach_device, then it's
> able to transfer "struct mtk_smi_iommu" to SMI whose probe maybe delayed
> by power-domain, then SMI could finish its probe.
>
> So If DRM probe is called after the time of calling bus_set_iommu and
> before the time of SMI probe finish, this problem can be reproduced.
>
> The root cause is that iommu_present cann't indicate that MTK-IOMMU and
> SMI is ready actually. in other words, the DRM cann't detect
> whether the SMI has probed done.
>
> If we move scpsys_init from module_initcall to subsys_initcall, the
> IOMMU and SMI could be probe done during the subsys_initcall period.
> this issue can be avoid.
> And as we grep before, there are also some examples whose power-domain
> is also not module_init[2].
> core_initcall(exynos4_pm_init_power_domain);
> subsys_initcall(imx_pgc_init);
>
> If you think that this patch will hide the problem above, then I have to
> add a new function, like below:
> //==================
> bool mtk_smi_larb_ready(struct device *larbdev)
> {
> 	struct mtk_smi_larb *larb = dev_get_drvdata(larbdev);
>
> 	return larb && !!larb->mmu;
> }
> //=================
>
> And in the probe of all the MTK-IOMMU consumer, we have to add this:
> //====================
> 	if (!mtk_smi_larb_ready(&larb_pdev->dev))
> 		return -EPROBE_DEFER;
> //====================
>
> The sequence is a little complex, If I don't explain clearly, please
> tell me.
> Any other suggestion about this? Thanks.

I'm still trying to find a different way to solve this. Unfortunately 
I'm not able to reproduce this on my mt8173-evb...

...but: iommu_present checkes if the bus->iommu_ops is set. If we set 
these option in the iommu after all components got initialized, we 
should be fine. Or did I miss something?

Would you mind trying the patch below (against v4.9-rc1)?
If you know of any way to reproduce this issue, I would be happy to know.

I'm adding Joerg, the iommu maintainer, maybe he has an idea.

Thanks a lot,
Matthias

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index b12c12d..9249011 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -602,10 +602,12 @@ static int mtk_iommu_probe(struct platform_device 
*pdev)
         if (ret)
                 return ret;

-       if (!iommu_present(&platform_bus_type))
+       ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, 
match);
+
+       if (ret && !iommu_present(&platform_bus_type))
                 bus_set_iommu(&platform_bus_type, &mtk_iommu_ops);

-       return component_master_add_with_match(dev, &mtk_iommu_com_ops, 
match);
+       return ret;
  }

  static int mtk_iommu_remove(struct platform_device *pdev)