[PATCH 0/7] Cleanup MediaTek clk reset drivers

[PATCH 0/7] Cleanup MediaTek clk reset drivers

[Rex-BC Chen]

In this series, we cleanup MediaTek clock reset drivers in clk/mediatek
folder. MediaTek clock reset driver is used to provide reset control
of modules controlled in clk, like infra_ao.

Rex-BC Chen (7):
  clk: mediatek: reset: Correct the logic of setting register
  clk: mediatek: reset: Rename reset function
  clk: mediatek: reset: Merge and revise reset register function
  clk: mediatek: reset: Add reset.h
  clk: mediatek: reset: Revise structure to control reset register
  clk: mediatek: reset: Add support for unregister reset controller
  clk: mediatek: reset: Add reset support for simple probe/remove

 drivers/clk/mediatek/clk-mt2701-eth.c |  15 ++-
 drivers/clk/mediatek/clk-mt2701-g3d.c |  15 ++-
 drivers/clk/mediatek/clk-mt2701-hif.c |  11 +-
 drivers/clk/mediatek/clk-mt2701.c     |  23 +++-
 drivers/clk/mediatek/clk-mt2712.c     |  31 ++++--
 drivers/clk/mediatek/clk-mt7622-eth.c |  15 ++-
 drivers/clk/mediatek/clk-mt7622-hif.c |  31 ++++--
 drivers/clk/mediatek/clk-mt7622.c     |  23 +++-
 drivers/clk/mediatek/clk-mt7629-eth.c |  15 ++-
 drivers/clk/mediatek/clk-mt7629-hif.c |  31 ++++--
 drivers/clk/mediatek/clk-mt8135.c     |  27 ++++-
 drivers/clk/mediatek/clk-mt8173.c     |  27 ++++-
 drivers/clk/mediatek/clk-mt8183.c     |  11 +-
 drivers/clk/mediatek/clk-mtk.c        |   6 +
 drivers/clk/mediatek/clk-mtk.h        |   8 +-
 drivers/clk/mediatek/reset.c          | 153 ++++++++++++++------------
 drivers/clk/mediatek/reset.h          |  32 ++++++
 17 files changed, 331 insertions(+), 143 deletions(-)
 create mode 100644 drivers/clk/mediatek/reset.h

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/7] clk: mediatek: reset: Correct the logic of setting register

[Rex-BC Chen]

Original assert/deassert bit is BIT(0), but it's more resonable to modify
them to BIT(id % 32) which is based on id.

This patch will not influence any previous driver because the reset is
only used for thermal. The id (MT8183_INFRACFG_AO_THERM_SW_RST) is 0.

Fixes: 64ebb57a3df6 ("clk: reset: Modify reset-controller driver")
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/reset.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
index bcec4b89f449..834d26e9bdfd 100644
--- a/drivers/clk/mediatek/reset.c
+++ b/drivers/clk/mediatek/reset.c
@@ -25,7 +25,7 @@ static int mtk_reset_assert_set_clr(struct reset_controller_dev *rcdev,
 	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
 	unsigned int reg = data->regofs + ((id / 32) << 4);
 
-	return regmap_write(data->regmap, reg, 1);
+	return regmap_write(data->regmap, reg, BIT(id % 32));
 }
 
 static int mtk_reset_deassert_set_clr(struct reset_controller_dev *rcdev,
@@ -34,7 +34,7 @@ static int mtk_reset_deassert_set_clr(struct reset_controller_dev *rcdev,
 	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
 	unsigned int reg = data->regofs + ((id / 32) << 4) + 0x4;
 
-	return regmap_write(data->regmap, reg, 1);
+	return regmap_write(data->regmap, reg, BIT(id % 32));
 }
 
 static int mtk_reset_assert(struct reset_controller_dev *rcdev,
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/7] clk: mediatek: reset: Correct the logic of setting register

[Chen-Yu Tsai]

Hi,

On Mon, Apr 18, 2022 at 9:22 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote:
>

The subject could be written as "Fix written reset bit offset" to make it
more specific.

> Original assert/deassert bit is BIT(0), but it's more resonable to modify
> them to BIT(id % 32) which is based on id.
>
> This patch will not influence any previous driver because the reset is
> only used for thermal. The id (MT8183_INFRACFG_AO_THERM_SW_RST) is 0.
>
> Fixes: 64ebb57a3df6 ("clk: reset: Modify reset-controller driver")
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

Otherwise,

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/7] clk: mediatek: reset: Correct the logic of setting register

[Rex-BC Chen]

On Tue, 2022-04-19 at 13:48 +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Mon, Apr 18, 2022 at 9:22 PM Rex-BC Chen <rex-bc.chen@mediatek.com
> > wrote:
> > 
> 
> The subject could be written as "Fix written reset bit offset" to
> make it
> more specific.

Hello ChenYu,

I will update the topic in next version.
Thanks for your suggestion.

BRs,
Rex

> 
> > Original assert/deassert bit is BIT(0), but it's more resonable to
> > modify
> > them to BIT(id % 32) which is based on id.
> > 
> > This patch will not influence any previous driver because the reset
> > is
> > only used for thermal. The id (MT8183_INFRACFG_AO_THERM_SW_RST) is
> > 0.
> > 
> > Fixes: 64ebb57a3df6 ("clk: reset: Modify reset-controller driver")
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> 
> Otherwise,
> 
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/7] clk: mediatek: reset: Rename reset function

[Rex-BC Chen]

There are two version for clock reset register control of MediaTek SoCs.
Since MT8183, the version 2 is adopted.

To make the driver more readable,
- Rename them to v2 for MT8183 and v1 for previous SoCs.
- Adjust the fuinction order in reset.c.

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/clk-mt2701-eth.c |  2 +-
 drivers/clk/mediatek/clk-mt2701-g3d.c |  2 +-
 drivers/clk/mediatek/clk-mt2701-hif.c |  2 +-
 drivers/clk/mediatek/clk-mt2701.c     |  4 +-
 drivers/clk/mediatek/clk-mt2712.c     |  4 +-
 drivers/clk/mediatek/clk-mt7622-eth.c |  2 +-
 drivers/clk/mediatek/clk-mt7622-hif.c |  4 +-
 drivers/clk/mediatek/clk-mt7622.c     |  4 +-
 drivers/clk/mediatek/clk-mt7629-eth.c |  2 +-
 drivers/clk/mediatek/clk-mt7629-hif.c |  4 +-
 drivers/clk/mediatek/clk-mt8135.c     |  4 +-
 drivers/clk/mediatek/clk-mt8173.c     |  4 +-
 drivers/clk/mediatek/clk-mt8183.c     |  2 +-
 drivers/clk/mediatek/clk-mtk.h        |  8 +--
 drivers/clk/mediatek/reset.c          | 76 ++++++++++++++-------------
 15 files changed, 64 insertions(+), 60 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701-eth.c b/drivers/clk/mediatek/clk-mt2701-eth.c
index 100ff6ca609e..f3e56a988177 100644
--- a/drivers/clk/mediatek/clk-mt2701-eth.c
+++ b/drivers/clk/mediatek/clk-mt2701-eth.c
@@ -58,7 +58,7 @@ static int clk_mt2701_eth_probe(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt2701-g3d.c b/drivers/clk/mediatek/clk-mt2701-g3d.c
index 1328c112a38f..5a41d4607e77 100644
--- a/drivers/clk/mediatek/clk-mt2701-g3d.c
+++ b/drivers/clk/mediatek/clk-mt2701-g3d.c
@@ -52,7 +52,7 @@ static int clk_mt2701_g3dsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0xc);
+	mtk_register_reset_controller_v1(node, 1, 0xc);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt2701-hif.c b/drivers/clk/mediatek/clk-mt2701-hif.c
index 61444881c539..426a54cb077b 100644
--- a/drivers/clk/mediatek/clk-mt2701-hif.c
+++ b/drivers/clk/mediatek/clk-mt2701-hif.c
@@ -57,7 +57,7 @@ static int clk_mt2701_hif_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return 0;
 }
diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
index 1eb3e4563c3f..0e103d6735bb 100644
--- a/drivers/clk/mediatek/clk-mt2701.c
+++ b/drivers/clk/mediatek/clk-mt2701.c
@@ -785,7 +785,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller(node, 2, 0x30);
+	mtk_register_reset_controller_v1(node, 2, 0x30);
 
 	return 0;
 }
@@ -908,7 +908,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller(node, 2, 0x0);
+	mtk_register_reset_controller_v1(node, 2, 0x0);
 
 	return 0;
 }
diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index ff72b9ab945b..e80fc89a5a07 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1361,7 +1361,7 @@ static int clk_mt2712_infra_probe(struct platform_device *pdev)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller(node, 2, 0x30);
+	mtk_register_reset_controller_v1(node, 2, 0x30);
 
 	return r;
 }
@@ -1383,7 +1383,7 @@ static int clk_mt2712_peri_probe(struct platform_device *pdev)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller(node, 2, 0);
+	mtk_register_reset_controller_v1(node, 2, 0);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7622-eth.c b/drivers/clk/mediatek/clk-mt7622-eth.c
index c9947dc7ba5a..13f346643e26 100644
--- a/drivers/clk/mediatek/clk-mt7622-eth.c
+++ b/drivers/clk/mediatek/clk-mt7622-eth.c
@@ -82,7 +82,7 @@ static int clk_mt7622_ethsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c b/drivers/clk/mediatek/clk-mt7622-hif.c
index 628be0c9f888..32855bb489a2 100644
--- a/drivers/clk/mediatek/clk-mt7622-hif.c
+++ b/drivers/clk/mediatek/clk-mt7622-hif.c
@@ -93,7 +93,7 @@ static int clk_mt7622_ssusbsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return r;
 }
@@ -115,7 +115,7 @@ static int clk_mt7622_pciesys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7622.c b/drivers/clk/mediatek/clk-mt7622.c
index 0e1fb30a1e98..953e81049957 100644
--- a/drivers/clk/mediatek/clk-mt7622.c
+++ b/drivers/clk/mediatek/clk-mt7622.c
@@ -663,7 +663,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller(node, 1, 0x30);
+	mtk_register_reset_controller_v1(node, 1, 0x30);
 
 	return 0;
 }
@@ -714,7 +714,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 
 	clk_prepare_enable(clk_data->clks[CLK_PERI_UART0_PD]);
 
-	mtk_register_reset_controller(node, 2, 0x0);
+	mtk_register_reset_controller_v1(node, 2, 0x0);
 
 	return 0;
 }
diff --git a/drivers/clk/mediatek/clk-mt7629-eth.c b/drivers/clk/mediatek/clk-mt7629-eth.c
index 88279d0ea1a7..e47b66c67079 100644
--- a/drivers/clk/mediatek/clk-mt7629-eth.c
+++ b/drivers/clk/mediatek/clk-mt7629-eth.c
@@ -92,7 +92,7 @@ static int clk_mt7629_ethsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7629-hif.c b/drivers/clk/mediatek/clk-mt7629-hif.c
index 5c5b37207afb..72f261f12e92 100644
--- a/drivers/clk/mediatek/clk-mt7629-hif.c
+++ b/drivers/clk/mediatek/clk-mt7629-hif.c
@@ -88,7 +88,7 @@ static int clk_mt7629_ssusbsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return r;
 }
@@ -110,7 +110,7 @@ static int clk_mt7629_pciesys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller(node, 1, 0x34);
+	mtk_register_reset_controller_v1(node, 1, 0x34);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt8135.c b/drivers/clk/mediatek/clk-mt8135.c
index 09ad272d51f1..d649e817b320 100644
--- a/drivers/clk/mediatek/clk-mt8135.c
+++ b/drivers/clk/mediatek/clk-mt8135.c
@@ -559,7 +559,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller(node, 2, 0x30);
+	mtk_register_reset_controller_v1(node, 2, 0x30);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8135-infracfg", mtk_infrasys_init);
 
@@ -587,7 +587,7 @@ static void __init mtk_pericfg_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller(node, 2, 0);
+	mtk_register_reset_controller_v1(node, 2, 0);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8135-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 46b7655feeaa..11becf2f7a9f 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -882,7 +882,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller(node, 2, 0x30);
+	mtk_register_reset_controller_v1(node, 2, 0x30);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8173-infracfg", mtk_infrasys_init);
 
@@ -910,7 +910,7 @@ static void __init mtk_pericfg_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller(node, 2, 0);
+	mtk_register_reset_controller_v1(node, 2, 0);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
index 68496554dd3d..ac76328c423d 100644
--- a/drivers/clk/mediatek/clk-mt8183.c
+++ b/drivers/clk/mediatek/clk-mt8183.c
@@ -1239,7 +1239,7 @@ static int clk_mt8183_infra_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	mtk_register_reset_controller_set_clr(node, 4, INFRA_RST0_SET_OFFSET);
+	mtk_register_reset_controller_v2(node, 4, INFRA_RST0_SET_OFFSET);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index bf6565aa7319..601db24a342b 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -190,11 +190,11 @@ void mtk_free_clk_data(struct clk_onecell_data *clk_data);
 struct clk *mtk_clk_register_ref2usb_tx(const char *name,
 			const char *parent_name, void __iomem *reg);
 
-void mtk_register_reset_controller(struct device_node *np,
-			unsigned int num_regs, int regofs);
+void mtk_register_reset_controller_v1(struct device_node *np,
+				      unsigned int num_regs, int regofs);
 
-void mtk_register_reset_controller_set_clr(struct device_node *np,
-	unsigned int num_regs, int regofs);
+void mtk_register_reset_controller_v2(struct device_node *np,
+				      unsigned int num_regs, int regofs);
 
 struct mtk_clk_desc {
 	const struct mtk_gate *clks;
diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
index 834d26e9bdfd..b1012b0c44d0 100644
--- a/drivers/clk/mediatek/reset.c
+++ b/drivers/clk/mediatek/reset.c
@@ -19,75 +19,79 @@ struct mtk_reset {
 	struct reset_controller_dev rcdev;
 };
 
-static int mtk_reset_assert_set_clr(struct reset_controller_dev *rcdev,
-	unsigned long id)
+static int mtk_reset_assert_v1(struct reset_controller_dev *rcdev,
+			       unsigned long id)
 {
 	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
-	unsigned int reg = data->regofs + ((id / 32) << 4);
 
-	return regmap_write(data->regmap, reg, BIT(id % 32));
+	return regmap_update_bits(data->regmap,
+				  data->regofs + ((id / 32) << 2),
+				  BIT(id % 32), ~0);
 }
 
-static int mtk_reset_deassert_set_clr(struct reset_controller_dev *rcdev,
-	unsigned long id)
+static int mtk_reset_deassert_v1(struct reset_controller_dev *rcdev,
+				 unsigned long id)
 {
 	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
-	unsigned int reg = data->regofs + ((id / 32) << 4) + 0x4;
 
-	return regmap_write(data->regmap, reg, BIT(id % 32));
+	return regmap_update_bits(data->regmap,
+				  data->regofs + ((id / 32) << 2),
+				  BIT(id % 32), 0);
 }
 
-static int mtk_reset_assert(struct reset_controller_dev *rcdev,
-			      unsigned long id)
+static int mtk_reset_assert_v2(struct reset_controller_dev *rcdev,
+			       unsigned long id)
 {
 	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
 
-	return regmap_update_bits(data->regmap, data->regofs + ((id / 32) << 2),
-			BIT(id % 32), ~0);
+	return regmap_write(data->regmap,
+			    data->regofs + ((id / 32) << 4),
+			    BIT(id % 32));
 }
 
-static int mtk_reset_deassert(struct reset_controller_dev *rcdev,
-				unsigned long id)
+static int mtk_reset_deassert_v2(struct reset_controller_dev *rcdev,
+				 unsigned long id)
 {
 	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
 
-	return regmap_update_bits(data->regmap, data->regofs + ((id / 32) << 2),
-			BIT(id % 32), 0);
+	return regmap_write(data->regmap,
+			    data->regofs + ((id / 32) << 4) + 0x4,
+			    BIT(id % 32));
 }
 
-static int mtk_reset(struct reset_controller_dev *rcdev,
-			      unsigned long id)
+static int mtk_reset_v1(struct reset_controller_dev *rcdev,
+			unsigned long id)
 {
 	int ret;
 
-	ret = mtk_reset_assert(rcdev, id);
+	ret = mtk_reset_assert_v1(rcdev, id);
 	if (ret)
 		return ret;
 
-	return mtk_reset_deassert(rcdev, id);
+	return mtk_reset_deassert_v1(rcdev, id);
 }
 
-static int mtk_reset_set_clr(struct reset_controller_dev *rcdev,
-	unsigned long id)
+static int mtk_reset_v2(struct reset_controller_dev *rcdev,
+			unsigned long id)
 {
 	int ret;
 
-	ret = mtk_reset_assert_set_clr(rcdev, id);
+	ret = mtk_reset_assert_v2(rcdev, id);
 	if (ret)
 		return ret;
-	return mtk_reset_deassert_set_clr(rcdev, id);
+	return mtk_reset_deassert_v2(rcdev, id);
 }
 
-static const struct reset_control_ops mtk_reset_ops = {
-	.assert = mtk_reset_assert,
-	.deassert = mtk_reset_deassert,
-	.reset = mtk_reset,
+static const struct reset_control_ops mtk_reset_ops_v1 = {
+	.assert = mtk_reset_assert_v1,
+	.deassert = mtk_reset_deassert_v1,
+	.reset = mtk_reset_v1,
 };
 
-static const struct reset_control_ops mtk_reset_ops_set_clr = {
-	.assert = mtk_reset_assert_set_clr,
-	.deassert = mtk_reset_deassert_set_clr,
-	.reset = mtk_reset_set_clr,
+static const struct reset_control_ops mtk_reset_ops_v2 = {
+	.assert = mtk_reset_assert_v2,
+	.deassert = mtk_reset_deassert_v2,
+	.reset = mtk_reset_v2,
 };
 
 static void mtk_register_reset_controller_common(struct device_node *np,
@@ -123,18 +127,18 @@ static void mtk_register_reset_controller_common(struct device_node *np,
 	}
 }
 
-void mtk_register_reset_controller(struct device_node *np,
+void mtk_register_reset_controller_v1(struct device_node *np,
 	unsigned int num_regs, int regofs)
 {
 	mtk_register_reset_controller_common(np, num_regs, regofs,
-		&mtk_reset_ops);
+					     &mtk_reset_ops_v1);
 }
 
-void mtk_register_reset_controller_set_clr(struct device_node *np,
+void mtk_register_reset_controller_v2(struct device_node *np,
 	unsigned int num_regs, int regofs)
 {
 	mtk_register_reset_controller_common(np, num_regs, regofs,
-		&mtk_reset_ops_set_clr);
+					     &mtk_reset_ops_v2);
 }
 
 MODULE_LICENSE("GPL");
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] clk: mediatek: reset: Rename reset function

[Chen-Yu Tsai]

On Mon, Apr 18, 2022 at 9:22 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote:
>
> There are two version for clock reset register control of MediaTek SoCs.
> Since MT8183, the version 2 is adopted.
>
> To make the driver more readable,
> - Rename them to v2 for MT8183 and v1 for previous SoCs.
> - Adjust the fuinction order in reset.c.

I'm not sure that the renaming actually helps, since it is not given that
people outside of MediaTek would know which chip use which version. The
original name of "_set_clr" at least relays how the hardware works, which
coupled with the register naming in the datasheets make it quite obvious
if the hardware is using the "set/clr" variant.

On a different note, the v1 hardware, where a hardware bit represents the
state, is quite common, and there is a common reset driver that handles it.
Perhaps that could be reused instead of code duplicated?
See drivers/reset/reset-simple.c.


Thanks
ChenYu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] clk: mediatek: reset: Rename reset function

[Rex-BC Chen]

On Tue, 2022-04-19 at 13:57 +0800, Chen-Yu Tsai wrote:
> On Mon, Apr 18, 2022 at 9:22 PM Rex-BC Chen <rex-bc.chen@mediatek.com
> > wrote:
> > 
> > There are two version for clock reset register control of MediaTek
> > SoCs.
> > Since MT8183, the version 2 is adopted.
> > 
> > To make the driver more readable,
> > - Rename them to v2 for MT8183 and v1 for previous SoCs.
> > - Adjust the fuinction order in reset.c.
> 
> I'm not sure that the renaming actually helps, since it is not given
> that
> people outside of MediaTek would know which chip use which version.
> The
> original name of "_set_clr" at least relays how the hardware works,
> which
> coupled with the register naming in the datasheets make it quite
> obvious
> if the hardware is using the "set/clr" variant.
> 
> On a different note, the v1 hardware, where a hardware bit represents
> the
> state, is quite common, and there is a common reset driver that
> handles it.
> Perhaps that could be reused instead of code duplicated?
> See drivers/reset/reset-simple.c.
> 
> 
> Thanks
> ChenYu

Hello ChenYu,

Thanks for your review.
I will use reset_simple_ops to replace v1 in next version.

What do you think the proper name of v2?
Or I just use mtk_reset_assert for v2?

BRs,
Rex


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] clk: mediatek: reset: Rename reset function

[Chen-Yu Tsai]

On Tue, Apr 19, 2022 at 2:37 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote:
>
> On Tue, 2022-04-19 at 13:57 +0800, Chen-Yu Tsai wrote:
> > On Mon, Apr 18, 2022 at 9:22 PM Rex-BC Chen <rex-bc.chen@mediatek.com
> > > wrote:
> > >
> > > There are two version for clock reset register control of MediaTek
> > > SoCs.
> > > Since MT8183, the version 2 is adopted.
> > >
> > > To make the driver more readable,
> > > - Rename them to v2 for MT8183 and v1 for previous SoCs.
> > > - Adjust the fuinction order in reset.c.
> >
> > I'm not sure that the renaming actually helps, since it is not given
> > that
> > people outside of MediaTek would know which chip use which version.
> > The
> > original name of "_set_clr" at least relays how the hardware works,
> > which
> > coupled with the register naming in the datasheets make it quite
> > obvious
> > if the hardware is using the "set/clr" variant.
> >
> > On a different note, the v1 hardware, where a hardware bit represents
> > the
> > state, is quite common, and there is a common reset driver that
> > handles it.
> > Perhaps that could be reused instead of code duplicated?
> > See drivers/reset/reset-simple.c.
> >
> >
> > Thanks
> > ChenYu
>
> Hello ChenYu,
>
> Thanks for your review.
> I will use reset_simple_ops to replace v1 in next version.
>
> What do you think the proper name of v2?
> Or I just use mtk_reset_assert for v2?

I would just keep the current name.


ChenYu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/7] clk: mediatek: reset: Merge and revise reset register function

[Rex-BC Chen]

Merge the reset register function of v1 and v2 into one function.
- Input the version number to determine which version we will use.
- Add return value of reset register function for error handling.
- Rename reset register function to "mtk_clk_register_rst_ctrl"

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/clk-mt2701-eth.c |  8 +++---
 drivers/clk/mediatek/clk-mt2701-g3d.c |  8 +++---
 drivers/clk/mediatek/clk-mt2701-hif.c |  4 +--
 drivers/clk/mediatek/clk-mt2701.c     |  8 ++----
 drivers/clk/mediatek/clk-mt2712.c     | 16 +++++------
 drivers/clk/mediatek/clk-mt7622-eth.c |  8 +++---
 drivers/clk/mediatek/clk-mt7622-hif.c | 16 +++++------
 drivers/clk/mediatek/clk-mt7622.c     |  8 ++----
 drivers/clk/mediatek/clk-mt7629-eth.c |  8 +++---
 drivers/clk/mediatek/clk-mt7629-hif.c | 16 +++++------
 drivers/clk/mediatek/clk-mt8135.c     | 12 +++++---
 drivers/clk/mediatek/clk-mt8173.c     | 12 +++++---
 drivers/clk/mediatek/clk-mt8183.c     |  5 ++--
 drivers/clk/mediatek/clk-mtk.h        | 13 +++++----
 drivers/clk/mediatek/reset.c          | 41 +++++++++++++--------------
 15 files changed, 90 insertions(+), 93 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701-eth.c b/drivers/clk/mediatek/clk-mt2701-eth.c
index f3e56a988177..71257714e6a6 100644
--- a/drivers/clk/mediatek/clk-mt2701-eth.c
+++ b/drivers/clk/mediatek/clk-mt2701-eth.c
@@ -53,14 +53,14 @@ static int clk_mt2701_eth_probe(struct platform_device *pdev)
 						clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static struct platform_driver clk_mt2701_eth_drv = {
diff --git a/drivers/clk/mediatek/clk-mt2701-g3d.c b/drivers/clk/mediatek/clk-mt2701-g3d.c
index 5a41d4607e77..f878ffba48a9 100644
--- a/drivers/clk/mediatek/clk-mt2701-g3d.c
+++ b/drivers/clk/mediatek/clk-mt2701-g3d.c
@@ -47,14 +47,14 @@ static int clk_mt2701_g3dsys_init(struct platform_device *pdev)
 			       clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0xc);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0xc, MTK_RST_V1);
 }
 
 static const struct of_device_id of_match_clk_mt2701_g3d[] = {
diff --git a/drivers/clk/mediatek/clk-mt2701-hif.c b/drivers/clk/mediatek/clk-mt2701-hif.c
index 426a54cb077b..ee53b227e99a 100644
--- a/drivers/clk/mediatek/clk-mt2701-hif.c
+++ b/drivers/clk/mediatek/clk-mt2701-hif.c
@@ -57,9 +57,7 @@ static int clk_mt2701_hif_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return 0;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static struct platform_driver clk_mt2701_hif_drv = {
diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
index 0e103d6735bb..c1100a20c7ed 100644
--- a/drivers/clk/mediatek/clk-mt2701.c
+++ b/drivers/clk/mediatek/clk-mt2701.c
@@ -785,9 +785,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller_v1(node, 2, 0x30);
-
-	return 0;
+	return mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
 }
 
 static const struct mtk_gate_regs peri0_cg_regs = {
@@ -908,9 +906,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller_v1(node, 2, 0x0);
-
-	return 0;
+	return mtk_clk_register_rst_ctrl(node, 2, 0x0, MTK_RST_V1);
 }
 
 #define MT8590_PLL_FMAX		(2000 * MHZ)
diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index e80fc89a5a07..a8cfa85f06a1 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1357,13 +1357,13 @@ static int clk_mt2712_infra_probe(struct platform_device *pdev)
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
 
-	if (r != 0)
+	if (r != 0) {
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 2, 0x30);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
 }
 
 static int clk_mt2712_peri_probe(struct platform_device *pdev)
@@ -1379,13 +1379,13 @@ static int clk_mt2712_peri_probe(struct platform_device *pdev)
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
 
-	if (r != 0)
+	if (r != 0) {
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 2, 0);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_V1);
 }
 
 static int clk_mt2712_mcu_probe(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/clk-mt7622-eth.c b/drivers/clk/mediatek/clk-mt7622-eth.c
index 13f346643e26..33359250fab3 100644
--- a/drivers/clk/mediatek/clk-mt7622-eth.c
+++ b/drivers/clk/mediatek/clk-mt7622-eth.c
@@ -77,14 +77,14 @@ static int clk_mt7622_ethsys_init(struct platform_device *pdev)
 			       clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static int clk_mt7622_sgmiisys_init(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c b/drivers/clk/mediatek/clk-mt7622-hif.c
index 32855bb489a2..6be894299ae3 100644
--- a/drivers/clk/mediatek/clk-mt7622-hif.c
+++ b/drivers/clk/mediatek/clk-mt7622-hif.c
@@ -88,14 +88,14 @@ static int clk_mt7622_ssusbsys_init(struct platform_device *pdev)
 			       clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static int clk_mt7622_pciesys_init(struct platform_device *pdev)
@@ -110,14 +110,14 @@ static int clk_mt7622_pciesys_init(struct platform_device *pdev)
 			       clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static const struct of_device_id of_match_clk_mt7622_hif[] = {
diff --git a/drivers/clk/mediatek/clk-mt7622.c b/drivers/clk/mediatek/clk-mt7622.c
index 953e81049957..e8387df38798 100644
--- a/drivers/clk/mediatek/clk-mt7622.c
+++ b/drivers/clk/mediatek/clk-mt7622.c
@@ -663,9 +663,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller_v1(node, 1, 0x30);
-
-	return 0;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x30, MTK_RST_V1);
 }
 
 static int mtk_apmixedsys_init(struct platform_device *pdev)
@@ -714,9 +712,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 
 	clk_prepare_enable(clk_data->clks[CLK_PERI_UART0_PD]);
 
-	mtk_register_reset_controller_v1(node, 2, 0x0);
-
-	return 0;
+	return mtk_clk_register_rst_ctrl(node, 2, 0x0, MTK_RST_V1);
 }
 
 static const struct of_device_id of_match_clk_mt7622[] = {
diff --git a/drivers/clk/mediatek/clk-mt7629-eth.c b/drivers/clk/mediatek/clk-mt7629-eth.c
index e47b66c67079..bf1791efe05d 100644
--- a/drivers/clk/mediatek/clk-mt7629-eth.c
+++ b/drivers/clk/mediatek/clk-mt7629-eth.c
@@ -87,14 +87,14 @@ static int clk_mt7629_ethsys_init(struct platform_device *pdev)
 	mtk_clk_register_gates(node, eth_clks, CLK_ETH_NR_CLK, clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static int clk_mt7629_sgmiisys_init(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/clk-mt7629-hif.c b/drivers/clk/mediatek/clk-mt7629-hif.c
index 72f261f12e92..23d22c69d47b 100644
--- a/drivers/clk/mediatek/clk-mt7629-hif.c
+++ b/drivers/clk/mediatek/clk-mt7629-hif.c
@@ -83,14 +83,14 @@ static int clk_mt7629_ssusbsys_init(struct platform_device *pdev)
 			       clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static int clk_mt7629_pciesys_init(struct platform_device *pdev)
@@ -105,14 +105,14 @@ static int clk_mt7629_pciesys_init(struct platform_device *pdev)
 			       clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
+		return r;
+	}
 
-	mtk_register_reset_controller_v1(node, 1, 0x34);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
 }
 
 static const struct of_device_id of_match_clk_mt7629_hif[] = {
diff --git a/drivers/clk/mediatek/clk-mt8135.c b/drivers/clk/mediatek/clk-mt8135.c
index d649e817b320..68ce0866c5b7 100644
--- a/drivers/clk/mediatek/clk-mt8135.c
+++ b/drivers/clk/mediatek/clk-mt8135.c
@@ -555,11 +555,13 @@ static void __init mtk_infrasys_init(struct device_node *node)
 	clk_prepare_enable(clk_data->clks[CLK_INFRA_M4U]);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+		return;
+	}
 
-	mtk_register_reset_controller_v1(node, 2, 0x30);
+	r = mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8135-infracfg", mtk_infrasys_init);
 
@@ -583,11 +585,13 @@ static void __init mtk_pericfg_init(struct device_node *node)
 			&mt8135_clk_lock, clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+		return;
+	}
 
-	mtk_register_reset_controller_v1(node, 2, 0);
+	r = mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_V1);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8135-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 11becf2f7a9f..fd56dbe8ff95 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -878,11 +878,13 @@ static void __init mtk_infrasys_init(struct device_node *node)
 				  clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+		return;
+	}
 
-	mtk_register_reset_controller_v1(node, 2, 0x30);
+	r = mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8173-infracfg", mtk_infrasys_init);
 
@@ -906,11 +908,13 @@ static void __init mtk_pericfg_init(struct device_node *node)
 			&mt8173_clk_lock, clk_data);
 
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
+	if (r) {
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+		return;
+	}
 
-	mtk_register_reset_controller_v1(node, 2, 0);
+	r = mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_V1);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
index ac76328c423d..e796af76f960 100644
--- a/drivers/clk/mediatek/clk-mt8183.c
+++ b/drivers/clk/mediatek/clk-mt8183.c
@@ -1239,9 +1239,8 @@ static int clk_mt8183_infra_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	mtk_register_reset_controller_v2(node, 4, INFRA_RST0_SET_OFFSET);
-
-	return r;
+	return mtk_clk_register_rst_ctrl(node, 4,
+					 INFRA_RST0_SET_OFFSET, MTK_RST_V2);
 }
 
 static int clk_mt8183_peri_probe(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index 601db24a342b..dafdf30fe94e 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -178,6 +178,12 @@ struct mtk_clk_divider {
 		.div_width = _width,				\
 }
 
+enum mtk_reset_version {
+	MTK_RST_V1 = 0,
+	MTK_RST_V2,
+	MTK_RST_MAX,
+};
+
 int mtk_clk_register_dividers(const struct mtk_clk_divider *mcds, int num,
 			      void __iomem *base, spinlock_t *lock,
 			      struct clk_onecell_data *clk_data);
@@ -190,11 +196,8 @@ void mtk_free_clk_data(struct clk_onecell_data *clk_data);
 struct clk *mtk_clk_register_ref2usb_tx(const char *name,
 			const char *parent_name, void __iomem *reg);
 
-void mtk_register_reset_controller_v1(struct device_node *np,
-				      unsigned int num_regs, int regofs);
-
-void mtk_register_reset_controller_v2(struct device_node *np,
-				      unsigned int num_regs, int regofs);
+int mtk_clk_register_rst_ctrl(struct device_node *np,
+			      u32 reg_num, u16 reg_ofs, u8 version);
 
 struct mtk_clk_desc {
 	const struct mtk_gate *clks;
diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
index b1012b0c44d0..2a55e8bf6b28 100644
--- a/drivers/clk/mediatek/reset.c
+++ b/drivers/clk/mediatek/reset.c
@@ -94,51 +94,48 @@ static const struct reset_control_ops mtk_reset_ops_v2 = {
 	.reset = mtk_reset_v2,
 };
 
-static void mtk_register_reset_controller_common(struct device_node *np,
-			unsigned int num_regs, int regofs,
-			const struct reset_control_ops *reset_ops)
+static const struct reset_control_ops *rst_op[MTK_RST_MAX] = {
+	[MTK_RST_V1] = &mtk_reset_ops_v1,
+	[MTK_RST_V2] = &mtk_reset_ops_v2,
+};
+
+int mtk_clk_register_rst_ctrl(struct device_node *np,
+			      u32 reg_num, u16 reg_ofs, u8 version)
 {
 	struct mtk_reset *data;
 	int ret;
 	struct regmap *regmap;
 
+	if (version >= MTK_RST_MAX) {
+		pr_err("Error version number: %d\n", version);
+		return -EINVAL;
+	}
+
 	regmap = device_node_to_regmap(np);
 	if (IS_ERR(regmap)) {
 		pr_err("Cannot find regmap for %pOF: %pe\n", np, regmap);
-		return;
+		return -EINVAL;
 	}
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
-		return;
+		return -ENOMEM;
 
 	data->regmap = regmap;
-	data->regofs = regofs;
+	data->regofs = reg_ofs;
 	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = num_regs * 32;
-	data->rcdev.ops = reset_ops;
+	data->rcdev.nr_resets = reg_num * 32;
+	data->rcdev.ops = rst_op[version];
 	data->rcdev.of_node = np;
 
 	ret = reset_controller_register(&data->rcdev);
 	if (ret) {
 		pr_err("could not register reset controller: %d\n", ret);
 		kfree(data);
-		return;
+		return -EINVAL;
 	}
-}
 
-void mtk_register_reset_controller_v1(struct device_node *np,
-	unsigned int num_regs, int regofs)
-{
-	mtk_register_reset_controller_common(np, num_regs, regofs,
-					     &mtk_reset_ops_v1);
-}
-
-void mtk_register_reset_controller_v2(struct device_node *np,
-	unsigned int num_regs, int regofs)
-{
-	mtk_register_reset_controller_common(np, num_regs, regofs,
-					     &mtk_reset_ops_v2);
+	return 0;
 }
 
 MODULE_LICENSE("GPL");
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] clk: mediatek: reset: Merge and revise reset register function

[Chen-Yu Tsai]

On Mon, Apr 18, 2022 at 9:22 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote:
>
> Merge the reset register function of v1 and v2 into one function.
> - Input the version number to determine which version we will use.
> - Add return value of reset register function for error handling.

Please do this in a separate patch. Also you are adding error messages
for when of_clk_add_provider() fails, which is not mentioned either.

Most of these platform drivers require a good cleanup of their error
handling anyway. It might be a good idea to tackle them later. Or if
they look the same, you could try using coccinelle to do a bulk cleanup.


Thanks
ChenYu

> - Rename reset register function to "mtk_clk_register_rst_ctrl"

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] clk: mediatek: reset: Merge and revise reset register function

[Rex-BC Chen]

On Tue, 2022-04-19 at 15:26 +0800, Chen-Yu Tsai wrote:
> On Mon, Apr 18, 2022 at 9:22 PM Rex-BC Chen <rex-bc.chen@mediatek.com
> > wrote:
> > 
> > Merge the reset register function of v1 and v2 into one function.
> > - Input the version number to determine which version we will use.
> > - Add return value of reset register function for error handling.
> 
> Please do this in a separate patch. Also you are adding error
> messages
> for when of_clk_add_provider() fails, which is not mentioned either.
> 
> Most of these platform drivers require a good cleanup of their error
> handling anyway. It might be a good idea to tackle them later. Or if
> they look the same, you could try using coccinelle to do a bulk
> cleanup.
> 
> 
> Thanks
> ChenYu

Hello ChenYu,

I will separate the error handle in another patch.
I think I will just add a return value. As for cleanup the clock error 
handling flow, it's not the purpose of this series.

BRs,
Rex
> 
> > - Rename reset register function to "mtk_clk_register_rst_ctrl"


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/7] clk: mediatek: reset: Add reset.h

[Rex-BC Chen]

Add a new file "reset.h" to place some definitions for clock reset.

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/clk-mtk.h | 10 +---------
 drivers/clk/mediatek/reset.h   | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 9 deletions(-)
 create mode 100644 drivers/clk/mediatek/reset.h

diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index dafdf30fe94e..dfb0549ceb6c 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <reset.h>
 
 #define MAX_MUX_GATE_BIT	31
 #define INVALID_MUX_GATE_BIT	(MAX_MUX_GATE_BIT + 1)
@@ -178,12 +179,6 @@ struct mtk_clk_divider {
 		.div_width = _width,				\
 }
 
-enum mtk_reset_version {
-	MTK_RST_V1 = 0,
-	MTK_RST_V2,
-	MTK_RST_MAX,
-};
-
 int mtk_clk_register_dividers(const struct mtk_clk_divider *mcds, int num,
 			      void __iomem *base, spinlock_t *lock,
 			      struct clk_onecell_data *clk_data);
@@ -196,9 +191,6 @@ void mtk_free_clk_data(struct clk_onecell_data *clk_data);
 struct clk *mtk_clk_register_ref2usb_tx(const char *name,
 			const char *parent_name, void __iomem *reg);
 
-int mtk_clk_register_rst_ctrl(struct device_node *np,
-			      u32 reg_num, u16 reg_ofs, u8 version);
-
 struct mtk_clk_desc {
 	const struct mtk_gate *clks;
 	size_t num_clks;
diff --git a/drivers/clk/mediatek/reset.h b/drivers/clk/mediatek/reset.h
new file mode 100644
index 000000000000..0af77531b918
--- /dev/null
+++ b/drivers/clk/mediatek/reset.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022 MediaTek Inc.
+ */
+
+#ifndef __DRV_CLK_MTK_RESET_H
+#define __DRV_CLK_MTK_RESET_H
+
+#include <linux/types.h>
+
+enum mtk_reset_version {
+	MTK_RST_V1 = 0,
+	MTK_RST_V2,
+	MTK_RST_MAX,
+};
+
+int mtk_clk_register_rst_ctrl(struct device_node *np,
+			      u32 reg_num, u16 reg_ofs, u8 version);
+
+#endif /* __DRV_CLK_MTK_RESET_H */
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] clk: mediatek: reset: Add reset.h

[AngeloGioacchino Del Regno]

Il 18/04/22 15:21, Rex-BC Chen ha scritto:
> Add a new file "reset.h" to place some definitions for clock reset.
> 
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/clk/mediatek/clk-mtk.h | 10 +---------
>   drivers/clk/mediatek/reset.h   | 20 ++++++++++++++++++++
>   2 files changed, 21 insertions(+), 9 deletions(-)
>   create mode 100644 drivers/clk/mediatek/reset.h
> 
> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> index dafdf30fe94e..dfb0549ceb6c 100644
> --- a/drivers/clk/mediatek/clk-mtk.h
> +++ b/drivers/clk/mediatek/clk-mtk.h
> @@ -12,6 +12,7 @@
>   #include <linux/kernel.h>
>   #include <linux/spinlock.h>
>   #include <linux/types.h>
> +#include <reset.h>

This should be

#include "reset.h"

...since the header is in the same directory.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] clk: mediatek: reset: Add reset.h

[Rex-BC Chen]

On Tue, 2022-04-19 at 12:46 +0200, AngeloGioacchino Del Regno wrote:
> Il 18/04/22 15:21, Rex-BC Chen ha scritto:
> > Add a new file "reset.h" to place some definitions for clock reset.
> > 
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > ---
> >   drivers/clk/mediatek/clk-mtk.h | 10 +---------
> >   drivers/clk/mediatek/reset.h   | 20 ++++++++++++++++++++
> >   2 files changed, 21 insertions(+), 9 deletions(-)
> >   create mode 100644 drivers/clk/mediatek/reset.h
> > 
> > diff --git a/drivers/clk/mediatek/clk-mtk.h
> > b/drivers/clk/mediatek/clk-mtk.h
> > index dafdf30fe94e..dfb0549ceb6c 100644
> > --- a/drivers/clk/mediatek/clk-mtk.h
> > +++ b/drivers/clk/mediatek/clk-mtk.h
> > @@ -12,6 +12,7 @@
> >   #include <linux/kernel.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/types.h>
> > +#include <reset.h>
> 
> This should be
> 
> #include "reset.h"
> 
> ...since the header is in the same directory.
> 
> 
Hello Angelo,

I will fix this in next version.
Thanks!

BRs,
Rex


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/7] clk: mediatek: reset: Revise structure to control reset register

[Rex-BC Chen]

Replace the structure "struct mtk_reset" to reset.h, and rename it as
"mtk_clk_rst_data". We use it to input the resset register data and
store reset controller device.

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/clk-mt2701-eth.c |  9 +++-
 drivers/clk/mediatek/clk-mt2701-g3d.c |  9 +++-
 drivers/clk/mediatek/clk-mt2701-hif.c |  9 +++-
 drivers/clk/mediatek/clk-mt2701.c     | 19 +++++++-
 drivers/clk/mediatek/clk-mt2712.c     | 19 +++++++-
 drivers/clk/mediatek/clk-mt7622-eth.c |  9 +++-
 drivers/clk/mediatek/clk-mt7622-hif.c | 19 +++++++-
 drivers/clk/mediatek/clk-mt7622.c     | 19 +++++++-
 drivers/clk/mediatek/clk-mt7629-eth.c |  9 +++-
 drivers/clk/mediatek/clk-mt7629-hif.c | 19 +++++++-
 drivers/clk/mediatek/clk-mt8135.c     | 19 +++++++-
 drivers/clk/mediatek/clk-mt8173.c     | 19 +++++++-
 drivers/clk/mediatek/clk-mt8183.c     | 10 ++++-
 drivers/clk/mediatek/reset.c          | 62 +++++++++++++--------------
 drivers/clk/mediatek/reset.h          | 12 +++++-
 15 files changed, 209 insertions(+), 53 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701-eth.c b/drivers/clk/mediatek/clk-mt2701-eth.c
index 71257714e6a6..16fc4a50d513 100644
--- a/drivers/clk/mediatek/clk-mt2701-eth.c
+++ b/drivers/clk/mediatek/clk-mt2701-eth.c
@@ -36,6 +36,13 @@ static const struct mtk_gate eth_clks[] = {
 	GATE_ETH(CLK_ETHSYS_CRYPTO, "crypto_clk", "ethif_sel", 29),
 };
 
+static struct mtk_clk_rst_data clk_rst_data = {
+	.supported = true,
+	.version = MTK_RST_V1,
+	.reg_num = 1,
+	.reg_ofs = 0x34,
+};
+
 static const struct of_device_id of_match_clk_mt2701_eth[] = {
 	{ .compatible = "mediatek,mt2701-ethsys", },
 	{}
@@ -60,7 +67,7 @@ static int clk_mt2701_eth_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data);
 }
 
 static struct platform_driver clk_mt2701_eth_drv = {
diff --git a/drivers/clk/mediatek/clk-mt2701-g3d.c b/drivers/clk/mediatek/clk-mt2701-g3d.c
index f878ffba48a9..bdaa210813f4 100644
--- a/drivers/clk/mediatek/clk-mt2701-g3d.c
+++ b/drivers/clk/mediatek/clk-mt2701-g3d.c
@@ -35,6 +35,13 @@ static const struct mtk_gate g3d_clks[] = {
 	GATE_G3D(CLK_G3DSYS_CORE, "g3d_core", "mfg_sel", 0),
 };
 
+static struct mtk_clk_rst_data clk_rst_data = {
+	.supported = true,
+	.version = MTK_RST_V1,
+	.reg_num = 1,
+	.reg_ofs = 0xc,
+};
+
 static int clk_mt2701_g3dsys_init(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -54,7 +61,7 @@ static int clk_mt2701_g3dsys_init(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0xc, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data);
 }
 
 static const struct of_device_id of_match_clk_mt2701_g3d[] = {
diff --git a/drivers/clk/mediatek/clk-mt2701-hif.c b/drivers/clk/mediatek/clk-mt2701-hif.c
index ee53b227e99a..b79cf799d8f6 100644
--- a/drivers/clk/mediatek/clk-mt2701-hif.c
+++ b/drivers/clk/mediatek/clk-mt2701-hif.c
@@ -33,6 +33,13 @@ static const struct mtk_gate hif_clks[] = {
 	GATE_HIF(CLK_HIFSYS_PCIE2, "pcie2_clk", "ethpll_500m_ck", 26),
 };
 
+static struct mtk_clk_rst_data clk_rst_data = {
+	.supported = true,
+	.version = MTK_RST_V1,
+	.reg_num = 1,
+	.reg_ofs = 0x34,
+};
+
 static const struct of_device_id of_match_clk_mt2701_hif[] = {
 	{ .compatible = "mediatek,mt2701-hifsys", },
 	{}
@@ -57,7 +64,7 @@ static int clk_mt2701_hif_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data);
 }
 
 static struct platform_driver clk_mt2701_hif_drv = {
diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
index c1100a20c7ed..a8ac3d11ced6 100644
--- a/drivers/clk/mediatek/clk-mt2701.c
+++ b/drivers/clk/mediatek/clk-mt2701.c
@@ -735,6 +735,21 @@ static const struct mtk_fixed_factor infra_fixed_divs[] = {
 	FACTOR(CLK_INFRA_CLK_13M, "clk13m", "clk26m", 1, 2),
 };
 
+static struct mtk_clk_rst_data clk_rst_data[] = {
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x30,
+	},
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x0,
+	},
+};
+
 static struct clk_onecell_data *infra_clk_data;
 
 static void __init mtk_infrasys_init_early(struct device_node *node)
@@ -785,7 +800,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	return mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[0]);
 }
 
 static const struct mtk_gate_regs peri0_cg_regs = {
@@ -906,7 +921,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	return mtk_clk_register_rst_ctrl(node, 2, 0x0, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[1]);
 }
 
 #define MT8590_PLL_FMAX		(2000 * MHZ)
diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index a8cfa85f06a1..cc92a54d84ce 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1258,6 +1258,21 @@ static const struct mtk_pll_data plls[] = {
 		0, 31, 0x0300, 4, 0, 0, 0, 0x0304, 0),
 };
 
+static struct mtk_clk_rst_data clk_rst_data[] = {
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x30,
+	},
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x0,
+	},
+};
+
 static int clk_mt2712_apmixed_probe(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -1363,7 +1378,7 @@ static int clk_mt2712_infra_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[0]);
 }
 
 static int clk_mt2712_peri_probe(struct platform_device *pdev)
@@ -1385,7 +1400,7 @@ static int clk_mt2712_peri_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[1]);
 }
 
 static int clk_mt2712_mcu_probe(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/clk-mt7622-eth.c b/drivers/clk/mediatek/clk-mt7622-eth.c
index 33359250fab3..2d5e00a3ae5b 100644
--- a/drivers/clk/mediatek/clk-mt7622-eth.c
+++ b/drivers/clk/mediatek/clk-mt7622-eth.c
@@ -65,6 +65,13 @@ static const struct mtk_gate sgmii_clks[] = {
 		   "ssusb_cdr_fb", 5),
 };
 
+static struct mtk_clk_rst_data clk_rst_data = {
+	.supported = true,
+	.version = MTK_RST_V1,
+	.reg_num = 1,
+	.reg_ofs = 0x34,
+};
+
 static int clk_mt7622_ethsys_init(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -84,7 +91,7 @@ static int clk_mt7622_ethsys_init(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data);
 }
 
 static int clk_mt7622_sgmiisys_init(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c b/drivers/clk/mediatek/clk-mt7622-hif.c
index 6be894299ae3..a10ecc5b88a1 100644
--- a/drivers/clk/mediatek/clk-mt7622-hif.c
+++ b/drivers/clk/mediatek/clk-mt7622-hif.c
@@ -76,6 +76,21 @@ static const struct mtk_gate pcie_clks[] = {
 	GATE_PCIE(CLK_SATA_PM_EN, "sata_pm_en", "univpll2_d4", 30),
 };
 
+static struct mtk_clk_rst_data clk_rst_data[] = {
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 1,
+		.reg_ofs = 0x34,
+	},
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 1,
+		.reg_ofs = 0x34,
+	},
+};
+
 static int clk_mt7622_ssusbsys_init(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -95,7 +110,7 @@ static int clk_mt7622_ssusbsys_init(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[0]);
 }
 
 static int clk_mt7622_pciesys_init(struct platform_device *pdev)
@@ -117,7 +132,7 @@ static int clk_mt7622_pciesys_init(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[1]);
 }
 
 static const struct of_device_id of_match_clk_mt7622_hif[] = {
diff --git a/drivers/clk/mediatek/clk-mt7622.c b/drivers/clk/mediatek/clk-mt7622.c
index e8387df38798..cd5181a43132 100644
--- a/drivers/clk/mediatek/clk-mt7622.c
+++ b/drivers/clk/mediatek/clk-mt7622.c
@@ -610,6 +610,21 @@ static struct mtk_composite peri_muxes[] = {
 	MUX(CLK_PERIBUS_SEL, "peribus_ck_sel", peribus_ck_parents, 0x05C, 0, 1),
 };
 
+static struct mtk_clk_rst_data clk_rst_data[] = {
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 1,
+		.reg_ofs = 0x30,
+	},
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x0,
+	},
+};
+
 static int mtk_topckgen_init(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -663,7 +678,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x30, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[0]);
 }
 
 static int mtk_apmixedsys_init(struct platform_device *pdev)
@@ -712,7 +727,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 
 	clk_prepare_enable(clk_data->clks[CLK_PERI_UART0_PD]);
 
-	return mtk_clk_register_rst_ctrl(node, 2, 0x0, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[1]);
 }
 
 static const struct of_device_id of_match_clk_mt7622[] = {
diff --git a/drivers/clk/mediatek/clk-mt7629-eth.c b/drivers/clk/mediatek/clk-mt7629-eth.c
index bf1791efe05d..c0ff826a9769 100644
--- a/drivers/clk/mediatek/clk-mt7629-eth.c
+++ b/drivers/clk/mediatek/clk-mt7629-eth.c
@@ -76,6 +76,13 @@ static const struct mtk_gate sgmii_clks[2][4] = {
 	}
 };
 
+static struct mtk_clk_rst_data clk_rst_data = {
+	.supported = true,
+	.version = MTK_RST_V1,
+	.reg_num = 1,
+	.reg_ofs = 0x34,
+};
+
 static int clk_mt7629_ethsys_init(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -94,7 +101,7 @@ static int clk_mt7629_ethsys_init(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data);
 }
 
 static int clk_mt7629_sgmiisys_init(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/clk-mt7629-hif.c b/drivers/clk/mediatek/clk-mt7629-hif.c
index 23d22c69d47b..3b81b7e546a4 100644
--- a/drivers/clk/mediatek/clk-mt7629-hif.c
+++ b/drivers/clk/mediatek/clk-mt7629-hif.c
@@ -71,6 +71,21 @@ static const struct mtk_gate pcie_clks[] = {
 	GATE_PCIE(CLK_PCIE_P0_PIPE_EN, "pcie_p0_pipe_en", "pcie0_pipe_en", 23),
 };
 
+static struct mtk_clk_rst_data clk_rst_data[] = {
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 1,
+		.reg_ofs = 0x34,
+	},
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 1,
+		.reg_ofs = 0x34,
+	},
+};
+
 static int clk_mt7629_ssusbsys_init(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -90,7 +105,7 @@ static int clk_mt7629_ssusbsys_init(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[0]);
 }
 
 static int clk_mt7629_pciesys_init(struct platform_device *pdev)
@@ -112,7 +127,7 @@ static int clk_mt7629_pciesys_init(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_V1);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data[1]);
 }
 
 static const struct of_device_id of_match_clk_mt7629_hif[] = {
diff --git a/drivers/clk/mediatek/clk-mt8135.c b/drivers/clk/mediatek/clk-mt8135.c
index 68ce0866c5b7..272a201c13ca 100644
--- a/drivers/clk/mediatek/clk-mt8135.c
+++ b/drivers/clk/mediatek/clk-mt8135.c
@@ -514,6 +514,21 @@ static const struct mtk_composite peri_clks[] __initconst = {
 	MUX(CLK_PERI_UART3_SEL, "uart3_ck_sel", uart_ck_sel_parents, 0x40c, 3, 1),
 };
 
+static struct mtk_clk_rst_data clk_rst_data[] = {
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x30,
+	},
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x0,
+	}
+};
+
 static void __init mtk_topckgen_init(struct device_node *node)
 {
 	struct clk_onecell_data *clk_data;
@@ -561,7 +576,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
 		return;
 	}
 
-	r = mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
+	r = mtk_clk_register_rst_ctrl(node, &clk_rst_data[0]);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8135-infracfg", mtk_infrasys_init);
 
@@ -591,7 +606,7 @@ static void __init mtk_pericfg_init(struct device_node *node)
 		return;
 	}
 
-	r = mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_V1);
+	r = mtk_clk_register_rst_ctrl(node, &clk_rst_data[1]);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8135-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index fd56dbe8ff95..56f324948a8c 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -819,6 +819,21 @@ static const struct mtk_gate venclt_clks[] __initconst = {
 	GATE_VENCLT(CLK_VENCLT_CKE1, "venclt_cke1", "venclt_sel", 4),
 };
 
+static struct mtk_clk_rst_data clk_rst_data[] = {
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x30,
+	},
+	{
+		.supported = true,
+		.version = MTK_RST_V1,
+		.reg_num = 2,
+		.reg_ofs = 0x0,
+	}
+};
+
 static struct clk_onecell_data *mt8173_top_clk_data __initdata;
 static struct clk_onecell_data *mt8173_pll_clk_data __initdata;
 
@@ -884,7 +899,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
 		return;
 	}
 
-	r = mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_V1);
+	r = mtk_clk_register_rst_ctrl(node, &clk_rst_data[0]);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8173-infracfg", mtk_infrasys_init);
 
@@ -914,7 +929,7 @@ static void __init mtk_pericfg_init(struct device_node *node)
 		return;
 	}
 
-	r = mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_V1);
+	r = mtk_clk_register_rst_ctrl(node, &clk_rst_data[1]);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
index e796af76f960..adf78d882c72 100644
--- a/drivers/clk/mediatek/clk-mt8183.c
+++ b/drivers/clk/mediatek/clk-mt8183.c
@@ -1153,6 +1153,13 @@ static const struct mtk_pll_data plls[] = {
 		0, 0, 32, 8, 0x02B4, 1, 0x02BC, 0x0014, 1, 0x02B8, 0, 0x02B4),
 };
 
+static struct mtk_clk_rst_data clk_rst_data = {
+	.supported = true,
+	.version = MTK_RST_V2,
+	.reg_num = 4,
+	.reg_ofs = INFRA_RST0_SET_OFFSET,
+};
+
 static int clk_mt8183_apmixed_probe(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
@@ -1239,8 +1246,7 @@ static int clk_mt8183_infra_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	return mtk_clk_register_rst_ctrl(node, 4,
-					 INFRA_RST0_SET_OFFSET, MTK_RST_V2);
+	return mtk_clk_register_rst_ctrl(node, &clk_rst_data);
 }
 
 static int clk_mt8183_peri_probe(struct platform_device *pdev)
diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
index 2a55e8bf6b28..709ae54efc18 100644
--- a/drivers/clk/mediatek/reset.c
+++ b/drivers/clk/mediatek/reset.c
@@ -13,49 +13,51 @@
 
 #include "clk-mtk.h"
 
-struct mtk_reset {
-	struct regmap *regmap;
-	int regofs;
-	struct reset_controller_dev rcdev;
-};
-
 static int mtk_reset_assert_v1(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
-	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
+	struct mtk_clk_rst_data *data = container_of(rcdev,
+						     struct mtk_clk_rst_data,
+						     rcdev);
 
 	return regmap_update_bits(data->regmap,
-				  data->regofs + ((id / 32) << 2),
+				  data->reg_ofs + ((id / 32) << 2),
 				  BIT(id % 32), ~0);
 }
 
 static int mtk_reset_deassert_v1(struct reset_controller_dev *rcdev,
 				 unsigned long id)
 {
-	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
+	struct mtk_clk_rst_data *data = container_of(rcdev,
+						     struct mtk_clk_rst_data,
+						     rcdev);
 
 	return regmap_update_bits(data->regmap,
-				  data->regofs + ((id / 32) << 2),
+				  data->reg_ofs + ((id / 32) << 2),
 				  BIT(id % 32), 0);
 }
 
 static int mtk_reset_assert_v2(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
-	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
+	struct mtk_clk_rst_data *data = container_of(rcdev,
+						     struct mtk_clk_rst_data,
+						     rcdev);
 
 	return regmap_write(data->regmap,
-			    data->regofs + ((id / 32) << 4),
+			    data->reg_ofs + ((id / 32) << 4),
 			    BIT(id % 32));
 }
 
 static int mtk_reset_deassert_v2(struct reset_controller_dev *rcdev,
 				 unsigned long id)
 {
-	struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
+	struct mtk_clk_rst_data *data = container_of(rcdev,
+						     struct mtk_clk_rst_data,
+						     rcdev);
 
 	return regmap_write(data->regmap,
-			    data->regofs + ((id / 32) << 4) + 0x4,
+			    data->reg_ofs + ((id / 32) << 4) + 0x4,
 			    BIT(id % 32));
 }
 
@@ -100,14 +102,18 @@ static const struct reset_control_ops *rst_op[MTK_RST_MAX] = {
 };
 
 int mtk_clk_register_rst_ctrl(struct device_node *np,
-			      u32 reg_num, u16 reg_ofs, u8 version)
+			      struct mtk_clk_rst_data *clk_rst)
 {
-	struct mtk_reset *data;
-	int ret;
 	struct regmap *regmap;
+	int ret;
 
-	if (version >= MTK_RST_MAX) {
-		pr_err("Error version number: %d\n", version);
+	if (!clk_rst) {
+		pr_err("mtk clock reset data is NULL\n");
+		return -EINVAL;
+	}
+
+	if (clk_rst->version >= MTK_RST_MAX) {
+		pr_err("Error version number: %d\n", clk_rst->version);
 		return -EINVAL;
 	}
 
@@ -117,21 +123,15 @@ int mtk_clk_register_rst_ctrl(struct device_node *np,
 		return -EINVAL;
 	}
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->regmap = regmap;
-	data->regofs = reg_ofs;
-	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = reg_num * 32;
-	data->rcdev.ops = rst_op[version];
-	data->rcdev.of_node = np;
+	clk_rst->regmap = regmap;
+	clk_rst->rcdev.owner = THIS_MODULE;
+	clk_rst->rcdev.nr_resets = clk_rst->reg_num * 32;
+	clk_rst->rcdev.ops = rst_op[clk_rst->version];
+	clk_rst->rcdev.of_node = np;
 
-	ret = reset_controller_register(&data->rcdev);
+	ret = reset_controller_register(&clk_rst->rcdev);
 	if (ret) {
 		pr_err("could not register reset controller: %d\n", ret);
-		kfree(data);
 		return -EINVAL;
 	}
 
diff --git a/drivers/clk/mediatek/reset.h b/drivers/clk/mediatek/reset.h
index 0af77531b918..851a29c92440 100644
--- a/drivers/clk/mediatek/reset.h
+++ b/drivers/clk/mediatek/reset.h
@@ -6,6 +6,7 @@
 #ifndef __DRV_CLK_MTK_RESET_H
 #define __DRV_CLK_MTK_RESET_H
 
+#include <linux/reset-controller.h>
 #include <linux/types.h>
 
 enum mtk_reset_version {
@@ -14,7 +15,16 @@ enum mtk_reset_version {
 	MTK_RST_MAX,
 };
 
+struct mtk_clk_rst_data {
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+	bool supported;
+	u8 version;
+	u32 reg_num;
+	u16 reg_ofs;
+};
+
 int mtk_clk_register_rst_ctrl(struct device_node *np,
-			      u32 reg_num, u16 reg_ofs, u8 version);
+			      struct mtk_clk_rst_data *clk_rst);
 
 #endif /* __DRV_CLK_MTK_RESET_H */
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] clk: mediatek: reset: Revise structure to control reset register

[AngeloGioacchino Del Regno]

Il 18/04/22 15:21, Rex-BC Chen ha scritto:
> Replace the structure "struct mtk_reset" to reset.h, and rename it as
> "mtk_clk_rst_data". We use it to input the resset register data and
> store reset controller device.
> 
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/clk/mediatek/clk-mt2701-eth.c |  9 +++-
>   drivers/clk/mediatek/clk-mt2701-g3d.c |  9 +++-
>   drivers/clk/mediatek/clk-mt2701-hif.c |  9 +++-
>   drivers/clk/mediatek/clk-mt2701.c     | 19 +++++++-
>   drivers/clk/mediatek/clk-mt2712.c     | 19 +++++++-
>   drivers/clk/mediatek/clk-mt7622-eth.c |  9 +++-
>   drivers/clk/mediatek/clk-mt7622-hif.c | 19 +++++++-
>   drivers/clk/mediatek/clk-mt7622.c     | 19 +++++++-
>   drivers/clk/mediatek/clk-mt7629-eth.c |  9 +++-
>   drivers/clk/mediatek/clk-mt7629-hif.c | 19 +++++++-
>   drivers/clk/mediatek/clk-mt8135.c     | 19 +++++++-
>   drivers/clk/mediatek/clk-mt8173.c     | 19 +++++++-
>   drivers/clk/mediatek/clk-mt8183.c     | 10 ++++-
>   drivers/clk/mediatek/reset.c          | 62 +++++++++++++--------------
>   drivers/clk/mediatek/reset.h          | 12 +++++-
>   15 files changed, 209 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt2701-eth.c b/drivers/clk/mediatek/clk-mt2701-eth.c
> index 71257714e6a6..16fc4a50d513 100644
> --- a/drivers/clk/mediatek/clk-mt2701-eth.c
> +++ b/drivers/clk/mediatek/clk-mt2701-eth.c
> @@ -36,6 +36,13 @@ static const struct mtk_gate eth_clks[] = {
>   	GATE_ETH(CLK_ETHSYS_CRYPTO, "crypto_clk", "ethif_sel", 29),
>   };
>   
> +static struct mtk_clk_rst_data clk_rst_data = {

static const struct mtk_clk_rst_data clk_rst_data[] = {

...because this is initialization data that is never supposed to change, and
in the init function of reset.c, you should never modify this struct.

> +	.supported = true,

It would make no sense to call mtk_clk_register_rst_ctrl() if the resets
are not supported, so this .supported member is useless and should be
removed.

I will avoid commenting on patch 7/7 and will say it here: I know that on that
patch you're checking for this one but, especially if this reset data becomes a
const you can, at that point, simply check if anything has been assigned to it
so that if it's not NULL, we implicitly know that said clock controller instance
supports resets, and we register them:

int mtk_clk_simple_probe(...)
{
	.. blah blah ..

	if (mcd->rst)
		r = mtk_clk_register_rst_ctrl(....)
	return r;
}


> +	.version = MTK_RST_V1,
> +	.reg_num = 1,
> +	.reg_ofs = 0x34,
> +};
> +
>   static const struct of_device_id of_match_clk_mt2701_eth[] = {
>   	{ .compatible = "mediatek,mt2701-ethsys", },
>   	{}

..snip..

> diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
> index c1100a20c7ed..a8ac3d11ced6 100644
> --- a/drivers/clk/mediatek/clk-mt2701.c
> +++ b/drivers/clk/mediatek/clk-mt2701.c
> @@ -735,6 +735,21 @@ static const struct mtk_fixed_factor infra_fixed_divs[] = {
>   	FACTOR(CLK_INFRA_CLK_13M, "clk13m", "clk26m", 1, 2),
>   };
>   

For increased readability, perhaps it's a good idea to do something like...

> +static struct mtk_clk_rst_data clk_rst_data[] = {
> +	{
	{	/* infrasys */

> +		.supported = true,
> +		.version = MTK_RST_V1,
> +		.reg_num = 2,
> +		.reg_ofs = 0x30,
> +	},

	{	/* pericfg */

> +	{
> +		.supported = true,
> +		.version = MTK_RST_V1,
> +		.reg_num = 2,
> +		.reg_ofs = 0x0,
> +	},
> +};
> +
>   static struct clk_onecell_data *infra_clk_data;
>   
>   static void __init mtk_infrasys_init_early(struct device_node *node)

..snip...

> diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c b/drivers/clk/mediatek/clk-mt7622-hif.c
> index 6be894299ae3..a10ecc5b88a1 100644
> --- a/drivers/clk/mediatek/clk-mt7622-hif.c
> +++ b/drivers/clk/mediatek/clk-mt7622-hif.c
> @@ -76,6 +76,21 @@ static const struct mtk_gate pcie_clks[] = {
>   	GATE_PCIE(CLK_SATA_PM_EN, "sata_pm_en", "univpll2_d4", 30),
>   };
>   
> +static struct mtk_clk_rst_data clk_rst_data[] = {
> +	{
> +		.supported = true,
> +		.version = MTK_RST_V1,
> +		.reg_num = 1,
> +		.reg_ofs = 0x34,
> +	},

I get that the intention here is to separate the reset data between ssusbsys
and pciesys, but both entries are declaring the exact same... since (as pointed out
earlier) this should be a const, you can use the same entry for both.

> +	{
> +		.supported = true,
> +		.version = MTK_RST_V1,
> +		.reg_num = 1,
> +		.reg_ofs = 0x34,
> +	},
> +};
> +
>   static int clk_mt7622_ssusbsys_init(struct platform_device *pdev)
>   {
>   	struct clk_onecell_data *clk_data;

..snip..


> diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
> index 2a55e8bf6b28..709ae54efc18 100644
> --- a/drivers/clk/mediatek/reset.c
> +++ b/drivers/clk/mediatek/reset.c

..snip..

> @@ -117,21 +123,15 @@ int mtk_clk_register_rst_ctrl(struct device_node *np,
>   		return -EINVAL;
>   	}
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	data->regmap = regmap;
> -	data->regofs = reg_ofs;
> -	data->rcdev.owner = THIS_MODULE;
> -	data->rcdev.nr_resets = reg_num * 32;
> -	data->rcdev.ops = rst_op[version];
> -	data->rcdev.of_node = np;


Here's a cleaner version of this:

int mtk_clk_register_rst_ctrl(struct device *dev,
			      const struct mtk_clk_rst_desc *rst_desc)
{
	....some code....

	struct mtk_reset_data *mtk_reset;

	mtk_reset = devm_kzalloc(dev, sizeof(*mtk_reset), GFP_KERNEL)
	if (!mtk_reset)
		return -ENOMEM;

	mtk_reset->desc = rst_desc;
	mtk_reset->regmap = regmap;
	mtk_reset->rcdev.owner = .... blah blah

	ret = devm_reset_controller_register(dev, &mtk_reset->rcdev);
	if (ret)
		return ret;

....so if you use the devm_* variant, you don't need patch 6/7, as unregistering
the reset controller will be taken care of by devm actions.

Besides, this is most probably not the case but remember that if you need to
perform more operations to unregister this reset controller, you can always
add a custom devm action with function devm_add_action_or_reset(dev, func, parm).


> +	clk_rst->regmap = regmap;
> +	clk_rst->rcdev.owner = THIS_MODULE;
> +	clk_rst->rcdev.nr_resets = clk_rst->reg_num * 32;
> +	clk_rst->rcdev.ops = rst_op[clk_rst->version];
> +	clk_rst->rcdev.of_node = np;
>   
> -	ret = reset_controller_register(&data->rcdev);
> +	ret = reset_controller_register(&clk_rst->rcdev);
>   	if (ret) {
>   		pr_err("could not register reset controller: %d\n", ret);
> -		kfree(data);
>   		return -EINVAL;
>   	}
>   
> diff --git a/drivers/clk/mediatek/reset.h b/drivers/clk/mediatek/reset.h
> index 0af77531b918..851a29c92440 100644
> --- a/drivers/clk/mediatek/reset.h
> +++ b/drivers/clk/mediatek/reset.h
> @@ -6,6 +6,7 @@
>   #ifndef __DRV_CLK_MTK_RESET_H
>   #define __DRV_CLK_MTK_RESET_H
>   
> +#include <linux/reset-controller.h>
>   #include <linux/types.h>
>   
>   enum mtk_reset_version {
> @@ -14,7 +15,16 @@ enum mtk_reset_version {
>   	MTK_RST_MAX,
>   };
>   
> +struct mtk_clk_rst_data {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +	bool supported;
> +	u8 version;
> +	u32 reg_num;
> +	u16 reg_ofs;
> +};

...so here you'd have something like:

struct mtk_clk_rst_desc {
	u8 version;
	u32 reg_num;
	u16 reg_ofs;
};

struct mtk_reset_data {
	struct regmap *regmap;
	struct reset_controller_dev rcdev;
	const struct mtk_clk_rst_desc *desc;
};


Cheers,
Angelo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] clk: mediatek: reset: Revise structure to control reset register

[Rex-BC Chen]

On Tue, 2022-04-19 at 12:46 +0200, AngeloGioacchino Del Regno wrote:
> Il 18/04/22 15:21, Rex-BC Chen ha scritto:
> > Replace the structure "struct mtk_reset" to reset.h, and rename it
> > as
> > "mtk_clk_rst_data". We use it to input the resset register data and
> > store reset controller device.
> > 
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > ---
> >   drivers/clk/mediatek/clk-mt2701-eth.c |  9 +++-
> >   drivers/clk/mediatek/clk-mt2701-g3d.c |  9 +++-
> >   drivers/clk/mediatek/clk-mt2701-hif.c |  9 +++-
> >   drivers/clk/mediatek/clk-mt2701.c     | 19 +++++++-
> >   drivers/clk/mediatek/clk-mt2712.c     | 19 +++++++-
> >   drivers/clk/mediatek/clk-mt7622-eth.c |  9 +++-
> >   drivers/clk/mediatek/clk-mt7622-hif.c | 19 +++++++-
> >   drivers/clk/mediatek/clk-mt7622.c     | 19 +++++++-
> >   drivers/clk/mediatek/clk-mt7629-eth.c |  9 +++-
> >   drivers/clk/mediatek/clk-mt7629-hif.c | 19 +++++++-
> >   drivers/clk/mediatek/clk-mt8135.c     | 19 +++++++-
> >   drivers/clk/mediatek/clk-mt8173.c     | 19 +++++++-
> >   drivers/clk/mediatek/clk-mt8183.c     | 10 ++++-
> >   drivers/clk/mediatek/reset.c          | 62 +++++++++++++---------
> > -----
> >   drivers/clk/mediatek/reset.h          | 12 +++++-
> >   15 files changed, 209 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/clk/mediatek/clk-mt2701-eth.c
> > b/drivers/clk/mediatek/clk-mt2701-eth.c
> > index 71257714e6a6..16fc4a50d513 100644
> > --- a/drivers/clk/mediatek/clk-mt2701-eth.c
> > +++ b/drivers/clk/mediatek/clk-mt2701-eth.c
> > @@ -36,6 +36,13 @@ static const struct mtk_gate eth_clks[] = {
> >   	GATE_ETH(CLK_ETHSYS_CRYPTO, "crypto_clk", "ethif_sel", 29),
> >   };
> >   
> > +static struct mtk_clk_rst_data clk_rst_data = {
> 
> static const struct mtk_clk_rst_data clk_rst_data[] = {
> 
> ...because this is initialization data that is never supposed to
> change, and
> in the init function of reset.c, you should never modify this struct.
> 
> > +	.supported = true,
> 
> It would make no sense to call mtk_clk_register_rst_ctrl() if the
> resets
> are not supported, so this .supported member is useless and should be
> removed.

Hello Angelo,

Yes, you are right. I will remove is.

> 
> I will avoid commenting on patch 7/7 and will say it here: I know
> that on that
> patch you're checking for this one but, especially if this reset data
> becomes a
> const you can, at that point, simply check if anything has been
> assigned to it
> so that if it's not NULL, we implicitly know that said clock
> controller instance
> supports resets, and we register them:
> 
> int mtk_clk_simple_probe(...)
> {
> 	.. blah blah ..
> 
> 	if (mcd->rst)
> 		r = mtk_clk_register_rst_ctrl(....)
> 	return r;
> }
> 
> 
> > +	.version = MTK_RST_V1,
> > +	.reg_num = 1,
> > +	.reg_ofs = 0x34,
> > +};
> > +
> >   static const struct of_device_id of_match_clk_mt2701_eth[] = {
> >   	{ .compatible = "mediatek,mt2701-ethsys", },
> >   	{}
> 
> ..snip..
> 
> > diff --git a/drivers/clk/mediatek/clk-mt2701.c
> > b/drivers/clk/mediatek/clk-mt2701.c
> > index c1100a20c7ed..a8ac3d11ced6 100644
> > --- a/drivers/clk/mediatek/clk-mt2701.c
> > +++ b/drivers/clk/mediatek/clk-mt2701.c
> > @@ -735,6 +735,21 @@ static const struct mtk_fixed_factor
> > infra_fixed_divs[] = {
> >   	FACTOR(CLK_INFRA_CLK_13M, "clk13m", "clk26m", 1, 2),
> >   };
> >   
> 
> For increased readability, perhaps it's a good idea to do something
> like...
> 

OK, I will do this.

> > +static struct mtk_clk_rst_data clk_rst_data[] = {
> > +	{
> 
> 	{	/* infrasys */
> 
> > +		.supported = true,
> > +		.version = MTK_RST_V1,
> > +		.reg_num = 2,
> > +		.reg_ofs = 0x30,
> > +	},
> 
> 	{	/* pericfg */
> 
> > +	{
> > +		.supported = true,
> > +		.version = MTK_RST_V1,
> > +		.reg_num = 2,
> > +		.reg_ofs = 0x0,
> > +	},
> > +};
> > +
> >   static struct clk_onecell_data *infra_clk_data;
> >   
> >   static void __init mtk_infrasys_init_early(struct device_node
> > *node)
> 
> ..snip...
> 
> > diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c
> > b/drivers/clk/mediatek/clk-mt7622-hif.c
> > index 6be894299ae3..a10ecc5b88a1 100644
> > --- a/drivers/clk/mediatek/clk-mt7622-hif.c
> > +++ b/drivers/clk/mediatek/clk-mt7622-hif.c
> > @@ -76,6 +76,21 @@ static const struct mtk_gate pcie_clks[] = {
> >   	GATE_PCIE(CLK_SATA_PM_EN, "sata_pm_en", "univpll2_d4", 30),
> >   };
> >   
> > +static struct mtk_clk_rst_data clk_rst_data[] = {
> > +	{
> > +		.supported = true,
> > +		.version = MTK_RST_V1,
> > +		.reg_num = 1,
> > +		.reg_ofs = 0x34,
> > +	},
> 
> I get that the intention here is to separate the reset data between
> ssusbsys
> and pciesys, but both entries are declaring the exact same... since
> (as pointed out
> earlier) this should be a const, you can use the same entry for both.
> 

OK, I will do this using new definition of structure.

> > +	{
> > +		.supported = true,
> > +		.version = MTK_RST_V1,
> > +		.reg_num = 1,
> > +		.reg_ofs = 0x34,
> > +	},
> > +};
> > +
> >   static int clk_mt7622_ssusbsys_init(struct platform_device *pdev)
> >   {
> >   	struct clk_onecell_data *clk_data;
> 
> ..snip..
> 
> 
> > diff --git a/drivers/clk/mediatek/reset.c
> > b/drivers/clk/mediatek/reset.c
> > index 2a55e8bf6b28..709ae54efc18 100644
> > --- a/drivers/clk/mediatek/reset.c
> > +++ b/drivers/clk/mediatek/reset.c
> 
> ..snip..
> 
> > @@ -117,21 +123,15 @@ int mtk_clk_register_rst_ctrl(struct
> > device_node *np,
> >   		return -EINVAL;
> >   	}
> >   
> > -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > -	if (!data)
> > -		return -ENOMEM;
> > -
> > -	data->regmap = regmap;
> > -	data->regofs = reg_ofs;
> > -	data->rcdev.owner = THIS_MODULE;
> > -	data->rcdev.nr_resets = reg_num * 32;
> > -	data->rcdev.ops = rst_op[version];
> > -	data->rcdev.of_node = np;
> 
> 
> Here's a cleaner version of this:
> 
> int mtk_clk_register_rst_ctrl(struct device *dev,
> 			      const struct mtk_clk_rst_desc *rst_desc)
> {
> 	....some code....
> 
> 	struct mtk_reset_data *mtk_reset;
> 
> 	mtk_reset = devm_kzalloc(dev, sizeof(*mtk_reset), GFP_KERNEL)
> 	if (!mtk_reset)
> 		return -ENOMEM;
> 
> 	mtk_reset->desc = rst_desc;
> 	mtk_reset->regmap = regmap;
> 	mtk_reset->rcdev.owner = .... blah blah
> 
> 	ret = devm_reset_controller_register(dev, &mtk_reset->rcdev);
> 	if (ret)
> 		return ret;
> 
> ....so if you use the devm_* variant, you don't need patch 6/7, as
> unregistering
> the reset controller will be taken care of by devm actions.
> 
> Besides, this is most probably not the case but remember that if you
> need to
> perform more operations to unregister this reset controller, you can
> always
> add a custom devm action with function devm_add_action_or_reset(dev,
> func, parm).
> 
> 

Yes, I have noticed that. But for some clock drivers, they only provide
device_node.
I will add a new register function using device.

> > +	clk_rst->regmap = regmap;
> > +	clk_rst->rcdev.owner = THIS_MODULE;
> > +	clk_rst->rcdev.nr_resets = clk_rst->reg_num * 32;
> > +	clk_rst->rcdev.ops = rst_op[clk_rst->version];
> > +	clk_rst->rcdev.of_node = np;
> >   
> > -	ret = reset_controller_register(&data->rcdev);
> > +	ret = reset_controller_register(&clk_rst->rcdev);
> >   	if (ret) {
> >   		pr_err("could not register reset controller: %d\n",
> > ret);
> > -		kfree(data);
> >   		return -EINVAL;
> >   	}
> >   
> > diff --git a/drivers/clk/mediatek/reset.h
> > b/drivers/clk/mediatek/reset.h
> > index 0af77531b918..851a29c92440 100644
> > --- a/drivers/clk/mediatek/reset.h
> > +++ b/drivers/clk/mediatek/reset.h
> > @@ -6,6 +6,7 @@
> >   #ifndef __DRV_CLK_MTK_RESET_H
> >   #define __DRV_CLK_MTK_RESET_H
> >   
> > +#include <linux/reset-controller.h>
> >   #include <linux/types.h>
> >   
> >   enum mtk_reset_version {
> > @@ -14,7 +15,16 @@ enum mtk_reset_version {
> >   	MTK_RST_MAX,
> >   };
> >   
> > +struct mtk_clk_rst_data {
> > +	struct reset_controller_dev rcdev;
> > +	struct regmap *regmap;
> > +	bool supported;
> > +	u8 version;
> > +	u32 reg_num;
> > +	u16 reg_ofs;
> > +};
> 
> ...so here you'd have something like:
> 
> struct mtk_clk_rst_desc {
> 	u8 version;
> 	u32 reg_num;
> 	u16 reg_ofs;
> };
> 
> struct mtk_reset_data {
> 	struct regmap *regmap;
> 	struct reset_controller_dev rcdev;
> 	const struct mtk_clk_rst_desc *desc;
> };
> 

OK. Thanks for your suggestion.

BRs,
Rex

> 
> Cheers,
> Angelo
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/7] clk: mediatek: reset: Add support for unregister reset controller

[Rex-BC Chen]

Add a function to unregister reset controller.

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/reset.c | 10 ++++++++++
 drivers/clk/mediatek/reset.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
index 709ae54efc18..19f7ac0f8145 100644
--- a/drivers/clk/mediatek/reset.c
+++ b/drivers/clk/mediatek/reset.c
@@ -138,4 +138,14 @@ int mtk_clk_register_rst_ctrl(struct device_node *np,
 	return 0;
 }
 
+int mtk_clk_unregister_rst_ctrl(struct mtk_clk_rst_data *clk_rst)
+{
+	if (!clk_rst)
+		return -EINVAL;
+
+	reset_controller_unregister(&clk_rst->rcdev);
+
+	return 0;
+}
+
 MODULE_LICENSE("GPL");
diff --git a/drivers/clk/mediatek/reset.h b/drivers/clk/mediatek/reset.h
index 851a29c92440..86a785630bb6 100644
--- a/drivers/clk/mediatek/reset.h
+++ b/drivers/clk/mediatek/reset.h
@@ -27,4 +27,6 @@ struct mtk_clk_rst_data {
 int mtk_clk_register_rst_ctrl(struct device_node *np,
 			      struct mtk_clk_rst_data *clk_rst);
 
+int mtk_clk_unregister_rst_ctrl(struct mtk_clk_rst_data *clk_rst);
+
 #endif /* __DRV_CLK_MTK_RESET_H */
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/7] clk: mediatek: reset: Add reset support for simple probe/remove

[Rex-BC Chen]

- Add a pointer of "mtk_clk_rst_data" to "mtk_clk_desc".
- Add register reset function in mtk_clk_simple_probe().
- Add unregister reset function in mtk_clk_simple_remove().

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/clk-mtk.c | 6 ++++++
 drivers/clk/mediatek/clk-mtk.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
index b4063261cf56..4ecdf0be26d5 100644
--- a/drivers/clk/mediatek/clk-mtk.c
+++ b/drivers/clk/mediatek/clk-mtk.c
@@ -453,6 +453,9 @@ int mtk_clk_simple_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, clk_data);
 
+	if (mcd->rst && mcd->rst->supported)
+		r = mtk_clk_register_rst_ctrl(node, mcd->rst);
+
 	return r;
 
 unregister_clks:
@@ -468,6 +471,9 @@ int mtk_clk_simple_remove(struct platform_device *pdev)
 	struct clk_onecell_data *clk_data = platform_get_drvdata(pdev);
 	struct device_node *node = pdev->dev.of_node;
 
+	if (mcd->rst && mcd->rst->supported)
+		mtk_clk_unregister_rst_ctrl(mcd->rst);
+
 	of_clk_del_provider(node);
 	mtk_clk_unregister_gates(mcd->clks, mcd->num_clks, clk_data);
 	mtk_free_clk_data(clk_data);
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index dfb0549ceb6c..453d12cb57ac 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -194,6 +194,7 @@ struct clk *mtk_clk_register_ref2usb_tx(const char *name,
 struct mtk_clk_desc {
 	const struct mtk_gate *clks;
 	size_t num_clks;
+	struct mtk_clk_rst_data *rst;
 };
 
 int mtk_clk_simple_probe(struct platform_device *pdev);
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel