[PATCH 0/2] meson8b: register the clock controller early

[PATCH 0/2] meson8b: register the clock controller early

[Martin Blumenstingl]

Some parts of the SoC are initialized early in the boot process, such
as the SRAM and the TWD timer. This means that the corresponding
drivers need to access the clock controller early as well.

Before this series the clock controller was registered as platform
driver, which is too late for the TWD timer (just as an example).
The reset controller (built into the clock controller driver) was
already registered early, basically for the same reason (the reset
lines are used to start the secondary CPUs, which is done very early
in the boot process).

Register the clock controller early enough so we're able to use the
TWD timer (as we need to pass a clock which is provided by this
clock controller) in a second step.

The second patch simplifies the reset controller (built into the clock
controller driver) because the regmap is already available early and
thus can be used by the reset controller driver as well.


Martin Blumenstingl (2):
  clk: meson: meson8b: register the clock controller early
  clk: meson: meson8b: use the regmap in the internal reset controller

 drivers/clk/meson/meson8b.c | 107 ++++++++++++++----------------------
 1 file changed, 40 insertions(+), 67 deletions(-)

-- 
2.18.0

[PATCH 1/2] clk: meson: meson8b: register the clock controller early

[Martin Blumenstingl]

Until now only the reset controller (part of the clock controller
register space) was registered early in the boot process, while the
clock controller itself was registered later on.
However, some parts of the SoC are initialized early in the boot process,
such as the SRAM and the TWD timer. The bootloader already enables these
clocks so we didn't see any issues so far.

Register the clock controller early so other drivers (such as the SRAM
and TWD timer) can use the clocks early in the boot process.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 94 ++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 60 deletions(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 17f56f600e09..1caaa780201b 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -11,7 +11,6 @@
 #include <linux/clk-provider.h>
 #include <linux/init.h>
 #include <linux/of_address.h>
-#include <linux/platform_device.h>
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
 #include <linux/regmap.h>
@@ -22,8 +21,6 @@
 
 static DEFINE_SPINLOCK(meson_clk_lock);
 
-static void __iomem *clk_base;
-
 struct meson8b_clk_reset {
 	struct reset_controller_dev reset;
 	void __iomem *base;
@@ -1106,62 +1103,12 @@ static const struct regmap_config clkc_regmap_config = {
 	.reg_stride     = 4,
 };
 
-static int meson8b_clkc_probe(struct platform_device *pdev)
-{
-	int ret, i;
-	struct device *dev = &pdev->dev;
-	struct regmap *map;
-
-	if (!clk_base)
-		return -ENXIO;
-
-	map = devm_regmap_init_mmio(dev, clk_base, &clkc_regmap_config);
-	if (IS_ERR(map))
-		return PTR_ERR(map);
-
-	/* Populate regmap for the regmap backed clocks */
-	for (i = 0; i < ARRAY_SIZE(meson8b_clk_regmaps); i++)
-		meson8b_clk_regmaps[i]->map = map;
-
-	/*
-	 * register all clks
-	 * CLKID_UNUSED = 0, so skip it and start with CLKID_XTAL = 1
-	 */
-	for (i = CLKID_XTAL; i < CLK_NR_CLKS; i++) {
-		/* array might be sparse */
-		if (!meson8b_hw_onecell_data.hws[i])
-			continue;
-
-		ret = devm_clk_hw_register(dev, meson8b_hw_onecell_data.hws[i]);
-		if (ret)
-			return ret;
-	}
-
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
-					   &meson8b_hw_onecell_data);
-}
-
-static const struct of_device_id meson8b_clkc_match_table[] = {
-	{ .compatible = "amlogic,meson8-clkc" },
-	{ .compatible = "amlogic,meson8b-clkc" },
-	{ .compatible = "amlogic,meson8m2-clkc" },
-	{ }
-};
-
-static struct platform_driver meson8b_driver = {
-	.probe		= meson8b_clkc_probe,
-	.driver		= {
-		.name	= "meson8b-clkc",
-		.of_match_table = meson8b_clkc_match_table,
-	},
-};
-
-builtin_platform_driver(meson8b_driver);
-
-static void __init meson8b_clkc_reset_init(struct device_node *np)
+static void __init meson8b_clkc_init(struct device_node *np)
 {
 	struct meson8b_clk_reset *rstc;
-	int ret;
+	void __iomem *clk_base;
+	struct regmap *map;
+	int i, ret;
 
 	/* Generic clocks, PLLs and some of the reset-bits */
 	clk_base = of_iomap(np, 1);
@@ -1170,6 +1117,10 @@ static void __init meson8b_clkc_reset_init(struct device_node *np)
 		return;
 	}
 
+	map = regmap_init_mmio(NULL, clk_base, &clkc_regmap_config);
+	if (IS_ERR(map))
+		return;
+
 	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
 	if (!rstc)
 		return;
@@ -1185,11 +1136,34 @@ static void __init meson8b_clkc_reset_init(struct device_node *np)
 		       __func__, ret);
 		return;
 	}
+
+	/* Populate regmap for the regmap backed clocks */
+	for (i = 0; i < ARRAY_SIZE(meson8b_clk_regmaps); i++)
+		meson8b_clk_regmaps[i]->map = map;
+
+	/*
+	 * register all clks
+	 * CLKID_UNUSED = 0, so skip it and start with CLKID_XTAL = 1
+	 */
+	for (i = CLKID_XTAL; i < CLK_NR_CLKS; i++) {
+		/* array might be sparse */
+		if (!meson8b_hw_onecell_data.hws[i])
+			continue;
+
+		ret = clk_hw_register(NULL, meson8b_hw_onecell_data.hws[i]);
+		if (ret)
+			return;
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+				     &meson8b_hw_onecell_data);
+	if (ret)
+		pr_err("%s: failed to register clock provider\n", __func__);
 }
 
 CLK_OF_DECLARE_DRIVER(meson8_clkc, "amlogic,meson8-clkc",
-		      meson8b_clkc_reset_init);
+		      meson8b_clkc_init);
 CLK_OF_DECLARE_DRIVER(meson8b_clkc, "amlogic,meson8b-clkc",
-		      meson8b_clkc_reset_init);
+		      meson8b_clkc_init);
 CLK_OF_DECLARE_DRIVER(meson8m2_clkc, "amlogic,meson8m2-clkc",
-		      meson8b_clkc_reset_init);
+		      meson8b_clkc_init);
-- 
2.18.0

[PATCH 2/2] clk: meson: meson8b: use the regmap in the internal reset controller

[Martin Blumenstingl]

For now the reset controller was using raw register access because the
early init did not initialize the regmap. However, now that clocks are
initialized early we can simply use the regmap also for the reset
controller.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 1caaa780201b..e961ee9fec20 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -23,7 +23,7 @@ static DEFINE_SPINLOCK(meson_clk_lock);
 
 struct meson8b_clk_reset {
 	struct reset_controller_dev reset;
-	void __iomem *base;
+	struct regmap *regmap;
 };
 
 static const struct pll_rate_table sys_pll_rate_table[] = {
@@ -1059,7 +1059,6 @@ static int meson8b_clk_reset_update(struct reset_controller_dev *rcdev,
 		container_of(rcdev, struct meson8b_clk_reset, reset);
 	unsigned long flags;
 	const struct meson8b_clk_reset_line *reset;
-	u32 val;
 
 	if (id >= ARRAY_SIZE(meson8b_clk_reset_bits))
 		return -EINVAL;
@@ -1068,12 +1067,12 @@ static int meson8b_clk_reset_update(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&meson_clk_lock, flags);
 
-	val = readl(meson8b_clk_reset->base + reset->reg);
 	if (assert)
-		val |= BIT(reset->bit_idx);
+		regmap_update_bits(meson8b_clk_reset->regmap, reset->reg,
+				   BIT(reset->bit_idx), BIT(reset->bit_idx));
 	else
-		val &= ~BIT(reset->bit_idx);
-	writel(val, meson8b_clk_reset->base + reset->reg);
+		regmap_update_bits(meson8b_clk_reset->regmap, reset->reg,
+				   BIT(reset->bit_idx), 0);
 
 	spin_unlock_irqrestore(&meson_clk_lock, flags);
 
@@ -1126,7 +1125,7 @@ static void __init meson8b_clkc_init(struct device_node *np)
 		return;
 
 	/* Reset Controller */
-	rstc->base = clk_base;
+	rstc->regmap = map;
 	rstc->reset.ops = &meson8b_clk_reset_ops;
 	rstc->reset.nr_resets = ARRAY_SIZE(meson8b_clk_reset_bits);
 	rstc->reset.of_node = np;
-- 
2.18.0

[PATCH 1/2] clk: meson: meson8b: register the clock controller early

[Neil Armstrong]

On 21/07/2018 21:13, Martin Blumenstingl wrote:
> Until now only the reset controller (part of the clock controller
> register space) was registered early in the boot process, while the
> clock controller itself was registered later on.
> However, some parts of the SoC are initialized early in the boot process,
> such as the SRAM and the TWD timer. The bootloader already enables these
> clocks so we didn't see any issues so far.
> 
> Register the clock controller early so other drivers (such as the SRAM
> and TWD timer) can use the clocks early in the boot process.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/meson8b.c | 94 ++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index 17f56f600e09..1caaa780201b 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -11,7 +11,6 @@
>  #include <linux/clk-provider.h>
>  #include <linux/init.h>
>  #include <linux/of_address.h>
> -#include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/slab.h>
>  #include <linux/regmap.h>
> @@ -22,8 +21,6 @@
>  
>  static DEFINE_SPINLOCK(meson_clk_lock);
>  
> -static void __iomem *clk_base;
> -
>  struct meson8b_clk_reset {
>  	struct reset_controller_dev reset;
>  	void __iomem *base;
> @@ -1106,62 +1103,12 @@ static const struct regmap_config clkc_regmap_config = {
>  	.reg_stride     = 4,
>  };
>  
> -static int meson8b_clkc_probe(struct platform_device *pdev)
> -{
> -	int ret, i;
> -	struct device *dev = &pdev->dev;
> -	struct regmap *map;
> -
> -	if (!clk_base)
> -		return -ENXIO;
> -
> -	map = devm_regmap_init_mmio(dev, clk_base, &clkc_regmap_config);
> -	if (IS_ERR(map))
> -		return PTR_ERR(map);
> -
> -	/* Populate regmap for the regmap backed clocks */
> -	for (i = 0; i < ARRAY_SIZE(meson8b_clk_regmaps); i++)
> -		meson8b_clk_regmaps[i]->map = map;
> -
> -	/*
> -	 * register all clks
> -	 * CLKID_UNUSED = 0, so skip it and start with CLKID_XTAL = 1
> -	 */
> -	for (i = CLKID_XTAL; i < CLK_NR_CLKS; i++) {
> -		/* array might be sparse */
> -		if (!meson8b_hw_onecell_data.hws[i])
> -			continue;
> -
> -		ret = devm_clk_hw_register(dev, meson8b_hw_onecell_data.hws[i]);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> -					   &meson8b_hw_onecell_data);
> -}
> -
> -static const struct of_device_id meson8b_clkc_match_table[] = {
> -	{ .compatible = "amlogic,meson8-clkc" },
> -	{ .compatible = "amlogic,meson8b-clkc" },
> -	{ .compatible = "amlogic,meson8m2-clkc" },
> -	{ }
> -};
> -
> -static struct platform_driver meson8b_driver = {
> -	.probe		= meson8b_clkc_probe,
> -	.driver		= {
> -		.name	= "meson8b-clkc",
> -		.of_match_table = meson8b_clkc_match_table,
> -	},
> -};
> -
> -builtin_platform_driver(meson8b_driver);
> -
> -static void __init meson8b_clkc_reset_init(struct device_node *np)
> +static void __init meson8b_clkc_init(struct device_node *np)
>  {
>  	struct meson8b_clk_reset *rstc;
> -	int ret;
> +	void __iomem *clk_base;
> +	struct regmap *map;
> +	int i, ret;
>  
>  	/* Generic clocks, PLLs and some of the reset-bits */
>  	clk_base = of_iomap(np, 1);
> @@ -1170,6 +1117,10 @@ static void __init meson8b_clkc_reset_init(struct device_node *np)
>  		return;
>  	}
>  
> +	map = regmap_init_mmio(NULL, clk_base, &clkc_regmap_config);
> +	if (IS_ERR(map))
> +		return;
> +
>  	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
>  	if (!rstc)
>  		return;
> @@ -1185,11 +1136,34 @@ static void __init meson8b_clkc_reset_init(struct device_node *np)
>  		       __func__, ret);
>  		return;
>  	}
> +
> +	/* Populate regmap for the regmap backed clocks */
> +	for (i = 0; i < ARRAY_SIZE(meson8b_clk_regmaps); i++)
> +		meson8b_clk_regmaps[i]->map = map;
> +
> +	/*
> +	 * register all clks
> +	 * CLKID_UNUSED = 0, so skip it and start with CLKID_XTAL = 1
> +	 */
> +	for (i = CLKID_XTAL; i < CLK_NR_CLKS; i++) {
> +		/* array might be sparse */
> +		if (!meson8b_hw_onecell_data.hws[i])
> +			continue;
> +
> +		ret = clk_hw_register(NULL, meson8b_hw_onecell_data.hws[i]);
> +		if (ret)
> +			return;
> +	}
> +
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
> +				     &meson8b_hw_onecell_data);
> +	if (ret)
> +		pr_err("%s: failed to register clock provider\n", __func__);
>  }
>  
>  CLK_OF_DECLARE_DRIVER(meson8_clkc, "amlogic,meson8-clkc",
> -		      meson8b_clkc_reset_init);
> +		      meson8b_clkc_init);
>  CLK_OF_DECLARE_DRIVER(meson8b_clkc, "amlogic,meson8b-clkc",
> -		      meson8b_clkc_reset_init);
> +		      meson8b_clkc_init);
>  CLK_OF_DECLARE_DRIVER(meson8m2_clkc, "amlogic,meson8m2-clkc",
> -		      meson8b_clkc_reset_init);
> +		      meson8b_clkc_init);
> 

Hi Martin,

Yes this will register early, but the CCF maintainers want to get rid of this and switch
new drivers to actual drivers, the solution is to switch to core_initcall/postcore_initcall/subsys_initcall
depending on when the SRAM and TWD are registered.

Neil

[PATCH 1/2] clk: meson: meson8b: register the clock controller early

[Martin Blumenstingl]

Hi Neil,

On Mon, Jul 23, 2018 at 9:54 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
[snip]
> >  CLK_OF_DECLARE_DRIVER(meson8_clkc, "amlogic,meson8-clkc",
> > -                   meson8b_clkc_reset_init);
> > +                   meson8b_clkc_init);
> >  CLK_OF_DECLARE_DRIVER(meson8b_clkc, "amlogic,meson8b-clkc",
> > -                   meson8b_clkc_reset_init);
> > +                   meson8b_clkc_init);
> >  CLK_OF_DECLARE_DRIVER(meson8m2_clkc, "amlogic,meson8m2-clkc",
> > -                   meson8b_clkc_reset_init);
> > +                   meson8b_clkc_init);
> >
>
> Hi Martin,
>
> Yes this will register early, but the CCF maintainers want to get rid of this and switch
> new drivers to actual drivers, the solution is to switch to core_initcall/postcore_initcall/subsys_initcall
> depending on when the SRAM and TWD are registered.
the CLK_OF_DECLARE_DRIVER has been there before
I can replace it with core_initcall/postcore_initcall/subsys_initcall
but I would like to do that in a separate patch (which can be part of
this series). would that be OK?


Regards
Martin