linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] samsung: exynos: convert clkout to module driver
@ 2020-10-01 16:56 Krzysztof Kozlowski
  2020-10-01 16:56 ` [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD Krzysztof Kozlowski
  2020-10-01 16:56 ` [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver Krzysztof Kozlowski
  0 siblings, 2 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-01 16:56 UTC (permalink / raw)
  To: Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc,
	linux-clk, linux-arm-kernel, linux-kernel
  Cc: Marek Szyprowski

Hi,

The patch #2 depends on #1.  I could take it via samsung-soc or both
could go via samsung-clk tree.

The patchset is a proper solution to issue reported by Marek [1], for
which a revert was done [2].

[1] https://lore.kernel.org/linux-samsung-soc/20200906142146.21266-1-krzk@kernel.org/T/#m726edc89ff2927cf5aaf8b165128b3e3c45a677a
[2] https://lore.kernel.org/linux-samsung-soc/20200921174818.15525-1-krzk@kernel.org/T/#u

Best regards,
Krzysztof


Krzysztof Kozlowski (2):
  soc: samsung: exynos-pmu: instantiate clkout driver as MFD
  clk: samsung: exynos-clkout: convert to module driver

 drivers/clk/samsung/clk-exynos-clkout.c | 198 +++++++++++++++++-------
 drivers/soc/samsung/exynos-pmu.c        |  11 ++
 2 files changed, 152 insertions(+), 57 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD
  2020-10-01 16:56 [PATCH 0/2] samsung: exynos: convert clkout to module driver Krzysztof Kozlowski
@ 2020-10-01 16:56 ` Krzysztof Kozlowski
  2020-10-14  2:44   ` Stephen Boyd
                     ` (2 more replies)
  2020-10-01 16:56 ` [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver Krzysztof Kozlowski
  1 sibling, 3 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-01 16:56 UTC (permalink / raw)
  To: Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc,
	linux-clk, linux-arm-kernel, linux-kernel
  Cc: Marek Szyprowski

The Exynos clock output (clkout) driver uses same register address space
(Power Management Unit address space) as Exynos PMU driver and same set
of compatibles.  It was modeled as clock provider instantiated with
CLK_OF_DECLARE_DRIVE().

This however brings ordering problems and lack of probe deferral,
therefore clkout driver should be converted to a regular module and
instantiated as a child of PMU driver to be able to use existing
compatibles and address space.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/soc/samsung/exynos-pmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 17304fa18429..a18c93a4646c 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -8,6 +8,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/mfd/core.h>
 #include <linux/mfd/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
@@ -97,6 +98,10 @@ static const struct of_device_id exynos_pmu_of_device_ids[] = {
 	{ /*sentinel*/ },
 };
 
+static const struct mfd_cell exynos_pmu_devs[] = {
+	{ .name = "exynos-clkout", },
+};
+
 struct regmap *exynos_get_pmu_regmap(void)
 {
 	struct device_node *np = of_find_matching_node(NULL,
@@ -110,6 +115,7 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);
 static int exynos_pmu_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	int ret;
 
 	pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pmu_base_addr))
@@ -128,6 +134,11 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pmu_context);
 
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pmu_devs,
+				   ARRAY_SIZE(exynos_pmu_devs), NULL, 0, NULL);
+	if (ret)
+		return ret;
+
 	if (devm_of_platform_populate(dev))
 		dev_err(dev, "Error populating children, reboot and poweroff might not work properly\n");
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver
  2020-10-01 16:56 [PATCH 0/2] samsung: exynos: convert clkout to module driver Krzysztof Kozlowski
  2020-10-01 16:56 ` [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD Krzysztof Kozlowski
@ 2020-10-01 16:56 ` Krzysztof Kozlowski
  2020-10-14  2:43   ` Stephen Boyd
                     ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-01 16:56 UTC (permalink / raw)
  To: Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc,
	linux-clk, linux-arm-kernel, linux-kernel
  Cc: Marek Szyprowski

The Exynos clkout driver depends on board input clock (typically XXTI or
XUSBXTI), however on Exynos4 boards these clocks were modeled as part of
SoC clocks (Exynos4 clocks driver).  Obviously this is not proper, but
correcting it would break DT backward compatibility.

Both drivers - clkout and Exynos4 clocks - register the clock providers
with CLK_OF_DECLARE/OF_DECLARE_1 so their order is fragile (in the
Makefile clkout is behind Exynos4 clock).  It will work only if the
Exynos4 clock driver comes up before clkout.

A change in DTS adding input clock reference to Exynos4 clocks input
PLL, see reverted commit eaf2d2f6895d ("ARM: dts: exynos: add input
clock to CMU in Exynos4412 Odroid"), caused probe reorder: the clkout
appeared before Exynos4 clock provider.  Since clkout depends on Exynos4
clocks and does not support deferred probe, this did not work and caused
later failure of usb3503 USB hub probe which needs clkout:

    [    5.007442] usb3503 0-0008: unable to request refclk (-517)

The Exynos clkout driver is not a critical/core clock so there is
actually no problem in instantiating it later, as a regular module.
This removes specific probe ordering and adds support for probe
deferral.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/clk/samsung/clk-exynos-clkout.c | 198 +++++++++++++++++-------
 1 file changed, 141 insertions(+), 57 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-clkout.c b/drivers/clk/samsung/clk-exynos-clkout.c
index 34ccb1d23bc3..68af082d4716 100644
--- a/drivers/clk/samsung/clk-exynos-clkout.c
+++ b/drivers/clk/samsung/clk-exynos-clkout.c
@@ -9,10 +9,13 @@
 #include <linux/slab.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/module.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
 
 #define EXYNOS_CLKOUT_NR_CLKS		1
 #define EXYNOS_CLKOUT_PARENTS		32
@@ -28,41 +31,103 @@ struct exynos_clkout {
 	struct clk_mux mux;
 	spinlock_t slock;
 	void __iomem *reg;
+	struct device_node *np;
 	u32 pmu_debug_save;
 	struct clk_hw_onecell_data data;
 };
 
-static struct exynos_clkout *clkout;
+struct exynos_clkout_variant {
+	u32 mux_mask;
+};
 
-static int exynos_clkout_suspend(void)
-{
-	clkout->pmu_debug_save = readl(clkout->reg + EXYNOS_PMU_DEBUG_REG);
+static const struct exynos_clkout_variant exynos_clkout_exynos4 = {
+	.mux_mask	= EXYNOS4_CLKOUT_MUX_MASK,
+};
 
-	return 0;
-}
+static const struct exynos_clkout_variant exynos_clkout_exynos5 = {
+	.mux_mask	= EXYNOS5_CLKOUT_MUX_MASK,
+};
 
-static void exynos_clkout_resume(void)
+static const struct of_device_id exynos_clkout_ids[] = {
+	{
+		.compatible = "samsung,exynos3250-pmu",
+		.data = &exynos_clkout_exynos4,
+	}, {
+		.compatible = "samsung,exynos4210-pmu",
+		.data = &exynos_clkout_exynos4,
+	}, {
+		.compatible = "samsung,exynos4412-pmu",
+		.data = &exynos_clkout_exynos4,
+	}, {
+		.compatible = "samsung,exynos5250-pmu",
+		.data = &exynos_clkout_exynos5,
+	}, {
+		.compatible = "samsung,exynos5410-pmu",
+		.data = &exynos_clkout_exynos5,
+	}, {
+		.compatible = "samsung,exynos5420-pmu",
+		.data = &exynos_clkout_exynos5,
+	}, {
+		.compatible = "samsung,exynos5433-pmu",
+		.data = &exynos_clkout_exynos5,
+	}, { }
+};
+
+/*
+ * Device will be instantiated as child of PMU device without its own
+ * device node.  Therefore match compatibles against parent.
+ */
+static int exynos_clkout_match_parent_dev(struct device *dev, u32 *mux_mask)
 {
-	writel(clkout->pmu_debug_save, clkout->reg + EXYNOS_PMU_DEBUG_REG);
-}
+	const struct exynos_clkout_variant *variant;
+	const struct of_device_id *match;
 
-static struct syscore_ops exynos_clkout_syscore_ops = {
-	.suspend = exynos_clkout_suspend,
-	.resume = exynos_clkout_resume,
-};
+	if (!dev->parent) {
+		dev_err(dev, "not instantiated from MFD\n");
+		return -EINVAL;
+	}
+
+	match = of_match_device(exynos_clkout_ids, dev->parent);
+	if (!match) {
+		dev_err(dev, "cannot match parent device\n");
+		return -EINVAL;
+	}
+	variant = match->data;
+
+	*mux_mask = variant->mux_mask;
+	dev_err(dev, "MATCH: %x\n", variant->mux_mask);
+
+	return 0;
+}
 
-static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
+static int exynos_clkout_probe(struct platform_device *pdev)
 {
 	const char *parent_names[EXYNOS_CLKOUT_PARENTS];
 	struct clk *parents[EXYNOS_CLKOUT_PARENTS];
-	int parent_count;
-	int ret;
-	int i;
+	struct exynos_clkout *clkout;
+	int parent_count, ret, i;
+	u32 mux_mask;
 
-	clkout = kzalloc(struct_size(clkout, data.hws, EXYNOS_CLKOUT_NR_CLKS),
-			 GFP_KERNEL);
+	clkout = devm_kzalloc(&pdev->dev,
+			      struct_size(clkout, data.hws, EXYNOS_CLKOUT_NR_CLKS),
+			      GFP_KERNEL);
 	if (!clkout)
-		return;
+		return -ENOMEM;
+
+	ret = exynos_clkout_match_parent_dev(&pdev->dev, &mux_mask);
+	if (ret)
+		return ret;
+
+	clkout->np = pdev->dev.of_node;
+	if (!clkout->np) {
+		/*
+		 * pdev->dev.parent was checked by exynos_clkout_match_parent_dev()
+		 * so it is not NULL.
+		 */
+		clkout->np = pdev->dev.parent->of_node;
+	}
+
+	platform_set_drvdata(pdev, clkout);
 
 	spin_lock_init(&clkout->slock);
 
@@ -71,7 +136,7 @@ static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
 		char name[] = "clkoutXX";
 
 		snprintf(name, sizeof(name), "clkout%d", i);
-		parents[i] = of_clk_get_by_name(node, name);
+		parents[i] = of_clk_get_by_name(clkout->np, name);
 		if (IS_ERR(parents[i])) {
 			parent_names[i] = "none";
 			continue;
@@ -82,11 +147,13 @@ static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
 	}
 
 	if (!parent_count)
-		goto free_clkout;
+		return -EINVAL;
 
-	clkout->reg = of_iomap(node, 0);
-	if (!clkout->reg)
+	clkout->reg = of_iomap(clkout->np, 0);
+	if (!clkout->reg) {
+		ret = -ENODEV;
 		goto clks_put;
+	}
 
 	clkout->gate.reg = clkout->reg + EXYNOS_PMU_DEBUG_REG;
 	clkout->gate.bit_idx = EXYNOS_CLKOUT_DISABLE_SHIFT;
@@ -103,17 +170,17 @@ static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
 				&clk_mux_ops, NULL, NULL, &clkout->gate.hw,
 				&clk_gate_ops, CLK_SET_RATE_PARENT
 				| CLK_SET_RATE_NO_REPARENT);
-	if (IS_ERR(clkout->data.hws[0]))
+	if (IS_ERR(clkout->data.hws[0])) {
+		ret = PTR_ERR(clkout->data.hws[0]);
 		goto err_unmap;
+	}
 
 	clkout->data.num = EXYNOS_CLKOUT_NR_CLKS;
-	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, &clkout->data);
+	ret = of_clk_add_hw_provider(clkout->np, of_clk_hw_onecell_get, &clkout->data);
 	if (ret)
 		goto err_clk_unreg;
 
-	register_syscore_ops(&exynos_clkout_syscore_ops);
-
-	return;
+	return 0;
 
 err_clk_unreg:
 	clk_hw_unregister(clkout->data.hws[0]);
@@ -123,38 +190,55 @@ static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
 	for (i = 0; i < EXYNOS_CLKOUT_PARENTS; ++i)
 		if (!IS_ERR(parents[i]))
 			clk_put(parents[i]);
-free_clkout:
-	kfree(clkout);
 
-	pr_err("%s: failed to register clkout clock\n", __func__);
+	dev_err(&pdev->dev, "failed to register clkout clock\n");
+
+	return ret;
 }
 
-/*
- * We use CLK_OF_DECLARE_DRIVER initialization method to avoid setting
- * the OF_POPULATED flag on the pmu device tree node, so later the
- * Exynos PMU platform device can be properly probed with PMU driver.
- */
+static int exynos_clkout_remove(struct platform_device *pdev)
+{
+	struct exynos_clkout *clkout = platform_get_drvdata(pdev);
+
+	of_clk_del_provider(clkout->np);
+	clk_hw_unregister(clkout->data.hws[0]);
+	iounmap(clkout->reg);
 
-static void __init exynos4_clkout_init(struct device_node *node)
+	return 0;
+}
+static int exynos_clkout_suspend(struct device *dev)
 {
-	exynos_clkout_init(node, EXYNOS4_CLKOUT_MUX_MASK);
+	struct exynos_clkout *clkout = dev_get_drvdata(dev);
+
+	clkout->pmu_debug_save = readl(clkout->reg + EXYNOS_PMU_DEBUG_REG);
+
+	return 0;
 }
-CLK_OF_DECLARE_DRIVER(exynos4210_clkout, "samsung,exynos4210-pmu",
-		exynos4_clkout_init);
-CLK_OF_DECLARE_DRIVER(exynos4412_clkout, "samsung,exynos4412-pmu",
-		exynos4_clkout_init);
-CLK_OF_DECLARE_DRIVER(exynos3250_clkout, "samsung,exynos3250-pmu",
-		exynos4_clkout_init);
-
-static void __init exynos5_clkout_init(struct device_node *node)
+
+static int exynos_clkout_resume(struct device *dev)
 {
-	exynos_clkout_init(node, EXYNOS5_CLKOUT_MUX_MASK);
+	struct exynos_clkout *clkout = dev_get_drvdata(dev);
+
+	writel(clkout->pmu_debug_save, clkout->reg + EXYNOS_PMU_DEBUG_REG);
+
+	return 0;
 }
-CLK_OF_DECLARE_DRIVER(exynos5250_clkout, "samsung,exynos5250-pmu",
-		exynos5_clkout_init);
-CLK_OF_DECLARE_DRIVER(exynos5410_clkout, "samsung,exynos5410-pmu",
-		exynos5_clkout_init);
-CLK_OF_DECLARE_DRIVER(exynos5420_clkout, "samsung,exynos5420-pmu",
-		exynos5_clkout_init);
-CLK_OF_DECLARE_DRIVER(exynos5433_clkout, "samsung,exynos5433-pmu",
-		exynos5_clkout_init);
+
+static SIMPLE_DEV_PM_OPS(exynos_clkout_pm_ops, exynos_clkout_suspend,
+			 exynos_clkout_resume);
+
+static struct platform_driver exynos_clkout_driver = {
+	.driver = {
+		.name = "exynos-clkout",
+		.of_match_table = exynos_clkout_ids,
+		.pm = &exynos_clkout_pm_ops,
+	},
+	.probe = exynos_clkout_probe,
+	.remove = exynos_clkout_remove,
+};
+module_platform_driver(exynos_clkout_driver);
+
+MODULE_AUTHOR("Krzysztof Kozlowski <krzk@kernel.org>");
+MODULE_AUTHOR("Tomasz Figa <tomasz.figa@gmail.com>");
+MODULE_DESCRIPTION("Samsung Exynos clock output driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver
  2020-10-01 16:56 ` [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver Krzysztof Kozlowski
@ 2020-10-14  2:43   ` Stephen Boyd
  2020-10-14  6:17     ` Krzysztof Kozlowski
  2020-10-23 11:42   ` Sylwester Nawrocki
  2020-10-28 22:07   ` Krzysztof Kozlowski
  2 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2020-10-14  2:43 UTC (permalink / raw)
  To: Chanwoo Choi, Krzysztof Kozlowski, Kukjin Kim, Michael Turquette,
	Sylwester Nawrocki, Tomasz Figa, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc
  Cc: Marek Szyprowski

Quoting Krzysztof Kozlowski (2020-10-01 09:56:46)
> diff --git a/drivers/clk/samsung/clk-exynos-clkout.c b/drivers/clk/samsung/clk-exynos-clkout.c
> index 34ccb1d23bc3..68af082d4716 100644
> --- a/drivers/clk/samsung/clk-exynos-clkout.c
> +++ b/drivers/clk/samsung/clk-exynos-clkout.c
> @@ -28,41 +31,103 @@ struct exynos_clkout {
[...]
> +       if (!match) {
> +               dev_err(dev, "cannot match parent device\n");
> +               return -EINVAL;
> +       }
> +       variant = match->data;
> +
> +       *mux_mask = variant->mux_mask;
> +       dev_err(dev, "MATCH: %x\n", variant->mux_mask);

Is this a debug print?

> +
> +       return 0;
> +}
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD
  2020-10-01 16:56 ` [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD Krzysztof Kozlowski
@ 2020-10-14  2:44   ` Stephen Boyd
  2020-10-23 11:34   ` Sylwester Nawrocki
  2020-10-28 22:02   ` Krzysztof Kozlowski
  2 siblings, 0 replies; 11+ messages in thread
From: Stephen Boyd @ 2020-10-14  2:44 UTC (permalink / raw)
  To: Chanwoo Choi, Krzysztof Kozlowski, Kukjin Kim, Michael Turquette,
	Sylwester Nawrocki, Tomasz Figa, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc
  Cc: Marek Szyprowski

Quoting Krzysztof Kozlowski (2020-10-01 09:56:45)
> The Exynos clock output (clkout) driver uses same register address space
> (Power Management Unit address space) as Exynos PMU driver and same set
> of compatibles.  It was modeled as clock provider instantiated with
> CLK_OF_DECLARE_DRIVE().
> 
> This however brings ordering problems and lack of probe deferral,
> therefore clkout driver should be converted to a regular module and
> instantiated as a child of PMU driver to be able to use existing
> compatibles and address space.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver
  2020-10-14  2:43   ` Stephen Boyd
@ 2020-10-14  6:17     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-14  6:17 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Chanwoo Choi, Kukjin Kim, Michael Turquette, Sylwester Nawrocki,
	Tomasz Figa, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc, Marek Szyprowski

On Tue, Oct 13, 2020 at 07:43:40PM -0700, Stephen Boyd wrote:
> Quoting Krzysztof Kozlowski (2020-10-01 09:56:46)
> > diff --git a/drivers/clk/samsung/clk-exynos-clkout.c b/drivers/clk/samsung/clk-exynos-clkout.c
> > index 34ccb1d23bc3..68af082d4716 100644
> > --- a/drivers/clk/samsung/clk-exynos-clkout.c
> > +++ b/drivers/clk/samsung/clk-exynos-clkout.c
> > @@ -28,41 +31,103 @@ struct exynos_clkout {
> [...]
> > +       if (!match) {
> > +               dev_err(dev, "cannot match parent device\n");
> > +               return -EINVAL;
> > +       }
> > +       variant = match->data;
> > +
> > +       *mux_mask = variant->mux_mask;
> > +       dev_err(dev, "MATCH: %x\n", variant->mux_mask);
> 
> Is this a debug print?

Debugging left over, thanks for catching this. I'll remove it.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD
  2020-10-01 16:56 ` [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD Krzysztof Kozlowski
  2020-10-14  2:44   ` Stephen Boyd
@ 2020-10-23 11:34   ` Sylwester Nawrocki
  2020-10-23 11:37     ` Krzysztof Kozlowski
  2020-10-28 22:02   ` Krzysztof Kozlowski
  2 siblings, 1 reply; 11+ messages in thread
From: Sylwester Nawrocki @ 2020-10-23 11:34 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Tomasz Figa, linux-clk, linux-arm-kernel,
	linux-kernel
  Cc: Sylwester Nawrocki, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, linux-samsung-soc, Marek Szyprowski

Hi,

On 10/1/20 18:56, Krzysztof Kozlowski wrote:
> The Exynos clock output (clkout) driver uses same register address space
> (Power Management Unit address space) as Exynos PMU driver and same set
> of compatibles.  It was modeled as clock provider instantiated with
> CLK_OF_DECLARE_DRIVE().
> 
> This however brings ordering problems and lack of probe deferral,
> therefore clkout driver should be converted to a regular module and
> instantiated as a child of PMU driver to be able to use existing
> compatibles and address space.

It might have been cleaner to have the CLKOUT device as a PMU subnode in DT, 
then device instantiation would be already covered by devm_of_platform_populate().
But it gets a bit complicated to make such a change in a backward compatible way.

I have tested both patches on Trats2, where CLKOUT provides master clock for
the audio codec.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
 

> @@ -128,6 +134,11 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, pmu_context);
>   
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pmu_devs,
> +				   ARRAY_SIZE(exynos_pmu_devs), NULL, 0, NULL);
> +	if (ret)
> +		return ret;
> +
>   	if (devm_of_platform_populate(dev))
>   		dev_err(dev, "Error populating children, reboot and poweroff might not work properly\n");



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD
  2020-10-23 11:34   ` Sylwester Nawrocki
@ 2020-10-23 11:37     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-23 11:37 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Tomasz Figa, linux-clk, linux-arm-kernel, linux-kernel,
	Sylwester Nawrocki, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, linux-samsung-soc, Marek Szyprowski

On Fri, Oct 23, 2020 at 01:34:19PM +0200, Sylwester Nawrocki wrote:
> Hi,
> 
> On 10/1/20 18:56, Krzysztof Kozlowski wrote:
> > The Exynos clock output (clkout) driver uses same register address space
> > (Power Management Unit address space) as Exynos PMU driver and same set
> > of compatibles.  It was modeled as clock provider instantiated with
> > CLK_OF_DECLARE_DRIVE().
> > 
> > This however brings ordering problems and lack of probe deferral,
> > therefore clkout driver should be converted to a regular module and
> > instantiated as a child of PMU driver to be able to use existing
> > compatibles and address space.
> 
> It might have been cleaner to have the CLKOUT device as a PMU subnode in DT, 
> then device instantiation would be already covered by devm_of_platform_populate().
> But it gets a bit complicated to make such a change in a backward compatible way.

Yes, I agree, but the backward compatibility would be here a pain.
Optionally the driver could check for new DTB and skip adding MFD
children... but this is just simpler.

> 
> I have tested both patches on Trats2, where CLKOUT provides master clock for
> the audio codec.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Thanks!

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver
  2020-10-01 16:56 ` [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver Krzysztof Kozlowski
  2020-10-14  2:43   ` Stephen Boyd
@ 2020-10-23 11:42   ` Sylwester Nawrocki
  2020-10-28 22:07   ` Krzysztof Kozlowski
  2 siblings, 0 replies; 11+ messages in thread
From: Sylwester Nawrocki @ 2020-10-23 11:42 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Tomasz Figa, linux-clk
  Cc: Sylwester Nawrocki, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, linux-samsung-soc, linux-arm-kernel,
	linux-kernel, Marek Szyprowski

On 10/1/20 18:56, Krzysztof Kozlowski wrote:
> The Exynos clkout driver depends on board input clock (typically XXTI or
> XUSBXTI), however on Exynos4 boards these clocks were modeled as part of
> SoC clocks (Exynos4 clocks driver).  Obviously this is not proper, but
> correcting it would break DT backward compatibility.
> 
> Both drivers - clkout and Exynos4 clocks - register the clock providers
> with CLK_OF_DECLARE/OF_DECLARE_1 so their order is fragile (in the
> Makefile clkout is behind Exynos4 clock).  It will work only if the
> Exynos4 clock driver comes up before clkout.
> 
> A change in DTS adding input clock reference to Exynos4 clocks input
> PLL, see reverted commit eaf2d2f6895d ("ARM: dts: exynos: add input
> clock to CMU in Exynos4412 Odroid"), caused probe reorder: the clkout
> appeared before Exynos4 clock provider.  Since clkout depends on Exynos4
> clocks and does not support deferred probe, this did not work and caused
> later failure of usb3503 USB hub probe which needs clkout:
> 
>      [    5.007442] usb3503 0-0008: unable to request refclk (-517)
> 
> The Exynos clkout driver is not a critical/core clock so there is
> actually no problem in instantiating it later, as a regular module.
> This removes specific probe ordering and adds support for probe
> deferral.


The patch looks good to me, I have tested it on Trats2, where CLKOUT
provides master clock for the audio codec.

Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

With the debug print removed feel free to apply it through your tree.
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

--
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD
  2020-10-01 16:56 ` [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD Krzysztof Kozlowski
  2020-10-14  2:44   ` Stephen Boyd
  2020-10-23 11:34   ` Sylwester Nawrocki
@ 2020-10-28 22:02   ` Krzysztof Kozlowski
  2 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-28 22:02 UTC (permalink / raw)
  To: Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, linux-samsung-soc, linux-clk,
	linux-arm-kernel, linux-kernel
  Cc: Marek Szyprowski

On Thu, Oct 01, 2020 at 06:56:45PM +0200, Krzysztof Kozlowski wrote:
> The Exynos clock output (clkout) driver uses same register address space
> (Power Management Unit address space) as Exynos PMU driver and same set
> of compatibles.  It was modeled as clock provider instantiated with
> CLK_OF_DECLARE_DRIVE().
> 
> This however brings ordering problems and lack of probe deferral,
> therefore clkout driver should be converted to a regular module and
> instantiated as a child of PMU driver to be able to use existing
> compatibles and address space.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/soc/samsung/exynos-pmu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Applied with all tags.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver
  2020-10-01 16:56 ` [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver Krzysztof Kozlowski
  2020-10-14  2:43   ` Stephen Boyd
  2020-10-23 11:42   ` Sylwester Nawrocki
@ 2020-10-28 22:07   ` Krzysztof Kozlowski
  2 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-28 22:07 UTC (permalink / raw)
  To: Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
	Stephen Boyd, Kukjin Kim, linux-samsung-soc, linux-clk,
	linux-arm-kernel, linux-kernel
  Cc: Marek Szyprowski

On Thu, Oct 01, 2020 at 06:56:46PM +0200, Krzysztof Kozlowski wrote:
> The Exynos clkout driver depends on board input clock (typically XXTI or
> XUSBXTI), however on Exynos4 boards these clocks were modeled as part of
> SoC clocks (Exynos4 clocks driver).  Obviously this is not proper, but
> correcting it would break DT backward compatibility.
> 
> Both drivers - clkout and Exynos4 clocks - register the clock providers
> with CLK_OF_DECLARE/OF_DECLARE_1 so their order is fragile (in the
> Makefile clkout is behind Exynos4 clock).  It will work only if the
> Exynos4 clock driver comes up before clkout.
> 
> A change in DTS adding input clock reference to Exynos4 clocks input
> PLL, see reverted commit eaf2d2f6895d ("ARM: dts: exynos: add input
> clock to CMU in Exynos4412 Odroid"), caused probe reorder: the clkout
> appeared before Exynos4 clock provider.  Since clkout depends on Exynos4
> clocks and does not support deferred probe, this did not work and caused
> later failure of usb3503 USB hub probe which needs clkout:
> 
>     [    5.007442] usb3503 0-0008: unable to request refclk (-517)
> 
> The Exynos clkout driver is not a critical/core clock so there is
> actually no problem in instantiating it later, as a regular module.
> This removes specific probe ordering and adds support for probe
> deferral.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/clk/samsung/clk-exynos-clkout.c | 198 +++++++++++++++++-------
>  1 file changed, 141 insertions(+), 57 deletions(-)

Applied (with fixes pointed out by Sylwester).

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-10-28 22:07 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01 16:56 [PATCH 0/2] samsung: exynos: convert clkout to module driver Krzysztof Kozlowski
2020-10-01 16:56 ` [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD Krzysztof Kozlowski
2020-10-14  2:44   ` Stephen Boyd
2020-10-23 11:34   ` Sylwester Nawrocki
2020-10-23 11:37     ` Krzysztof Kozlowski
2020-10-28 22:02   ` Krzysztof Kozlowski
2020-10-01 16:56 ` [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver Krzysztof Kozlowski
2020-10-14  2:43   ` Stephen Boyd
2020-10-14  6:17     ` Krzysztof Kozlowski
2020-10-23 11:42   ` Sylwester Nawrocki
2020-10-28 22:07   ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).